Page MenuHomePhabricator

We should not attempt to scale images which are already small
Closed, ResolvedPublic2 Story Points

Description

In Page previews we resize thumbnails by mangling the URL. For images which are already small e..g https://upload.wikimedia.org/wikipedia/en/d/dc/CITES_40th_annverisary_logo.jpg this produces an invalid URL ie. https://upload.wikimedia.org/wikipedia/en/d/dc/300px-TES_40th_annverisary_logo.jpg

The problem is caused because the thumbnail is not prefixed with a dimension e.g. [0-9]{3}px
When this is missing we should not attempt to mangle the URL.

Test case

Add a test case with the following sample input:

"thumbnail": {
"source": "https://upload.wikimedia.org/wikipedia/en/d/dc/CITES_40th_annverisary_logo.jpg",
"width": 319,
"height": 313,
"original": "https://upload.wikimedia.org/wikipedia/en/d/dc/CITES_40th_annverisary_logo.jpg"
},
"originalimage": {
"source": "https://upload.wikimedia.org/wikipedia/en/d/dc/CITES_40th_annverisary_logo.jpg",
"width": 319,
"height": 313
},

Replication steps

Illustrated by the following example:

  1. Go to https://en.wikipedia.org/w/index.php?title=Wikipedia:Today%27s_featured_article/emergency&oldid=750730632#Emergency_2.
  2. Hover over the link to https://en.wikipedia.org/wiki/CITES (currently at revision 802217134).
  3. The thumbnail is broken.
  4. https://en.wikipedia.org/api/rest_v1/page/summary/CITES returns a working thumbnail URL (https://upload.wikimedia.org/wikipedia/en/d/dc/CITES_40th_annverisary_logo.jpg).
  5. The page's Javascript somehow makes a request to https://upload.wikimedia.org/wikipedia/en/d/dc/300px-TES_40th_annverisary_logo.jpg, which is a nonexistent asset (note the first two letters of the image URL have been truncated, and then 300px- has been added).

Acceptance criteria

  • There is a unit test
  • The url is not mangled when the prefix is not present

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 26 2017, 10:30 AM
Jdlrobson triaged this task as Normal priority.Sep 26 2017, 7:25 PM
Jdlrobson updated the task description. (Show Details)
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptSep 26 2017, 7:31 PM
Jdlrobson renamed this task from Some page preview thumbnails are broken. to We should not attempt to scale images which are already small.Sep 26 2017, 7:31 PM
Jdlrobson moved this task from To Triage to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: Jdlrobson.

Thanks for the bug report!

Jdlrobson set the point value for this task to 2.Oct 11 2017, 4:50 PM
Jdlrobson claimed this task.

I will update the testing criteria.

Jdlrobson added a subscriber: phuedx.

I'm struggling to replicate this now (https://en.wikipedia.beta.wmflabs.org/wiki/Test_previews). It's possible that T173434 fixed this, as we're now using a common thumbnail size. @phuedx can you take a look?

It's probably best to add a test case regardless where the image inputted to the function is https://upload.wikimedia.org/wikipedia/en/d/dc/CITES_40th_annverisary_logo.jpg in case this happens again or we change thumbnail size again.

Change 383831 had a related patch set uploaded (by Phuedx; owner: Phuedx):
[mediawiki/extensions/Popups@master] gateway/rest: Handle large "small" thumbnails

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

I'm struggling to replicate this now (https://en.wikipedia.beta.wmflabs.org/wiki/Test_previews). It's possible that T173434 fixed this, as we're now using a common thumbnail size. @phuedx can you take a look?

The fix for T173434: Consider using more common image sizes for Page previews did fix this bug.

This bug is a symptom of the Page Summary API and Page Previews (the "client") having a different notion of what size of thumbnail is being requested. Prior to T173434, the client operated as if the requested thumbnail size was 300px but in reality the API request 320px. From the POV of the Page Summary API, the thumbnail can't be produced but from the POV of the client, a thumbnail should be produced.

I've submitted a test case covering this issue but I'm not convinced that it should be fixed. Instead, now that the client requests images that are inline with the Page Summary API (and yield better cache hit rates on cache_upload!) we should track upstream changes to the Page Summary API if and when they're suggested.

Jdlrobson closed this task as Resolved.Oct 12 2017, 6:13 PM

Let's call this resolved then.
We can always reopen at another time if we reintroduce this error. Thanks @TheDragonFire for the bug report.

Change 383831 abandoned by Phuedx:
gateway/rest: Handle large "small" thumbnails

Reason:
Per T176729#3680617.

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