Thanks Nick,
Maybe best to not cut it too fine for anything "massive" though ... my mental model of what's going on is something like this:
[SNIP]
Thanks for laying this out - it would seem that the first week of March would be the very last time to hope to put a change like this in place (so as not to interfere with the periods of grumpiness :).
Would it be nicer to have a single function / static method that did this (by passing in the classname as a parameter), rather than adding the same boilerplate code to every class in MediaWiki? Rough, probably-buggy, example code below.
Also adds creating singletons; Also adds allowing the class names to be overridden to instantiate a different class via a global. Whether any of these are desirable behaviour is for others to say (how abstract do you want to get?) - it was just a quick & dirty test mockup :
Absolutely! I like your Factory class a lot - this would definitely reduce the amount of code replication (always a good thing). Thanks also for providing the test cases and a Singleton implementation. Great stuff!
One thing I'm not sure exactly how to handle is the fact that different class constructors take different numbers of arguments. Maybe something like this? (haven't tested it, includes Simetrical's change):
========================== /* ** Instantiate a new object, running before and after hooks. */ public static function wfNew( $class, &$args = NULL ) { $objResult = NULL; wfRunHooks('BeforeInstantiate_' . $class, array( &$objResult, &$args) );
// must use is_a because instanceof doesn't work with strings if (!is_object($objResult) || !is_a($objResult, self::getRealClass())) { $real_class = self::getRealClass( $class ); $objResult = call_user_func_array( array(new ReflectionClass($real_class), 'newInstance'), $args ); // do the 'is_a' check again here in case someone messed up the $wgClassOverrides // Note: drops reference to previously created object if incorrect (side effects?) if (!is_object($objResult) || !is_a($objResult, self::getRealClass())) { $objResult = call_user_func_array( array(new ReflectionClass($class), 'newInstance'), $args ); } }
wfRunHooks('AfterInstantiate_' . $class, array( &$objResult, &$args) ); return $objResult; } ==========================
It would seem that the only purpose for 'BeforeIsntantiate_*' would be to modify the arguments, since subclass instantiation could be as easily achieved through the $wgClassOverrides. Should we still pass $objResult as one of the parameters?
Are there reasons why this patch wouldn't be accepted that I should
consider
before undertaking the work to implement it?
I can't comment on whether others will accept it, only on whether I think
it's
worth doing, and for that I guess my 3 personal evaluation criteria would
be:
How much performance overhead would it add in real-world tests?
I have no idea - is there a recommended framework for executing load tests against MediaWiki?
How much harder would it make it for external people to understand what's
going on in the
code?
Yeah, any time you add functionality you run the risk of making the code harder to read for the uninitiated. I suspect that adding a Factory class to handle instantiation may make the code a little more daunting for newcomers.
I personally feel that the added functionality (in this case) outweighs the potential for new-contributor confusion. However I am quite biased, as a developer of MW Extensions :)
What does the community think?
How many people would use these style of hooks / overrides? (I.e. Not much point in doing it if nobody uses it; not much point in software that
does
wonderful things if it's so slow to use that nobody wants to use it; and
I'm sure
we've lost something worth having if newcomers look at the code and are
baffled
as to what's going on).
I would use this quite a bit (can't speak for everybody). I know everyone gets tired of saying that "MediaWiki is not a Content Management System" - I say this all the time. I believe with the Factory class suggested above, people could develop extensions which turn MW into a CMS. Here are some problems that I believe would be helped:
Editor Approval - Every so often someone asks how they can 'proof' all MW changes before the 'go live'. Perhaps have a staging area where content is proposed before it gets published? I know this feature is against the wiki principle and is currently unachievable with existing hooks.
Namespace Skins - I have a good friend that some time ago hacked MW to allow each Namespace to have its own Skin (regardless of user preference). He ended up hacking Title to get the job done. If it were possible to hijack it, he could have left the source in its pristine condition.
Alternate Parsers - Extension developers could implement alternate parsers. This _could_ put an end to the _italicized_ and *bold* debates. It might also offer a way to allow pages to use any of a number of different parsers for different purposes. Maybe have some pages use MW syntax, while others use full XHTML with the aid of a WYSIWYG editor (not a full solution to that debate, but it's something).
These are just three that I can think of off the top of my head. Can anyone think of any more?
Thanks again everyone who's participating in this discussion - I appreciate it.
-- Jim
On 2/15/07, Simetrical Simetrical+wikilist@gmail.com wrote:
Might it be a good idea, in these factory functions, to check whether the object returned is an instance of the intended class? Like, if someone replaces all Title objects with MyTitles that *don't* extend Title, that would cause some havoc, not least as we start adding type hinting. It's probably best to catch that particular mistake early.
So instead of "if ($objResult === NULL)", something like:
if (!is_object($objResult) || !is_a($objResult, self::getRealClass())) // must use is_a because instanceof doesn't work with strings
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l