Massive Hook Proposal

classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Massive Hook Proposal

Jim R. Wilson
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:


Step 1: For each class in MW, create a static factory method for class
instantiation.  The factory method would take the same number of arguments
as the class constructor, and under normal circumstances would simply call
the constructor.  Here's an example for the UploadForm class (
SpecialUpload.php):

--------------------------------------------------------------
class UploadForm {

// ...

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

Step 2: Replace all calls to the traditional "new Whatever($args)" with
"Whatever::instantiate($args)".

By doing this, extension developers gain the power to modify any class
before it's ever used, possibly even replacing it with a custom class
they've developed.

What do you think?  I'm genuinely interested in hearing what everyone has to
say.  Thanks!

(Please note that I haven't tested any code for this yet - so if my syntax
in the example is out of order, I apologize in advance).

-- Jim R. Wilson
_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Mark Clements (HappyDog)
"Jim Wilson" <[hidden email]> wrote in
message news:[hidden email]...
> 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
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Kasimir Gabert
On 2/14/07, Mark Clements <[hidden email]> wrote:

> "Jim Wilson" <[hidden email]> wrote in
> message news:[hidden email]...
> > 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
> [hidden email]
> 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.

--
Kasimir Gabert

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Boris Eetgerink
In reply to this post by Jim R. Wilson
Jim Wilson wrote:
> 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.
[...]

I support this idea. It makes MediaWiki very easy to modify, without the
need to dig into the core code.
It would be possible to replace the instantiated class with a subclass,
allowing you to add functions to it. All modifications would be neatly
placed in the extensions folder and in LocalSettings.php. I like it!

Boris

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Aryeh Gregor
In reply to this post by Jim R. Wilson
On 2/14/07, Jim Wilson <[hidden email]> wrote:
> 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: . . .

It seems like a good idea.  It should be reverse-compatible as long as
we don't change the public interface (well, or protected interface)
for classes, which would generally break reverse-compatibility anyway.

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Jim R. Wilson
Thanks everyone.  My goal was to come up with a passive way to put hooks
"everywhere" - or as close to everwhere as possible.  Is there a better
solution?  I'm open to suggestions.

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.

-- Jim

On 2/14/07, Simetrical <[hidden email]> wrote:

>
> On 2/14/07, Jim Wilson <[hidden email]> wrote:
> > 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: . . .
>
> It seems like a good idea.  It should be reverse-compatible as long as
> we don't change the public interface (well, or protected interface)
> for classes, which would generally break reverse-compatibility anyway.
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> http://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Brion Vibber
-----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 SIGNATURE-----
Version: GnuPG v1.4.2.2 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF00FBwRnhpk1wk44RAt4SAJ9h7hXhpPDuiYAVcjYxgT9U2aVAcQCfVGIk
aEny2OzSpT90Kem/KWmm+pY=
=S60U
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Jim R. Wilson
Gotcha - so when's the submission deadline for patches to 1.10.0?

On 2/14/07, Brion Vibber <[hidden email]> wrote:

>
> -----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.10that
> > 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 SIGNATURE-----
> Version: GnuPG v1.4.2.2 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFF00FBwRnhpk1wk44RAt4SAJ9h7hXhpPDuiYAVcjYxgT9U2aVAcQCfVGIk
> aEny2OzSpT90Kem/KWmm+pY=
> =S60U
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> http://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Brion Vibber
In reply to this post by Jim R. Wilson
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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

Well, the obvious limitation here is that you're instantiating the
object with a fixed class first. This method means you'd have to either
proxy the original object or throw it away and create a new one (hope
there's no side effects!)

Subclassing could be a nice way to do things in various circumstances,
where proxy objects are IMHO kind of ugly.


> Step 2: Replace all calls to the traditional "new Whatever($args)" with
> "Whatever::instantiate($args)".

Note that we use factory methods fairly extensively, usually with
multiple such methods.

- -- brion vibber (brion @ pobox.com / brion @ wikimedia.org)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF00YRwRnhpk1wk44RAvBSAJ99HbW0HJwJUJfNzcq+4VFSjsKnuQCgg0JM
CCiYNWsRi6pZIYQIB4AS28o=
=K57/
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Brion Vibber
In reply to this post by Jim R. Wilson
-----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)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF00nwwRnhpk1wk44RAnZ6AJ0aMeWYHO0hJ/dbM+PTL1wzqnXgkgCgvBeZ
x2dU8UXm1IK2ZxDbsXvpX8M=
=KSM7
-----END PGP SIGNATURE-----

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Jim R. Wilson
Thanks Brion,

> Well, the obvious limitation here is that you're instantiating the
> object with a fixed class first. This method means you'd have to either
> proxy the original object or throw it away and create a new one (hope
> there's no side effects!)

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;
}

> Subclassing could be a nice way to do things in various circumstances,
> where proxy objects are IMHO kind of ugly.

I also like the idea of Subclassing.  Could you expand on what you mean by
proxy objects in this context?  Perhaps with an example?

> Note that we use factory methods fairly extensively, usually with
> multiple such methods.

Right right - such as "Title::newFromDBkey()", which in turn calls "new
Title()" internally.  By replacing this internal call with another factory
method such as "Title::instantiate()", we still give extension devs a chance
to hijack it.

Thanks again for the feedback - I really appreciate your comments.

At the current time there is a class of problems that can only be solved by
hacking the source - as used to be the case for Semantic MW.  Of course,
increasing the volume of wfRunHooks calls will help to reduce the problem
space (as was the case for Semantic).

As people want to do more with MW as a development platform, the number of
use-cases increases, and I hope we can find a way to blow the API wide open
to make nearly any modification possible, without resorting to hacking the
core code.

Thanks again,

Jim


On 2/14/07, Brion Vibber <[hidden email]> wrote:

>
> -----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)
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.2.2 (Darwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iD8DBQFF00nwwRnhpk1wk44RAnZ6AJ0aMeWYHO0hJ/dbM+PTL1wzqnXgkgCgvBeZ
> x2dU8UXm1IK2ZxDbsXvpX8M=
> =KSM7
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> http://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Mark Clements (HappyDog)
"Jim Wilson" <[hidden email]> wrote in
message news:[hidden email]...
>
> 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)




