Removing the Hooks class

List overview All Threads
Download

newer

older

Saturday: Weekend Testing Americas...

[Language Engineering] Office hour...

Jeroen De Dauw

4 Apr 2013 4 Apr '13

1:51 a.m.

Hey, I'm curious what the list thinks of deprecating and eventually removing the Hooks class. Some relevant info: /** * Hooks class. * * Used to supersede $wgHooks, because globals are EVIL. * * @since 1.18 */ https://github.com/wikimedia/mediawiki-core/blob/master/includes/Hooks.php#… I personally find the comment hilarious and hope you see why when looking at the "class". Looks like usage in core and extensions is not to extensive, so switching to something more sane seems quite feasible. Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --

Show replies by date

Chad

4 Apr 4 Apr

1:55 a.m.

On Wed, Apr 3, 2013 at 9:51 PM, Jeroen De Dauw <jeroendedauw(a)gmail.com> wrote:

...

It's like a global, only slower :D If you've got any better ideas, I'm all for it. -Chad

Tyler Romeo

1:55 a.m.

On Apr 3, 2013 9:52 PM, "Jeroen De Dauw" <jeroendedauw(a)gmail.com> wrote:

...

Hey, I'm curious what the list thinks of deprecating and eventually removing

the

...

Hooks class. Some relevant info: /** * Hooks class. * * Used to supersede $wgHooks, because globals are EVIL. * * @since 1.18 */

https://github.com/wikimedia/mediawiki-core/blob/master/includes/Hooks.php#…

...

I personally find the comment hilarious and hope you see why when looking at the "class". Looks like usage in core and extensions is not to extensive, so switching to something more sane seems quite feasible. Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. -- _______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

Can this please please please be done *after* my patch clarifying hook execution logic is merged? I'll include a link when I'm on my desktop, but if I have to rebase it again it's never going to get through. Also, what alternative implementation are you considering?

Daniel Friesen

4:54 a.m.

On Wed, 03 Apr 2013 18:51:54 -0700, Jeroen De Dauw <jeroendedauw(a)gmail.com> wrote:

...

Hey, I'm curious what the list thinks of deprecating and eventually removing the Hooks class.

I see no reason to get rid of the hooks class. We use static classes other places in core. And there's no reason to revert to hideous functions like we had before. There's enough code logic there to justify packaging it into a static class. Interfaces like these have really slow uptake because extensions always wait a long time to pick them up because using them fundamentally means dropping compatibility with a version of MW earlier than the one it was introduced in. Hooks was only added in 1.18 and extensions have a habit of retaining compatibility for old versions of MediaWiki for a significant time even after we make that version EOL. It's not unexpected to see extensions not using the interface yet. Quite frankly: Hooks::register( 'BeforePageDisplay', 'MyExt::pageDisplay' ); Is quite nicer than the backcompat: $wgHooks['BeforePageDisplay'][] = 'MyExt::pageDisplay'; In the event of a global typo it'll error out instead of silently not working. The argument structure is cleaner and you can't inadvertently break everything by forgetting to add the "[]" which to the casual extension author may not be totally apparent. Oh... ;) and no spending hours trying to find out why your hook isn't being called cause you moved your hook call to an extension function so you could vary by user config but forgot to add `global $wgHooks;` to your function. If in the future we require core and extensions to register the hooks they call it also leaves the possibility of warning extension authors when they typo a hook name, with a proper trace where they made the mistake. And it gives us room to consider the possibility of doing something like turning: Hooks::register( 'BeforePageDisplay', array( 'MyExt::pageDisplay', $data ) ); Into just: Hooks::register( 'BeforePageDisplay', 'MyExt::pageDisplay', $data );

...

""Used to supersede $wgHooks, because globals are EVIL."" I personally find the comment hilarious and hope you see why when looking at the "class". Looks like usage in core and extensions is not to extensive, so switching to something more sane seems quite feasible.

