GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Edit Task
  • Edit Related Tasks...
  • Create Subtask
  • Edit Parent Tasks
  • Edit Subtasks
  • Merge Duplicates In
  • Close As Duplicate
  • Edit Commits
  • Edit Mocks
  • Mute Notifications
  • Protect as security issue
  • MediaWiki-extensions-Linter
  • (Parsoid)
  • Parsoid

This view

  • Original page
  • New address

GeezerVison is showing phabricator.wikimedia.org — T216003 Linter fails to detect multiple "upright" parameters as a Bogus file option

Page MenuHomePhabricator
Log In
Create Task
Maniphest T216003

Linter fails to detect multiple "upright" parameters as a Bogus file option
Closed, ResolvedPublic
Actions

  • Edit Task
  • Edit Related Tasks...
  • Create Subtask
  • Edit Parent Tasks
  • Edit Subtasks
  • Merge Duplicates In
  • Close As Duplicate
  • Edit Related Objects...
  • Edit Commits
  • Edit Mocks
  • Mute Notifications
  • Protect as security issue
Assigned To
Arlolra
Authored By
Jonesey95
Feb 13 2019, 8:51 AM2019-02-13 08:51:54 (UTC+0)
Project Tags
  • MediaWiki-extensions-Linter (Parsoid)
  • Parsoid (Linting)
  • MW-1.39-notes (1.39.0-wmf.8; 2022-04-18)
Referenced Files
None
Subscribers
Aklapper
Arlolra
cscott
Jonesey95
NicoV

Description

Linter fails to detect duplicate upright parameters like this:

[[File:Temppeliaukio_Church_Interior.JPG|thumb|upright|right|upright=1.15|The [[Temppeliaukio Church]] in Finland is carved out of a single rock.]]

It should see that there are two "upright" parameters and flag them as a Bogus file option.

Details

Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
Percent decode titles in media link optionArlolramediawiki/coremaster+133 -0
Bump wikimedia/parsoid to 0.16.0-a5Subramanya Sastrymediawiki/vendormaster+556 -2 K
Lint invalid upright optionsArlolramediawiki/services/parsoidmaster+34 -4
Customize query in gerrit

Related Objects

  • Mentions
Mentioned In
T306216: Media options don't have the whitespace leniency as template arguments
T305628: Make all media options use the 'last one wins' strategy
T297443: Add title/alt attributes from caption to media where caption isn't visible
T274382: Make File syntax processing match the specification and documentation
T257581: Visual Editor adds link= parameter to image when only caption is edited
T216566: Capitalized versions of valid File options are usually ignored, but are not flagged as Linter bogus file options.
Mentioned Here
T306216: Media options don't have the whitespace leniency as template arguments
T305628: Make all media options use the 'last one wins' strategy

Event Timeline

Jonesey95 created this task.Feb 13 2019, 8:51 AM2019-02-13 08:51:54 (UTC+0)
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 13 2019, 8:51 AM2019-02-13 08:51:54 (UTC+0)
Legoktm added a project: Parsoid-Linter.Feb 13 2019, 6:00 PM2019-02-13 18:00:27 (UTC+0)
Jonesey95 added a comment.Feb 19 2019, 6:59 PM2019-02-19 18:59:49 (UTC+0)
Comment Actions

Possibly related: <code>|upright =1.5|</code> (with a space between "upright" and the equals sign) is ignored (the image renders as upright=1.0), but is not detected as a bogus option.

Jonesey95 added a comment.Feb 19 2019, 9:07 PM2019-02-19 21:07:22 (UTC+0)
Comment Actions

A variant on this bug:

[[File:Temppeliaukio_Church_Interior.JPG|thumb|right|upright=2|upright|The [[Temppeliaukio Church]] in Finland is carved out of a single rock.]]

In this example, the second "upright" is marked as a Linter bogus file option, but it is actually the "upright=2" that is being ignored. The Linter error is correct, but the options are being processed incorrectly (the first valid option should be processed in order to be consistent with handling of the other named parameters like left/right and thumb/frame).

