Wikimedia Bugzilla is closed!

The problem comes from BetaFeaturesHooks::getPreferences, in the code

  $prefs[$key]['requirements']['skins'] = array_intersect( $prefs[$key]['requirements']['skins'], Skin::getAllowedSkins() );

because Skin::getAllowedSkins() is an array key-value and we want the key and not the value, so it should be used array_keys( Skin::getAllowedSkins() ).

I prepare the patch.

Change 160213 had a related patch set uploaded by Seb35:
Display the required skins

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

The current state on the Wikimedia project is without the names of the required skins. In the case it is wanted to keep the current state (for Wikimedia and/or for the master branch of BetaFeatures), this feature can be hidden with CSS, similarly to the browser requirements, by editing BetaFeatures/resources/betafeatures.less.

Fixing this bug will result in big red warnings being displayed under most BetaFeatures. The warnings also mention the hidden Minerva skin (e.g. for VisualEditor). Furthermore, they override the warnings about JavaScript being required.

The bug is real and the fix is correct, but I'm pretty sure people started relying on the bug being a feature, and fixing this brings out a few more bugs. The existing beta features need be reviewed in context of this first, and I have no idea who's going to do it. :/

Thanks for your comment, I didn’t paid attention to the JS warning who disappeared, but by further investigating:
1/ it is a feature to hide the warning "JS is required" when JS is enabled (see resources/betafeatures.js, there is the comment "We're here so JavaScript works"), and
2/ it was previously displayed because no skins were found in BetaFeaturesHooks::getPreferences (there are warnings in the debug log "[BetaFeatures] The visualeditor-enable BetaFeature has no valid skins installed."), which results not to add these beta features in the wgBetaFeaturesFeatures JS variables, which results betafeatures.js don’t loop into these beta features and don’t hide the JS warning (and don’t test if the browser is supported).

So the bug is deeper I first thought, and I find it worth fixing it. But I aggree the skins requirements are not necessary and it is a change from the current behaviour, so I hide them in the LESS file (to keep consistency with current behaviour).

Change 160213 merged by jenkins-bot:
Display the required skins

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

Hah, fascinating. Okay, let's do this then.

Should we mark this as resolved after merging the patch, or do you want to actually display this information?

I'm… confused.

The purpose of the Beta Feature warning section is to inform you why enabling it won't be useful to you.

I.e.:

* if you don't have JS working, tell you if it needs JS;
* if you don't use Vector, tell you if it only works for that skin;
* if you don't have Internet Explorer, tell you if it only works in that browser.

If none of these are met (i.e., it should work), the warnings should not appear.

The title of this bug suggests that this is not understood.

Yes, I didn’t describe exactly the bug: the allowed skins were never displayed in the HTML, neither when the current skin is an allowed skin, neither when the current skin is not an allowed skin.

With the patch, the JS warning is now correctly triggered. When a blacklist is specified for a beta feature, it is always added in the HTML.

For the skin, you are right, it should be displayed only when the current skin is not allowed. See next patch.

Change 161629 had a related patch set uploaded by Seb35:
Display allowed skins if and only if the current skin is not allowed

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

(In reply to Seb35 from comment #9)
> Yes, I didn’t describe exactly the bug: the allowed skins were never
> displayed in the HTML, neither when the current skin is an allowed skin,
> neither when the current skin is not an allowed skin.
> 
> With the patch, the JS warning is now correctly triggered. When a blacklist
> is specified for a beta feature, it is always added in the HTML.
> 
> For the skin, you are right, it should be displayed only when the current
> skin is not allowed. See next patch.

This looks good, thank you. :-)

Change 161629 merged by jenkins-bot:
Display allowed skins if and only if the current skin is not allowed

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

Thank you!