WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#15018 closed enhancement (fixed)

Filtering get_meta_sql for advanced queries

Reported by: sc0ttkclark Owned by: scribu
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.1
Component: Plugins Keywords:
Focuses: Cc:

Description

Currently in the code for 3.1 (trunk) there are no filters running on the new function _wp_meta_sql which would be useful for plugins to perform more complex MySQL functionality on specific columns.

I suggest adding a filter ;)

To the tune of my last ticket - it checks if the value returned is null, if it's not null then it will return the value, otherwise it will continue on like normal.

DOWNSIDE: The downside to the filter running as-is right now is that this function is used by both get_users and get_posts (down the code pipeline) and get_posts >> WP_Query::query has a 'suppress_filters' option which when set to true skips over these types of filters. This being inside of _wp_meta_sql with no way to get at that 'suppress_filters' flag. Does it make sense to account for $q and $qv or alternatively &$this in the _wp_meta_sql function for checking that flag?

Attachments (6)

wp-includes-functions-meta-filter.patch (525 bytes) - added by sc0ttkclark 5 years ago.
Initial Patch including the new filter
wp-includes-classes-filter.patch (556 bytes) - added by sc0ttkclark 4 years ago.
Updated to reflect new name / location of function in 3.1
wp-includes-classes-filter-end.patch (443 bytes) - added by sc0ttkclark 4 years ago.
Moved filter to end (simplifying in the process)
15018.patch (1.6 KB) - added by sc0ttkclark 4 years ago.
Standardizing implementation to match get_tax_sql and get_search_sql (implementation in specific location, for access to $this)
15018.2.patch (1.6 KB) - added by sc0ttkclark 4 years ago.
Standardizing implementation to match get_tax_sql and get_search_sql (implementation in specific location, for access to $this)
15018.3.patch (1.6 KB) - added by sc0ttkclark 4 years ago.
Standardizing implementation to match get_tax_sql and get_search_sql (implementation in specific location, for access to $this)

Download all attachments as: .zip

Change History (40)

@sc0ttkclark5 years ago

Initial Patch including the new filter

comment:1 @sc0ttkclark5 years ago

Also, my reference to my last ticket was meant to include #14766

comment:2 @scribu5 years ago

  • Milestone changed from Awaiting Review to 3.1

I was thinking it would be more useful if you were able to modify $wp_query->meta_query, before it was sent to _wp_meta_sql().

This can be achieved by moving it up in parse_query() and then plugins could use existing filters, like 'parse_query' and 'pre_get_posts'.

comment:3 @scribu5 years ago

  • Owner set to scribu
  • Status changed from new to accepted

comment:4 @sc0ttkclark5 years ago

That would also be useful, as long as these filters remain to. The filters I'm suggesting would allow a plugin to intercept the meta info and return a more advanced query allowing a themed to continue using core theming functions vs using custom wpdb queries. One example is casting the meta value into a datetime and other various operations. Simply modifying the meta_query in the beginning wouldn't accomplish that. I'm not against your suggestion of adding that though.

comment:5 @sc0ttkclark5 years ago

too* and themer*

Sorry just woke up and am posting from my iPhone.

comment:6 @sc0ttkclark5 years ago

I must also note that these filters would make it possible for the Pods plugin to integrate with core where it currently cannot. Pods 2.0 will allow devs to choose the storage models on ther WP Object custom fields. In this case, WP Users and WP Post Types would be covered at the same time by adding these filters. That is just in addition to allowing other plugins that add custom meta boxes and fields to Post Types to be able to control it's fields and how it is compared or where the data is stored.

comment:7 @scribu5 years ago

  • Keywords has-patch needs-refresh added; metadata removed

@sc0ttkclark4 years ago

Updated to reflect new name / location of function in 3.1

comment:8 @sc0ttkclark4 years ago

  • Keywords needs-refresh removed
  • Summary changed from Filtering _wp_meta_sql for advanced queries to Filtering get_meta_sql for advanced queries

Function name / location changed in 3.1, refreshed with a patch!

comment:9 @scribu4 years ago

  • Keywords needs-patch added; has-patch removed

I think that in this case, the filter should be at the end, passing the normal string as the first argument.

comment:10 @sc0ttkclark4 years ago

I have no objections. Will update patch tonight.

comment:11 @jane4 years ago

Freeze in November 1, so try to get the patch up asap so that if there's new dev feedback that needs to be addressed, it can be done before this deadline for new enhancements.

comment:12 @sc0ttkclark4 years ago