I also documented a bunch of edge cases, some of which behave differently based on which parameter is being processed, at https://www.mediawiki.org/wiki/Help:Images. It seems like someone might need to go back to the spec for the File display options and make sure that the code is written to the spec. (...or write a spec...) I keep finding odd little quirks.

ssastry triaged this task as Medium priority.Mar 11 2019, 5:08 PM2019-03-11 17:08:20 (UTC+0)
ssastry moved this task from Backlog to Parsoid on the MediaWiki-extensions-Linter board.
Jonesey95 added a comment.Edited · Jan 12 2020, 7:06 AM2020-01-12 07:06:00 (UTC+0)
Comment Actions

One of the notes I added to Help:Images: "If there is a space character between alt and the equals sign, the alt statement will be treated as a caption." This behavior is contrary to how editors are used to working with template parameters. Yes, I know that images are not templates, but it is difficult to manage this inconsistent behavior, and there does not appear to be a good design reason for treating these spaces as syntactically meaningful while (for example) stripping out spaces after the equals sign.

You can see this alt= versus alt = behavior here: https://en.wikipedia.org/w/index.php?title=User:Jonesey95/sandbox3&oldid=935129500

Jonesey95 added a comment.Feb 6 2020, 5:12 PM2020-02-06 17:12:20 (UTC+0)
Comment Actions

Here is another variant on this bug:

https://en.wikipedia.org/w/index.php?title=User:Jonesey95/sandbox3&oldid=939466925

The following image options are not detected as bogus by the Linter engine:

upright=2S
upright 0.75S

In all cases, a caption is present, so the invalid upright option without an equals sign should either be flagged as excess text or as incorrect syntax for the upright option.

Jonesey95 mentioned this in T216566: Capitalized versions of valid File options are usually ignored, but are not flagged as Linter bogus file options..Feb 21 2020, 5:30 PM2020-02-21 17:30:19 (UTC+0)
Aklapper edited projects, added: Parsoid; removed: Parsoid-Linter.Feb 29 2020, 5:13 PM2020-02-29 17:13:23 (UTC+0) ·
ssastry moved this task from Needs Triage to Linting on the Parsoid board.Feb 29 2020, 6:04 PM2020-02-29 18:04:30 (UTC+0)
Jonesey95 added a comment.Jul 8 2020, 6:39 PM2020-07-08 18:39:28 (UTC+0)
Comment Actions

Another variant on this bug: the following wikicode renders on en.WP with the "link" section as the caption, and Linter does not flag the real caption as extra text. That looks like two bugs in one image call.

[[File:NYC Bus with "masks required" sign.jpg|thumb|New York City transit bus. 177th Street near Devoe Ave, Bronx, NY. Route sign reads, "Masks Required"|link=File:NYC_Bus_with_%22masks_required%22_sign.jpg|alt=]]

This works fine:

[[File:NYC Bus with "masks required" sign.jpg|thumb|New York City transit bus. 177th Street near Devoe Ave, Bronx, NY. Route sign reads, "Masks Required"|link=File:NYC_Bus_with_"masks_required"_sign.jpg|alt=]]

Jonesey95 mentioned this in T257581: Visual Editor adds link= parameter to image when only caption is edited.Jul 9 2020, 2:35 PM2020-07-09 14:35:30 (UTC+0)
NicoV subscribed.Oct 3 2020, 7:13 AM2020-10-03 07:13:57 (UTC+0)
Jonesey95 added a comment.Oct 13 2020, 9:19 PM2020-10-13 21:19:40 (UTC+0)
Comment Actions

Here's another upright option that is not detected as bogus by Linter:

