Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Improved class concept
<?php
/*****
All new versions will be posted at
https://github.com/rmccue/Rotor_WPPlugin
Please use that repository instead of this Gist.
******/
/*
Plugin Name: WP_Plugin class test
Description: A better test!
Author: Ryan McCue
Version: 1.0
Author URI: http://ryanmccue.info/
*/
class WP_Plugin {
public function __construct() {
$self = new ReflectionClass($this);
foreach ($self->getMethods(ReflectionMethod::IS_PUBLIC) as $method) {
$params = $method->getNumberOfParameters();
$hooks = array('filter' => array(), 'action' => array());
if (strpos($method->name, 'filter_') === 0) {
$hook = substr($method->name, 7);
$hooks['action'][$hook] = 10;
}
elseif (strpos($method->name, 'action_') === 0) {
$hook = substr($method->name, 7);
$hooks['action'][$hook] = 10;
}
else {
$doc = $method->getDocComment();
if (empty($doc) || (strpos($doc, '@wordpress-filter') === false && strpos($doc, '@wordpress-action') === false)) {
continue;
}
preg_match_all('#^\s+\*\s*@wordpress-(action|filter)\s+([\w-]+)(\s*\d+)?#im', $doc, $matches, PREG_SET_ORDER);
var_dump($matches);
if (empty($matches)) {
continue;
}
foreach ($matches as $match) {
$type = $match[1];
$hook = $match[2];
$priority = 10;
if (!empty($match[3])) {
$priority = (int) $match[3];
}
$hooks[$type][$hook] = $priority;
}
}
var_dump(__LINE__, $hooks);
foreach ($hooks['filter'] as $hook => $priority) {
add_filter($hook, array($this, $method->name), $priority, $params);
}
foreach ($hooks['action'] as $hook => $priority) {
add_action($hook, array($this, $method->name), $priority, $params);
}
}
}
}
// Here's an example of a plugin that would benefit from the above
class Some_Plugin extends WP_Plugin {
// Will filter the_title
public function filter_the_title( $title ) {
return $title . '123';
}
// Will filter the_content
public function filter_the_content( $content ) {
return $content . ' Add more content.';
}
// Will run during wp_footer
public function action_wp_footer() {
echo "I'm in the footer!";
}
/**
* @wordpress-action init
* @wordpress-action admin_init 25
*/
public function my_init() {
echo "I'm in the footer!";
}
}
$some_plugin = new Some_Plugin();
@Steveorevo

This comment has been minimized.

Copy link

@Steveorevo Steveorevo commented Jan 17, 2012

Nice. Should
$name = substr($method->name, 9);
be
$name = substr($method->name, 7);
yes?

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 17, 2012

Whoops, miscounted!

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 17, 2012

Really cool! I wouldn’t run the plugin on load, a dedicated hook seems to be more … predictable. See my fork for an example.

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 17, 2012

I’ve added a priority in my fork. Not very nice, but it works. :)

@Steveorevo

This comment has been minimized.

Copy link

@Steveorevo Steveorevo commented Jan 17, 2012

I like the priority approach. Looks good. I'd put it at the end as an option to the function signature so that omitting it would default to 10 or whatever the current default is. I.e. a function signature of action_wp_footer_10() looks good to me. :-)

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 17, 2012

Yes, that would be easier to type. But then the function would need either a regex or some rather complicated substr() acrobatics. I'm not sure what’s better.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