Btw, I hope you're not finding the comment funny by misreading "Used" as "In the past" when it's actually the action "to use".

...

Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil.

-- ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]

Jeroen De Dauw

5:15 a.m.

Hey,

...

I see no reason to get rid of the hooks class.

Given you also do not understand why I think the comment is funny, I recommend you read up on why writing static code is harmful. And on how global state can hide in static "classes".

...

We use static classes other places in core.

https://yourlogicalfallacyis.com/bandwagon In almost all such cases I have seen in core this kind of use of static is bad.

...

And there's no reason to revert to hideous functions like we had before.

No one is suggesting that. Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. --

Dmitriy Sintsov

5:23 a.m.

...

4 Апрель 2013 г. 9:16:49 пользователь Jeroen De Dauw (jeroendedauw(a)gmail.com) написал: Hey,

I see no reason to get rid of the hooks class.

Given you also do not understand why I think the comment is funny, I recommend you read up on why writing static code is harmful. And on how global state can hide in static "classes".

We use static classes other places in core.

https://yourlogicalfallacyis.com/bandwagon In almost all such cases I have seen in core this kind of use of static is bad.

And there's no reason to revert to hideous functions like we had before.

No one is suggesting that. Cheers --

Why the hooks should not be static? Multi-site (farm) built-in support in core without $wgConf? Common page table across multiple sites? Dmitriy

Daniel Friesen

6:11 a.m.

On Wed, 03 Apr 2013 22:23:41 -0700, Dmitriy Sintsov <questpc(a)rambler.ru> wrote:

...

4 Апрель 2013 г. 9:16:49 пользователь Jeroen De Dauw (jeroendedauw(a)gmail.com) написал: Hey,

I see no reason to get rid of the hooks class.

Given you also do not understand why I think the comment is funny, I recommend you read up on why writing static code is harmful. And on how global state can hide in static "classes".

We use static classes other places in core.

https://yourlogicalfallacyis.com/bandwagon In almost all such cases I have seen in core this kind of use of static is bad.

And there's no reason to revert to hideous functions like we had

before. No one is suggesting that. Cheers --

Why the hooks should not be static? Multi-site (farm) built-in support in core without $wgConf? Common page table across multiple sites? Dmitriy

How do you envision non-static hooks working and supporting multiple wikis? -- ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]

Dmitriy Sintsov

6:43 a.m.

...

4 Апрель 2013 г. 10:11:44 пользователь Daniel Friesen (daniel(a)nadir-seen-fire.com) написал: On Wed, 03 Apr 2013 22:23:41 -0700, Dmitriy Sintsov <questpc(a)rambler.ru> wrote:

4 Апрель 2013 г. 9:16:49 пользователь Jeroen De Dauw (jeroendedauw(a)gmail.com) написал: Hey,

I see no reason to get rid of the hooks class.

Given you also do not understand why I think the comment is funny, I recommend you read up on why writing static code is harmful. And on how global state can hide in static "classes".

We use static classes other places in core.

https://yourlogicalfallacyis.com/bandwagon In almost all such cases I have seen in core this kind of use of static is bad.

And there's no reason to revert to hideous functions like we had

before. No one is suggesting that. Cheers --

Why the hooks should not be static? Multi-site (farm) built-in support in core without $wgConf? Common page table across multiple sites? Dmitriy

How do you envision non-static hooks working and supporting multiple wikis?

If hooks will be non-static, should the hooks become the members of RequestContext, maybe? Dmitriy

Daniel Friesen

7:23 a.m.

On Wed, 03 Apr 2013 23:43:20 -0700, Dmitriy Sintsov <questpc(a)rambler.ru> wrote:

...

> 4 Апрель 2013 г. 9:16:49 пользователь Jeroen De Dauw >>

(jeroendedauw(a)gmail.com) написал:

> Hey, > > I see no reason to get rid of the hooks class. > Given you also do not understand why I think the comment is funny, I > recommend you read up on why writing static code is harmful. And on