[[File:Liloy National High School ESF Building.jpg|thumb|upright=1.5"|right|The Liloy National High School 2-story ESF Building.]]

Note the quote mark after the upright value.

Jonesey95 mentioned this in T274382: Make File syntax processing match the specification and documentation.Feb 10 2021, 3:35 PM2021-02-10 15:35:27 (UTC+0)
Jonesey95 added a comment.Mar 4 2021, 12:29 AM2021-03-04 00:29:09 (UTC+0)
Comment Actions

It appears that the first error (duplicate upright) is detected now after some Linter bug patches, but the second error (a quote mark following the number) is still not detected. See this test case:

https://en.wikipedia.org/w/index.php?title=User:Jonesey95/sandbox&oldid=1010141113

Jonesey95 added a comment.Mar 11 2021, 4:38 PM2021-03-11 16:38:41 (UTC+0)
Comment Actions

And another variant found in the wild:
https://en.wikipedia.org/w/index.php?title=User:Jonesey95/sandbox&oldid=1011570863

[[Image:gray437.png|thumb|upright=0.7px|Human lower leg anatomy]]

Jonesey95 mentioned this in T297443: Add title/alt attributes from caption to media where caption isn't visible.Jan 14 2022, 5:19 AM2022-01-14 05:19:58 (UTC+0)
gerritbot added a comment.Apr 6 2022, 12:16 AM2022-04-06 00:16:38 (UTC+0)
Comment Actions

Change 777474 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/services/parsoid@master] Lint invalid upright options

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

gerritbot added a project: Patch-For-Review.Apr 6 2022, 12:16 AM2022-04-06 00:16:39 (UTC+0)
gerritbot added a comment.Apr 6 2022, 4:57 PM2022-04-06 16:57:37 (UTC+0)
Comment Actions

Change 777474 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Lint invalid upright options

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

Maintenance_bot removed a project: Patch-For-Review.Apr 6 2022, 5:31 PM2022-04-06 17:31:36 (UTC+0)
Arlolra subscribed.Apr 6 2022, 6:35 PM2022-04-06 18:35:26 (UTC+0)
Comment Actions
In T216003#5795258, @Jonesey95 wrote:

One of the notes I added to Help:Images: "If there is a space character between alt and the equals sign, the alt statement will be treated as a caption." This behavior is contrary to how editors are used to working with template parameters. Yes, I know that images are not templates, but it is difficult to manage this inconsistent behavior, and there does not appear to be a good design reason for treating these spaces as syntactically meaningful while (for example) stripping out spaces after the equals sign.

You can see this alt= versus alt = behavior here: https://en.wikipedia.org/w/index.php?title=User:Jonesey95/sandbox3&oldid=935129500

This comes from pattern matching,

{
                "name": "img_alt",
                "aliases": [
                    "alt=$1"
                ],
                "case-sensitive": ""
            },

https://github.com/wikimedia/parsoid/blob/master/baseconfig/enwiki.json#L1158

Arlolra added a comment.Apr 6 2022, 6:48 PM2022-04-06 18:48:52 (UTC+0)
Comment Actions
In T216003#6290901, @Jonesey95 wrote:

Another variant on this bug: the following wikicode renders on en.WP with the "link" section as the caption, and Linter does not flag the real caption as extra text. That looks like two bugs in one image call.

[[File:NYC Bus with "masks required" sign.jpg|thumb|New York City transit bus. 177th Street near Devoe Ave, Bronx, NY. Route sign reads, "Masks Required"|link=File:NYC_Bus_with_%22masks_required%22_sign.jpg|alt=]]

This works fine:

[[File:NYC Bus with "masks required" sign.jpg|thumb|New York City transit bus. 177th Street near Devoe Ave, Bronx, NY. Route sign reads, "Masks Required"|link=File:NYC_Bus_with_"masks_required"_sign.jpg|alt=]]

Parsoid doesn't detect this because it renders them identically. % is an invalid title char and I suppose Parsoid is (incorrectly?) doing percent decoding of the string before trying to construct a Title object with it.

Arlolra added a comment.Apr 6 2022, 6:56 PM2022-04-06 18:56:14 (UTC+0)
Comment Actions
In T216003#6881206, @Jonesey95 wrote:

It appears that the first error (duplicate upright) is detected now after some Linter bug patches, but the second error (a quote mark following the number) is still not detected. See this test case:

https://en.wikipedia.org/w/index.php?title=User:Jonesey95/sandbox&oldid=1010141113

Should be fixed with T216003#7834043

Jonesey95 added a comment.Apr 6 2022, 6:58 PM2022-04-06 18:58:27 (UTC+0)
Comment Actions
In T216003#7836229, @Arlolra wrote:
In T216003#6290901, @Jonesey95 wrote:

Another variant on this bug: the following wikicode renders on en.WP with the "link" section as the caption, and Linter does not flag the real caption as extra text. That looks like two bugs in one image call.

[[File:NYC Bus with "masks required" sign.jpg|thumb|New York City transit bus. 177th Street near Devoe Ave, Bronx, NY. Route sign reads, "Masks Required"|link=File:NYC_Bus_with_%22masks_required%22_sign.jpg|alt=]]

Parsoid doesn't detect this because it renders them identically. % is an invalid title char and I suppose Parsoid is (incorrectly?) doing percent decoding of the string before trying to construct a Title object with it.

If that is the case, a % or other invalid character in link= should be reported as a Linter error. It should not just fail silently.

Arlolra added a comment.Apr 6 2022, 7:04 PM2022-04-06 19:04:21 (UTC+0)
Comment Actions

If that is the case, a % or other invalid character in link= should be reported as a Linter error. It should not just fail silently.

Maybe. It could be that the legacy parser is being inconsistent and it some cases it does percent decoding and, in this, it isn't. I need to investigate it further and see if it's better to just fix that. There is a test I know of with link=< that both parsers handle the same way. So, it isn't all invalid characters.

Arlolra added a comment.Apr 6 2022, 7:13 PM2022-04-06 19:13:33 (UTC+0)
Comment Actions

For example, [[:File:NYC_Bus_with_%22masks_required%22_sign.jpg]] seems to work

gerritbot added a comment.Apr 6 2022, 8:26 PM2022-04-06 20:26:36 (UTC+0)
Comment Actions

Change 777870 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/core@master] Percent decode titles in media link option

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

gerritbot added a project: Patch-For-Review.Apr 6 2022, 8:26 PM2022-04-06 20:26:36 (UTC+0)
Arlolra added a comment.Apr 6 2022, 11:44 PM2022-04-06 23:44:03 (UTC+0)
Comment Actions
In T216003#4966381, @Jonesey95 wrote:

A variant on this bug:

[[File:Temppeliaukio_Church_Interior.JPG|thumb|right|upright=2|upright|The [[Temppeliaukio Church]] in Finland is carved out of a single rock.]]

In this example, the second "upright" is marked as a Linter bogus file option, but it is actually the "upright=2" that is being ignored. The Linter error is correct, but the options are being processed incorrectly (the first valid option should be processed in order to be consistent with handling of the other named parameters like left/right and thumb/frame).

I also documented a bunch of edge cases, some of which behave differently based on which parameter is being processed, at https://www.mediawiki.org/wiki/Help:Images. It seems like someone might need to go back to the spec for the File display options and make sure that the code is written to the spec. (...or write a spec...) I keep finding odd little quirks.

This is a bit of a mess. So, the format (framed, thumbnail, etc) is first one wins. Dimensions (width, height) are last one wins. Horizontal and vertical alignment (left, right, etc) are first one wins. Caption is last one wins. The legacy parser and Parsoid agree on that. However, for any other media option, Parsoid is first one wins and the legacy parser is last one wins. This is where the discrepancy with upright originates.

https://github.com/wikimedia/mediawiki/blob/master/includes/parser/Parser.php#L5353-L5442

Arlolra added a comment.Apr 7 2022, 12:09 AM2022-04-07 00:09:52 (UTC+0)
Comment Actions

If both |upright| and |upright=number| are present, the first |upright| will be ignored. See task T216003 for details.

This could be clearer in the documentation. It could be confused that somehow specifying the =number will take precedent, when it's really just that the last upright definition is the one that's used.

If there is a space character between "upright" and the equals sign (e.g. |upright =number|), or if the "upright" value contains non-numeric characters, the upright value will be ignored. See task T216003 for details.

This is the same pattern matching confusion with |alt= in T216003#7836205. I agree that it's unfortunate that this doesn't have the same leniency as in template arguments.

{
    "name": "img_upright",
    "aliases": [
        "upright",
        "upright=$1",
        "upright $1"
    ],
    "case-sensitive": ""
},
cscott subscribed.Apr 7 2022, 1:55 AM2022-04-07 01:55:00 (UTC+0)
Comment Actions

Caption is last one wins, but also remember that "anything not recognized as a valid option is a caption". That behavior actually makes the syntax very hard to extend & very hard to deprecate no-longer-acception options without creating a bunch of inadvertent captions.

I think consistency is a virtue here, and it seems like the legacy parser is "most consistent" w/r/t a "last one wins" policy, across a number of different attribute types. So I'd support changing Parsoid to match that -- but I'd hope that we're also linting or tracking-categorying when this happens, because I'd *like* to say that behavior is "undefined" if you have duplicate options.

Arlolra mentioned this in T305628: Make all media options use the 'last one wins' strategy.Apr 7 2022, 12:24 PM2022-04-07 12:24:29 (UTC+0)
Comment Actions

I think consistency is a virtue here, and it seems like the legacy parser is "most consistent" w/r/t a "last one wins" policy

I filed T305628 to make that change

but I'd hope that we're also linting or tracking-categorying when this happens

We've been linting bogus / duplicate media options for quite some time now

gerritbot added a comment.Apr 11 2022, 6:17 PM2022-04-11 18:17:05 (UTC+0)
Comment Actions

Change 779085 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.16.0-a5

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

gerritbot added a comment.Apr 11 2022, 7:06 PM2022-04-11 19:06:22 (UTC+0)
Comment Actions

Change 779085 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.16.0-a5

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

gerritbot added a comment.Apr 14 2022, 1:51 PM2022-04-14 13:51:58 (UTC+0)
Comment Actions

Change 777870 merged by jenkins-bot:

[mediawiki/core@master] Percent decode titles in media link option

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

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.8; 2022-04-18).Apr 14 2022, 2:00 PM2022-04-14 14:00:40 (UTC+0)
Maintenance_bot removed a project: Patch-For-Review.Apr 14 2022, 2:31 PM2022-04-14 14:31:02 (UTC+0)
Arlolra mentioned this in T306216: Media options don't have the whitespace leniency as template arguments.Apr 14 2022, 7:34 PM2022-04-14 19:34:57 (UTC+0)
Arlolra added a comment.Apr 14 2022, 7:37 PM2022-04-14 19:37:12 (UTC+0)
Comment Actions
In T216003#7837007, @Arlolra wrote:

This is the same pattern matching confusion with |alt= in T216003#7836205. I agree that it's unfortunate that this doesn't have the same leniency as in template arguments.

Filed as T306216

Arlolra added a comment.Apr 14 2022, 7:37 PM2022-04-14 19:37:54 (UTC+0)
Comment Actions
In T216003#7836261, @Arlolra wrote:

Maybe. It could be that the legacy parser is being inconsistent and it some cases it does percent decoding and, in this, it isn't. I need to investigate it further and see if it's better to just fix that. There is a test I know of with link=< that both parsers handle the same way. So, it isn't all invalid characters.

Addressed by T216003#7855217

Arlolra closed this task as Resolved.Apr 14 2022, 7:40 PM2022-04-14 19:40:56 (UTC+0)
Arlolra claimed this task.
Comment Actions

I think everything here is now fixed or covered by another task.

Log In to Comment
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.