Last modified: 2013-12-17 09:32:19 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T60137, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 58137 - Kill SkinTemplateOutputPageBeforeExec hook
Kill SkinTemplateOutputPageBeforeExec hook
Status: NEW
Product: MediaWiki
Classification: Unclassified
Interface (Other open bugs)
1.23.0
All All
: Low normal (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: code_quality
  Show dependency treegraph
 
Reported: 2013-12-07 00:43 UTC by Jon
Modified: 2013-12-17 09:32 UTC (History)
7 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Jon 2013-12-07 00:43:06 UTC
This hook seems to be very generic and very dangerous - it allows any extension to redefine template variables in a skin and as a result change the behaviour of a skin. We are running into various problems in the skin in MobileFrontend where language_urls in the context of our skin means a list of languages but that seems to differ from core.

I believe we should understand the existing uses of it and provide more generic hooks to guide usage.

e.g. in the case of languages a SkinAddLanguageUrl hook would be better as this would also change the return result of getLanguages.

Can someone make a case for the existing use cases of this hook and why the hook runs so close to the outputting of the HTML as I'm struggling to see why a generic hook like this would be better then a more specific one.
Comment 1 Kunal Mehta (Legoktm) 2013-12-07 00:46:15 UTC
For reference: [[mw:Category:SkinTemplateOutputPageBeforeExec extensions]]
Comment 2 Siebrand Mazeland 2013-12-07 01:24:10 UTC
Around 60 usages identified in MediaWiki extensions in Gerrit.
Comment 3 Tyler Romeo 2013-12-07 07:07:29 UTC
> MediaWiki hook
> Allows extension to change functionality

That's the purpose of hooks. The issue here is not with the hook, but with extensions that are misusing the hook.

For example, let's say we make the hook more specific in an attempt to make it less "generic and dangerous", and instead the hook allows extensions to *only* change language_urls. Extension developers could still misuse the hook and insert non-language URLs into the template.

The problem is that the template parameters are not well-defined enough, and because of that extension developers are misusing the template parameters. It would be ridiculous to instead make fifty different hooks, each for a different template parameter that needs to be altered.
Comment 4 Daniel Friesen 2013-12-07 20:25:10 UTC
By all means encourage extension authors to use more relevant hooks, but the SkinTemplateOutputPageBeforeExec hook is used for more than just changing language_urls.
- Sometimes extensions have a need to modify other template attributes, such as bodytext at the last moment.
- This hook is currently the only way to modify the sidebar with things that you do not want to be cached.
- It's also the only method of adding footerlinks and changing it's contents. See https://www.mediawiki.org/wiki/Manual:Footer

I'm going to have to -1 this idea.
Comment 5 Jon 2013-12-08 01:58:18 UTC
This is what I'm getting at. There should be explicit hooks that encourage adding links to footer, transforming body text before display and adding things to side bar. A generic hook like this adds all sorts of complexity. In my opinion it's way too powerful in it's current form and cannot possibly be skin agnostic.
Comment 6 Gabriel Wicke 2013-12-09 18:51:23 UTC
We should definitely move towards narrower and more precisely defined interfaces. A lot of extension functionality can also be implemented client-side without the need for hooks at all.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links