Page MenuHomePhabricator

Images: Handle templated image options containing pipes
Closed, ResolvedPublic0 Estimated Story Points

Description

Some perverse templates such as {{Largethumb}} return a string with several options separated by pipes ("thumb|<number>px"). In the PHP parser those pipes are then picked up by the link regexp, so result in the regular image options.

In our attribute handling we expand attributes separately, so can end up with strings like the above in a single attribute value. In [[:de:Portal:Thüringen]] for example that is the case for the href attribute, which then caused a crash after retrieving image properties.

We will likely need to handle expanded attributes containing a pipe properly by re-parsing their string representation. This will be difficult to map onto our current template-affected attribute handling.


Version: unspecified
Severity: normal

Details

Reference
bz49400

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:06 AM
bzimport set Reference to bz49400.

Related URL: https://gerrit.wikimedia.org/r/67853 (Gerrit Change Ic7652979ec3d5fa35995aac9362c812ab4ede173)

https://gerrit.wikimedia.org/r/67853 (Gerrit Change Ic7652979ec3d5fa35995aac9362c812ab4ede173) | change APPROVED and MERGED [by jenkins-bot]

There are around 110 uses on enwp
https://en.wikipedia.org/w/index.php?title=Special:WhatLinksHere/Template:Largethumb&limit=110
They are all in the article, and most of them are [[491]]-[[635]] (years). I'm guessing they can be subst'd without much concern, to converted to normal thumbs.

On nlwp, it is way over 5000 uses
https://nl.wikipedia.org/w/index.php?title=Speciaal:VerwijzingenNaarHier/Sjabloon:Largethumb&limit=5000

They are in the pages; not in a template, and nlwp doesnt appear keen to subst: them. Has anyone proposed that their default thumbnail be set to this value?

only four uses here:
https://nds-nl.wikipedia.org/wiki/Spesiaal:Verwiezingen_naor_disse_pagina/Mal:Largethumb

Ugh, I guess we have to get to this sooner than later then and find a reasonable solution.

Except for eight cases, the parser fails for nl.wp pages at http://parsoid.wmflabs.org:8001/topfails all contain '{{largethumb}}'.

~ 1 in 225 nlwp articles (0.45%) have this bug. This bug affects the whole page.

There are two independent issues here, it appears.

  1. Image attributes that are template-affected are not properly marked up during parse and hence doesn't rt back to its original form. This is an issue independent of {{largethumb}} usage. Once we fix this, even if the {{largethumb}} template doesn't parse correctly, it will RT correctly. This may be easier to fix. Will create a separate bug report for it.

[subbu@earth tests] echo '[[File:Telephone exchange 1.svg|thumb|{{echo|261px}}]]' | node parse --wt2wt
[[File:Telephone exchange 1.svg|thumb|261px]]

  1. Proper rendering of the image in VE depends on us correctly handling the largethumb template (and other such templated image options containing pipes).

Change 76821 had a related patch set uploaded by Cscott:
Add new parserTests for image attributes coming from templates.

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

Change 76821 merged by jenkins-bot:
Add new parserTests for image attributes coming from templates.

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

Change 110095 had a related patch set uploaded by Subramanya Sastry:
WIP: Set up data-mw for all templated image attributes.

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

Here is the latest on this.

https://gerrit.wikimedia.org/r/#/c/110095/ provides support for templated image options. It identifies individual image options that came from a template and provides VE sufficient information to make them editable. Parsoid can then serialize the edited attributes back to transclusions. See https://www.mediawiki.org/wiki/Parsoid/MediaWiki_DOM_spec#Transclusion-affected_attributes for how Parsoid conveys this information to clients (VE).

However, {{largethumb}} is still a special case. That gerrit patch also adds support for parsing largethumb as two image options. However, Parsoid cannot provide editing support for it, i.e. we cannot mark up both "thumb" and "261px" as coming from templates, that they are connected, and that VE cannot edit them individually. As you can see from the DOM spec (linked above), the spec is already fairly complex and we do not want to complicate the spec further to support this edge case. This is not just a complication for Parsoid, but for VE, and any other clients that need to deal with this spec.

That said, there are two options for how we can move forward.

  1. Parsoid can mark images from {{largethumb}} uneditable so that VE doesn't attempt to deal with that template. This also means that the wikitext for these images will always be preserved and can only be edited in wikitext mode.
  1. Parsoid makes the image editable. When the page is edited in VE, Parsoid preserves the image including {{largethumb}} transclusion when the image itself is not edited (even if other parts of the page are edited - which should be the vast majority of edits). But, if the image itself is edited, the {{largethumb}} template will effectively be subst-ed because Parsoid flattens out {{largethumb}} to regular options: "thumb" and "261px".

As of now, gerrit patch 110095 implements solution 1. as an initial step. However, in the long run, we feel solution 2 is a better solution that provides editing support and also gradually moves towards reduced largethumb usage (by subst-ing largethumb images edited in VE). John Vandenburg has suggested in comment 3 (#c3 above) that if nlwiki set 261px as the default thumbnail size, editors could simply use |thumb instead of |{{largethumb}} and get the same effect. Not sure if that is viable, but collating all suggestions here in one place.

It would be good to get some input on these options.

Change 110095 merged by jenkins-bot:
Set up data-mw for all templated image attributes.

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

This problem exists because nl.wp wants two thumb sizes. Why not add 'largethumb' as a core part of syntax? i.e. Has that approach been explored and rejected?

All patches merged; resetting bug status

We need to figure out what else to do with it as far as editing goes. As it stands, Parsoid supports rendering and roundtripping for these templates.

The solution to this bug is probably something like "largethumb" in core or T90914. But we could also create a {{largethumb2|File:Foo.jpg|some|options|caption}} template which expanded to a complete <figure> tag; that would allow pages which use {{largethumb2}} to be edited in VE. Ideally we'd use something like wikilint to automatically apply the {{largethumb2}} template.

Nothing to be here for this quarter .. So, I am removing the VE Q3 blocker part.

As stated in T51400#579910, Parsoid has implemented solution 1 which has been live in production since February 2014. This prevents largethumb images from being edited in VisualEditor and also ensures that those images roundtrip correctly.

If T88040: Update DOM spec for representing multiple attributes coming from a template is resolved, we could potentially unblock the editing support for these images -- to be investigated. Other possibilities for dealing with largethumb have been discussed in comments above (T51400#579925, T51400#1071119). In addition, as far as I know, VE does not yet support editing of templated-affected attributes. So, that editing support would have to be added to VE before these images with template-generated attributes can be edited.

So, TL:DR; is that the status quo seems fine for enabling VE, i.e .this task should no longer be a blocker. It prevents editing of these largethumb-marked images in VE, but it shouldn't corrupt those pages either, nor should it block editing of other parts of the page.

The current name of this ticket "Images: Handle templated image options containing pipes" suggests that handling templated image options or image options without pipes would work. They don't. Consider renaming to "Images: Handle templated image options".

On nl.wp largethumb=="260px|thumb". Creating a single option template like large=="260px" would change [[file.foo.jpg|{{largethumb}}]] into [[file.foo.jpg|{{large}}|thumb]]. The first isn't editable in VE. The latter isn't editable in VE either, though the template contains only a single attribute, and does not contain pipes.

Parsoid supports templated image options. Editability in VE of these is not a Parsoid issue. But, since then, the largethumb issues have been resolved differently and VE has been enabled on nlwiki.
Other missing functionality is tracked in their own bugs (ex: T88040). So, resolving this.