Page MenuHomePhabricator

Refactor Wikibase config variables used by MobileFrontend
Closed, ResolvedPublic3 Estimated Story Points

Description

Spawned off of T138738 to reduce confusion around the meaning of wikibase-related config variables.

AC

  • wgMFUseWikibaseDescription -> wgMFUseWikibase
  • Fold wgMFDisplayWikibaseDescription and wgMFDisplayWikibaseDescriptionsAsTaglines into new wgMFDisplayWikibaseDescriptions, changing it from a boolean to an object describing which features on which to display descriptions
  • Deprecation of wgMFUseWikibaseDescription and wgMFDisplayWikibaseDescription

[ ] Corresponding variables changed in mediawiki-config as well (To be done in follow-up task when variables are fully deprecated)

Event Timeline

Change 296342 had a related patch set uploaded (by Jhobs):
Refactor wikibase config variables to be less confusing

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

Jdlrobson triaged this task as Medium priority.Jun 28 2016, 5:10 PM

Those names are still shit. After discussing with @phuedx and @Jdlrobson, I'm gonna try a few different ways and see what sticks, then update the task accordingly.

jhobs set the point value for this task to 3.

Freeing this up as I'll be out of town next week.

^ Fixing a failing unit test case shouldn't block an initial review…

phuedx added a subscriber: bmansurov.

@bmansurov: I've tried to address your last (largest) comment in the latest patch set. Thanks for the feedback.

Change 298261 had a related patch set uploaded (by Phuedx):
Add MobileContext#shouldShowWikibaseDescriptions

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

Change 298262 had a related patch set uploaded (by Phuedx):
Extend MobileContext#shouldShowWikibaseDescriptions

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

Change 298263 had a related patch set uploaded (by Phuedx):
Deprecate $wgMFUseWikibaseDescription

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

There are two more changes to add to this sequence: the introduction of $wgMFDisplayWikibaseDescriptions and the extendSearchParams function, both of which I'm extracting currently…

Change 298426 had a related patch set uploaded (by Phuedx):
Deprecate $wgMFDisplayWikibaseDescription

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

Change 298430 had a related patch set uploaded (by Phuedx):
Add wgMFDisplayWikibaseDescriptions to RL config

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

Change 298432 had a related patch set uploaded (by Phuedx):
Use wgMFDisplayWikibaseDescriptions in client

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

Change 298433 had a related patch set uploaded (by Phuedx):
Further split $wgMFDisplayWikibaseDescriptions

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

Change 298434 had a related patch set uploaded (by Phuedx):
Add extendSearchParams helper function

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

There are a few open questions on the patchsets (see +1s and -1s) but nothing too major. I hope we can wrap this work up Wednesday - we may want to hangout after standup to do so.

Change 296342 abandoned by Jdlrobson:
Refactor Wikibase config variables to be less confusing

Reason:
This has now been split up into https://gerrit.wikimedia.org/r/298262 and friends - let's carry conversation over there.

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

Change 298261 merged by jenkins-bot:
Add MobileContext#shouldShowWikibaseDescriptions

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

@Jdlrobson: I've responded to your comment. Perhaps we can nip this in the bud immediately before/after standup.

Change 298262 merged by jenkins-bot:
Extend MobileContext#shouldShowWikibaseDescriptions

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

Change 298263 merged by jenkins-bot:
Deprecate $wgMFUseWikibaseDescription

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

Change 298426 merged by jenkins-bot:
Deprecate $wgMFDisplayWikibaseDescription

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

Change 298430 merged by jenkins-bot:
Add wgMFDisplayWikibaseDescriptions to RL config

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

I'm going to carefully go over the Wikimedia config today to ensure this doesn't interrupt service on any live wikis given this discussion. Expect subtasks! :)

Change 298432 merged by jenkins-bot:
Use wgMFDisplayWikibaseDescriptions in client

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

So I had a super close look at this.
We only introduced MFDisplayWikibaseDescriptionsAsTaglines June 27th. Before this change enabling $wgMFUseWikibaseDescription would control the Wikidata description tagline. We've now deprecated this config variable 2 weeks later in favour of $wgMFDisplayWikibaseDescriptions = [ 'tagline' => true ].

The result of this is any site using the sole config variable $wgMFUseWikibaseDescription = true from before June 27th to show taglines who upgrades will now lose their tagline unless they turn on $wgMFDisplayWikibaseDescriptions immediately.

This is all very confusing and I doubt many people are using Wikibase for this purpose so I think we should just accept this is the case.

Another engineer should feel free to sign off knowing the above BUT they should open a new task for next sprint to remove all the deprecated config variables as soon as these changes are live everywhere (from 21st July 2016).

So I had a super close look at this.
We only introduced MFDisplayWikibaseDescriptionsAsTaglines June 27th. Before this change enabling $wgMFUseWikibaseDescription would control the Wikidata description tagline. We've now deprecated this config variable 2 weeks later in favour of $wgMFDisplayWikibaseDescriptions = [ 'tagline' => true ].

Just for context: This was because it was expected that there would be a slight overhaul on these config variables shortly after introducing $wgMFDisplayWikibaseDescriptionsAsTaglines, but it was not expected that the most appropriate solution would be to use $wgMFDisplayWikibaseDescriptions as an object (although it was known to be a potential solution). Thus, we didn't initially think that we'd be introducing the taglines variable and then immediately removing it. It took more investigation to determine that was the path forward. I think the slight confusion here in terms of refactoring a new variable immediately is understandable and permissible given that we're doing it in such a close timeframe and it was announced in the original task that a follow-up refactoring would be done.

Another engineer should feel free to sign off knowing the above BUT they should open a new task for next sprint to remove all the deprecated config variables as soon as these changes are live everywhere (from 21st July 2016).

As someone who contributed a fair amount to this task, I'd rather leave this to someone with fresher eyes.

I nominate @Jhernandez given his distance from the task for sign off.

@Jdlrobson: 298434: Add extendSearchParams helper function and its dependency need to re-reviewed as they didn't get merged for one reason or another…

Change 298433 merged by jenkins-bot:
Further split $wgMFDisplayWikibaseDescriptions

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

Change 298434 merged by jenkins-bot:
Add extendSearchParams helper function

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

I've updated https://www.mediawiki.org/wiki/Extension:MobileFrontend docs.

Please don't forget to update the docs.

Seems done