On 2/14/07, Mark Clements gmane@kennel17.co.uk wrote:

"Jim Wilson" wilson.jim.r@gmail.com wrote in message news:ac08e8d0702140532sdd74668u487a6aeab0eaa35e@mail.gmail.com...

...

Dear Wiki Devs,

I have a proposal for adding new hooks into the MW codebase, and I'm

looking

...

for feedback (positive and negative). The goal of the proposed changes is to give Extension Devs the ability to cleanly hijack just about any class

in

...

the system. Here are the details:

[SNIP]

...

---

class UploadForm {

// ...

function instantiate( &$request ) { $uploadForm = new UploadForm($request); wfRunHooks('UploadFormInstantiate', array( &$uploadForm, &$request )); return $uploadForm; }

---

[SNIP]

...

What do you think? I'm genuinely interested in hearing what everyone has

to

...

say. Thanks!

No real comment on the idea at the moment, except that it might be better to use "Instantiate_ClassName" as the hook, so that they can be easily grouped in documentation, or referred to as "Instantiate_*" hooks. It also clearer what the hook does if you come across one you haven't seen before, I think.

• Mark Clements (HappyDog)

---

Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l

Hello Jim,

I would definitely vote for this idea. It could potentially be a solution to many questions that people are posting, such as how to turn a wiki into a content management system. It would definitely make upgrading easier with wikis that have heavily hacked core code.

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Jim Wilson wrote:

Also, if implemented, what version(s) should be targeted? I'm thinking 1.8.4, 1.9.3 and 1.10.0 - your thoughts? Is there going to be a 1.6.10 that would benefit from this? Thanks in advance.

Massive far-reaching changes like that would never ever go into maintenance releases of old versions.

Is it a security fix? no.

Is it a vital bug fix necessary for the software to run? no.

- -- brion vibber (brion @ pobox.com / brion @ wikimedia.org)

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1

Jim Wilson wrote:

Gotcha - so when's the submission deadline for patches to 1.10.0?

1.10 will branch from trunk in the first week of April per our quarterly release cycle.

- -- brion vibber (brion @ pobox.com / brion @ wikimedia.org)

"Jim Wilson" wilson.jim.r@gmail.com wrote in message news:ac08e8d0702140949u83cc418hd6f54fca61b1ec64@mail.gmail.com...

How do you feel about this alternative? (continuing the UploadForm

example):

function instantiate( &$request ) { $uploadForm = NULL; wfRunHooks('BeforeInstantiate_UploadForm', array( &$uploadForm, &$request )); if ($uploadForm != NULL) { return $uploadForm; } $uploadForm = new UploadForm($request); wfRunHooks('AfterInstantiate_UploadForm', array( &$uploadForm,

&$request

)); return $uploadForm; }

AfterInstantiate_X should be run regardless of where the object came from. The above should probably be (in some kind of pseudo-code form):

function instantiate( &$args) { $objResult = NULL; wfRunHooks('BeforeInstantiate_X', array( &$objResult, &$args));

if ($objResult === NULL) $objResult = new Object($args);

wfRunHooks('AfterInstantiate_X', array( &$ojResult, &$args)); return $objResult; }

- Mark Clements (HappyDog)

...

Gotcha - so when's the submission deadline for patches to 1.10.0?

1.10 will branch from trunk in the first week of April per our quarterly release cycle.

Maybe best to not cut it too fine for anything "massive" though ... my mental model of what's going on is something like this:

V - Quarterly release | .... .... >>>> Zone of normal grumpiness. T .... I .... M - Three weeks to release E >>> Zone of steadily increasing grumpiness - One week to release | >>> Zone of extreme grumpiness! Bugfixes above new features. v - Quarterly release .... .... Rinse, repeat.

Step 1: For each class in MW, create a static factory method for class instantiation. ... Step 2: Replace all calls to the traditional "new Whatever($args)" with "Whatever::instantiate($args)".

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 :

====================================================================== <?php

require_once '/var/www/hosts/mediawiki/wiki/maintenance/commandLine.inc';

/* ** An array of class names to override, where the key is the standard ** class name, and the value is the overridden class name (which should ** extended the standard class and/or implement the same interfaces that ** it does). */ global $wgClassOverrides; $wgClassOverrides = array();

class Factory {

/* ** Instantiate a new object, running before and after hooks. */ public static function wfNew( $class, &$args = NULL ) { $objResult = NULL; wfRunHooks('BeforeInstantiate_' . $class, array( &$objResult, &$args) );

if ($objResult === NULL) { $real_class = self::getRealClass( $class ); $objResult = new $real_class ($args); }

wfRunHooks('AfterInstantiate_' . $class, array( &$objResult, &$args) ); return $objResult; }

/* ** Returns the classname to use, allowing classnames to be overridden. ** Recursively calls itself, so as to allow stacking overrides. */ private static function getRealClass( $class ) { global $wgClassOverrides; if( isset ( $wgClassOverrides[$class] ) ) { return self::getRealClass ( $wgClassOverrides[$class] ); } else { return $class; } }

/* ** An array of singletons, where the key is the class name, and the ** value is the singleton object. */ private $singletons = array();

/* ** Returns a singleton for the specified class. */ public function & singleton( $class ) { $realClass = self::getRealClass( $class ); if( !isset( $this->singletons[$realClass] ) ) { $this->singletons[$realClass] = self::wfNew( $realClass ); } return $this->singletons[$realClass]; } }

// ------------- test code for the above -----------------

$wgHooks['BeforeInstantiate_Title'][] = 'BeforeInstantiate_Title'; $wgHooks['AfterInstantiate_Title' ][] = 'AfterInstantiate_Title';

/* ** Test dummy hooks for Title */ function BeforeInstantiate_Title () { print "In " . __FUNCTION__ . "\n"; }

function AfterInstantiate_Title () { print "In " . __FUNCTION__ . "\n"; }

/* ** Empty test class. */ class myTitle extends Title {

}

// we want to override the Title class, with the myTitle class. $wgClassOverrides['Title'] = 'myTitle';

// Should print out the two "In XXX" lines for the Before then After hooks. $title = Factory::wfNew( 'Title' );

// should give the class as myTitle. print "$title is a: " . get_class( $title ) . "\n";

// The factory that can build itself before it exists... :-) global $wgFactory; $wgFactory = Factory::wfNew( 'Factory' );

// test class that will print out a line when being constructed. class testTitle extends Title { function __construct () { print "Constructing one " . get_class( $this ) . "\n"; } }

// create a singleton $s1 = $wgFactory->singleton( 'testTitle' ); $s2 = $wgFactory->singleton( 'testTitle' ); $wgClassOverrides['fakeTitle'] = 'testTitle'; $s3 = $wgFactory->singleton( 'fakeTitle' );

// should only get one printout from the block of lines above.

?> ======================================================================

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? How much harder would it make it for external people to understand what's going on in the code? 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).

All the best, Nick.

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

This would run into serious problems with stuff like system messages (which contain lots of wikitext, at least by default) and hardcoded strings (many of which are at least going to be HTML, if perhaps not often wikitext). Nice idea, though.

Thanks Simetrical, that's a good point. Definitely something to consider when attempting to extend the Parser.

Can anyone think of something they currently can't achieve with the existing hooks? Something that might be solvable with a Factory class?

-- Jim

On 2/15/07, Simetrical Simetrical+wikilist@gmail.com wrote:

On 2/15/07, Jim Wilson wilson.jim.r@gmail.com wrote:

...

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).

