Page MenuHomePhabricator

Move OutputPage::setTarget() call from BeforePageDisplay to an earlier hook
Open, LowPublic



Having confidence in progress made on T127268 is dependent on Logstash telemetry from violations, which would currently be too spammy due to Gadgets being loaded unconditionally and violating the targets filter by default without a way for users to fix that. (T171180).

Analysis from @TheDJ shows that the way Gadgets enqueues the enabled Gadget modules could easily filter by target, but this is currently not working as intended because the OutputPage::mTarget member is not set by the time Gadgets queues its modules.

Further progress on the matter is blocked on moving the setTarget() call in MobileFrontend to an earlier hook. This is expected to be uncontroversial given the target state is declarative and aside from ResourceLoader (which only consumes it upon skin rendering) nothing reads from this variable.

Developer notes

  • Existing code inside the onBeforePageDisplay hook setting target is removed.
  • The following code is added to onOutputPageBeforeHTML in includes/MobileFrontend.hooks.php:
		if ( $context->shouldDisplayMobileView() ) {
						// set the mobile target
						if ( !$context->isBlacklistedPage() ) {
							$out->setTarget( 'mobile' );

QA steps

  • When I visit a mobile page all requests for JavaScript and stylesheets append the query string parameter target=mobile

Acceptance criteria / Sign off steps

  • There has been no increase in JS/CSS related to this change
  • MediaWiki:Mobile.css/js is loaded on the mobile domain
  • MediaWiki:Common.css/js is not loaded on the mobile domain

Event Timeline

Krinkle created this task.Jan 9 2019, 5:29 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 9 2019, 5:29 AM
Jdlrobson triaged this task as Low priority.Jan 9 2019, 9:25 PM

Further progress on the matter is blocked on moving the setTarget() call in MobileFrontend to an earlier hook.

Which hook were you thinking of? onBeforePageDisplay is the one we currently use. I don't know exactly what early needs, but this should be non-controversial and easy to get done once we know where to put it.

BeforePageDisplay is the very last (relevant) hook that can be used to change the state of OutputPage. It is most commonly used to queue modules.

MobileFrontend needs to be changed to change the "target" state from an earlier hook (any will do, can be sightly earlier, or very early; doesn't matter) so that e.g. Gadgets can queue its modules in a way that varies by the target.

Working on this is just blocked on deciding which hook to use - any straw-man proposals off the top of your head? Is hook order documented anywhere?

Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Analysis to Triaged but Future on the Readers-Web-Backlog board.
Krinkle added a comment.EditedFeb 19 2019, 1:13 PM

Hm.. I couldn't tell whether that actually runs first, nor whether it only runs once. But yeah, if that works, let's do it!

Literally any hook that runs on page views before BeforePageDisplay will do.