Thanks for your notice Jane!! Really appreciated, working on after this reply gets sent!

@sc0ttkclark4 years ago

Moved filter to end (simplifying in the process)

comment:13 @sc0ttkclark4 years ago

  • Keywords has-patch added; needs-patch removed

Attached revised Patch which moved filter to end (simplifying in the process)

comment:14 @scribu4 years ago

I've given some more thought to this and I don't think a filter would be appropriate here.

get_meta_sql() is supposed to be a protected method, used internally by classes that extend WP_Object_Query. If we add a filter, we will be forced to keep the method as is.

Besides, there will be other filters applied on the clauses later.

comment:15 @sc0ttkclark4 years ago

More thought is welcome - however the filters later are filters later. What problems would the filters cause under protected method?

comment:16 @scribu4 years ago

I'm wondering, wouldn't a filter on get_tax_sql() be needed also?

comment:17 @sc0ttkclark4 years ago

I didn't notice that had been created, from the time I opened this ticket up to this point a bunch of reworking has been done regarding the organization of these functions. I'll take a look at that, but if you're looking at it from my point of view, yes that'd be necessary too.

comment:18 @sc0ttkclark4 years ago

It makes most sense to start new tickets for get_tax_sql so will do that.

comment:20 @scribu4 years ago

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

(In [16106]) Add 'get_meta_sql' filter. Props sc0ttkclark for initial patch. Fixes #15018

@sc0ttkclark4 years ago

Standardizing implementation to match get_tax_sql and get_search_sql (implementation in specific location, for access to $this)

@sc0ttkclark4 years ago

Standardizing implementation to match get_tax_sql and get_search_sql (implementation in specific location, for access to $this)

comment:21 @sc0ttkclark4 years ago

  • Keywords needs-review added
  • Resolution fixed deleted
  • Status changed from closed to reopened

To standardize implementation to match get_tax_sql #15249 and get_search_sql #15250 (implementation in specific location, for access to $this) - I recommend we make those same modifications to this code as well.

Also, I submitted the wrong patch a moment ago (twice actually), I updated it (15018.3.patch should be used)

comment:22 @scribu4 years ago

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

$this is no longer available. See [16266]

comment:23 @sc0ttkclark4 years ago

  • Keywords has-patch needs-review removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

So there's no way to pass $this into the function as an additional argument purely for filter sake?

For example:

$clauses = get_meta_sql( $qmeta_query?, 'post', $wpdb->posts, 'ID', &$this );

comment:24 @sc0ttkclark4 years ago

Oops, example got eaten up!

$clauses = get_meta_sql( $q['meta_query'], 'post', $wpdb->posts, 'ID', &$this );

comment:25 @westi4 years ago

  • Keywords needs-patch added

A patch to add a context param to get_meta_sql and to use apply_filters_ref_array instead would be a good idea IMHO.

That way plugins can know which object is actually having it's query filtered otherwise it gets message.

comment:26 @scribu4 years ago

That way plugins can know which object is actually having it's query filtered otherwise it gets message.

They already get the $meta_type as a parameter. Also, they can modify the 'meta_query' var before get_meta_sql() is called.

Also, what if I want to use get_meta_sql() in a plugin, but I don't have a $this to pass?

comment:27 @sc0ttkclark4 years ago

Well, since it's gone this direction as a global function vs a class function, I guess it makes sense to not require the &$this but make it optional? Otherwise, I'm okay with leaving the modification out, it would just give more information to be able to be used on the filter in question.

comment:28 @westi4 years ago

You still need the context if call from WP_Query else the function is next to useless as you can't target any extra query variables you may have added - this is why apply_filters_ref_array exists os that we can pass objects as context.

In the case of use in global scope without an object as context we should be able to cope with being passes either null or just a new Object().

comment:29 @sc0ttkclark4 years ago

Thx @westi - Glad I'm not crazy alone here ;)

comment:30 @scribu4 years ago

You still need the context if call from WP_Query else the function is next to useless as you can't target any extra query variables you may have added

Could you provide an example where you would need access to the other query variables from the 'get_meta_sql' filter?

comment:31 @scribu4 years ago

(In [16286]) Add context to get_meta_sql(). See #15018

comment:32 @scribu4 years ago

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

comment:33 @nacin4 years ago

Is there an issue with optionally passing null by reference if context is not supplied?

comment:34 @scribu4 years ago

$context is a variable, so it shouldn't matter if it's null or not.

comment:35 @sc0ttkclark4 years ago

  • Keywords needs-patch removed
Note: See TracTickets for help on using tickets.