Page MenuHomePhabricator

Put skin preference in the correct place
Closed, ResolvedPublic2 Estimated Story PointsSpike

Description

In what order do user preferences present themselves on a given tab of Special:Preferences? In T242381 and previously in the Popups codebase, the order of preferences has been important. The current desired presentation is skins, (Vector) skin preferences, reading preferences:

In the beta cluster this was not the case.
We originally decided to do a spike to explore this, but after a little further investigation it seems the easiest solution is to nest the new skin options inside the skin preferences as skin preferences can be nested magically using '/' in the key

Acceptance criteria

  • Use the rendering/skin/skin-prefs instead of rendering/skin-prefs as the section for skin preferences. This will guarantee the skin preferences appear after skin related settings while not interfering with the existing Popups code.

QA steps

See T242381

Developer notes

A patch has already been written proving such a solution is viable (see above). It involves a minor change to core and Vector.
There is also an opportunity here to make Monobook use the same section for its "responsive monobook" option but that should be considered outside the scope of estimation and non-blocking since Monobook is not maintained by this team.

Original spike context

This spike seeks to answer firstly: for all wikis in production, how do Vector skin preferences present themselves? If Vector's onGetPreferences() hook is always executed after Popups' hook, no changes will be necessary. We think that's the case as Vector is the last entry on extension-list.

HOWEVER, if the order cannot be guaranteed, how should we fix that?

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Spike". · View Herald TranscriptFeb 25 2020, 9:22 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 574542 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/Popups@master] [Special:Preferences] [PHP] Position Popups' prefs below skin prefs

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

Change 574542 abandoned by Niedzielski:
[Special:Preferences] [PHP] Position Popups' prefs below skin prefs

Reason:
Abandoning until needed.

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

Change 575102 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Make sure Vector skin preferences always follows skin

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

Thinking about this some more the easiest way to do this is to nest the preference under the skin itself. It's position is guaranteed to be above Popups. I've demonstrated this in https://gerrit.wikimedia.org/r/575102

ovasileva raised the priority of this task from Medium to High.Feb 28 2020, 1:10 PM
Jdlrobson renamed this task from [Spike] In what order do user preferences present themselves? to Put skin preference in the correct place.Feb 28 2020, 8:00 PM
Jdlrobson removed a project: Spike.
Jdlrobson updated the task description. (Show Details)

Change 575102 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Make sure Vector skin preferences always follows skin

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

This spike seeks to answer firstly: for all wikis in production, how do Vector skin preferences present themselves? If Vector's onGetPreferences() hook is always executed after Popups' hook, no changes will be necessary. We think that's the case as Vector is the last entry on extension-list.

HOWEVER, if the order cannot be guaranteed, how should we fix that?


That is for sure inconvenient, but I don't think it's as crazy as you make it seem. The code is weird but the result is well-defined, and it's unlikely to break terribly.

MediaWiki uses ordered associative arrays in other places too, so it's not entirely outlandish (in our small ecosystem, at least), e.g. when defining the order of menu items in skins (https://github.com/wikimedia/mediawiki-extensions-VisualEditor/blob/cc557cfef508fdf3f1388b4c7809ab3b79de2dcb/includes/VisualEditorHooks.php#L494 – we used a different approach to affect the order, yours actually seems a bit neater).

As long as it is backwards-compatible, that seems like a nice idea. I've seen similar approaches elsewhere (e.g. if you've ever looked at Phabricator's code… it orders user settings in a similar way, by defining numbers which are compared to each other: https://github.com/wikimedia/phabricator-extensions/commit/9f897dbcb9cbebea42ad716633da3bcf83e315de#diff-e6f99b1e58a88d7d37a84495d7573452R19 (no, I don't know what that preference does, apparently it does nothing – I was looking at that code because I was trying to find out :D)).

Code for this is part of HTMLForm, so any changes here would potentially affect not only preferences, but also hundreds of places in core and extensions – please tread carefully if you end up working on this.

Thinking about this some more the easiest way to do this is to nest the preference under the skin itself. It's position is guaranteed to be above Popups. I've demonstrated this in https://gerrit.wikimedia.org/r/575102

That's cool, and seems to work well. Nested sections are supported by HTMLForm, but it hasn't been used for preferences before, so this code isn't really battle-tested like the rest of the system. It's certainly the most sensible way to get the order right though.

I was worried slightly if this is going to render right in preferences (but it seems fine), and whether Special:GlobalPreferences will handle it right (but it also seems fine – although it has another funny bug your preference triggers, T246491).

ovasileva set the point value for this task to 2.Mar 2 2020, 6:25 PM
Jdlrobson updated the task description. (Show Details)Mar 2 2020, 8:13 PM

We decided to go for the straightforward fix here @matmarex rather than the risky HTMLForm changes. Thanks for the code review and the feedback.

phuedx claimed this task.Mar 4 2020, 6:04 PM
ovasileva closed this task as Resolved.Mar 5 2020, 2:17 PM

This LGTM.

I note that there's a TODO attached to HTMLForm around documenting the behaviour of the section property (see https://gerrit.wikimedia.org/g/mediawiki/core/+/680757355a84d0ce44d5312d5c0f1740adfc0a77/includes/htmlform/HTMLForm.php#129). AFAICT this is a novel usage of the property (see https://codesearch.wmflabs.org/search/?q=%27section%27%20\%3D\%3E%20%27[^%27]*\%2F[^%27]*\%2F) so I see no reason why documentation is out of scope for this task. @Jdlrobson, @matmarex?

phuedx removed phuedx as the assignee of this task.Mar 5 2020, 2:23 PM
phuedx added a subscriber: phuedx.
phuedx reopened this task as Open.Mar 5 2020, 6:20 PM
phuedx added a comment.Mar 5 2020, 7:43 PM

@Jdlrobson: Thanks! Could you also add that line to the DocBlock for HTMLForm and remove the @todo?

The link is already in HTMLForm - https://github.com/wikimedia/mediawiki/blob/master/includes/htmlform/HTMLForm.php#L39.

To be honest I'm not sure when and why we began repeating already documented information - one source of truth that can be translated - mediawiki.org should suffice IMO. Once we've worked out where the canonical source should be (i'm guessing wiki) we can port that documentation.

It's not clear to me where the documentation canonically should live. JDForrester suggests the code is the canonical source. This looks like more work than I anticipated and is not documentation I touched during this change so I'd like to suggest pulling this out into a separate task and having that conversation. This doesn't seem like a priority for me at least right now given the team's priorities.

The discussion of the pros and cons of duplicating documentation between the codebase and mediawiki.org is an important one. I agree that it's out of scope for this task.

This looks like more work than I anticipated and is not documentation I touched during this change so I'd like to suggest pulling this out into a separate task and having that conversation.

Initially, I was confident that copying the updated documentation for that one property, formatting it correctly, removing the @todo at the bottom of the DocBlock, and having a peer review would require less effort overall than creating a new task and shepherding it through our process. Having just looked into how the form on Special:Preferences is built, for example, I agree that this is worthy of a separate task.

phuedx closed this task as Resolved.Mar 9 2020, 11:50 AM
phuedx claimed this task.

☝️💪