_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Jim R. Wilson
Thanks Mark,

So what's my best path to move forward with this?  A giant diff patch that
implements Mark's version for every class?  A series of small, independently
verifiable diffs?  What special concerns should I take into account when
undertaking such a sweeping change (above and beyond what's on
MediaWiki.orgthat is)?

Are there reasons why this patch wouldn't be accepted that I should consider
before undertaking the work to implement it?

Thanks in advance for any advice - I know how to do the code, it's the
process of getting it into the code base where I need guidance.  Thanks
again.

-- Jim

On 2/14/07, Mark Clements <[hidden email]> wrote:

>
> "Jim Wilson" <[hidden email]> wrote in
> message news:[hidden email]...
> >
> > 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)
>
>
>
>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> http://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Nick Jenkins
In reply to this post by Brion Vibber
> > 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.

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Aryeh Gregor
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
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Jim R. Wilson
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 <[hidden email]> 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
> [hidden email]
> http://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Aryeh Gregor
On 2/15/07, Jim Wilson <[hidden email]> 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
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Jim R. Wilson
> 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 <[hidden email]> wrote:

>
> On 2/15/07, Jim Wilson <[hidden email]> 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
> [hidden email]
> http://lists.wikimedia.org/mailman/listinfo/wikitech-l
>
_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Jim Hu
What's the hooking way to redirect search to a custom advanced search  
system?  I could imagine doing this by extending class  
SpecialSearch...but I don't know how to get the system to instantiate  
the subclass instead of the base class.

"Imagine" here includes imagining that I actually understand oo  
programming, a condition that currently evals to false. : )

Jim
=====================================
Jim Hu
Associate Professor
Dept. of Biochemistry and Biophysics
2128 TAMU
Texas A&M Univ.
College Station, TX 77843-2128
979-862-4054


On Feb 15, 2007, at 11:26 AM, Jim Wilson wrote:

>> 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 <[hidden email]> wrote:
>>
>> On 2/15/07, Jim Wilson <[hidden email]> 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
>> [hidden email]
>> http://lists.wikimedia.org/mailman/listinfo/wikitech-l
>>
> _______________________________________________
> Wikitech-l mailing list
> [hidden email]
> http://lists.wikimedia.org/mailman/listinfo/wikitech-l

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
Reply | Threaded
Open this post in threaded view
|

Re: Massive Hook Proposal

Lane, Ryan
In reply to this post by Jim R. Wilson
> 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

_______________________________________________
Wikitech-l mailing list
[hidden email]
http://lists.wikimedia.org/mailman/listinfo/wikitech-l
12