Page MenuHomePhabricator
Create Task
Maniphest T285571

Broken parameter search widget in VisualEditor template editor
Closed, ResolvedPublic2 Estimated Story PointsBUG REPORT
Actions

Description

In the template editor in VisualEditor you can add parameters to a template via an input widget with a suggester dropdown. You can either pick parameters that are from a pre-defined list of parameters from the template's TemplateData documentation. Or you can add your own parameter if you want.

This suggester sometimes blocks you from adding a parameter back that was removed before.

The reason for this misbehavior is that the suggester holds a reference to the template model. The suggester is reused since https://gerrit.wikimedia.org/r/698547. But the template model is not always the same. It might be a new one. From this point on the suggester pulls data from an outdated model.

This causes a wide variety of confusing misbehavior.

One way to reproduce:

  • Edit any article with VisualEditor.
  • Add a template, e.g. "Infobox Film".
  • Add an arbitrary parameter, e.g. "q".
  • Don't enter a value.
  • Save.
  • Edit the template again. The parameter is gone. That's fine. It was empty, that's why it was removed.
  • Add "q" back.
  • Save again.
  • Edit again.
  • Try to add "q" back again. From here on it doesn't work any more.

Caused by:

Details

SubjectRepoBranchLines +/-
Revert "Extract "show all" to placeholder class"mediawiki/extensions/VisualEditormaster+37 -27
Hotfix for broken "Extract show all to placeholder class"mediawiki/extensions/VisualEditorwmf/1.37.0-wmf.11+1 -1
Hotfix for broken "Extract show all to placeholder class"mediawiki/extensions/VisualEditormaster+1 -1
Customize query in gerrit

Related Objects

Event Timeline

thiemowmde created this task.Jun 25 2021, 5:44 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 25 2021, 5:44 PM
gerritbot added a comment.Jun 25 2021, 5:45 PM

Change 701423 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Revert "Extract "show all" to placeholder class"

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

gerritbot added a project: Patch-For-Review.Jun 25 2021, 5:45 PM
thiemowmde claimed this task.Jun 27 2021, 4:26 PM
thiemowmde moved this task from Sprint Backlog to Doing on the WMDE-TechWish-Sprint-2021-06-23 board.
thiemowmde set the point value for this task to 2.
gerritbot added a comment.Jun 27 2021, 4:29 PM

Change 701690 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@master] Hotfix for broken "Extract show all to placeholder class"

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

gerritbot added a comment.Jun 27 2021, 4:30 PM

Change 701644 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/VisualEditor@wmf/1.37.0-wmf.11] Hotfix for broken "Extract show all to placeholder class"

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

thiemowmde updated the task description. (Show Details)Jun 28 2021, 11:18 AM
gerritbot added a comment.Jun 28 2021, 11:24 AM

Change 701690 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Hotfix for broken "Extract show all to placeholder class"

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

Lena_WMDE added a project: WMDE-Templates-FocusArea.Jun 28 2021, 11:47 AM
ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.12; 2021-06-28).Jun 28 2021, 12:00 PM
matmarex merged tasks: T285627: VisualEditor "forgets" some template parameters while editing a template, T285641: Can't place more than one reference per edit..Jun 28 2021, 12:52 PM
matmarex added subscribers: Tgr, Paz_e_concordia.
matmarex subscribed.
matmarex added a comment.Jun 28 2021, 12:55 PM

I did not look into them closely, but I am guessing that the merged bug reports have the same root cause as this. If not, please reopen and I'll investigate.

thiemowmde added a subscriber: Urbanecm.Jun 28 2021, 1:31 PM

Yes, it's all the same bug, as far as I can see. For the record: I invested hours developing and carefully testing the most minimal 1-line fix I could think of. I tried to assist backporting this fix 2 hours ago. I made sure to have a browser window open where I can clearly reproduce the issue, ready to test it. But it got refused for reasons I still don't get. Quote: "uncertainities whether the patch is a good solution for the issue". This is new. It makes it sound like the people assisting backports are meant to be familiar with the code and do actual 360 degree reviews. I don't see this reflected in the guidelines, have never seen this before, and don't think this is even possible. If this would be a requirement it would be impossible to backport anything that's not absolutely trivial.

What am I missing?

Somebody else might try again later – or not. My motivation to do this again is zero at the moment.

matmarex added a comment.Jun 28 2021, 2:03 PM

For context, there seems to have been a discussion on IRC about the deployment: https://wm-bot.wmflabs.org/libera_logs/%23wikimedia-operations/20210628.txt (relevant part starting at [11:00:04]).

It looks like the hotfix patch has not been reviewed yet at the time the backport window was scheduled. I think it's understandable for the deployer to not want to deploy it. I probably would have preferred deploying a revert patch as well, even if it changes more lines of code.

Anyway, it looks like the hotfix patch is reviewed now. I'll schedule it for the next deployment window today, and test the fix on production wikis afterwards.

gerritbot added a comment.Jun 28 2021, 6:18 PM

Change 701644 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@wmf/1.37.0-wmf.11] Hotfix for broken "Extract show all to placeholder class"

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

Stashbot mentioned this in T284636: Extract new parameter (placeholder) "show all" implementation.Jun 28 2021, 6:24 PM

Mentioned in SAL (#wikimedia-operations) [2021-06-28T18:24:51Z] <urbanecm@deploy1002> Synchronized php-1.37.0-wmf.11/extensions/VisualEditor/: rEVED794a46c861db: Hotfix for broken "Extract show all to placeholder class" (T284636; T285571) (duration: 00m 57s)

matmarex added a comment.Jun 28 2021, 6:28 PM

The backport is deployed now, and the problem should be fixed on all wikis. I'm leaving it to WMDE folks to close the task, since you pulled it onto your workboard.

Thank you for the patch @thiemowmde (and thanks also for reporting the problem before everyone else).

matmarex moved this task from To Triage to Triaged on the VisualEditor board.Jun 28 2021, 6:31 PM
ReleaseTaggerBot edited projects, added MW-1.37-notes (1.37.0-wmf.11; 2021-06-21); removed MW-1.37-notes (1.37.0-wmf.12; 2021-06-28).Jun 28 2021, 7:00 PM
WMDE-Fisch moved this task from Doing to Demo on the WMDE-TechWish-Sprint-2021-06-23 board.Jun 29 2021, 8:17 AM
gerritbot added a comment.Jun 29 2021, 9:55 AM

Change 701423 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Revert "Extract "show all" to placeholder class"

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

ReleaseTaggerBot edited projects, added MW-1.37-notes (1.37.0-wmf.14; 2021-07-12); removed MW-1.37-notes (1.37.0-wmf.11; 2021-06-21).Jun 29 2021, 10:00 AM
Maintenance_bot removed a project: Patch-For-Review.Jun 29 2021, 10:10 AM
Lena_WMDE moved this task from Backlog to In sprint on the WMDE-Templates-FocusArea board.Jun 30 2021, 3:11 PM
WMDE-Fisch closed this task as Resolved.Jul 6 2021, 11:12 AM
WMDE-Fisch moved this task from Demo to Done on the WMDE-TechWish-Sprint-2021-06-23 board.Jul 6 2021, 11:41 AM
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