On Fri, Oct 3, 2008 at 2:38 AM, brion@svn.wikimedia.org wrote:
Revision: 41584 Author: brion Date: 2008-10-03 00:38:33 +0000 (Fri, 03 Oct 2008)
Log Message:
Back out r41548 "Classes derived from SpecialPage can now specify a run() method, which will be executed after all magic performed by SpecialPage::execute()"
It's unclear what benefit this brings, and the magic calling is scary and icky.
Modified Paths:
trunk/phase3/RELEASE-NOTES trunk/phase3/includes/SpecialPage.php
Modified: trunk/phase3/RELEASE-NOTES
--- trunk/phase3/RELEASE-NOTES 2008-10-03 00:28:52 UTC (rev 41583) +++ trunk/phase3/RELEASE-NOTES 2008-10-03 00:38:33 UTC (rev 41584) @@ -145,8 +145,6 @@ $wgExternalLinkTarget
- api.php now sends "Retry-After" and "X-Database-Lag" HTTP headers if the maxlag
check fails, just like index.php does -* Classes derived from SpecialPage can now specify a run() method, which will
- be executed after all magic performed by SpecialPage::execute()
The problem currently is that in case you have your own special page overriding SpecialPage::execute, you have to do all setHeaders() and similar calls yourself, as well as the hooks. This basically means that your are copying code from the parent, which sucks and also is not futureproof in case somebody decides to add more magic to SpecialPage::execute.
Bryan
On Fri, Oct 3, 2008 at 6:38 AM, Bryan Tong Minh bryan.tongminh@gmail.comwrote:
On Fri, Oct 3, 2008 at 2:38 AM, brion@svn.wikimedia.org wrote:
Revision: 41584 Author: brion Date: 2008-10-03 00:38:33 +0000 (Fri, 03 Oct 2008)
Log Message:
Back out r41548 "Classes derived from SpecialPage can now specify a run()
method, which will be executed after all magic performed by SpecialPage::execute()"
It's unclear what benefit this brings, and the magic calling is scary and
icky.
Modified Paths:
trunk/phase3/RELEASE-NOTES trunk/phase3/includes/SpecialPage.php
Modified: trunk/phase3/RELEASE-NOTES
--- trunk/phase3/RELEASE-NOTES 2008-10-03 00:28:52 UTC (rev 41583) +++ trunk/phase3/RELEASE-NOTES 2008-10-03 00:38:33 UTC (rev 41584) @@ -145,8 +145,6 @@ $wgExternalLinkTarget
- api.php now sends "Retry-After" and "X-Database-Lag" HTTP headers if
the maxlag
check fails, just like index.php does -* Classes derived from SpecialPage can now specify a run() method, which
will
- be executed after all magic performed by SpecialPage::execute()
The problem currently is that in case you have your own special page overriding SpecialPage::execute, you have to do all setHeaders() and similar calls yourself, as well as the hooks. This basically means that your are copying code from the parent, which sucks and also is not futureproof in case somebody decides to add more magic to SpecialPage::execute.
Bryan
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wasn't calling parent::execute() a solution around the setHeaders() and broken hooks issues?
-Chad
On Fri, Oct 3, 2008 at 2:15 PM, Chad innocentkiller@gmail.com wrote:
On Fri, Oct 3, 2008 at 6:38 AM, Bryan Tong Minh bryan.tongminh@gmail.comwrote:
On Fri, Oct 3, 2008 at 2:38 AM, brion@svn.wikimedia.org wrote:
Revision: 41584 Author: brion Date: 2008-10-03 00:38:33 +0000 (Fri, 03 Oct 2008)
Log Message:
Back out r41548 "Classes derived from SpecialPage can now specify a run()
method, which will be executed after all magic performed by SpecialPage::execute()"
It's unclear what benefit this brings, and the magic calling is scary and
icky.
Modified Paths:
trunk/phase3/RELEASE-NOTES trunk/phase3/includes/SpecialPage.php
Modified: trunk/phase3/RELEASE-NOTES
--- trunk/phase3/RELEASE-NOTES 2008-10-03 00:28:52 UTC (rev 41583) +++ trunk/phase3/RELEASE-NOTES 2008-10-03 00:38:33 UTC (rev 41584) @@ -145,8 +145,6 @@ $wgExternalLinkTarget
- api.php now sends "Retry-After" and "X-Database-Lag" HTTP headers if
the maxlag
check fails, just like index.php does -* Classes derived from SpecialPage can now specify a run() method, which
will
- be executed after all magic performed by SpecialPage::execute()
The problem currently is that in case you have your own special page overriding SpecialPage::execute, you have to do all setHeaders() and similar calls yourself, as well as the hooks. This basically means that your are copying code from the parent, which sucks and also is not futureproof in case somebody decides to add more magic to SpecialPage::execute.
Bryan
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Wasn't calling parent::execute() a solution around the setHeaders() and broken hooks issues?
-Chad _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
No. A failing require_once and hooks that are executed in the wrong order, among other things.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Bryan Tong Minh wrote:
The problem currently is that in case you have your own special page overriding SpecialPage::execute, you have to do all setHeaders() and similar calls yourself, as well as the hooks. This basically means that your are copying code from the parent, which sucks and also is not futureproof in case somebody decides to add more magic to SpecialPage::execute.
Perhaps it would be better to fix the setHeaders() issue, so it's called prior to execute()?
More generally, just slapping in a replacement for the entire execute() method sounds like a bad idea. If you're extending a base class, you probably want to *alter* its behavior, not replace it. The right way to do this is to factor the code sensibly in the first place, so you only have to replace the relevant parts instead of reimplenting the whole execute() method.
Adding a generic run() method doesn't help you there -- it saves you one line of boilerplate and makes you redo everything else.
- -- brion
On Fri, Oct 3, 2008 at 12:29 PM, Brion Vibber brion@wikimedia.org wrote:
Perhaps it would be better to fix the setHeaders() issue, so it's called prior to execute()?
Would moving it up a level to executePath() and removing the child calls fix this? If so, should be fairly trivial.
While we're at it, moving the hook calls up a level and making the default execute() a no-op might be ideal. If subclasses are required to implement execute, which they are, then they shouldn't have to be redoing what the parent execute() did (by either doing it manually or calling parent::execute()).
-Chad
On Fri, Oct 3, 2008 at 7:50 PM, Chad innocentkiller@gmail.com wrote:
On Fri, Oct 3, 2008 at 12:29 PM, Brion Vibber brion@wikimedia.org wrote:
Perhaps it would be better to fix the setHeaders() issue, so it's called prior to execute()?
Would moving it up a level to executePath() and removing the child calls fix this? If so, should be fairly trivial.
While we're at it, moving the hook calls up a level and making the default execute() a no-op might be ideal. If subclasses are required to implement execute, which they are, then they shouldn't have to be redoing what the parent execute() did (by either doing it manually or calling parent::execute()).
-Chad _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
The problem is that many special pages still use the wfSpecialPage() scheme.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Bryan Tong Minh wrote:
The problem is that many special pages still use the wfSpecialPage() scheme.
Why's that a problem?
If there's a need, migrate the old code.
- -- brion
On Fri, Oct 3, 2008 at 6:29 PM, Brion Vibber brion@wikimedia.org wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Bryan Tong Minh wrote:
The problem currently is that in case you have your own special page overriding SpecialPage::execute, you have to do all setHeaders() and similar calls yourself, as well as the hooks. This basically means that your are copying code from the parent, which sucks and also is not futureproof in case somebody decides to add more magic to SpecialPage::execute.
Perhaps it would be better to fix the setHeaders() issue, so it's called prior to execute()?
Perhaps, but that would also require the hooks to move.
More generally, just slapping in a replacement for the entire execute() method sounds like a bad idea. If you're extending a base class, you probably want to *alter* its behavior, not replace it. The right way to do this is to factor the code sensibly in the first place, so you only have to replace the relevant parts instead of reimplenting the whole execute() method.
But that will break backwards compatibility for a really widely used interface. I think that is not worth the effort.
Adding a generic run() method doesn't help you there -- it saves you one line of boilerplate and makes you redo everything else.
Well, more like 5-6, but I get your point.
Bryan
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Bryan Tong Minh wrote:
On Fri, Oct 3, 2008 at 6:29 PM, Brion Vibber brion@wikimedia.org wrote:
Perhaps it would be better to fix the setHeaders() issue, so it's called prior to execute()?
Perhaps, but that would also require the hooks to move.
What hooks are you referring to?
More generally, just slapping in a replacement for the entire execute() method sounds like a bad idea. If you're extending a base class, you probably want to *alter* its behavior, not replace it. The right way to do this is to factor the code sensibly in the first place, so you only have to replace the relevant parts instead of reimplenting the whole execute() method.
But that will break backwards compatibility for a really widely used interface. I think that is not worth the effort.
It sounds like you're not working with an interface that's designed to be extended, so nothing you do in a child class is going to be nice.
I would recommend refactoring whatever specific interface you're looking at (some particular special page, I can only assume?) so it's sanely extensible.
- -- brion
On Fri, Oct 3, 2008 at 10:24 PM, Brion Vibber brion@wikimedia.org wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Bryan Tong Minh wrote:
On Fri, Oct 3, 2008 at 6:29 PM, Brion Vibber brion@wikimedia.org wrote:
Perhaps it would be better to fix the setHeaders() issue, so it's called prior to execute()?
Perhaps, but that would also require the hooks to move.
What hooks are you referring to?
SpecialPageExecuteBeforePage and SpecialPageExecuteAfterPage
More generally, just slapping in a replacement for the entire execute() method sounds like a bad idea. If you're extending a base class, you probably want to *alter* its behavior, not replace it. The right way to do this is to factor the code sensibly in the first place, so you only have to replace the relevant parts instead of reimplenting the whole execute() method.
But that will break backwards compatibility for a really widely used interface. I think that is not worth the effort.
It sounds like you're not working with an interface that's designed to be extended, so nothing you do in a child class is going to be nice.
I would recommend refactoring whatever specific interface you're looking at (some particular special page, I can only assume?) so it's sanely extensible.
Perhaps I was not clear enough, but the interface I am referring to is the SpecialPage base class itself.
Bryan Tong Minh wrote:
On Fri, Oct 3, 2008 at 6:29 PM, Brion Vibber brion@wikimedia.org wrote:
Perhaps it would be better to fix the setHeaders() issue, so it's called prior to execute()?
Perhaps, but that would also require the hooks to move.
Please, consider extensions that don't expect any of setHeaders(), outputHeaders() or hooks to be executed. I have one such extension, KML Export, which doesn't use $wgOut (except for some headers), and also doesn't make any sense to call the special page hooks, since it outputs KML data, not HTML.
For such special page, it makes sense to overload execute and have full control of the execution since the beginning.
It makes sense to me to have another "shallower" method to overload in order to output common (most) special pages. But I would do it a little different in a few aspects:
1. The words "execute" and "run" are almost synonyms to me, confusing. I would call it "page", "output", "render" or something else;
2. Instead of checking for is_callable(array($this, 'run')), I would create a page() method (or run(), render(), etc), and move the call_user_func call in there. That would be the "correct" way of using virtual functions.
That way, extensions can choose to overload either execute() (deeper) or page() (swallower) depending on their needs. Most special pages would overload the latter, and left execute() doing the checks it currently does.
More generally, just slapping in a replacement for the entire execute() method sounds like a bad idea. If you're extending a base class, you probably want to *alter* its behavior, not replace it. The right way to do this is to factor the code sensibly in the first place, so you only have to replace the relevant parts instead of reimplenting the whole execute() method.
I think that my proposal above fits this requirement, without breaking any existing extensions, and fulfilling Bryan's original intent.
On Sat, Oct 4, 2008 at 3:24 AM, Juliano F. Ravasi ml@juliano.info wrote:
I think that my proposal above fits this requirement, without breaking any existing extensions, and fulfilling Bryan's original intent.
Indeed. I first tried something along those lines, but I got a stuck because the hooks may change the function to be called and because I was not sure whether the require_once in execute() would result the wfSpecialFunction to be in local or global variable scope.
Bryan
Bryan Tong Minh wrote:
Indeed. I first tried something along those lines, but I got a stuck because the hooks may change the function to be called
That is fine. Just pass $this->mFunction directly to the hook, there is no need to make a $func copy as it is now.
and because I was not sure whether the require_once in execute() would result the wfSpecialFunction to be in local or global variable scope.
Functions declared inside other functions (or methods) in PHP are global, so it is ok.
See the patch attached, I just tested and it works.
On Sat, Oct 4, 2008 at 11:04 PM, Juliano F. Ravasi ml@juliano.info wrote:
See the patch attached, I just tested and it works.
Attachments to this list are silently stripped.
Aryeh Gregor wrote:
On Sat, Oct 4, 2008 at 11:04 PM, Juliano F. Ravasi ml@juliano.info wrote:
See the patch attached, I just tested and it works.
Attachments to this list are silently stripped.
Ugh... ok.
Here: http://juliano.info/ext/mediawiki-jr/mediawiki-1.14alpha-specialpage.patch
On Sun, Oct 5, 2008 at 5:04 AM, Juliano F. Ravasi ml@juliano.info wrote:
Bryan Tong Minh wrote:
Indeed. I first tried something along those lines, but I got a stuck because the hooks may change the function to be called
That is fine. Just pass $this->mFunction directly to the hook, there is no need to make a $func copy as it is now.
Are we absolutely certain that execute is never called more than once per special page? I'd assume so, but I'm not really sure.
Bryan
Bryan Tong Minh wrote:
Are we absolutely certain that execute is never called more than once per special page? I'd assume so, but I'm not really sure.
I can't see how, unless some special page extension does something really crazy, what could be considered a bug in that extension.
Juliano
wikitech-l@lists.wikimedia.org