how

> global state can hide in static "classes". > > We use static classes other places in core. > https://yourlogicalfallacyis.com/bandwagon > In almost all such cases I have seen in core this kind of use of

static >> is

bad. > And there's no reason to revert to hideous functions like we had before. No one is suggesting that. Cheers --

Why the hooks should not be static? Multi-site (farm) built-in

support > in core without $wgConf? Common page table across multiple sites?

Dmitriy

How do you envision non-static hooks working and supporting multiple wikis?

If hooks will be non-static, should the hooks become the members of RequestContext, maybe? Dmitriy

No, hooks are not a request thing. Hooks are at a level more global than the request context and are also relevant in places where there is no request context such as maintenance scripts, parsing, and things that should be isolated and not tied to a request. RequestContext also uses the hook system. Think of it this way. A request context is for a request. Pretending we handle multiple requests in one script run we would have a separate request for each. However hooks are something done at the level of extensions. The level this part of extensions are part of is environment init, along with the loading of executable code into memory. You do this once at the start of the script execution. Not each request. If we were to run MediaWiki in a daemon like way of serving requests like python and ruby support where the application is loaded into a process and request after request is passed to it. We'd still load hooks once at the start not once per request. (and we'd probably kill off the lame stuff we have to do to avoid stuff from being initialized when it's not used in a particular request since in a daemon it's only initialized once). -- ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]

Tyler Romeo

12:04 p.m.

I still fully maintain that entirely static classes is the most useless thing and is just an extremely crude method of hiding global context. We're using PHP 5.3 in the core, might as well just use namespaces at that point, because that's what a static class is: a namespace with global variables and functions. It entirely defeats the purpose of object oriented programming. Nonetheless, I don't see any other good way to implement Hooks other than as a global state. Even though there are times where hooks are executing in a context, like Daniel said, there are times where they are not, e.g., maintenance scripts or any other place outside the normal request/response workflow. *-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo(a)gmail.com

Max Semenik

3:15 p.m.

On 04.04.2013, 16:04 Tyler wrote:

...

There's a difference: static classes can have private variables, allowing to hide implementation. -- Best regards, Max Semenik ([[User:MaxSem]])

Chad

3:17 p.m.

On Thu, Apr 4, 2013 at 11:15 AM, Max Semenik <maxsem.wiki(a)gmail.com> wrote:

...

On 04.04.2013, 16:04 Tyler wrote:

There's a difference: static classes can have private variables, allowing to hide implementation.

And variables inside a global function are also scoped, so they don't leak implementation either. The point is semi-valid for functions, though. -Chad

Max Semenik

3:28 p.m.

On 04.04.2013, 19:17 Chad wrote:

...

On Thu, Apr 4, 2013 at 11:15 AM, Max Semenik <maxsem.wiki(a)gmail.com> wrote: > On 04.04.2013, 16:04 Tyler wrote: > >> I still fully maintain that entirely static classes is the most useless >> thing and is just an extremely crude method of hiding global context. We're >> using PHP 5.3 in the core, might as well just use namespaces at that point, >> because that's what a static class is: a namespace with global variables >> and functions. It entirely defeats the purpose of object oriented >> programming. > > There's a difference: static classes can have private variables, > allowing to hide implementation. >

...

And variables inside a global function are also scoped, so they don't leak implementation either. The point is semi-valid for functions, though.

Yeah, but what if you need more than one global function? ;) -- Best regards, Max Semenik ([[User:MaxSem]])

Chad

3:40 p.m.

On Thu, Apr 4, 2013 at 11:28 AM, Max Semenik <maxsem.wiki(a)gmail.com> wrote:

...

On 04.04.2013, 19:17 Chad wrote:

And variables inside a global function are also scoped, so they don't leak implementation either. The point is semi-valid for functions, though.

Yeah, but what if you need more than one global function? ;)

