WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 months ago

#8833 closed enhancement (maybelater)

extract pluggable.php function logic into separate functions

Reported by: wnorris Owned by: Viper007Bond
Milestone: Priority: normal
Severity: normal Version: 2.8
Component: Plugins Keywords: dev-feedback
Focuses: Cc:

Description

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

Attachments (6)

8833.patch (114.0 KB) - added by Viper007Bond 5 years ago.
First pass, needs through testing and phpDoc feedback
8833.2.patch (114.0 KB) - added by Viper007Bond 5 years ago.
Very minor phpdocs fix (had periods instead of spaces)
8833.3-notworking.patch (125.4 KB) - added by Viper007Bond 5 years ago.
Don't use this patch, it has a bug or two and is a work in progress
8833.3.patch (124.8 KB) - added by Viper007Bond 5 years ago.
Working as far as I can tell, needs testing and feedback
8833.4.patch (124.8 KB) - added by Viper007Bond 5 years ago.
Patch refresh for [11147]
8833.5.patch (124.8 KB) - added by Viper007Bond 5 years ago.
Remove an accidental part of a collision error message

Download all attachments as: .zip

Change History (33)

comment:1 mrmist5 years ago

  • Keywords needs-patch added

comment:2 wnorris5 years ago

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.

comment:3 dougal5 years ago

  • Keywords pluggable forward compatibility future-proofing fallback added

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.

comment:4 ryan5 years ago

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

comment:5 Viper007Bond5 years ago

I like this, a lot.

comment:6 follow-up: ryan5 years ago

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

comment:7 in reply to: ↑ 6 Viper007Bond5 years ago

  • Owner set to Viper007Bond

Replying to ryan:

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

Okay. :)

Viper007Bond5 years ago

First pass, needs through testing and phpDoc feedback

comment:8 Viper007Bond5 years ago

  • Keywords has-patch needs-testing added; needs-patch pluggable forward compatibility future-proofing fallback removed
  • Version set to 2.8

Viper007Bond5 years ago

Very minor phpdocs fix (had periods instead of spaces)

comment:9 Viper007Bond5 years ago

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.
  1. Prefixed the new function name with an underscore and changed @since to 2.8.0 since it's a new function.
  1. 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()
  1. Went back to the original function in pluggable.php and stripped out it's contents, replacing it with the new internal function.
  1. 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.
  1. 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.

comment:10 Viper007Bond5 years ago

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.

comment:11 follow-up: DD325 years ago

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

comment:12 in reply to: ↑ 11 Viper007Bond5 years ago

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.

Viper007Bond5 years ago

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

Viper007Bond5 years ago

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

comment:13 Viper007Bond5 years ago

  • Milestone changed from 2.8 to Future Release

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

Viper007Bond5 years ago

Patch refresh for [11147]

Viper007Bond5 years ago

Remove an accidental part of a collision error message

comment:14 follow-up: Denis-de-Bernardy5 years ago

patch is broken against 11256

comment:15 in reply to: ↑ 14 Viper007Bond5 years ago

  • Keywords dev-feedback added

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.

comment:16 Denis-de-Bernardy5 years ago

I know the feeling...

comment:17 Denis-de-Bernardy5 years ago

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

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.

comment:18 Viper007Bond5 years ago

Oh, right, forgot about that function.

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

comment:19 Denis-de-Bernardy5 years ago

  • Keywords close added

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

comment:20 azaozz5 years ago

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.

comment:21 dmeiser3 years ago

  • Cc dmeiser@… added

comment:22 scribu3 years ago

Marked #18405 as duplicate.

comment:23 F J Kaiser3 years ago

  • Cc 24-7@… added

comment:24 follow-up: johnbillion20 months ago

  • Keywords close removed

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.

comment:25 in reply to: ↑ 24 SergeyBiryukov20 months ago

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.

comment:26 bpetty4 months ago

  • Cc bpetty added
  • Keywords close added

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

comment:27 SergeyBiryukov4 months ago

  • Keywords needs-patch close removed
  • Milestone Future Release deleted
  • Resolution set to maybelater
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.