Deprecate Skin::bottomScripts and SkinMustache::tailElement in favor of OutputPage::tailElement
Closed, ResolvedPublic
Actions

Description

The method Skin::bottomScripts is a wrapper for OutputPage but runs the hook SkinAfterBottomScripts using the current skin.

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/611328/5/includes/skins/SkinMustache.php#58 introduces SkinMustache::tailElement.

We should consider updating OutputPage with a tailElement method that takes a parameter skin and fold the code inside Skin::bottomScripts in light of T140664#6287065. In the long run, OutputPage will be able to call this method itself and we can remove communication between Skin to OutputPage and keep that one way.

The fact that this method on Skin exists means it can be overridden and do things such as not execute client side JS which adding unnecessary complexity.

Details

Related Changes in Gerrit:

	Subject	Repo	Branch	Lines +/-
	Soft deprecate Skin::bottomScripts	mediawiki/core	master	+14 -3
	Introduce OutputPage::tailElement	mediawiki/core	master	+23 -22

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Open		None	T263213 Data shared between SkinMustache and SkinTemplate should be common
		Resolved		Ammarpad	T257704 Deprecate Skin::bottomScripts and SkinMustache::tailElement in favor of OutputPage::tailElement

Event Timeline

Jdlrobson created this task.Jul 10 2020, 8:17 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 10 2020, 8:17 PM

Jdlrobson renamed this task from Deprecate Skin::bottomScripts and OutputPage::tailElement ? to Deprecate Skin::bottomScripts and add OutputPage::tailElement ?.Jul 10 2020, 8:17 PM

Jdlrobson updated the task description. (Show Details)

Jdlrobson moved this task from Phase 3 - standardise template data to Phase 2 - Deprecate and remove Skin methods for readability on the MediaWiki-Core-Skin-Architecture board.Jul 15 2020, 8:47 PM

Jdlrobson moved this task from Backlog to Skinning system on the MediaWiki-User-Interface board.Aug 27 2020, 4:48 PM

Jdlrobson triaged this task as Medium priority.Sep 4 2020, 9:28 PM

@Krinkle thoughts on this? Any objections to making this a recommendation rather than question? This should help with separation of OutputPage and Skin which helps towards T140664.

Jdlrobson renamed this task from Deprecate Skin::bottomScripts and add OutputPage::tailElement ? to Deprecate Skin::bottomScripts and SkinMustache::tailElement in favor of OutputPage::tailElement.Oct 2 2020, 7:55 PM

Jdlrobson added subscribers: Ammarpad, Mainframe98, Isarra, matmarex.Oct 16 2020, 11:44 PM

Jdlrobson added a project: MW-1.36-release.Oct 29 2020, 5:10 PM

Jdlrobson moved this task from Phase 2 - Deprecate and remove Skin methods for readability to It's complicated on the MediaWiki-Core-Skin-Architecture board.

@Jdlrobson I'm not sure I understand. Are you proposing to move the code from Skin to OutputPage, and then have the Skin call the OutputPage method, passing itself as a parameter, and then running a hook on-demand? That seems opposite of a one-way communication where OutputPage creates a skin and injects relevant data. How would this work long-term? What value does the interim stage add?

How would this work long-term?

I am suggesting all this code is moved out of Skin. Per description "In the long run, OutputPage will be able to call this method itself and we can remove communication between Skin to OutputPage and keep that one way."

The Skin::tailElement better belongs in OutputPage. On the long run that means Skin::generateHTML would return the HTML inside the body and would not call the OutputPage methods as it currently does to generate the closing and opening body tags and head:

My expectation is that in future access to OutputPage by Skin will not be necessary and OutputPage::output will use a skin instance to generate only the contents of the body tag.

Jdlrobson moved this task from It's complicated to Phase 3 - standardise template data on the MediaWiki-Core-Skin-Architecture board.Nov 5 2020, 7:53 PM

Jdlrobson raised the priority of this task from Medium to High.Nov 19 2020, 7:34 PM

Jdlrobson moved this task from Phase 3 - standardise template data to It's complicated on the MediaWiki-Core-Skin-Architecture board.Feb 26 2021, 3:19 AM

Is there anything stopping us from doing this in 1.36 or should we punt to 1.37?

Being bold per Jon.

DannyS712 moved this task from Blocker to To deprecate or remove on the MW-1.37-release board.Jul 7 2021, 10:21 PM

Jdlrobson moved this task from It's complicated to PHP internals on the MediaWiki-Core-Skin-Architecture board.Jul 23 2021, 12:21 AM

Jdlrobson moved this task from PHP internals to 1.37 release on the MediaWiki-Core-Skin-Architecture board.

Jdlrobson added a parent task: T263213: Data shared between SkinMustache and SkinTemplate should be common.Jul 30 2021, 7:19 PM

Jdlrobson mentioned this in T263213: Data shared between SkinMustache and SkinTemplate should be common.

Ammarpad claimed this task.Jul 31 2021, 10:00 AM

This will be needed for T263213 as I want to deprecate the BaseTemplate reporttime variable.

@Ammarpad, you've claimed this task for two weeks now. Do you have a patch ready?

I will work on it. It's low-priority MediaWiki-Core-Skin-Architecture work that's why

Hey there. This task is proposed as a blocker to MediaWiki 1.37, which will be cut in less than three weeks' time. Please consider whether this will make that deadline, and if not, move it to block the MediaWiki 1.38 release (MW-1.38-release) or remove as a blocker entirely.