It makes it easier to support PHP4. -Chad

Antoine Musso

5 Apr 5 Apr

12:40 p.m.

Le 04/04/13 17:28, Max Semenik a écrit :

...

Yeah, but what if you need more than one global function? ;)

I would write a small class having static functions. -- Antoine "hashar" Musso

Daniel Friesen

4 Apr 4 Apr

3:43 p.m.

The case for a static class I see: Over functions: * Uses the AutoLoader when needed, no explicit require * Nicely groups the entire implementation of hooks into a compartment Over namespaced functions: * Uses the AutoLoader when needed, no explicit require * Gives us proper state across methods when we need it. Last I checked PHP does not allow namespace scoped semi-globals. -- ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]

Daniel Friesen

3:43 p.m.

On Wed, 03 Apr 2013 22:15:27 -0700, Jeroen De Dauw <jeroendedauw(a)gmail.com> wrote:

...

Hey,

I see no reason to get rid of the hooks class.

Given you also do not understand why I think the comment is funny, I recommend you read up on why writing static code is harmful. And on how global state can hide in static "classes".

Maybe you should try linking to a specific reason. I understand perfectly fine that globals and static classes both have global state. But I also understand that classes even when static still have advantages over functions and global variables. Hook registration is a matter of connecting core code paths to extension code paths. It is pretty much inherently a static operation. I was also trying to make sure that you were in fact commenting on "globals are evil" and not mistaking "Used to supersede $wgHooks" for something like "This was written to supersede $wgHooks but no-one uses it so it doesn't supersede $wgHooks anymore".

...

We use static classes other places in core.

https://yourlogicalfallacyis.com/bandwagon In almost all such cases I have seen in core this kind of use of static is bad.

Linker, Html, and Xml included? MWNamespace could use some instance methods. But registering hooks is pretty much a static operation when extensions themselves are basically static things that we don't load as an instance. In any case you're cherry picking sentences from a paragraph of things that belong together. That borders on https://yourlogicalfallacyis.com/strawman and maybe https://yourlogicalfallacyis.com/the-fallacy-fallacy I never implied that just because we use static classes elsewhere in core that it alone was a reason to use a static class. "We use static classes other places in core. And there's no reason to revert to hideous functions like we had before." Was supposed to be read together. And was meant to indicate that using static methods instead of functions is not out of step with our coding guidelines.

...

And there's no reason to revert to hideous functions like we had before.

No one is suggesting that.

Then instead of talking about removing the Hooks class why don't you try instead talking about what kind of hooks system we should create to supersede the Hooks class. Usually when someone talks about deprecating and "removing" something static that replaces something static they're not talking about replacing it with something non-static. Nothing about your message read as "Lets replace Hooks with something non-static". It read much more clearly as "No one is using Hooks, how about we kill it".

...

Cheers -- Jeroen De Dauw http://www.bn2vs.com Don't panic. Don't be evil. -- _______________________________________________ Wikitech-l mailing list Wikitech-l(a)lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l

-- ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]

Daniel Friesen

3:43 p.m.

On Wed, 03 Apr 2013 22:15:27 -0700, Jeroen De Dauw <jeroendedauw(a)gmail.com> wrote:

...

Hey,

I see no reason to get rid of the hooks class.

Given you also do not understand why I think the comment is funny, I recommend you read up on why writing static code is harmful. And on how global state can hide in static "classes".

...

We use static classes other places in core.

https://yourlogicalfallacyis.com/bandwagon In almost all such cases I have seen in core this kind of use of static is bad.

...

And there's no reason to revert to hideous functions like we had before.

No one is suggesting that.

...

-- ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]

vitalif＠yourcmc.ru

8:57 p.m.

...

