Page MenuHomePhabricator

Deploy PageBanner extension on English Wikivoyage project
Closed, ResolvedPublic

Description

Track the security review status of the deployment of WikidataPageBanner extension on English Wikivoyage as required by https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment#Review_for_deployment
The extension has been developed as a GSoC proposal for the wikivoyage community with English Wikivoyage being the first wiki to adopt it.
This https://phabricator.wikimedia.org/T77925 summarizes the desire for the extension on English Wikivoage.
The following links tell about the discussions that took place regarding the extension and its refinement:

Major bugs have been resolved from Wikidata-Page-Banner workboard. This task will track blockers during the review process.
WikidataPageBanner Extension is documented at http://mediawiki.org/wiki/Extension:WikidataPageBanner

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson raised the priority of this task from to Needs Triage.May 4 2015, 5:21 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task to In discussion on the Wikidata-Page-Banner board.
Jdlrobson subscribed.

Now might be a good time to reach out to the community and understand the best approach for roll out.
The community may want to a beta feature for example to allow them some time to test and gain confidence.

@Nicolas_Raoul what would you recommend?

How about telling the community:

  1. The staging site is at http://pagebanner.wmflabs.org/wiki/Testp1 please test it in various cases and report any problem
  2. If no problem is found, it will be rolled out on 2015/XX/YY

That would set a sense of reality/urgency and a clear schedule letting people some time to do their tests.

I guess a selective roll out (only 50 articles for instance) is not possible, right?

Provided there are plans in place for when to roll back (e.g. turn off extension) that would be fine.
We also need to be sure that the user never sees duplicate banners. I will add a task to think about this.

After discussion with @Nicolas_Raoul on meeting of July 1, the strategy decided to deploy the extension is to first test the extension, on a few test pages on wikivoyage, then we can go ahead and announce about the same on traveller's pub, seeking the help of User:Traveller100 who has a bot which has done work with respect to adding {{pagebanner}} on wikivoyage pages. The same bot can be used to modify the syntax of {{pagebanner|...}} to {{PAGEBANNER:...}} to suit the extension once it gets a go.

I guess a selective roll out (only 50 articles for instance) is not possible, right?

A selective roll out would be possible by leaving the variable $wgPBImage empty, in which case it'll add no banners by default

I'll be unavailable for the next week but please be sure to capture any issues the community raises in some Phabricator tasks so we can get them resolved!

@Krenair I have informed @Sumit about this yes and he is in the process of kicking this off. We are aiming for a launch at the end of GSoc.

(I should add we already have community support so in a way this process has already been kicked off)

Sumit set Security to None.

Now that the Wikivoyage community is unanimous in calling for Sumit's new extension to be deployed, and that the task is listed at https://phabricator.wikimedia.org/T33235, I guess we just need to wait for anyone with the right permissions to actually deploy it?
I guess code review is not necessary as @Jdlrobson has performed reviews already?

@Nicolas_Raoul, deployment review is different from code-review and needs to take place, to check for and fix any vulnerabilities in general that might be created by the extension on deployment. See https://www.mediawiki.org/wiki/Writing_an_extension_for_deployment#Review_for_deployment

@kaldari and @csteipp would you mind reviewing this extension with respect to a deployment to English Wikivoyage?

I'm out this week, but I should be able to get to it next week.

Do you have an external driver on this?

Next week would work fine @csteipp - thank you!

17th August 2015 is the suggested pencils down date for Google Summer of Code and I'd like to see my student achieve his goal by then so I want to give him as much time as he needs to respond to bug fixes for example security fixes.

Jdlrobson renamed this task from Launch PageBanner extension on Wikivoyage projects to Deploy PageBanner extension on English Wikivoyage project.Jul 22 2015, 4:15 PM

Quick code review:

  • Configuration settings should be prefixed, a name like $wgStandardSizes is too general.
  • Should use extension.json - https://www.mediawiki.org/wiki/Manual:Extension_registration
  • !defined( 'MEDIAWIKI' ) check not needed unless you plan to support pre-MW 1.24
  • This is adding a new parser function, has it been reviewed by Parsoid/VE teams?
  • Shouldn't call OOUI\Theme::setSingleton directly.
  • There are some hacks to work around TOC generation, this should probably be blocked on T105520?
  • Use "self::" instead of "static::" unless necessary
  • ext.WikidataPageBanner has a dependency upon oojs-ui, but it's a style only module?

Quick code review:

  • Configuration settings should be prefixed, a name like $wgStandardSizes is too general.

>T106586

> T106587

  • !defined( 'MEDIAWIKI' ) check not needed unless you plan to support pre-MW 1.24

I'm not sure whether this support is needed. I will leave it to @Sumit to decide.

  • This is adding a new parser function, has it been reviewed by Parsoid/VE teams?

Have cc'ed @ssastry to take a look.

  • Shouldn't call OOUI\Theme::setSingleton directly.

Could you elaborate on proper usage here? Are there any docs which provide best usage? (Note I did ask @matmarex to review this)

  • There are some hacks to work around TOC generation, this should probably be blocked on T105520?

Maybe... would be great if you could chip in on that bug, if there are any reasons, it is the way it is, but I don't personally think this needs to block deployment.

  • Use "self::" instead of "static::" unless necessary

