extract pluggable.php function logic into separate functions

[wnorris]

Currently, when a plugin overrides one of the pluggable.php functions with a custom implementation, there is no way to fall back to the standard version of the function. Instead, plugin authors have to copy and paste all of the logic from pluggable.php into their custom function. I'd love to see the actual logic separated out into "private" functions that are simply called the public ones. For example:

if (!function_exists('wp_get_current_user')) {
    function wp_get_current_user() {
        return _wp_get_current_user();
    }
}

function _wp_get_current_user() {
    /* normal logic for getting the current user */
}

This would allow me to override the function, but still fallback to the standard implementation if I need to...

if (!function_exists('wp_get_current_user')) {
    function wp_get_current_user() {
        if ( /* some condition */ ) {
            /* my own custom get current user logic */
        } else {
            return _wp_get_current_user();
        }
    }
}

This wouldn't actually take that much work, and I'm happy to do it. Not sure if it would be best to keep it all in pluggable.php, or if the standard implementations should be moved to pluggable.standard.php or something similar. ??

Original wp-hackers thread: ​http://groups.google.com/group/wp-hackers/browse_thread/thread/31295f83a13dc025

[wnorris]

In fact, this is very similar to how set_current_user() already works... it's simply a wrapper that calls wp_set_current_user(). The only problem with this one in particular, is that all the other functions in pluggable.php call wp_set_current_user directly, instead of calling the wrapper function (elsewhere in WordPress, most of the code correctly calls the wrapper function). It's the right idea, just needs to be cleaned up.

[dougal]

As I said on the wp-hackers list, this is an elegant idea. It's essentially the procedural programming eqivalent to being able to call a super() function in OOP. Well, sort of :)

A great way to provide the ability to override functionality, but allowing a fallback to the original function when needed, without sacrificing forward compatibility.

[ryan]

I think you can put the standard implementations in the appropriate include files. User and login stuff in user.php, for example.

[Viper007Bond]

I like this, a lot.

[ryan]

Someone needs to like it enough to patch it. :-)

[Viper007Bond]

Replying to ryan:

Someone needs to like it enough to patch it. :-)

Okay. :)

[Viper007Bond]

First pass, needs through testing and phpDoc feedback

[Viper007Bond]

Very minor phpdocs fix (had periods instead of spaces)

[Viper007Bond]

A summary of my patch:

