WordPress.org

Make WordPress Core

Opened 16 months ago

Last modified 14 months ago

#26673 new enhancement

Enhance wp_parse_args() by adding filters before/after the array_merge()

Reported by: johnjamesjacoby Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description (last modified by johnjamesjacoby)

I'd like to be able to modify existing WordPress core functionality without needing to:

  • Rewrite functions
  • Make a child theme to replace one template file
  • Searching through code and finding there is no filter where I need one

We use wp_parse_args() to parse an $args parameter against default known parameters for some functionality. bbPress's bbp_parse_args() function applies filters before and after the array_merge() which allows for just-in-time or brute-force filtering of parsed arguments. Each call to bbp_parse_args() includes a unique string so each call has unique filters applied to them.

The incoming patch introduces invisible flexibility for each call to wp_parse_args() when a unique string is passed. If no string is passed, no filters are applied, meaning it's completely backwards compatible and allows for filters to be rolled in on a case by case basis later.

Attachments (1)

26673.patch (2.0 KB) - added by johnjamesjacoby 16 months ago.

Download all attachments as: .zip

Change History (24)

@johnjamesjacoby16 months ago

comment:1 @johnjamesjacoby16 months ago

  • Description modified (diff)

comment:2 @johnjamesjacoby16 months ago

  • Summary changed from Improve wp_parse_args() by adding filters before/after the array_merge() to Enhance wp_parse_args() by adding filters before/after the array_merge()

comment:3 @johnjamesjacoby16 months ago

In several places in WordPress core, calls to wp_parse_args() are immediately followed by a call to apply_filters() on the parsed results. What I'm proposing is that we:

  • Leave existing filters untouched
  • Add filters (via unique keys) for any calls that are missing parter calls to apply_filters()
  • Enforce that new calls to wp_parse_args() always come packaged with a unique key

comment:4 follow-up: @nacin16 months ago

I'm not sure we'd want to allow such brute-forcing. The approach we have discussed previously is to add filters directly to argument arrays wherever they are desired. wp_parse_args() should be considered a low-level utility that should ideally stay filter-free. See #11212 for a lot more.

comment:5 in reply to: ↑ 4 @johnjamesjacoby16 months ago

Replying to nacin:

I'm not sure we'd want to allow such brute-forcing.

We already do in places where filters are applied immediately after wp_parse_args() calls.

The approach we have discussed previously is to add filters directly to argument arrays wherever they are desired. wp_parse_args() should be considered a low-level utility that should ideally stay filter-free. See #11212 for a lot more.

Great discussion there, and neat how we both came to the same conclusion having run into the same issues. Understanding that it's a low-level utility, even 'request' has a filter on it for brute-forcing.

For what it's worth, bbPress already heavily utilizes this, and BuddyPress will start with 2.0. It's a great (albeit powerful) way to increase the flexibility of the codebase, and offload some decision making from the development process onto an existing API.

comment:6 follow-up: @nacin16 months ago

'request' isn't part of a utility function, though. It's an important part of WordPress. It's low-level API, but it isn't a utility. My point is there isn't a filter in array_merge(), either. Or wp_list_pluck(), wp_list_filter(), absint(), is_serialized(), add_query_arg(), path_join(), wp_send_json(), wp_parse_id_list(), wp_array_slice_assoc(), and that's just from skimming functions.php. wp_parse_args() is just a glorified array_merge(). (wp_parse_args() was written for a time when you could pass query strings to everything. A lot of newer instances of the function should possibly just be array_merge() instead, for where query string calls don't make sense.)

Filters, in general, increase the maintenance burden on the codebase. If wp_parse_args() gained a filter, it would be extremely hard to keep usage of that filter backwards compatible, especially as functions evolve over time. If the filter was explicitly declared instead, it could A) have its own explicit documentation, B) be passed context based on other arguments of the function, C) allow the inputs and outputs of wp_parse_args() to be changed at will, based on evolution of the codebase.

comment:7 follow-up: @MikeSchinkel16 months ago

It's rare for me to argue against a filter but I have to side with @nacin on this one. Adding a filter here if used could increase the potential for unwanted side-effects more than a trivial amount. and for code that uses wp_parse_args() a lot (like my libraries) it could have significant performance implications.

Much better to use/add targeted, low impact filters and actions, IMHO at least.

comment:8 in reply to: ↑ 6 @johnjamesjacoby16 months ago

Replying to nacin:

'request' isn't part of a utility function, though. It's an important part of WordPress. It's low-level API, but it isn't a utility. My point is there isn't a filter in array_merge(), either. Or (...)

I understand your points, and still believe because of the way we use wp_parse_args() that it's a fine candidate for the type of dynamic filtering that's relatively common through-out the codebase already, in: WP_Customize_Setting::value(), get_option(), update_option(), get_user_option(), map_meta_cap(), get_the_term_list(), comment_form(), get_file_data(), add_metadata(), update_metadata(), etc...