From what I remember there were some problems getting the unit tests to pass with the mocks @Sumit was using whilst using self. @Sumit maybe you could post a patch changing back to self:: to see if we can debug that problem?

  • ext.WikidataPageBanner has a dependency upon oojs-ui, but it's a style only module?

oojs-ui-styles is probably the answer =>T106588

  • Shouldn't call OOUI\Theme::setSingleton directly.

Could you elaborate on proper usage here? Are there any docs which provide best usage? (Note I did ask @matmarex to review this)

Indeed, calling OOUI\Theme::setSingleton() is entirely unnecessary if you also call OutputPage::enableOOUI() (which you usually should when using server-side OOUI widgets, and which this extension correctly does), as the latter calls the former. I reviewed https://gerrit.wikimedia.org/r/#/c/215029/ which added the OutputPage::enableOOUI() call, I did not review or see https://gerrit.wikimedia.org/r/#/c/221459/ which needlessly added the OOUI\Theme::setSingleton() call. If something does actually break if you remove it, please file a bug and assign to me, and I'll debug it.


Aside: there is some inherent complexity here, for which OutputPage::enableOOUI() may be a leaky abstraction. It does two separate things.

The first is:

		OOUI\Theme::setSingleton( new OOUI\MediaWikiTheme() );
		OOUI\Element::setDefaultDir( $this->getLanguage()->getDir() );

This part sets up some global configuration which is required to stringify OOUI widgets to HTML. Theme::setSingleton call is required (failing to call it will result in an exception later on), Element::setDefaultDir is optional (only needed when you generate widgets in RTL language). It is valid to instantiate OOUI widgets before calling either of these methods, as long as you don't stringify them (either by calling ->toString() explicitly, or concatenating with a string implicitly by using the . operator or any of PHP methods which require or produce strings, like implode()).

WikidataPageBanner indeed instantiates a IconWidget without calling these methods (or OutputPage::enableOOUI()), in a parser hook – but this is okay, because you store the object in the ParserOutput without stringifying (with setProperty), and call OutputPage::enableOOUI() before you stringify later from a different hook.

The second thing OutputPage::enableOOUI() does is simpler:

		$this->addModuleStyles( array(
			'oojs-ui.styles',
			'oojs-ui.styles.icons',
			'oojs-ui.styles.indicators',
			'oojs-ui.styles.textures',
			'mediawiki.widgets.styles',
		) );

This just add the various styles to the output, that are required to properly render the HTML of OOUI widgets.

It is not inconceivable that you might need to do one of these things without the other (e.g. in more complex parser hooks, that actually generate output), in which case you might want to call OOUI\Theme::setSingleton yourself. But apparently not in this case.

(I should probably turn this comment into proper documentation somewhere.)

T107941 tracks what is needed to support this in Parsoid. With a cursory look at the extension, I had misunderstood its functionality. Tim reviewed it today and updated it. Parsoid doesn't support display hooks right now and this extension relies on that. We can deal with this later. James also said we don't have to resolve this right now for VE support. So, no blockers from our end wrt deployment to WikiVoyage. But, when it comes to turning on Parsoid HTML read view or VE editing support on WikiVoyage, this will need fixing.

It seems like we are in a state to deploy this extension?
If there are no objections I will find someone to deploy next week.

I've talked to @greg and @kaldari and the current proposal is to get this deployed Wed 12th August at 9am-10am PST.

@Nicolas_Raoul I would want you there for deployment to make sure it all goes smoothly and nothing breaks on wikivoyage - you know the site better than all of us.
@Sumit if I understand correctly we can deploy the extension, and then slowly roll out config changes/switch over templates - so worst case scenario if there are any bugs we can leave the extension deployed but unused? @Sumit, I know it's early for you but it would be great to have you around for some of the deployment :)

Please let me know if this works or doesn't work for either of you so we can plan accordingly!

I submitted a few small patches for code quality things. I haven't tested it myself, but is image usage being tracked in the imagelinks/globalimagelinks (through GlobalUsage) table?

Good question @Legoktm .. unless it happens automagically I suspect not. Can you open a task with instructions on how to check / implement that?

Thanks for the patches - I will take a look!

@Sumit if I understand correctly we can deploy the extension, and then slowly roll out config changes/switch over templates - so worst case scenario if there are any bugs we can leave the extension deployed but unused?

Yes, http://pagebanner.wmflabs.org/w/index.php?title=Template:Pagebanner&action=edit contains most of the code that makes use of Extension:Pagebanner in a template and at the same time preserves the way http://en.wikivoyage.org/wiki/Template:Pagebanner is used on pages. After enabling the extension, it can be tested on a few select pages before modifying http://en.wikivoyage.org/wiki/Template:Pagebanner. In the worst case if something goes wrong, Template:Pagebanner on English wikivoyage could be reverted to the previous version till things resolve.

@Jdlrobson
Design feedback - I think page action icons (edit and watch) should be below the banner. The text can wrap around them, so there won't be a lot of wasted space.

wikivoyage-11.png (549×1 px, 282 KB)

To avoid unexpected failures in production there should be automated test for its Wikibase dependency.

Change 231032 had a related patch set uploaded (by Jdlrobson):
Deploy WikidataPageBanner extension

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

Change 231032 merged by jenkins-bot:
Deploy WikidataPageBanner extension

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

Awesome. Thanks @kaldari !
Next step is to enable all the features T108839 - am collecting blocking tasks on that card.