I tried someone almost identical to this while building out a very large class library, and I backed away from it. I'll leave more detailed comments here: http://kovshenin.com/2012/01/hey-wordpress-how-about-a-wp_plugin-class-3797/#comments

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@toscho I'm not a big fan of having the priority in the method name, I was going to go with the PHPDoc route (something like @priority 20 e.g.). Doing so means you can't differentiate between a filter named some_filter_10 and some_filter with priority 10.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@rmccue Agreed about naming. PHPDoc would be great but too expensive to parse on every page load, and probably too complicated to cache it and only load on a file change. Thoughts on this? https://gist.github.com/1630212 and https://gist.github.com/1630788

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@mikeschinkel I like it, but it's a bit less automated. Also, we're already using Reflection, which gives us the PHPDoc via $method->getDocComment(). Just need some regex for that, which is easy.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@rmccue - I didn't realize that about the $method->getDocComment(). What's the validity of extending PHPDoc with new properties? (I have no idea if that's a good or bad practice.) If not a bad idea it would certainly be a good way to handle it.

So?:

/**
 * @priority 25
 * @hookname load-page-new.php
 */

or this *(though this feels kludgy?):

/**
 * @nohook
 */

But not sure about how it's less automated? add_autohook_support( __FILE__ ) isn't much different from extends WP_Plugin and it doesn't require a subclass which is potentially problematic for certain use-cases given PHP's lack of multiple inheritance.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@mikeschinkel Adding new properties is fine. Case in point is PHPUnit, which adds various things, like @dataProvider. PHPDoc and variants (I prefer ApiGen personally) just ignore it.

Probably more:

/**
 * @priority 25
 * @action load-page-new.php
 */

That would be secondary to name-based hooking (i.e. action_wp_footer), but gives us the benefit that it works with illegal method names, like that hook.

PHP's support for multiple inheritance isn't really an issue, since the base class can extend it and subclasses don't need it. For static methods (which is my preferred), you'll need a method anyway though. Personally, I prefer doing it automatically via __construct() since automation is better.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

(Sidenote: ``` is non-standard Markdown, use indentation by a tab or 4 spaces instead)

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 18, 2012

Just to keep things interesting … what about functions that are used on more than one filter? See http://wordpress.stackexchange.com/a/12387/73 for an example. Support for multiple hooks per function would be useful.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@rmccue As for the name-based hooking, I tried that for a project and it quickly gets overwhelming with long hooks names and just feels really wrong. Try it a while before you advocate it. Personally I'd really hate to see that become a "blessed" approach in WordPress.

Use in __construct() is a problem for several reasons. As proposed WP_Plugin would be specific to plugins; what about classes for hooks created in themes? Using a base class closes a flexibility door; using a function call does not. One could subclass WP_Plugin but that would be semantically ugly and might bring along unwanted functionality if WP_Plugin is enhanced in the future. Plus anyone subclassing the plugin's class might create their own constructor and forget to call parent::__construct().

But most importantly, I think the idea to add a base class would need a lot more discussion and would require a lot more thought than adding of this simple feature. Using a function means one cannot use this feature in their own classes that already have a base class without needing to worry about any other ramifications. If we want a base class then I think that should be another separate discussion entirely.

So I've updated my add_autohook_support() function and its example but using PHPDoc tags @wp-nohook, @wp-action, @wp-filter and @wp-priority so that it's clear when see them they are WordPress specific and that there will be no chance of conflict with future tags added to the standard.

P.S. I just the back ticks because that's in the GitHub Flavoried Markdown docs linked to the edit window. But thanks, spaces are better.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@toscho - The proposal I posted can address your use case using an @wp-action or @wp-filter tag in the PHPDoc comment. Thanks again to @rmmcue for the great idea to use the PHPDoc comment.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@mikeschinkel I use it on most of my projects, but I agree, it does get overwhelming. I find that WordPress hooks aren't usually the culprits, it's more likely to be another plugin I'm interfacing with that has that problem.

I do agree that it's possible that functionality could change in the future, but I think that would be the responsibility of whoever is creating the class to make an API promise. Breaking any sort of API is something that I completely abhor, and something I try and avoid wherever possible. Forgetting to call the parent's constructor is a possible problem, but one that would be noticed very quickly (nothing would be hooked at all), so I don't see that as a particularly bad thing.

Apart from this, what would you envision being in a base class? I can't see too many other things that couldn't be supported through forward compatibility; perhaps even a @wp-class-supports tag similar to add_theme_support, etc. If you'd rather discuss it on your blog or something more long form, feel free. GitHub comments aren't always the best forum. :)

I do like the prefixing, good call there. I'll be prefixing mine with something different personally. I'm not a fan of the nohook though, I think you should be explicit if you're going to hook in. I don't want extra methods to be accidentally hooked, and I personally feel that's too much magic.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

I've implemented a @wp-action and @wp-filter giving support for multiple hooks for methods which don't use magic naming. Why did I do that? I think it's misleading to call a method action_init and yet have it hooked to both admin_init and init (which is just an example anyway, doing that would be redundant).

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@rmmccue - I agree about changing the API, but there's explicit changes and implicit changes. If WordPress adds a new function to core then they have implicitly changed their API. If I had previously written a function in my plugin with the same name then their new API would conflict with my plugin. That's the level of changes I'm referring to, i.e. if we had a WP_Plugin class and then WordPress added new methods to it.

The issue is not that I envision something being in the base class but that I'm not able to envision what other people might come up with. In that case, I err on the side of caution.

As for "too much magic" I see your point, but then it goes against what Konstantin was trying to achieve, to minimize the amount of work required. Maybe the solution to our differences is to do the following:

  • Create both a add_autohook_support() function and a WP_Plugin class which has its __construct() method calling add_authhook_support() which gives us all a choice which approach to use.
  • Add a @wp-autohook PHPDoc comment tag for the class with 'implicit' or 'explicit' as potential values, and 'explicit' could be default.

Would that reconcile our differences on implementation?

I agree about using our blogs to discuss but I'm moving my blog and because of client work haven't finished so don't want to blog there until I have it moved.

BTW, looking at your most recent update I see you don't address priority, which would be easy? I do see you caught the need to use use both @wp-filter and @wp-action for the same method and with multiple methods, which is a requirement I missed.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@mikeschinkel When I refer to changing the API, I refer to the author of the class changing it. Classes should be named to not conflict with possible core classes, which is why my actual one will not be named WP_Plugin. WordPress would not be able to add new methods anyway, as they would have to redefine the class from scratch, which would cause a fatal error due to duplicate naming.

I agree, which is why forward compatibility is a big thing. My implementation will be like every API I own: a promise of forward compatibility with enough notice of any incompatible changes that you'll have several versions to migrate.

I think there's a huge distinction between naming your method action_init and having that hooked to init, versus naming it init and having it hooked to init. I occasionally call my methods names that might conflict with WP hook names, and I don't want to think about whether that's going to be hooked. To me, that seems to be setting yourself up for disaster, and it's also both too much magic and not enough: it means I have to worry about how the method itself works and whether the method will be getting hooked or not.

I think the best solution here would be simply to agree to disagree and have separate implementations. :)

Once I clear up the class and add some extra things to it, I'll post on my blog about it and set up a proper GitHub repository for it, rather than using Gist.

Regarding priority, I'd see that being specified as @wp-action init 10, being optional, of course. The code for this means I have to rewrite my code a bit, so I'll be leaving that until tonight when I can sit down and have some time to think properly about implementation.

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 18, 2012

I like the auto hook idea. Very much: https://gist.github.com/1631723 :)

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@rmmcue - My understanding was that this was a proposal for inclusion in WordPress core, hence the naming of WP_Plugin and hence the concern that someone else might change it out from under you. If it's for your own use that's different.

As for WordPress not being able to add new methods, do you mean in your own class or one they may provide in a future core? I assume you were referring to the former and not the latter?

I like your implementation of priority, and I've updated mine to use your syntax of allowing priority after hook name, but it's not a good solution for when you do not need to specify the hook name so @wp-priority is still needed in that case.

We are going to have to agree to disagree about the action_ and filter_ prefixes. After trying really hard for several months to make that convention work I've come to believe it is a hack and too Drupalish, but if you like it for your own classes then who am I to argue? :-) Still it's a shame we can't agree on something not yet in core that could potentially become defacto standard, especially since it seems we are settling on the almost the same but incompatible implementation of extended PHPDoc tags.

So I guess it's a race to see who can blog there's first then. ;-) You'll probably win that race given I have lots to do before I can update my blog.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@tosco - Nice. But consider prefixing PHPDoc tags with @wp-?

BTW, it's funny how you really like auto hook and @rmmcue really hates it. :)

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 18, 2012

@mikeschinkel filter and action are not reserved PHPDoc parameters, and since WP is already the context (obviously) I'd rather keep things as simple as possible.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@mikeschinkel I was referring to including it in my own library. I don't believe something like this will make it into core, since there are still a few too many developers stuck in a PHP 4 mindset (no public/protected on methods, using objects instead of static methods, etc.). I do think that this has a possibility of being included in the future, but not in the next couple of versions.

Personally, I'm not a big fan of the naming syntax, although I can see the benefits. I'm supporting it here because I know people do like that syntax and will want to use it. In my own classes, I'll be using only the PHPDoc (if I do use this myself, since it depends on how long the implementation takes). I do like the @wp-priority idea though.

On your code itself: you can reduce that down to a single regex for priority as well, without parsing it later. You also don't allow both actions and filters for the same method (although really, you have no distinction anyway, which I'm not a fan of). Also, $hooks = false; could simply be continue; instead.

I'm not so sure about that race, I'm horrible with my blogging habits :)

@toscho Of course, but if PHPDoc itself added them, there could be problems. I'm still debating on the wp- prefix personally, since it does make it a bit cleaner without. (Also, when I say "PHPDoc itself", I mean the pseudo-documentation standard, not the parser. The parser is no longer being worked on, from what I can gather.)

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@tosco - People reading your code who are not intimately familiar with PHPDoc parameters might assume they are standard and go looking for what they mean and have difficulty figuring it out. Also, you should prefix them for the same reasons that you should prefix function names in your WordPress plugins, because the standard may get extended to include them in the future.

BTW, I suggested wp- and not t5- because I'm thinking we are iterating toward something we'd propose on trac.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@mikeschinkel FWIW, my actual class uses a different prefix. I only changed it to wp- because I'd rather not reveal the name yet.

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 18, 2012

Okay, I’m convinced. I added the wp- prefix.

Mike, do you really think a ticket has a chance where we both are involved? No way.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@rmccue - regarding it being a candidate for core, hope spring eternal. ;-)

I'm supporting it here because I know people do like that syntax and will want to use it.

Decisions, not options?

On your code itself: you can reduce that down to a single regex for priority as well, without parsing it later.

I don't follow. Each method could have a different priority and I only call preg_match() once per method?

You also don't allow both actions and filters for the same method (although really, you have no distinction anyway, which I'm not a fan of).

I don't see any reason to make a distinction; if WordPress core makes a distinction then we could update this code when they do, but until then I don't see a benefit.

I also think it would be confusing to allow a method to be used as both an action and a filter; better to force one or the other IMO, unless you can give me a good use-case where it should really be allowed.

Also, I focused on minimizing the about of string processing and regex matching because I don't think the extra processing is worth it. I could see this code getting used a lot in a plugin with lots of classes (i.e. a plugin collection like Sunrise.)

Also, $hooks = false; could simply be continue; instead.

True, thanks. Updated.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@toscho - Mike, do you really think a ticket has a chance where we both are involved? No way.

ROFL! :-)

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@rmccue FWIW, my actual class uses a different prefix. I only changed it to wp- because I'd rather not reveal the name yet.

Ooooh, mysterious. ;-)

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@rmccue @toscho - Last comment for the night. It's been really great collaborating on this even though we all have our own versions. Would like to see more of this...

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@mikeschinkel

Decisions, not options?

A great mantra, but not one to stick by religiously. In this case, I'm optimising for the most common use case: one action/filter, default priority. For that, simply prefixing should be more than enough. For anything more advanced, you need to step up the syntax, and that's the idea of the PHPDoc tags.

I don't follow. Each method could have a different priority and I only call preg_match() once per method?

The regex I'm using is ^\s+\*\s*@wp-filter\s*([\w-]+)\s*(\d+)? which will give you the priority at the same time, rather than having to split it later. It also avoids having to use the dot metacharacter, which you should always avoid when you can be explicit.

I don't see any reason to make a distinction; if WordPress core makes a distinction then we could update this code when they do, but until then I don't see a benefit.

By the time that's done, this code might be copied and the problem will already be there. In addition, not making a distinction relies on a private API, that being the $wp_filter variable. If the format of that changes and filters and actions do become distinct, then that is entirely our problem for relying on a private API.

I also think it would be confusing to allow a method to be used as both an action and a filter; better to force one or the other IMO, unless you can give me a good use-case where it should really be allowed.

I do agree, but since it's explicitly marked by the user, I think it's perfectly fine. We might not be able to think of a use-case, but it doesn't mean one doesn't exist.

Also, I focused on minimizing the about of string processing and regex matching because I don't think the extra processing is worth it. I could see this code getting used a lot in a plugin with lots of classes (i.e. a plugin collection like Sunrise.)

I'm also a big fan of that (I rewrote Mark Jaquith's WP readme.txt parser to avoid regex matching). There are optimisations that could be done here, but we're only hashing out rough methods here, so that's OK. In this case, you can pick which type using @wp-(action|filter), and grab that through the results without much bother.

It's been really great collaborating on this even though we all have our own versions. Would like to see more of this...

Agreed. I'm only just able to properly participate in these sorts of discussions since I finally have some proper free time on my hands.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

I've published my blog post about this now.

@GaryJones

This comment has been minimized.

Copy link

@GaryJones GaryJones commented Jan 18, 2012

In terms of tag prefix, please consider section 5.3.1 of (work in progress) https://github.com/mvriel/Docblox/blob/develop/docs/PSR.md (ping @mvriel) - in this case, the vendor would be WordPress or wordpress, not wp.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@GaryJones Fantastic, I had no idea there was anything resembling a standard. I think the vendor name prefix is the best one for WP.

@GaryJones

This comment has been minimized.

Copy link

@GaryJones GaryJones commented Jan 18, 2012

It's not an agreed standard yet, but Mike (author of phpDocumentor replacement DocBlox) is hoping to push it to the PHP Standards Working Group when it's done so that all of the PHP document generators out there can be interoperable by having everyone who wants to make use of them document their code consistently.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

I've now updated the class to include priorities, and a wordpress prefix instead.

@GaryJones Yeah, it's a fantastic idea. I'd love to see a lot more PSRs.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

Fixed my implementation (had a few things in the wrong place).

@GaryJones

This comment has been minimized.

Copy link

@GaryJones GaryJones commented Jan 18, 2012

FWIW, if any docblocks tags were going to be used for priority, I'd like to see them also used for the action or filter name - examples being:

class My_Plugin extends WP_Plugin {

    /**
     * Short description.
     *
     * Long description...
     *
     * @since 1.0.0
     *
     * @wordpress-action admin_init
     */
    public function my_randomly_named_function() {
        //...
    }

    /**
     * Short description.
     *
     * Long description...
     *
     * @since 1.0.0
     *
     * @wordpress-filter the_content 99
     * 
     * @param string $arg The existing content of the current post.
     * 
     * @return string The amended content of the current post.
     */
    public function my_filter_function( $arg ) {
        //...
        return $arg;
    }

}

Here, the priority on the action defaults to 10 as it's not specified. I do prefer the separate -action and -filter tags, since it's clear at a glance what the function is touching, and also document generators may have their own plugins written that can extract a set of filter hook function out separately from a set of action hook functions.

The first argument of wordpress-(action|filter) would be the name of the hook, the second optional argument would be the priority. As has already been mentioned, the number of arguments being passed could be determined by Reflection, or a third argument to the tag could be used if absolutely necessary.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@GaryJones That's exactly what this implementation does, thanks for documenting it. :)

@GaryJones

This comment has been minimized.

Copy link

@GaryJones GaryJones commented Jan 18, 2012

@rmccue - no problem - thought it was, but easier to sum up in one example that read through all these comments!

To pick up on one point - Ryan originally (and currently in the gist) wanted to prefix function names with action_ and filter_ to avoid otherwise randomly named functions getting automatically hooked in should new hooks be added that match the name of the function. However, there would also be a potential for a new hook to be added in whose name already starts with action_ or filter_, which in theory would then need plugin class methods to be named, say, action_action_foo for the prefix stripping to be done correctly. You'd be swapping one problem for a similar problem, since there are no reserved keywords or phrases on the naming of WP hooks. There's also a stink of Hungarian notation about them...

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 18, 2012

@GaryJones I agree, I'm not a big fan of them for that reason (Hungarian-ish). I'm considering removing them completely, but I do see merit to them.

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 18, 2012

I have simplified my class T5_Autohook. And I use the wordpress- now too. Should I move this to a real repo?

Also, I don’t think anymore it has to be a plugin … maybe a mu-plugin? What about themes? Oh, we need this in core!

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@GaryJones Big thanks for the PSR. However, I'm curious why wp- would not be appropriate since that's the namespace WordPress uses for so many thing?

@rmccue Using wordpress- implies an addition to WordPress core, but you said that's not your direction? Also @GaryJones provides yet another reason not to use action_ or filter_ prefixes. Hoping you can let go of your attachment to them. :-)

@toscho Agreed, it would be nice to get something like this into core; realistically I wonder if there is any chance? However...

I'm a bit depressed about what Rarst pointed out regarding how opcode caches can discard the information. For them we'd need to scan the source ourselves, which could be a bit much for larger files and would certainly need to be cached on file change (maybe using this function) which would force us to do on the first available hook call otherwise hook calls could be missed, or do the writing to wp_options directly.

Things that make you go hmm....

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 18, 2012

Hm, stripped comments are a big problem. Running our own tokenizer is way too much work for this purpose, isn’t it?

By the way, I just implemented support for shortcodes too. :)

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@toscho - Yes, our own tokenizer is problem too much work. Not sure what the best next step should be...

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 18, 2012

eAccelerator has to be compiled with --with-eaccelerator-doc-comment-inclusion to use PHPDoc. You cannot set this at runtime. Bah!

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@toscho - Not as elegant, but what if we used method constants?

function load_page() {
   const WP_ACTION = 'load-page.php 99';
   // Do work here...
}
@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 18, 2012

@mikeschinkel You cannot redeclare constants. Applying multiple methods to an action would be … tricky.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 18, 2012

@toscho - Why would we need to redeclare? Just reserve a set of names (WP_AUTOHOOK, WP_ACTION, WP_FILTER, WP_PRIORITY and WP_NOHOOK) for this use.

Multiple methods could look like this:

function load_page() {
   const WP_ACTION = 'load-page.php 99, load-page-new.php 99';
   // Do work here...
}
@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 19, 2012

@mikeschinkel I think @GaryJones makes more of a point towards removing those rather than getting rid of the prefixes. In my classes at least, there's no chance of me implementing implicit hooking; it would break compatibility with my plugins that offer public APIs.

Regarding eAccelerator, https://github.com/Andrewsville/PHP-Token-Reflection should work fine. We can use that if eAccelerator is detected at all, and doc comments aren't found for any method, so it wouldn't be a performance hit for anyone except people using that mode. I'll start a proper repo and add all this sometime today.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 19, 2012

@rmccue:

I think @GaryJones makes more of a point towards removing those rather than getting rid of the prefixes.

I don't understand. Removing what vs. getting rid of prefixes?

In my classes at least, there's no chance of me implementing implicit hooking; it would break compatibility with my plugins that offer public APIs. ... I'll start a proper repo and add all this sometime today.

For your own classes, of course. But the very fact you published this publicly in reference to someone else's post and plan to start a proper repo implies you want to influence others. In that case "you not breaking compatibility with your own plugins" is not a high enough bar.

Regarding eAccelerator, https://github.com/Andrewsville/PHP-Token-Reflection should work fine.

For those using eAccellerator it would be a big performance hit, rather ironic considering the reason they'd be using eAccelerator.

Anyway, I wish it was more than me and you debating this. I'd really like to hear numerous other's perspective on all of this.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 19, 2012

@mikeschinkel Also, regarding class constants, that would be redeclaring each constant per method, which you can't do.

I don't understand. Removing what vs. getting rid of prefixes?

Removing the non-PHPDoc option.

For your own classes, of course. But the very fact you published this publicly in reference to someone else's post and plan to start a proper repo implies you want to influence others. In that case "you not breaking compatibility with your own plugins" is not a high enough bar.

The thing I'm considering though is that if this breaks my plugins, it's definitely going to break someone else's plugins too. Again, I'd rather people explicitly state that they want to hook in. I agree with @GaryJones that it can become redundant if an action/filter begins with action_ or filter_, but from a grep of WP's source, this doesn't occur at the moment. In any case, a redundant name is better than magic behind the scenes that has to be undone.

For those using eAccellerator it would be a big performance hit, rather ironic considering the reason they'd be using eAccelerator.

I agree, but we can give them a notice in the admin that they need to recompile eAccelerator without that.

Anyway, I wish it was more than me and you debating this. I'd really like to hear numerous other's perspective on all of this.

I wonder if WP-Hackers would be interested in such a discussion.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 19, 2012

@rmccue:

In any case, a redundant name is better than magic behind the scenes that has to be undone.

If I want to name a function filter_foo() I want to be able to and not be blocked by automatic hooking. To me action_ and filter_ combined with __construct are the most offensive magic. Thus we are at an impasse on that point.

(Note: current code in Sunrise uses action_ and do_ prefixes for what we call "instance" filters and actions. Our plans are to rip the prefixes out and replace with a more elegant solution before beta.)

I wonder if WP-Hackers would be interested in such a discussion.

Unfortunately I fear they would mostly be annoyed by it. :-(

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 19, 2012

@mikeschinkel

If I want to name a function filter_foo() I want to be able to and not be blocked by automatic hooking. To me action_ and filter_ combined with __construct are the most offensive magic.

To be clear: I'm not talking about magic in terms of doing things automatically. From that context, all of this is magical because it's all done automatically. I refer to magic as something happening without me telling it to nor knowing why it happened.

For non-prefixed methods, my class might go from working one day to not the next without me changing anything (WP adding a filter/action, e.g.). That, to me, is horrible magic, because I now need to debug why that's not working. In addition, it can cause problems in places that I'm not even looking at: imagine if they add a filter to an Atom API. Suddenly, some sites will experience issues that you didn't catch, because your code is only supposed to add "Hello Dolly" to the admin.

On the other hand, being explicit about it and saying "I want this hooked in" says to the code "I know what I'm doing, and this is what I would like to happen". If a new filter/action is added, it makes no difference, since there's no chance of any extra methods being involved apart from the ones I have specified. The only way to break this would be to change the filter/action, which is then WordPress changing their API, and that almost never occurs due to their regard for backward compatibility.

On the topic of prefixes vs. PHPDoc: I do see the merit in being able to prefix a method with action_ or filter_, but I'm more in favour of PHPDoc tags. I'm writing up a full set of classes at the moment that I'll push to GitHub soon that will have a toggle for that, disabled by default.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 19, 2012

@rmccue - You keep focusing on the implicit option of my code and ignoring the explicit option I proposed. You don't have to use function naming prefixes, which is what I'm objecting to, to require hooks be explicit. You can use one of the other options to declare them explicitly.

Which, ironically is exactly what add_action() and add_filter() do so I'm really not sure why this even interests you. You are just adding overhead to do things explicitly in a non-standard way.

But your last sentence makes me think maybe we've just been talking past each other. I (would have) prefer(ed) the PHPDoc approach (if it hadn't been for the opcode cache issue.) My biggest objection still are the function name prefixes action_ and filter_.

But whatever, we've beat this into the ground. Time for some other topic to occupy our time?

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 19, 2012

@mikeschinkel The explicit option is equal to mine, hence why I haven't commented on it, assuming you mean the PHPDoc. If you mean the constants, I mentioned above that this would not work.

You are just adding overhead to do things explicitly in a non-standard way.

It makes coding much easier to have this in the same place. Having to go to a ::bootstrap() or ->__construct() every time I want to check a hook makes no sense to me.

if it hadn't been for the opcode cache issue

Which can be worked around without much issue (although adds overhead, but that's an unfortunate effect of their server configuration). The only other thing I can think of is turning this into a compiler, which would work, but might be a bit of a pain. (e.g. it parses the comments and creates a new method with all the relevant code to hook it in)

My biggest objection still are the function name prefixes action_ and filter_.

My argument has been against your preferred option (as far as I can tell), which is no prefix at all. As far as I can tell, it has all the detriments of your argument but with several others, hence why I'm unconvinced.

In any case, here's the full version, including a static class. As for moving forward on that, I think a nohook tag would satisfy your concern about wanting to have a filter_foo() method. If you're sure about having no prefixes, I'll add that, but I don't personally see it as having merit. (By which I mean, I'll add configurable prefixes, defaulting to action_ and filter_)

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 19, 2012

@rmccue

If you mean the constants, I mentioned above that this would not work.

Why would constants not work? I don't see your mention, but I also don't see why they wouldn't work using reflection to grab the constant's value vs. the DocComment. They would also be more performant than DocComments, i.e. no need for preg_match().

My argument has been against your preferred option (as far as I can tell),

Ok, please stop. This is not a political race where you have to position me. So stop putting words into my mouth.

I have no "preferred" option, both are useful. Implicit is good for rapid prototyping and private use, and Explicit is good for published code.

Which can be worked around without much issue

I really, really wanted the PHPDoc approach to work. But I frankly believe the workaround would be too processing intensive and/or kludgy. My personal opinion only. At this point I'm not sure what a workable solution would be.

In any case, here's the full version, including a static class....

Thanks for the offer to accommodate my concerns, but your use of a base class makes it a non-starter for me. We'll just have to diverge at this point.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 19, 2012

@mikeschinkel

Why would constants not work?

Apologies, I actually misread your original comment and came to the same conclusion as @toscho, as I thought the plan was to redefine constants for each method. I see you clarified that in your response, although I missed that on my first reading.

It's certainly possible, and I do see some merit in it, although I think this suffers from the same problem as simply hooking in a method. The advantage of this class from my point of view is the ability to have the hooks with the method itself, so I think you lose some of that with a single constant. Another thought I've had though is that we could have prefixed constants, so WP_ACTION_..., but I think that becomes a bit kludgy. I'm not sure if you can get the full benefits from the constants.

Ok, please stop. This is not a political race where you have to position me. So stop putting words into my mouth.

Apologies, I came off as unnecessarily hostile and that was not my intention at all. I was trying to boil it down to the core to see if we were, as you said, "talking past each other".

Implicit is good for rapid prototyping and private use, and Explicit is good for published code.

I do agree. My issue was that people will forget to switch to explicit when publishing their code, opening it to breakages.

I think that overriding the prefixes however wouldn't have this problem, as it's immediately obvious that you've specified this in the code.

I really, really wanted the PHPDoc approach to work. But I frankly believe the workaround would be too processing intensive and/or kludgy.

I do have to agree that it will be a bit kludgy, but that's one of the side-effects of working around bugs (SimplePie has entire classes dedicated to working around bugs). (I do see this as a bug in eAccelerator, not a feature. Other libraries such as Doctrine rely on PHPDoc, and stripping it has almost no effect on performance.)

To be honest, I'm not sure if it's worth working around, or if it's better to simply tell the user that their installation is broken.

your use of a base class makes it a non-starter for me

I've updated the class, and Rotor_WPPlugin_Base::_register_hooks can now be called publically and without extending.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 19, 2012

@rmccue - Thanks, it's all good.

At this point I'm going to go back and focus on Sunrise and it's sponsoring project for a while.

@markjaquith

This comment has been minimized.

Copy link

@markjaquith markjaquith commented Jan 20, 2012

Seen this? I worked on this a while ago: https://gist.github.com/3b79a70d9bf8fa3603f5

@markjaquith

This comment has been minimized.

Copy link

@markjaquith markjaquith commented Jan 20, 2012

I should also add that while I was momentarily excited about the idea of automatic hooks, I've not used them, for the following reasons:

  • The names look clunky. My standard was {action|filter}__{name}{_priority(optional)}. So: action__init or filter__the_title_20.
  • You have no control over when the hooking happens. The best practice is to "layer" your hooks. Not everything can or should be registered at init or plugins_loaded. This sort of layered hooking should happen in almost every plugin, but it's not possible with automatic hooking.
@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 20, 2012

@markjaquith Of course, you can delay the hook execution, and you don’t need those ugly function names. See https://gist.github.com/1631723 for another example. The only problem now is the availability of PHPDoc comments for the reflection API.
For my use cases it is good enough. For WordPress it has to be more … compatible.

@markjaquith

This comment has been minimized.

Copy link

@markjaquith markjaquith commented Jan 20, 2012

@toscho — I'd rather focus on something that all plugin authors could use, not company-internal code that will never see the light of day.

To that end, I'd rather do something that simplifies hooking, without requiring esoteric PHP setups, strict PHPDoc comments, or clunky method names.

Something like this, for instance:

<?php
class Foo_Plugin extends WP_Plugin {
    public function __construct() {
        $this->add_action( 'init' ); // Hook name only. Vast majority will be this.
    }

    public function init() {
        $this->add_filter( 'the_title', 25 ); // Hook name and priority. Some will be this.
        $this->add_filter( 'the_title', 'the_title2' ); // Hook name and non-standard method name name. Almost never used.
    }

    public function the_title( $title ) {
        return str_replace( 'foo', 'bar', $title );
    }

    public function the_title2( $title ) {
        return str_replace( 'foo2', 'bar2', $title );
    }
}

WP_Plugin::register_plugin( 'Foo_Plugin' );
@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 20, 2012

@markjaquith I like the idea of the variable parameters to add_filter, but I'm also still a fan of the PHPDoc comments. I still think the benefits outweigh the problems. (Especially given that eAccelerator doesn't appear to be supported any longer.)

As per your original implementation, I'm not a fan of having the priority in the name, however it's otherwise identical to my implementation with:

parent::__construct(array('filter' => 'filter__', 'action' => 'action__'));

I agree with not always hooking, but implementing an alternative to that is probably going to be hard (e.g, adding a @wp-add-hook-on plugins-loaded).

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 20, 2012

Implemented the variable parameters to add_filter/add_action

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 20, 2012

@markjaquith:

Questions about your proposal:

  • Do you propose this syntax: $this->add_filter( 'the_title', 'the_title2', 25 ); when a plugin needs hookname <> method name and when you need priority?
  • How would you propose support when static methods are used for hooks? This is a preferred architecture for some plugin developers including myself because it's easier to remove hooks and doesn't require an otherwise useless instance to hang around in memory.
  • How do you envision themes that need to implement hooks using the proposed WP_Plugin class that want to use a class to limit public name usage in namespace for their themes? Would themes also use WP_Plugin, or would you introduce a WP_Theme class that supports the same methods?
  • What about a plugin that contains multiple classes? Would you envision all classes inherit from WP_Plugin?
  • Do you envision core adding class:static_method() APIs in the future that theme developers would need to call vs. the more simplistic the_function_as_template_tag() standard currently provided for theme developers?

Non-standard method name for hooks

You mention in your code comment that they are almost never used, and yes they are rare but I wanted to delineate for others who may not recognize (some of) the places where they are required or preferred:

  • Hooks that include filenames such as 'load-plugins.php'.
  • Hooks with dashes such as and 'admin_head-$page'.
  • Multiple hooks using one function such as one used for 'load-page.php' and 'load-page-new.php'.

Automatic Hooks

As discussion above in the long thread, it would be possible to do automatic hooking on an exclusion basis vs. an inclusion basis.

Your auto-hook example

I noticed you used 99 as number of parameters for all hooks. I have not explored that, does it not cause problems? And if not, why doesn't WordPress core use it?

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 20, 2012

@mikeschinkel

I noticed you used 99 as number of parameters for all hooks. I have not explored that, does it not cause problems? And if not, why doesn't WordPress core use it?

It's essentially equivalent to all parameters, since no action/filter will ever have that many parameters. It shouldn't cause any problems, but some functions which are hooked in multiple times might need different numbers of parameters for each hook, so that could cause a problem. I do agree though, it should be the default in WordPress.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 20, 2012

@rmccue - Thanks. I'm also curious about the political reason for not having it in core in addition to any technical reason.

@markjaquith

This comment has been minimized.

Copy link

@markjaquith markjaquith commented Jan 20, 2012

@mikeschinkel

Very basic implementation here: https://gist.github.com/1645537

Do you propose this syntax: $this->add_filter( 'the_title', 'the_title2', 25 ); when a plugin needs hookname <> method name and when you need priority?

Yes. Or you can flip the priority and method name parameters. As one is always a string and the other is always an integer, we can easily tell what was intended. This allows you to leave off the one you don't need to provide (method name with default priority, or priority with default method name).

How would you propose support when static methods are used for hooks? This is a preferred architecture for some plugin developers including myself because it's easier to remove hooks and doesn't require an otherwise useless instance to hang around in memory.

Not really interested in that pattern. You have to store your ephemeral instance data somewhere. It's rare for an instance to have no data. And if it does, there's no reason to worry about a memory footprint. Would rather just go with one pattern rather than go "oops, gotta hold on to some data here... better change this to an instance now!"

How do you envision themes that need to implement hooks using the proposed WP_Plugin class that want to use a class to limit public name usage in namespace for their themes? Would themes also use WP_Plugin, or would you introduce a WP_Theme class that supports the same methods?

I might just have WP_Theme extend from WP_Plugin, and then have themes extend from WP_Theme.

What about a plugin that contains multiple classes? Would you envision all classes inherit from WP_Plugin?

Depends. If they want to trigger things in the main plugin instance, they should have an interface to the main plugin... some sort of global plugin factory that gives you the instance. If they want to use WP_Plugin methods, then they should inherit from it. At a certain point, a plugin is so complex that someone can just design their own personalized convenience wrapper.

I noticed you used 99 as number of parameters for all hooks. I have not explored that, does it not cause problems? And if not, why doesn't WordPress core use it?

We don't use it in WordPress core because built-in PHP functions cannot be passed more parameters than they accept without causing problems. The built-in PHP functions aren't build with PHP, so they don't behave the same way. It was discussed in this ticket. http://core.trac.wordpress.org/ticket/14671 I proposed an idea, but didn't really like it, and it's just sat there.

5ubliminal had some interesting comments towards the end about using reflection which might be worth investigating.

One other closing thought: do not underestimate how mind-meltingly confusing it is to someone reading your code when a comment changes how the code works.

@markjaquith

This comment has been minimized.

Copy link

@markjaquith markjaquith commented Jan 20, 2012

You should also check out @scribu's wp-scb-framework.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 20, 2012

@markjaquith

You should also check out @scribu's wp-scb-framework.

Oh cool, wish I'd known about that before. Looks very similar in terms of hooking ability, although the PHPDoc tags aren't prefixed.

Not really interested in that pattern. You have to store your ephemeral instance data somewhere. It's rare for an instance to have no data. And if it does, there's no reason to worry about a memory footprint. Would rather just go with one pattern rather than go "oops, gotta hold on to some data here... better change this to an instance now!"

That can be stored in static properties if needed. Using objects is an antipattern in the WordPress plugin community that I'd very much like to see die.

5ubliminal had some interesting comments towards the end about using reflection which might be worth investigating.

See https://github.com/rmccue/Rotor_WPPlugin/blob/master/plugin.php#L48 - Works fantastically.

One other closing thought: do not underestimate how mind-meltingly confusing it is to someone reading your code when a comment changes how the code works.

I think that's a good point. Most people do see it in terms of being ignored, but I think there's a certain familiarity with it from plugin headers. In any case, it's certainly worth noting in a plugin class how methods are hooked in.

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 20, 2012

@markjaquith - Thanks for your quick reply.

Not really interested in that pattern.

That's disheartening to learn, at least if you are considering this for inclusion in core.

You have to store your ephemeral instance data somewhere. It's rare for an instance to have no data.

I find the static pattern works better than the instance pattern. For example I have a set of plugins that have over 50 classes where most use static methods and its rare for any of them to have any ephemeral data. Yes there are classes that are used for instances, but they are typically not coupled to WordPress and thus don't implement hooks using their methods.

We don't use it in WordPress core because built-in PHP functions cannot be passed more parameters than they accept without causing problems.

Gotcha, thanks. That clarifies why for methods in a class it wouldn't be an issue.

You should also check out @scribu's wp-scb-framework.

What specifically about it? It seems rather simple.

@markjaquith

This comment has been minimized.

Copy link

@markjaquith markjaquith commented Jan 20, 2012

That can be stored in static properties if needed. Using objects is an antipattern in the WordPress plugin community that I'd very much like to see die.

How are they appreciably different? I mostly see it as a syntactic difference:

  • $this->foo vs self::$foo
  • $this->bar() vs self::bar()
  • add_action( 'foo', array( $this, 'foo' ) ) vs add_action( 'foo', array( __CLASS__, 'foo' ) )
@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 20, 2012

How are they appreciably different? I mostly see it as a syntactic difference:

new PluginClass(); is a misuse of PHP objects. Objects should be used when you actually need objects, not simply for grouping methods together (for that, the best option is a namespace, but static methods in a class is good enough). You'll only ever have one copy of your plugin in memory, so having the ability to create a new instance has no use.

In addition, $myplugin = new PluginClass(); is using global variables, which is another antipattern I hate (especially when it's for no reason).

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 20, 2012

@markjaquith - I concur with @rmccue on his points and will add that it's much easier to remove a hook with a static method than an instance method. You only need the class name to remove the hooked static method but you need the actual instance to remove the hooked instance method.

@markjaquith

This comment has been minimized.

Copy link

@markjaquith markjaquith commented Jan 20, 2012

That's disheartening to learn, at least if you are considering this for inclusion in core.

We're not considering it for core at this time. I was just talking in terms of "if I were going to write my own plugin framework...". We've been talking about plugin frameworks for years, and I'm happy to let people go in a bunch of directions with it independent of core and see what happens. The only real difference between putting it in core and having it be a third party thing is that as a third party thing you have to take care of versioning (i.e. WP_Plugin_v34) so that plugin A which expects version 34 doesn't get served version 22 of the framework class.

I find the static pattern works better than the instance pattern.

Works better how? Honestly asking here. I've always seen it as a syntactic difference, and I just have a (weak) preference for the instance pattern. Skinny arrows look nicer than a T_PAAMAYIM_NEKUDOTAYIM to me. :-)

What specifically about it? It seems rather simple.

Just some prior art. And I always find @scribu's code fascinating to read!

@markjaquith

This comment has been minimized.

Copy link

@markjaquith markjaquith commented Jan 20, 2012

Ah, we cross-posted.

In addition, $myplugin = new PluginClass(); is using global variables

Well, that is its own issue, and certainly something to discourage.

Objects should be used when you actually need objects, not simply for grouping methods together (for that, the best option is a namespace, but static methods in a class is good enough). You'll only ever have one copy of your plugin in memory, so having the ability to create a new instance has no use.

I can see that argument... but that's still a bit of a stylistic observation.

I concur with @rmccue on his points and will add that it's much easier to remove a hook with a static method than an instance method. You only need the class name to remove the hooked static method but you need the actual instance to remove the hooked instance method.

Aha. This is a solid objection (get it? objection? — ::crickets::). I've always stashed the instance in Class_Name::$instance to allow for people to hook in, but that isn't as obvious as just using the class name. Okay, you've given me something to think about!

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 20, 2012

@mikeschinkel:

You only need the class name to remove the hooked static method but you need the actual instance to remove the hooked instance method.

Great point.

@markjaquith:

Works better how? Honestly asking here. I've always seen it as a syntactic difference, and I just have a (weak) preference for the instance pattern. Skinny arrows look nicer than a T_PAAMAYIM_NEKUDOTAYIM to me. :-)

"Works better" meaning "works better with my mindset". Using an object (to me) makes no sense, because there's no sense of differentiation between objects. If I make two PluginClass objects, what's the difference? Compare to the WP_User class, which is a correct use of objects.

My mindset is always to default to static classes and only use objects if necessary, rather than the other way around.

Well, that is its own issue, and certainly something to discourage.

The problem is that one leads to another. I also don't particularly like the look of new PluginClass without assignment, it looks ugly to me. :)

I've always stashed the instance in Class_Name::$instance

So, you're using the singleton pattern? That's another thing I hate (although, I have used them where it made sense). Singletons are essentially just a reimplementation of static classes, since you go from Class_Name::method() to Class_Name::$instance->method()

The only real difference between putting it in core and having it be a third party thing is that as a third party thing you have to take care of versioning (i.e. WP_Plugin_v34) so that plugin A which expects version 34 doesn't get served version 22 of the framework class.

This can be a problem, but it's simply a case of keeping backwards and forwards compatibility.

This is a solid objection (get it? objection? — ::crickets::)

slow clap

(I get the feeling this all deserves a proper blog post rather than discussion in comments.)

@mikeschinkel

This comment has been minimized.

Copy link

@mikeschinkel mikeschinkel commented Jan 20, 2012

@markjaquith -

We're not considering it for core at this time.

Cool then.

The only real difference between putting it in core and having it be a third party thing is that as a third party thing you have to take care of versioning (i.e. WP_Plugin_v34) so that plugin A which expects version 34 doesn't get served version 22 of the framework class.

Yeah, versioning is a major PITA, one we are trying to figure out a best practice for now. And I really do not like having to embed the version number into the class name.

We especially need it for loading shared libraries within clients plugins. For example we have two plugins, one for Client A and another for Client B and we want to use in both client's plugins our own (for example) whizbang.php library that is not a plugin (plugins seem best suited for things an end-user can and should control, and putting the dependency into mu-plugins is not viable for a plugin distributed via wordpress.org to end-users.) Best we can come up with it a registration system that will only load the latest version if the two plugins have different versions of whizbang.php. Then it will be up to us to ensure backward compatibility with new releases.

Any chance of getting something into core that can help multiple plugins share the same library? Essentially a standard feature that would allow a plugin to call a function like register_library( $name, $filename, $version ) and then load the latest one after 'plugins_loaded' hook?

(static pattern) Works better how? Honestly asking here.

The biggest reasons were already stated but here they are again, and two more:

  • No unnecessary instance lying around, especially one that might otherwise get stored in a global variable.
  • It's easier to remove static method hooks.
  • It enforces the notion that hooks are a singular thing, not one per instance. With instance methods people who don't understand the architecture might include the hooks in classes designed for creating numerous instances and thus the hooks get triggered multiple times, potentially with negative consequences. That won't happen if the static pattern is used.
  • Least important, a personal preference is to avoid recommending __construct() to would-be plugin developers because the leading underscores make it look more cryptic to the themers who are already afraid of PHP and people might forget to call parent::__construct() if they create their own __construct() in their plugin's class. I always create a static method on_load() and that's where I hook 'init' and potentially others.

Just some prior art. And I always find @scribu's code fascinating to read!

I've always respected your opinion on WordPress coding since my very first WordPress-related project was working with you so I'm hopeful when we release Sunrise to open beta you will find its code similarly fascinating. Given your public comments regarding the future of WordPress I think you are the core team member who will be most likely to appreciate what we've been building.

@thefuxia

This comment has been minimized.

Copy link

@thefuxia thefuxia commented Jan 20, 2012

@mikeschinkel

Any chance of getting something into core that can help multiple plugins share the same library?

Oh. I can haz a vizion?

screen shot

BTW I prefer real instances because it is easier to extend such classes. Yes, PHP now inherits static methods too, but you still cannot rely on it. As for removing filters/actions: If you want to allow this, you can store the object in a registry (WP still hasn’t one) or in a global variable (bah!).

@GaryJones

This comment has been minimized.

Copy link

@GaryJones GaryJones commented Jan 20, 2012

One trouble with using methods that are named in a certain way, is that you can't have two methods hooked to the same hook with the same priority - for example, scripts() and styles() methods hooked to wp_enqueue_scripts that you may want to be individually unhookable or only conditionally hook able depending if a user wants to not use plugin styles, etc.

@rmccue

This comment has been minimized.

Copy link
Owner Author

@rmccue rmccue commented Jan 20, 2012

@toscho:

BTW I prefer real instances because it is easier to extend such classes. Yes, PHP now inherits static methods too, but you still cannot rely on it.

Yes you can, PHP has always inherited static methods. The only problem with it was that self was always bound to the class it was defined in. With late static binding, you can use static instead, but that sucks.

You'll notice in my add_filter/add_action methods for the static class, it works around this for PHP <5.3

@GaryJones:

you can't have two methods [...] that you may want to be individually unhookable or only conditionally hook able depending if a user wants to not use plugin styles, etc

That's one place where you definitely need code, so I agree with Mark's sentiment on making hooking itself easier.

@peterchester

This comment has been minimized.

Copy link

@peterchester peterchester commented Mar 20, 2012

@mikeschinkel

We recently made a library management class for our premium plugins since they often share code. We went off the assumption that we'll want to use the latest version since we can't load the same class name at different versions.

https://gist.github.com/2134425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment