Page MenuHomePhabricator

HttpRequestFactory get method always return null (was: All local file description pages pointing to Commons do not display description locally from 1.34.0-wmf.4)
Closed, ResolvedPublic

Event Timeline

Shizhao created this task.May 10 2019, 4:09 AM
Restricted Application added projects: Commons, Multimedia. · View Herald TranscriptMay 10 2019, 4:09 AM
Restricted Application added subscribers: Cosine02, Aklapper. · View Herald Transcript

Is [[MediaWiki:Sharedupload-desc-here]] replaced with [[MediaWiki:Sharedupload-desc-there]]?

TheDJ added a subscriber: TheDJ.

Also on en.wp

TheDJ updated the task description. (Show Details)May 10 2019, 9:37 AM
Tgr triaged this task as High priority.May 10 2019, 4:16 PM
Tgr added a subscriber: Tgr.

Seems like all description pages are affected.

4nn1l2 added a subscriber: 4nn1l2.May 10 2019, 11:22 PM

Issue related to WBMI? Related to T221071?

Cirdan added a subscriber: Cirdan.May 12 2019, 9:32 AM
Krenair added a comment.EditedMay 13 2019, 7:28 AM

I did a bit of fiddling around on beta with diffs looking a bit like this:

diff --git a/includes/filerepo/file/ForeignDBFile.php b/includes/filerepo/file/ForeignDBFile.php
index e083a4e4e0..a8c24a5517 100644
--- a/includes/filerepo/file/ForeignDBFile.php
+++ b/includes/filerepo/file/ForeignDBFile.php
@@ -164,11 +164,15 @@ class ForeignDBFile extends LocalFile {
                        ),
                        $this->repo->descriptionCacheExpiry ?: $cache::TTL_UNCACHEABLE,
                        function ( $oldValue, &$ttl, array &$setOpts ) use ( $renderUrl, $fname ) {
-                               wfDebug( "Fetching shared description from $renderUrl\n" );
+                               wfDebug( "Fetching shared description from $renderUrl using new method\n" );
                                $res = MediaWikiServices::getInstance()->getHttpRequestFactory()->
                                        get( $renderUrl, [], $fname );
+//                             $res = Http::get( $renderUrl, [], $fname );
                                if ( !$res ) {
                                        $ttl = WANObjectCache::TTL_UNCACHEABLE;
+                                       wfDebug( "FAILED to fetch shared description from $renderUrl\n" );
+                               } else {
+                                       wfDebug( "SUCCESSfully fetched shared description from $renderUrl, strlen " . strlen($res) . "\n" );
                                }
 
                                return $res;

results:

2019-05-13 07:20:31 [XNkav6wQBHcAACubsi4AAACW] deployment-mediawiki-07 enwiki 1.34.0-alpha wfDebug DEBUG: Fetching shared description from https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Test78.jpg&action=render&uselang=en using traditional http::get {"seconds_elapsed":"0.2258","memory_used":" 16.0M"} 
2019-05-13 07:20:31 [XNkav6wQBHcAACubsi4AAACW] deployment-mediawiki-07 enwiki 1.34.0-alpha wfDebug DEBUG: SUCCESSfully fetched shared description from https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Test78.jpg&action=render&uselang=en, strlen 7479 {"seconds_elapsed":"0.6600","memory_used":" 16.0M"}
2019-05-13 07:22:00 [XNkbFqwQBHcAACubso4AAACD] deployment-mediawiki-07 enwiki 1.34.0-alpha wfDebug DEBUG: Fetching shared description from https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Reyu_2.jpg&action=render&uselang=en using new method {"seconds_elapsed":"2.9309","memory_used":" 22.0M"} 
2019-05-13 07:22:01 [XNkbFqwQBHcAACubso4AAACD] deployment-mediawiki-07 enwiki 1.34.0-alpha wfDebug DEBUG: FAILED to fetch shared description from https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Reyu_2.jpg&action=render&uselang=en {"seconds_elapsed":"3.4031","memory_used":" 22.0M"}
hphpd> =MediaWiki\MediaWikiServices::getInstance()->getHttpRequestFactory()->get( 'https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Reyu_2.jpg&action=render&uselang=en' );
=MediaWiki\MediaWikiServices::getInstance()->getHttpRequestFactory()->get( 'https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Reyu_2.jpg&action=render&uselang=en' );
null
hphpd> =Http::get( 'https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Reyu_2.jpg&action=render&uselang=en' );
=Http::get( 'https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Reyu_2.jpg&action=render&uselang=en' );
"<div class=\"mw-parser-output\"><h2><span class=\"mw-headline\" id=\"Summary\">Summary</span></h2>\n<div class=\"hproduct\">\n<table class=\"toccolours vevent mw-content-ltr\" style=\"width: 100%; di
There are more characters. Continue? [y/N]n

Doesn't even seem to be a problem relating to being internal, this is a request from beta to prod commons:

hphpd> =Http::get( 'https://commons.wikimedia.org/w/index.php?title=Image:The_cutters\'_practical_guide_to_the_cutting_of_ladies\'_garments.djvu&action=render&uselang=en' );
=Http::get( 'https://commons.wikimedia.org/w/index.php?title=Image:The_cutters\'_practical_guide_to_the_cutting_of_ladies\'_garments.djvu&action=render&uselang=en' );
"<div class=\"mw-parser-output\"><h2><span class=\"mw-headline\" id=\"Summary\">Summary</span></h2>\n<div class=\"hproduct commons-file-information-table\">\n<table class=\"fileinfotpl-type-informatio
There are more characters. Continue? [y/N]n
n
hphpd> =MediaWiki\MediaWikiServices::getInstance()->getHttpRequestFactory()->get( 'https://commons.wikimedia.org/w/index.php?title=Image:The_cutters\'_practical_guide_to_the_cutting_of_ladies\'_garments.djvu&action=render&uselang=en' );
=MediaWiki\MediaWikiServices::getInstance()->getHttpRequestFactory()->get( 'https://commons.wikimedia.org/w/index.php?title=Image:The_cutters\'_practical_guide_to_the_cutting_of_ladies\'_garments.djvu&action=render&uselang=en' );
null
Krenair raised the priority of this task from High to Unbreak Now!.May 13 2019, 7:55 AM
Krenair added a project: WMF-Legal.

I'm going to be bold and bump this to UBN given the implications (that I probably don't fully understand) of attribution/licensing information not getting shown where it should normally be - if anyone disagrees feel free to revert. I need to go AFK for a few hours but hopefully someone will pick this up.

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptMay 13 2019, 7:55 AM
Krenair renamed this task from Some file description page on Commons cannot be displayed locally to All file description page on Commons cannot be displayed locally from 1.34.0-wmf.4.May 13 2019, 8:00 AM
Krenair renamed this task from All file description page on Commons cannot be displayed locally from 1.34.0-wmf.4 to All local file description pages pointing to Commons do not display description locally from 1.34.0-wmf.4.
Joe added a subscriber: Joe.May 13 2019, 8:47 AM
awight added a subscriber: awight.May 13 2019, 8:53 AM

Change 509777 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Return result from HttpRequestFactory get and post methods

https://gerrit.wikimedia.org/r/509777

Oops. Missing return statement.

public function get( $url, array $options = [], $caller = __METHOD__ ) {
    $this->request( 'GET', $url, $options, $caller );
}

This is very embarrassing. I should have caught this in review. Especially the fact that there are no tests. Fix is up.

hashar added a subscriber: hashar.May 13 2019, 9:47 AM

Based on @Krenair comment above, the repro case is:

<?php
use MediaWiki\MediaWikiServices;

$url = 'https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Image:Reyu_2.jpg&action=render&uselang=en';

$newhttp = MediaWikiServices::getInstance()->getHttpRequestFactory();
$newhttp->get( $url );
// WRONG: null

Http::get( $url );
// GOOD: some HTML payload

Thus I guess https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/504012/ is indeed to blame.

From a quick look, includes/http/HttpRequestFactory.php is broken and fails to return payload. Something that would have been caught if the change had any test to cover the class properly. So it has to be rollback. Kudos for the finding.

Change 509781 had a related patch set uploaded (by Hashar; owner: Hashar):
[mediawiki/core@master] Revert "Deprecate the Http class"

https://gerrit.wikimedia.org/r/509781

hashar renamed this task from All local file description pages pointing to Commons do not display description locally from 1.34.0-wmf.4 to HttpRequestFactory get method always return null (was: All local file description pages pointing to Commons do not display description locally from 1.34.0-wmf.4).May 13 2019, 9:51 AM

