Allow closures as callbacks

[scribu]

PHP 5.3 allows closures, which means we can replace this:

add_action('init', create_function('', '//do something'));

with this:

add_action('init', function() { // do something });

and it only requires two lines of extra code.

[Denis-de-Bernardy]

As much as I *love* that syntax, I'm having a hard time imagining how to remove_filter() the mess if we need to.

[Denis-de-Bernardy]

Not that removing a lambda function is any easier, but still...

[scribu]

Replying to Denis-de-Bernardy:

As much as I *love* that syntax, I'm having a hard time imagining how to remove_filter() the mess if we need to.

You could do this:

$func = function() {...};

add_action('foobar', $func);
remove_action('foobar', $func);

but that is sort of self-defeating.

[scribu]

A performance enhancement from Nikolay was as follows:

if ( class_exists('Closure') ) :
  function add_filter(..) {
    // version of function that accepts closures
  }
else :
  function add_filter(..) {
    // version of function that doesn't accept closures
  }

It avoids the extra if clause inside the function, when possible. The dissadvantage, of course, is duplicate code.

[westi]

I'm not sure we need to do this.

We shouldn't be encouraging create_function in this case anyway as there are possible security risk.

Closures make it easier to write multi-line functions here which make the code even harder to read.

Also looking at the php docs the fact that they are currently implemented by the Closure class should not be relied on - I'm not quite sure why they don't work exactly like create_function does that way it would all just work ;-)

[scribu]

updated patch

[scribu]

I'm not sure we need to do this.

As of [12090], we could do this:

add_action('init', array(function() { // do something }));

but eliminating the array() is easily done with ​closures.2.diff

We shouldn't be encouraging create_function in this case anyway as there are possible security risk.

Precisely: You can't create closures in the same way you can pass a string to create_function(), so it's actually encouraging safer code.

Closures make it easier to write multi-line functions here which make the code even harder to read.

It's the same as closures in jQuery. Here's a counter example:

add_action('init', function() { 
    if ( isset($_GET['restricted']) )
        die('Not so fast...');
});

Looks familiar? :-)

Also looking at the php docs the fact that they are currently implemented by the Closure class should not be relied on.

closures.2.diff doesn't rely on that anymore.

[dd32]

I'm not sure we need to do this.

Me too. We dont -need- to do this, But might as well support new PHP syntax.

We shouldn't be encouraging create_function in this case anyway as there are possible security risk.

Precisely: You can't create closures in the same way you can pass a string to create_function(), so it's actually encouraging safer code.

I quite agree, its virtually moving it to a lamba function and passing that as the action hook.. exactly the same as create_function, Just without all the PHP code in a string.. which can be insecure if done wrong.

Only down side to this, is that some plugins may use it and those without the latest PHP will not be able to use it. But thats a call on the authors behalf, if they choose to use PHP 5.whatever syntax, then they're the ones removing the ability for others to use their plugins.

[hakre]

Quite a theoretic discussion this is, right? First of all what's said is that the callback pseudo-type PHP has does handle lambda functions (which is just the ​standard PHP behaviour as documented: "As of PHP 5.3.0 it is possible to also pass a ​closure to a callback parameter"). That just for the log.

Next to the fact that this wordpress enhancement which just is an existing PHP feature (we do not need to patch anything, this will work out of the box), you should keep in mind that you need PHP version 5.3 or higher for it. Unless the system requirements do not call for that PHP version (WP core is far away from that), suggesting to use it is just wrong, it will actually defect exsiting code in case core code is changed based on this tickets suggestion.

I suggest to close this ticket as invalid. I'll add it to the ​PHP 5 list on my codex pages so it does not get lost.

[scribu]

Next to the fact that this wordpress enhancement which just is an existing PHP feature (we do not need to patch anything, this will work out of the box), you should keep in mind that you need PHP version 5.3 or higher for it.

If this would work out of the box on a server with PHP 5.3 installed, I wouldn't have opened this ticket.

Unless the system requirements do not call for that PHP version (WP core is far away from that), suggesting to use it is just wrong, it will actually defect exsiting code in case core code is changed based on this tickets suggestion.

I agree that WP core is very far away from PHP 5.3 (that's why the milestone is set to Future Release), but I don't agree with the statement that this would negatively affect existing code.

[dd32]

but I don't agree with the statement that this would negatively affect existing code.

I concur with you there.

A plugin developer using this syntax is the same as a plugin developer using a php5-only function. Its their choice to use it, it'll work, but it'll mean that their minimum PHP requirement is boosted.

[hakre]

Replying to scribu:

Unless the system requirements do not call for that PHP version (WP core is far away from that), suggesting to use it is just wrong, it will actually defect exsiting code in case core code is changed based on this tickets suggestion.

I agree that WP core is very far away from PHP 5.3 (that's why the milestone is set to Future Release), but I don't agree with the statement that this would negatively affect existing code.

you misread me: What I meant is that exising code would be negativly affected by using closures with a WP PHP requirement of below 5.3 as of today and for the next 3-4(?) years.

As written this ticket is highly theoretical. When 5.3 is minimum system requirement than every coder can actually use cosures because of the system requirements. I just do not see a need for a ticket here but maybe you can shed some light onto this. To showcase something in the future? I mean this trac is about the core code, right? Otherwise I could open tickets that showcase the possibilities we get with PHP 5 regarding objects, visibility and inheritance, exceptions and interfaces - something infact I already use in plugins w/o boosting the requirements too much.

[dd32]

When 5.3 is minimum system requirement than every coder can actually use cosures because of the system requirements.

A lot of people ignore the WP Minimum requirements, Plugin authors and those of us who use WordPress as a base for other projects have the right to require other minium requirements. PHP5+ is a very common Plugin rule. PHP 5.3+ is perfectly respectable for other projects due to PHP-functionalities.

I mean this trac is about the core code, right?

Trac is about the Core code, Its the place people come to get Enhancements and defects fixed. Just because something is made theoretically possible, it doesnt mean that everyone has to use it.

I could open tickets that showcase the possibilities we get with PHP 5 regarding objects, visibility and inheritance, exceptions and interfaces - something infact I already use in plugins w/o boosting the requirements too much.

Theres the difference, That requires changes to WordPress which raise WordPress's minimum requirement. This does not. WordPress can stay as PHP 4.3+, But allows Plugin authors to seemlessly use PHP5.3 functionality if they have support for it in their php.

just FYI, WordPress already has bit of PHP5+-only code, its just disabled on those with older installs. If its possible to enable more functionalities, without affecting core in a negitive way, then why should it be rejected.

As westi said earlier, We dont -need- this, But it does no harm, and for those who integrate WordPress into other projects, This allows them to use a functionality which they may use themselves.

Listing this for 3.0 as a commit candidate.

[westi]

I'm much happier with the current patch as it doesn't rely on the Closure class.

I would like to add some tests for this to the unit-tests too but I don't have a PHP 5.3 setup at the moment.

It passes all the current tests so i think we can put this in trunk for people that want to to use.

[westi]

(In [12641]) Allow people to attach PHP 5.3 Closures to filters and actions. Fixes #10493 props scribu.

[mikeschinkel]

Suggestion to consider to address the concern for removing?

Instead of

add_action('init', function() { // do something });

How about?:

add_action('init', array('name',function() { // do something }));

The latter allows a name to be associated with the function but not one that will conflict with a function name. It could be optional for those who want to all their use of the hook to be removed.

Just an idea...

[scribu]

-10.

If you know you might need to remove it, give it a name by making it a regular function.

Also, you can do this, which is equivalent to using create_function():

$closure = function() { // whatever }

add_action('wp_head', $closure);

...

remove_action('wp_head', $closure);

[mikeschinkel]

@scribu I don't care strongly about this, but do feel compelled to point out that your alternative is not at all equivalent to what I suggested.

Making a regular function pollutes the public namespace for functions whereas my proposed solution only need be unique among hooks used in this manner; every function that WordPress required a developer to add greater potential for conflict.

Your second suggestion also presumes that a.) the person adding the action had the forethought to save the closure and/or b.) the person needing to remove the action actually has access to the saved closure, both of which are unlikely except with the most forward thinking of plugin and theme developers.

FWIW.

[Denis-de-Bernardy]

@Mike: both pollute the namespace. it's just that in one case it's a lambda that can't be removed, whereas in the other it's a nearly lambda that can. I don't care much myself. I consider both to be bad practice. :-P

[mikeschinkel]

@Denis-de-Bernardy "lambda's polluting the namespace" is an oxymoron.

[scribu]

Seeing a couple of notices when using closures:

Notice: Undefined offset: 0 in .../wp/wp-includes/plugin.php on line 742 
Notice: Undefined offset: 0 in .../wp/wp-includes/plugin.php on line 760

[scribu]

Fix notices

[scribu]

closures.3.diff fixes the notices and it's easier to follow.

[nacin]

spl_object_hash is PHP 5.2.

[scribu]

Yes, but since closures are PHP 5.3, the function will exist.

[westi]

Dug into this a little and the array cast on the closure was just generating an empty array.

Going with a slightly different patch so we still use the rest of the code as before even when passed an object.

[westi]

(In [14690]) Fix notices/usage of closures as action/filter handlers. Fixes #10493 based on patch from scribu.

[scribu]

fix remaining notice

[scribu]

closures.4.diff fixes the remaining notice:

Notice: Undefined offset: 1 in /home/cristi/svn/wp/wp-includes/plugin.php on line 750

[westi]

Replying to scribu:

closures.4.diff fixes the remaining notice:

Notice: Undefined offset: 1 in /home/cristi/svn/wp/wp-includes/plugin.php on line 750

have you got an example we reproduces this notice as I didn't see any notices in my testing.

Cheers

[scribu]

Sure:

add_action('admin_notices', function() {
	echo 'hi';
});

[westi]

(In [14691]) Fix remaining notice when using closures. Fixes #10493 props scribu.

[westi]

Replying to scribu:

Sure:

add_action('admin_notices', function() {
	echo 'hi';
});

Thanks - turned out wordpress-tests was hiding all errors from me even with WP_DEBUG enabled. I've rectified that now :-)