Page MenuHomePhabricator

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

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.

Event Timeline

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 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 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 raised the priority of this task from Medium to High.Nov 19 2020, 7:34 PM

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

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.

Change 715061 had a related patch set uploaded (by Ammarpad; author: Ammarpad):

[mediawiki/core@master] Introduce OutputPage::tailElement

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

Change 715061 merged by jenkins-bot:

[mediawiki/core@master] Introduce OutputPage::tailElement

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

Change 715557 had a related patch set uploaded (by Ammarpad; author: Ammarpad):

[mediawiki/core@master] Merge Skin::bottomScripts to OutputPage::getBottomScripts

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

@Jdlrobson have you descoped this? See the second patch

Change 715557 merged by jenkins-bot:

[mediawiki/core@master] Soft deprecate Skin::bottomScripts

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