Change 509777 merged by jenkins-bot:
[mediawiki/core@master] Return result from HttpRequestFactory get and post methods

https://gerrit.wikimedia.org/r/509777

So previously we would have:

Http::get() which returns from Http::request() which returned the content.

With Gerrit 504012, the legacy API is now calling the new one so we have:

Http::get() returning from HttpRequestFactory::request() which does return the content.

BUT HttpRequestFactory::get() does not return anything:

public function get( $url, array $options = [], $caller = __METHOD__ ) {
    $this->request( 'GET', $url, $options, $caller );
}

It is thus broken :-/

I would love to just revert the patch and have it properly tested. Though one can just monkey patch the HttpFactory methods to actually return!

Change 509781 abandoned by Hashar:
Revert "Deprecate the Http class"

Reason:
Monkey patched by https://gerrit.wikimedia.org/r/#/c/mediawiki/core/ /509777/

https://gerrit.wikimedia.org/r/509781

Change 509783 had a related patch set uploaded (by Hashar; owner: Daniel Kinzler):
[mediawiki/core@wmf/1.34.0-wmf.4] Return result from HttpRequestFactory get and post methods

https://gerrit.wikimedia.org/r/509783

Luckily Http::get is not used much in mediawiki/core:

includes/externalstore/ExternalStoreHttp.php:           return Http::get( $url, [], __METHOD__ );
includes/filerepo/file/File.php:                                        $res = Http::get( $renderUrl, [], $fname );
includes/filerepo/file/ForeignDBFile.php:                               $res = Http::get( $renderUrl, [], $fname );
includes/import/ImportableUploadRevisionImporter.php:           $data = Http::get( $src, [], __METHOD__ );

Change 509783 merged by jenkins-bot:
[mediawiki/core@wmf/1.34.0-wmf.4] Return result from HttpRequestFactory get and post methods

https://gerrit.wikimedia.org/r/509783

Change 509786 had a related patch set uploaded (by Daniel Kinzler; owner: Daniel Kinzler):
[mediawiki/core@master] Add tests for HttpRequestFactoryTest.

https://gerrit.wikimedia.org/r/509786

Mentioned in SAL (#wikimedia-operations) [2019-05-13T10:39:53Z] <jforrester@deploy1001> Synchronized php-1.34.0-wmf.4/includes/http/HttpRequestFactory.php: T222935 Hot-deploy fix for HttpRequestFactory (duration: 00m 50s)

Jdforrester-WMF closed this task as Resolved.May 13 2019, 10:44 AM
Jdforrester-WMF assigned this task to daniel.
Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Production now appears fixed.

Follow-up work to add tests underway.

Change 509786 merged by jenkins-bot:
[mediawiki/core@master] Add tests for HttpRequestFactoryTest.

https://gerrit.wikimedia.org/r/509786

Tgr added a comment.May 13 2019, 4:22 PM

Note that this caused T222954: Imageinfo queries to non-Commons wikis about Commons files return incomplete extmetadata which will take some cache purging to fix. Not sure if there are other usages where caching could be an issue.

Change 509905 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[operations/mediawiki-config@master] Invalidate CommonsMetadata cache for entries affected by T222935

https://gerrit.wikimedia.org/r/509905

Change 509905 merged by jenkins-bot:
[operations/mediawiki-config@master] Invalidate CommonsMetadata cache for entries affected by T222935

https://gerrit.wikimedia.org/r/509905

Shizhao moved this task from Backlog to Closed on the Chinese-Sites board.May 14 2019, 2:40 AM