Page MenuHomePhabricator

Rename WPBSkinBlacklist
Closed, ResolvedPublic2 Estimated Story Points

Description

WikidataPageBanner has a config flag called WPBSkinBlacklist and a method isSkinBlacklisted.

  • Rename WPBSkinBlacklist to WPBSkinDisabled
  • Rename isSkinBlacklisted to isSkinDisabled
  • Update mediawiki-config

QA steps

Check https://en.wikivoyage.beta.wmflabs.org/wiki/Banner in all the different skins and ensure it shows.

QA Results - Beta

QA Results - Prod

Related Objects

StatusSubtypeAssignedTask
OpenNone
Resolvedovasileva

Event Timeline

Restricted Application added a project: Wikidata. · View Herald TranscriptJun 6 2020, 11:19 PM
ovasileva triaged this task as Medium priority.Jun 9 2020, 3:42 PM
ovasileva set the point value for this task to 2.Jul 1 2020, 4:41 PM

Change 609141 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/WikidataPageBanner@master] Rename WPBSkinBlacklist to WPBSkinEnabled and invert logic

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikidataPageBanner/ /609141

Peter.ovchyn updated the task description. (Show Details)Jul 2 2020, 12:13 PM

@Jdlrobson I renamed opt name and inverted logic accordingly. Is there any instruction on how to change mediawiki-config ?

Reedy added a comment.Jul 2 2020, 12:40 PM

@Jdlrobson I renamed opt name and inverted logic accordingly. Is there any instruction on how to change mediawiki-config ?

So with how your patch current is, it's pretty difficult.

In Wikimedia production, we generally support two different versions of MW at once, for most of a week. So when you explicitly rename things, without leaving the old version, it's potentially hard to migrate. Depending on how it's used

For migrationary purposes (of config vars, at least. Though, you could argue there should be the same for function renames as per the deprecation policy), it's easier to add the new one, make it have prescedence in the code, wait for the new code to be deployed everwhere and stable, update medawiki-config, and then remove the old variable at some point later.

However, looking at the usage in mediawiki-config, it's a no-op and the same as default, currently:

'wgWPBSkinBlacklist' => [
	'default' => [],
],

So that should really just be removed from InitialiseSettings.php, as it serves no value.

However, your inverting the logic, which then means we've presumably go to list all the skins. Does that really make sense?

You can contribute patches to the operations/mediawiki-config like any other repo. Deployment just requires being included in Backport windows

I can confirm the config is empty so no need to worry about mediawiki config. Have flagged the same concern about inverting logic on the associated gerrit patch.

@Jdlrobson I think the root of issue is in task description and naming. WPBSkinEnabled and isSkinEnabled is something totally opposite to WPBSkinBlacklist and isSkinBlacklisted. I'd rather offer to rename that to WPBSkinDisabled and isSkinDisabled? What do yo think?

Yes, the issue started here. The task description inverted the logic.

Rename WPBSkinBlacklist to WPBSkinEnabled
Rename isSkinBlacklisted to isSkinEnabled

You should however retain the logic in the patch. That's isSkinDisabled() for the method and WPBSkinDisabled for the config variable.

Change 609490 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[operations/mediawiki-config@master] Rename WPBSkinBlacklist to WPBSkinDisabled

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

Peter.ovchyn updated the task description. (Show Details)Jul 3 2020, 8:25 PM

Change 609141 merged by jenkins-bot:
[mediawiki/extensions/WikidataPageBanner@master] Rename WPBSkinBlacklist and isSkinBlacklisted to WPBSkinDisabled and isSkinDisabled respectively

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

Jdlrobson added a subscriber: Peter.ovchyn.

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

Check https://en.wikivoyage.beta.wmflabs.org/wiki/Banner in all the different skins and ensure it shows.

✅ AC1: New Vector


✅ AC2: Legacy Vector

✅ AC3: Minerva Neue

✅ AC4: Modern

✅ AC5: MonoBook

✅ AC6: Timeless

Edtadros updated the task description. (Show Details)Jul 25 2020, 5:12 PM

Change 609490 merged by jenkins-bot:
[operations/mediawiki-config@master] Remove WPBSkinBlacklist

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

Mentioned in SAL (#wikimedia-operations) [2020-07-27T18:13:28Z] <catrope@deploy1001> Synchronized wmf-config/InitialiseSettings.php: Remove WPBSkinBlacklist (T254675) (duration: 00m 57s)

Edtadros reassigned this task from Edtadros to ovasileva.Aug 5 2020, 4:01 PM
Edtadros added a subscriber: Edtadros.

Test Result - Prod

Status: ✅ PASS
Environment: testwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

Check https://test.wikipedia.org/wiki/Banner in all the different skins and ensure it shows.

✅ AC1: New Vector


✅ AC2: Legacy Vector

✅ AC3: Minerva Neue

✅ AC4: Modern

✅ AC5: MonoBook

✅ AC6: Timeless

Edtadros updated the task description. (Show Details)Aug 5 2020, 4:02 PM
ovasileva closed this task as Resolved.Aug 6 2020, 2:04 PM

Looks good!

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:41 PM