Make WordPress Core

Opened 15 years ago

Closed 14 years ago

Last modified 14 years ago

#10493 closed enhancement (fixed)

Allow closures as callbacks

Reported by: scribu's profile scribu Owned by: westi's profile westi
Milestone: 3.0 Priority: low
Severity: minor Version: 2.9
Component: Plugins Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (5)

closure_testing.php (474 bytes) - added by scribu 15 years ago.
closures.diff (555 bytes) - added by scribu 15 years ago.
closures.2.diff (742 bytes) - added by scribu 14 years ago.
updated patch
closures.3.diff (434 bytes) - added by scribu 14 years ago.
Fix notices
closures.4.diff (704 bytes) - added by scribu 14 years ago.
fix remaining notice

Download all attachments as: .zip

Change History (40)

@scribu
15 years ago

#1 follow-up: @Denis-de-Bernardy
15 years ago

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

#2 @Denis-de-Bernardy
15 years ago

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

#3 in reply to: ↑ 1 @scribu
15 years ago

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.

#4 @scribu
15 years ago

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.

#5 follow-up: @westi
14 years ago

  • Milestone changed from 2.9 to Future Release

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
14 years ago

updated patch

#6 in reply to: ↑ 5 @scribu
14 years ago

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.

#7 @scribu
14 years ago

  • Milestone changed from Future Release to 3.0

#8 @dd32
14 years ago

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.

#9 @hakre
14 years ago

  • Milestone changed from 3.0 to Future Release

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.

#10 follow-up: @scribu
14 years ago

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.

#11 @dd32
14 years ago

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.

#12 in reply to: ↑ 10 @hakre
14 years ago

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.

#13 @dd32
14 years ago

  • Component changed from General to Plugins
  • Keywords commit added
  • Milestone changed from Future Release to 3.0

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.

#14 @westi
14 years ago

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.

#15 @westi
14 years ago

  • Resolution set to fixed
  • Status changed from new to closed

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

#16 @mikeschinkel
14 years ago

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

#17 @mikeschinkel
14 years ago

  • Cc mikeschinkel@… added

#18 @scribu
14 years ago

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

#19 @mikeschinkel
14 years ago

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

#20 @Denis-de-Bernardy
14 years ago

@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

#21 @mikeschinkel
14 years ago

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

#22 @scribu
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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
14 years ago

Fix notices

#23 @scribu
14 years ago

  • Priority changed from normal to low

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

#24 @scribu
14 years ago

  • Severity changed from normal to minor

#25 @nacin
14 years ago

  • Keywords needs-patch added; has-patch commit removed

spl_object_hash is PHP 5.2.

#26 @scribu
14 years ago

  • Keywords has-patch added; needs-patch removed

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

#27 @nacin
14 years ago

  • Owner changed from scribu to westi
  • Status changed from reopened to assigned

#28 @westi
14 years ago

  • Status changed from assigned to accepted

#29 @westi
14 years ago

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.

#30 @westi
14 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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

@scribu
14 years ago

fix remaining notice

#31 follow-up: @scribu
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

closures.4.diff fixes the remaining notice:

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

#32 in reply to: ↑ 31 @westi
14 years ago

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

#33 follow-up: @scribu
14 years ago

Sure:

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

#34 @westi
14 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

#35 in reply to: ↑ 33 @westi
14 years ago

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

Note: See TracTickets for help on using tickets.