Page MenuHomePhabricator

Cite silently ignores all parameters in <ref> with more than two parameters
Closed, ResolvedPublic5 Story Points

Description

A bogus return value in line https://phabricator.wikimedia.org/diffusion/ECIT/browse/master/includes/Cite.php$422 which is already marked with a FIXME makes it so that the extension ignores all parameters in a <ref …> the moment there are more than 2 parameters. This makes the extension behave utterly inconcistent:

  • <ref a="…" b="…"> is invalid and results in an error, but <ref a="…" b="…" c="…"> appears as if it is fine.
  • <ref name="…" follow="…"> is blocked with an error message (because the two parameters are in conflict) but <ref name="…" follow="…" dummy="…"> is accepted.
  • <ref name="…" group="…"> works as it should, but in <ref name="…" group="…" dummy="…"> the name and group are silently ignored.
  • <ref name="…" dummy="…"> is blocked because of the unknown parameter, but <ref name="…" dummy="…" foo="…"> is not.

Since we are planning to possibly add more parameters as part of a German-Community-Wishlist project (see T100645 and it's sub-tasks), the best solution is to remove the > 2 check entirely and rely on the later check for unknown parameters. Tests should be in place to make sure the (intended) behavior does not change.

TODOs:

  • There are currently zero tests for any of these edge cases!
  • Since when does this issue exists? The bogus return false exists basically "forever", at least since 2008. It always had the effect that all parameters are ignored.
  • Can we estimate how many pages are affected?
  • Should we revisit the decision to fail hard with an error message the moment an unknown parameter is detected?

Details

Related Gerrit Patches:

Event Timeline

Change 478645 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Add missing test cases for code in Cite::refArg

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

I would not invest more time into the issue at the moment and wait for it when we start working on the other Cite wish. - The current sprint is packed enough with more important things that should be finished before we go to full beta with the FileExporter/Importer.

thiemowmde triaged this task as Normal priority.Dec 10 2018, 1:19 PM
thiemowmde added a project: patch-welcome.
thiemowmde updated the task description. (Show Details)

Change 478645 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Add missing test cases for code in Cite::refArg

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

Change 478986 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Cite@master] Fix <ref> ignoring all parameters when there are more than two

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

Change 478986 merged by jenkins-bot:
[mediawiki/extensions/Cite@master] Fix <ref> ignoring all parameters when there are more than two

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

Johan added a subscriber: Johan.Dec 12 2018, 7:01 AM

I see the User-notice tag has been added – something like this for Tech News?

If the <code><nowiki><ref></nowiki></code> tag has parameters that don't work it gives an error message. A parameter is for example <code>name="..."</code> in <code><nowiki><ref name="..."></nowiki></code>. If the <code><nowiki><ref></nowiki></code> tag has more than two parameters all parameters are ignored. You don't get a warning that they don't work. This will soon be fixed.

Johan added a comment.Dec 12 2018, 7:10 AM

This has been added to https://meta.wikimedia.org/wiki/Tech/News/2018/51. Please edit before 14:00 UTC tomorrow (Thursday) if anything is wrong.

thiemowmde updated the task description. (Show Details)Dec 12 2018, 9:39 AM
Lea_WMDE closed this task as Resolved.Dec 18 2018, 1:07 PM
WMDE-Fisch set the point value for this task to 5.Jan 11 2019, 11:39 AM