1. I took the original function (along with all of it's phpDocs) and copy/pasted it into another file.

2. Prefixed the new function name with an underscore and changed @since to 2.8.0 since it's a new function.

3. Added the following after all of the description and before the @since:

 * This function should not be used directly. Use the pluggable version instead.
 * @see pluggable_function()

4. Went back to the original function in pluggable.php and stripped out it's contents, replacing it with the new internal function.

5. Removed any @uses since the pluggable function no longer actually used them (they're still over with the new internal function though). Also removed any description bits specific to that implementation of the plugin function but that weren't generic descriptors of what the function should do.

6. Added the following after the description and before @since:

 * @see _pluggable_function()

Note I did not add a @uses _pluggable_function() which is one of my questions (is that needed in addition to or instead of the @uses?).

Please leave any and all feedback.

[Viper007Bond]

Figures I read the wp-hackers thread after I spend over 2 hours writing the above patch. Bah.

Maybe current patch now with a possible switch to filter based replacements later.

[DD32]

Figures I read the wp-hackers thread after I spend over 2 hours writing the above patch

Not to worry. I think theres even a Trac ticket for it.

This'd have to be done anyway.. so that the pluggable functions were just a return apply_filters(__FUNCTION__, false);

[Viper007Bond]

Replying to DD32:

Not to worry. I think theres even a Trac ticket for it.

This'd have to be done anyway.. so that the pluggable functions were just a return apply_filters(__FUNCTION__, false);

I don't see another ticket, but yeah, I realized that this would need to be done either way earlier today.

Infact I think I may just tackle that while I'm at it.

[Viper007Bond]

Don't use this patch, it has a bug or two and is a work in progress

[Viper007Bond]

Working as far as I can tell, needs testing and feedback

[Viper007Bond]

Okay, there's a totally filters/actions based patch. Seems to work perfectly, but needs testing. :)

[Viper007Bond]

Patch refresh for [11147]

[Viper007Bond]

Remove an accidental part of a collision error message

[Denis-de-Bernardy]

patch is broken against 11256

[Viper007Bond]

Replying to Denis-de-Bernardy:

patch is broken against 11256

Yes, I stopped updating the patch until I hear whether there is interest in this or not. No sense in devoting time to it if it won't get committed.

[Denis-de-Bernardy]

I know the feeling...

[Denis-de-Bernardy]

Just for reference, though, this one:

function wp_password_change_notification( &$user ) { 
   do_action( 'pluggable-wp_password_change_notification', $user );
}

should be using do_action_ref_array(), or whatever it's called, due to the reference.

[Viper007Bond]

Oh, right, forgot about that function.

If this ticket gets the green light, I'll update my patch to include that.

[Denis-de-Bernardy]

will close in a month if there still isn't any feedback. no point in letting this rot for another year.

[azaozz]

The original idea is good and 8833.2.patch looks good. However I'm not sure how the filter based replacements would behave when there are several plugins using them.

Currently when two or more plugins try to replace the same pluggable function the first one "wins" (and keeps working) and the second fails. If we add something like 8833.2.patch, the second would be able to use the default function and still work in most cases. However if we pass filtered values from one plugin to the other chances are that both would fail for some functions.

IMHO switching from !function_exists() to add_filter()/add_action() would require a lot of testing both as a usable solution and for regressions/back-compat and would break some plugins.

[scribu]

Marked #18405 as duplicate.

[johnbillion]

I know there's not a lot of traction on this ticket, but I'd like to add a bit of feedback (partly because it was recently mentioned again on hackers).

With more than one plugin trying to override a given pluggable function, there will only be one winner. The problem is that some of the pluggable functions are too wide in scope, and two different plugins may be trying to override the same pluggable function in its entirety just to change on bit of its functionality.

I think many of the pluggable functions should be split into multiple functions, with their individual bits of functionality being filterable. This way, plugins can override (and "win") more tightly focused functions.

One example is wp_mail(), which does far more than just send an email. It parses headers (including the 'from', 'cc' and 'bcc' fields), sets the 'from' name and address, sets content types, deals with charsets, adds custom headers, adds attachments, and finally sends an email.

Changing this from from being pluggable to being filterable doesn't overcome the issue where you may want to run multiple email-related plugins at the same time, such as one which gives you HTML emails and one which sends your emails through an email delivery service.

Another example is the various notification functions such as wp_notify_moderator(), wp_notify_postauthor() and wp_new_user_notification(). These functions have a mixture of logic in them determining whether or not to send a notification, the notification message itself, and additional stuff such as the 'from' headers in the email. What a user cannot do, for example, is install one plugin to change the text in these emails and another plugin which allows you to control when and whether these notifications are sent.

A good first step would be to look at splitting some of these functions up, and deciding which of the subsequent functions should have their output filterable (eg. the text sent in notification emails) and which should have their callback function filterable (eg. the function which actually sends email).

I'd be willing to start looking at this over the next few weeks if it'll get some traction.

[SergeyBiryukov]

Replying to johnbillion:

Changing this from from being pluggable to being filterable doesn't overcome the issue where you may want to run multiple email-related plugins at the same time, such as one which gives you HTML emails and one which sends your emails through an email delivery service.

I guess if there were filters for that (some probably already exist), plugins would be updated to use them instead of copying the functions.

[bpetty]

I believe this ticket is just sitting here with no action because when it comes down to it, it doesn't actually solve anything, or make it any easier to implement multiple related plugins to work together. In fact, it would actually make it even harder to predict what is going to happen when a pluggable method is called.

As @azaozz pointed out, only one plugin overriding a pluggable method wins, period. It doesn't matter if it's still possible to call the default method or not, the fact remains that it's no longer being called reliably 100% of the time, and letting another plugin still call the default implementation would then mean that the original plugin overriding the pluggable method then technically breaks since it's not reliably being called when it should be either. The proposed patch doesn't fix the "one plugin wins" scenario, it just makes it more complicated.

When it comes down to it, @johnbillion is right. This really just comes down to individual use cases where additional actions and filters may be necessary to still encourage core usage rather than implementing pluggable methods (which is definitely still preferred), and if a plugin does implement a pluggable method, nothing in core is going to be able to do anything about it. It's the responsibility of each plugin to ensure compatibility with the other plugins that do it (which is still perfectly possible without any changes in core code or behavior currently).