wp_parse_args() is just a glorified array_merge(). (wp_parse_args() was written for a time when you could pass query strings to everything. A lot of newer instances of the function should possibly just be array_merge() instead, for where query string calls don't make sense.)

I understand that, and the history, and still think there's an opportunity to provide developers a more forgiving and flexible experience.

Filters, in general, increase the maintenance burden on the codebase.

I agree that hardcoded ones do. I disagree that filters in functions like get_option() have burdened anyone.

If wp_parse_args() gained a filter, it would be extremely hard to keep usage of that filter backwards compatible, especially as functions evolve over time.

How so? The luxury of using array_merge() and wp_parse_args() is flexibility with arguments without worrying about traditional function and method parameters being deprecated, out of order, etc...

If the filter was explicitly declared instead, it could A) have its own explicit documentation,

We can document filters above calls to wp_parse_args() the way we do with apply_filters().

B) be passed context based on other arguments of the function,

This is a minor limitation compared to overall gains IMO.

C) allow the inputs and outputs of wp_parse_args() to be changed at will, based on evolution of the codebase.

They already can be now, and this enhancement doesn't inhibit that in any way other than adding an additional parameter to wp_parse_args() which, ironically, is what makes using it great.

comment:9 in reply to: ↑ 7 @johnjamesjacoby16 months ago

Replying to MikeSchinkel:

It's rare for me to argue against a filter but I have to side with @nacin on this one. Adding a filter here if used could increase the potential for unwanted side-effects more than a trivial amount. and for code that uses wp_parse_args() a lot (like my libraries) it could have significant performance implications.

You've misunderstood the code. If your libraries don't pass a filter key, nothing changes -- no filters are applied, and no micro-performance hit occurs as a result of your code.

Much better to use/add targeted, low impact filters and actions, IMHO at least.

There's nothing wrong with targeted filters and actions, and I'm not trying to eliminate them. This isn't one VS the other.

comment:10 @MikeSchinkel16 months ago

You've misunderstood the code. If your libraries don't pass a filter key, nothing changes -- no filters are applied, and no micro-performance hit occurs as a result of your code.

Okay, didn't realize.

But there is at least one if to determine if it should, right? So some performance affect but I'd agree probably not a concern.

Okay, objection retracted if it only affects explicit usage.

Last edited 16 months ago by MikeSchinkel (previous) (diff)

comment:11 @jdgrimes16 months ago

  • Cc jdg@… added

comment:12 @alex-ye15 months ago

  • Cc nashwan.doaqan@… added

+1 for the idea, but I am not sure it's good for the core!

comment:13 @nacin14 months ago

  • Component changed from Plugins to General

comment:14 @mordauk14 months ago

+1 to adding the filters.

comment:15 follow-up: @nacin14 months ago

  • Milestone changed from 3.9 to Awaiting Review

I'm still -1. Frankly, I'd like to stop using wp_parse_args(). array_merge() is enough. It's an edge case to be accepting query string arguments in so many newer situations where this function is used. For reasons explained previously, I'd encourage your own filter use.

comment:16 follow-up: @mordauk14 months ago

Stop using wp_parse_args() throughout core? I like wp_parse_args() for how it allows flexible input formats. Is there a disadvantage to that I'm not seeing?

comment:17 in reply to: ↑ 16 @nacin14 months ago

Replying to mordauk:

Stop using wp_parse_args() throughout core? I like wp_parse_args() for how it allows flexible input formats. Is there a disadvantage to that I'm not seeing?

I'm not saying it should be removed. I'd like to stop blindly adding it in situations where we expect and only want an array. That's probably not going to happen, but I'd like to continue to treat it like a low-level utility function not unlike array_merge().

comment:18 in reply to: ↑ 15 ; follow-up: @MikeSchinkel14 months ago

Replying to nacin:

I'm still -1. Frankly, I'd like to stop using wp_parse_args(). array_merge() is enough.

In our libraries we are using wp_parse_args() instead of array_merge() and we use it a lot because it can parse strings and make calling functions intended to be used like template tags easier for themers. FWIW.

-Mike

comment:19 @mordauk14 months ago

Ah! That makes much more sense, thanks for the clarification.

comment:20 in reply to: ↑ 18 @nacin14 months ago

Replying to MikeSchinkel:

In our libraries we are using wp_parse_args() instead of array_merge() and we use it a lot because it can parse strings and make calling functions intended to be used like template tags easier for themers. FWIW.

I think it's great for template tags.

comment:21 follow-up: @johnjamesjacoby14 months ago

It could be so much more flexible, and we could greatly reduce the overhead of cherry picking when and why certain filters exist.

wp_parse_args() isn't array_merge(), it's a powerful WordPress function that could be a standardized approach to filtering all default array arguments.

comment:22 in reply to: ↑ 21 @nacin14 months ago

Can you show me a patch that turns a few wp_parse_args() into this. Say, the ones in class-wp-upgrader.php and comment-template.php.

I also don't see a need for two filters. It just slows things down. Have the filter run after the merge, but be passed the initial defaults and arguments too.

comment:23 @nacin14 months ago

Replying to johnjamesjacoby:

we could greatly reduce the overhead of cherry picking when and why certain filters exist.

That cherry picking is deliberate. If everything everywhere was filtered it'd be infinitely harder to maintain and evolve the codebase. Despite their appearance (and even relative speed), hooks are not necessarily light.

Note: See TracTickets for help on using tickets.