Page MenuHomePhabricator

"deprecated" parameter handling
Closed, ResolvedPublic3 Estimated Story Points

Description

  • If a deprecate parameter is in use in the wikitext (no matter if it does have a value or not), show it in the sidebar.
  • For deprecated parameters not in use, hide them in the sidebar so that they cannot be added.
  • Removing a deprecated parameter does not immediately remove it's entry in the sidebar. The behavior should be similar to an undocumented parameter.
  • Make sure we never create a dirty diff on save.

Note: Deprecated parameters will be prevented from being added through the undocumented parameter input, added in T272487: New input for undocumented parameters. This is covered in the related ticket about error handling, see T285869: Add error handling for the undocumented parameter input.

Related Objects

StatusSubtypeAssignedTask
OpenNone
ResolvedWMDE-Fisch
ResolvedWMDE-Fisch
ResolvedAndrew-WMDE
ResolvedWMDE-Fisch
ResolvedAndrew-WMDE
ResolvedBUG REPORTawight
OpenNone
OpenNone
Resolvedthiemowmde
ResolvedWMDE-Fisch
ResolvedLena_WMDE
ResolvedNone
Resolvedawight
ResolvedAndrew-WMDE
InvalidNone
Resolvedthiemowmde
ResolvedECohen_WMDE
OpenNone

Event Timeline

Lena_WMDE set the point value for this task to 3.

Change 704982 had a related patch set uploaded (by Andrew-WMDE; author: Andrew-WMDE):

[mediawiki/extensions/VisualEditor@master] [WIP] Hide deprecated parameters if they don't have an existing value

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

Change 704982 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Hide deprecated parameters if they don't have a value

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

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

[mediawiki/extensions/VisualEditor@master] Revert \"Hide deprecated parameters if they don't have a value\"

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

From today's demo time:

  • The basic idea to not offer a checkbox for empty deprecated parameters works fine.
  • However, it should be possible to still add such a deprecated parameter the same way as an undocumented parameter. It appears like this is broken.
  • We worried this might have broken the old workflow that is currently deployed on all wikis. Luckily this is not the case as the problematic code is wrapped in a if ( …NewSidebar ) conditional. Still we reverted the patch for the moment just to be sure. We will redo it.

Change 709647 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Revert \"Hide deprecated parameters if they don't have a value\"

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

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

[mediawiki/extensions/VisualEditor@master] [WIP] Initially hide empty deprecated parameters

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

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

[mediawiki/extensions/VisualEditor@master] Remove redundant ve.dm.MWTemplatePlaceholderModel.isEmpty

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

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

[mediawiki/extensions/VisualEditor@master] Document definition of \"empty\" in ve.ui.MWParameterPage

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

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

[mediawiki/extensions/VisualEditor@master] Update ambiguous definition of isEmpty in the model

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

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

[mediawiki/extensions/VisualEditor@master] Update and encode definition of empty for template parameters

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

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

[mediawiki/extensions/VisualEditor@master] Drop deprecated parameters when they are empty

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

I found more issues with the original patch https://gerrit.wikimedia.org/r/704982.

Before, the new sidebar assumed there is a 1:1 relationship between the list of checkboxes and the list of parameters that are tracked in the spec (these include documented and undocumented, used and unused parameters, just as the list of checkboxes). The only exception is the placeholder, but this is always at the end and doesn't matter because of this. The patch broke this assumption, resulting in weird behavior when adding undocumented parameters. Their order got messed up.

Another issue I run into was the realization that the task, as it is described, essentially forces us to create a new type of parameter: A parameter that doesn't have a checkbox but still exists in the template (this must be done to not cause dirty diffs). This confuses all code that checks if a parameter is already in the template – and there is a lot of such code. I tried to introduce some kind of "this parameter exists but is hidden" flag, but gave up. The code got messy and unmaintainable very fast.

Another idea is to create all checkboxes (this avoids the mismatches described above) and just hide the one that should not be there. Unfortunately this requires the same amount of messy code to deal with checkboxes that can be hidden – a concept that didn't exist before.

I figured that the benefits of such a rather complicated approach are not worth it. Instead I suggest to actually remove empty deprecated parameters the moment the template is edited. This is rare anyway. This brings everything back into a stable, predictable state: There is still no checkbox for the deprecated parameter, but this is fine, because the parameter actually doesn't exist in the template. It behaves basically the same as an undocumented parameter. You can search for it's name and add it, which creates the checkbox, and puts it at the correct position.

TL;DR: The 3rd acceptance criteria doesn't work without either making the code a mess or significant investments (for a rather niche edge-case). 👈 @Lena_WMDE

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

[mediawiki/extensions/VisualEditor@master] Don't offer deprecated parameters when they aren't used

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

Minor updates from offline discussion:

  • Deprecated parameters will not be available like undocumented parameters, instead they will be blocked unless already present in the template invocation.
  • An existing deprecated parameter in the invocation is not treated specially just because its value is empty. We decided that an empty value is valid, may have an effect in template code, and should be rendered the same as if there were a non-empty value in the field.

Change 710051 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Don't offer deprecated parameters when they aren't used

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

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

[mediawiki/extensions/VisualEditor@master] Fix off-by-1 error when deprecated parameters are hidden

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

Change 709686 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Remove redundant ve.dm.MWTemplatePlaceholderModel.isEmpty

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

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

[mediawiki/extensions/VisualEditor@master] Split long ve.ui.MWTransclusionOutlineTemplateWidget methods

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

Change 710232 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Fix off-by-1 error when deprecated parameters are hidden

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

Change 709700 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Better name for ambiguous \"empty\" concept in the model

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

Change 709742 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/VisualEditor@master] Drop deprecated parameters when they are empty

Reason:

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

Change 709685 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/VisualEditor@master] [WIP] Initially hide empty deprecated parameters

Reason:

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

Change 709707 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Ignore default values as not being valuable as well

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

Change 710537 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Split long ve.ui.MWTransclusionOutlineTemplateWidget methods

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

Change 709694 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Rename and document definition of \"empty\" in ve.ui.MWParameterPage

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

WMDE-Fisch moved this task from Demo to Done on the WMDE-TechWish-Sprint-2021-08-04 board.