This would run into serious problems with stuff like system messages (which contain lots of wikitext, at least by default) and hardcoded strings (many of which are at least going to be HTML, if perhaps not often wikitext). Nice idea, though.

---

Wikitech-l mailing list Wikitech-l@lists.wikimedia.org http://lists.wikimedia.org/mailman/listinfo/wikitech-l

Can anyone think of something they currently can't achieve with the existing hooks? Something that might be solvable with a Factory class?

-- Jim

Well, say for instance I wanted to replace the built in gallery stuff with my own. I could either hack up ImageGallery, or use a different syntax for the new gallery using a parser extension. I would then have to make sure users use <mygallery></mygallery> instead of <gallery></gallery>, and if I decided I wanted to go back to regular galleries later I would have to change a bunch of wiki pages.

If I could hijack the ImageGallery class, I could replace that functionality without worrying about compatibility problems later.

Is there another way to do this? Can I override <gallery></gallery> with my own extension, or would they conflict?

V/r,

Ryan Lane

Simetrical 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

Correct me if I'm wrong, but this looks like you're going to instantiate the "normal" class if the extension provided an object of the wrong class. It would be way more helpful to the extension developer if it threw an exception or some sort of error message instead, e.g. "Error: ".typeof($objResult)." is not a subclass of ".self::getRealClass() or something like that.