Page MenuHomePhabricator

Refactor Wikibase config variables used by MobileFrontend
Closed, ResolvedPublic3 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)

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterAdd extendSearchParams helper function
mediawiki/extensions/MobileFrontend : masterFurther split $wgMFDisplayWikibaseDescriptions
mediawiki/extensions/MobileFrontend : masterUse wgMFDisplayWikibaseDescriptions in client
mediawiki/extensions/MobileFrontend : masterExtend MobileContext#shouldShowWikibaseDescriptions
mediawiki/extensions/MobileFrontend : masterAdd wgMFDisplayWikibaseDescriptions to RL config
mediawiki/extensions/MobileFrontend : masterDeprecate $wgMFDisplayWikibaseDescription
mediawiki/extensions/MobileFrontend : masterDeprecate $wgMFUseWikibaseDescription
mediawiki/extensions/MobileFrontend : masterAdd MobileContext#shouldShowWikibaseDescriptions
mediawiki/extensions/MobileFrontend : masterRefactor Wikibase config variables to be less confusing

Event Timeline

Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptJun 27 2016, 9:17 PM

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 updated the task description. (Show Details)Jul 1 2016, 11:49 PM
jhobs set the point value for this task to 3.
jhobs removed jhobs as the assignee of this task.Jul 2 2016, 1:31 AM

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).

jhobs added a comment.EditedJul 14 2016, 7:10 PM

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

Jhernandez closed this task as Resolved.Jul 18 2016, 11:12 AM

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

Please don't forget to update the docs.

Seems done