I second that! Also I have an idea: maybe it would be good for mediawiki it the "initialisation state" along with all constants/global variables/extension metadata/preinitialised parser/preloaded PHP files, could be cached somewhere as the whole and just loaded on each request instead of being sequentially initialised? (extension metadata = hooks/special pages/i18n files/resource modules/credits/etc) If it's a good idea then I think sequential setting of hooks via a method call is not that good? (because hooks become even less "declarative"?) Or it's not worth it and the initialisation overhead is small?

Daniel Friesen

5 Apr 5 Apr

1:22 a.m.

On Thu, 04 Apr 2013 13:57:42 -0700, <vitalif(a)yourcmc.ru> wrote:

...

You can't cache program state and loaded code like that in PHP. We explicitly have to abuse the autoloader and develop other patterns to avoid loading unused portions of code because if we don't our initialization is unreasonably long. In other languages of course like python and ruby. The application runs more like a daemon. Initialization can be done once when the program starts up. Then many requests are handled by the same process. But PHP pretends to be CGI even when not CGI. HipHop is an improvement. But we're not quite there yet and most people probably won't try using it or need it. -- ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]

Tyler Romeo

10:03 a.m.

On Thu, Apr 4, 2013 at 9:22 PM, Daniel Friesen <daniel(a)nadir-seen-fire.com>wrote;wrote:

...

In other languages of course like python and ruby. The application runs more like a daemon. Initialization can be done once when the program starts up. Then many requests are handled by the same process. But PHP pretends to be CGI even when not CGI. HipHop is an improvement. But we're not quite there yet and most people probably won't try using it or need it.

*sigh* how I wish PHP would do something like Python and Ruby. Also, does MW even work at all with HipHop anymore? *-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo(a)gmail.com

Brad Jorsch

10:44 a.m.

On Fri, Apr 5, 2013 at 6:03 AM, Tyler Romeo <tylerromeo(a)gmail.com> wrote:

...

Also, does MW even work at all with HipHop anymore?

https://www.youtube.com/watch?v=ju8suSJY0zQ -- Brad Jorsch Software Engineer Wikimedia Foundation

Tyler Romeo

12:36 p.m.

On Fri, Apr 5, 2013 at 6:44 AM, Brad Jorsch <bjorsch(a)wikimedia.org> wrote:

...

On Fri, Apr 5, 2013 at 6:03 AM, Tyler Romeo <tylerromeo(a)gmail.com> wrote:

Also, does MW even work at all with HipHop anymore?

https://www.youtube.com/watch?v=ju8suSJY0zQ

Oh boy. 1:08. I'm gonna have fun today. *-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo(a)gmail.com

vitalif＠yourcmc.ru

1:45 p.m.

...

Yeah, I understand it, the idea was to serialize globals like $wgHooks $wgAutoloadClasses and etc - and load them in the beginning of each request... So each extension would be separated in two parts: (1) metadata, executed once and then cached and (2) classes, cached by opcode cacher and loaded by a slim autoloader. By this approach you'll get rid of executing even the main file of each extension; the downside is that it of course would require some extension rewriting. I'm curious is such feature going to result in any performance benefit or not :)

Daniel Friesen

6 Apr 6 Apr

7:12 p.m.

On Fri, 05 Apr 2013 06:45:52 -0700, <vitalif(a)yourcmc.ru> wrote:

...

Opcode caches are pretty efficient at optimizing simple stuff. The difference between loading serialized data and executing the simple opcode cached files that generated that data should be negligible. And if you go the serialize route things start to become more complex and less flexible. Our biggest performance penalties came from stuff like loading dozens of classes that aren't being used and loading all of the i18n. Which we had to rewrite with a per-language cache. -- ~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]

4036

days inactive

4038

days old

wikitech-l@lists.wikimedia.org

Manage subscription

24 comments

9 participants

tags (0)

participants (9)

Antoine Musso
Brad Jorsch
Chad
Daniel Friesen
Dmitriy Sintsov
Jeroen De Dauw
Max Semenik
Tyler Romeo
vitalif＠yourcmc.ru