WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 10 months ago

#15250 closed enhancement (wontfix)

Filtering get_tax_sql for advanced queries

Reported by: sc0ttkclark Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: Query Keywords: has-patch
Focuses: Cc:

Description

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

I suggest adding a filter ;)

Attachments (7)

15250.patch (584 bytes) - added by sc0ttkclark 5 years ago.
Initial Patch including the new filter
context.15250.patch (1.4 KB) - added by scribu 5 years ago.
'get_tax_sql' filter with context
15250.2.patch (785 bytes) - added by sc0ttkclark 5 years ago.
Updated patch for latest SVN Revision (16692)
15250.3.patch (596 bytes) - added by sc0ttkclark 3 years ago.
Refreshing this patch w/ 3.5 trunk, updated filter args with ones now used
15250.4.patch (577 bytes) - added by sc0ttkclark 3 years ago.
Changing to use of apply_filters
15250.diff (701 bytes) - added by wonderboymusic 15 months ago.
15250.5.patch (1.3 KB) - added by DrewAPicture 15 months ago.
Filter docs

Download all attachments as: .zip

Change History (37)

comment:1 @sc0ttkclark5 years ago

At the moment, it appears this function has disappeared from trunk for whatever reason. It's no longer in /wp-includes/classes.php so I'll keep an eye out for where it's being moved to before patching.

comment:2 @scribu5 years ago

WP_Object_Query has been moved to wp-includes/class-wp-object-query.php

comment:3 @scribu5 years ago

  • Type changed from feature request to enhancement

@sc0ttkclark5 years ago

Initial Patch including the new filter

comment:5 @sc0ttkclark5 years ago

Added patch that includes the new filter in query.php (not within get_tax_sql function itself due to the lack of information the function is given like no access to $this)

comment:6 @scribu5 years ago

Since get_tax_sql() is a method of WP_Object_Query, which is the parent class, you do have access to $this.

comment:7 @scribu5 years ago

Why wouldn't passing $tax_query and $object_id_column to the filter be enough?

comment:8 @sc0ttkclark5 years ago

If it has access to $this, then that would be fine within the function. It was just a misunderstanding on my side.

@scribu5 years ago

'get_tax_sql' filter with context

comment:9 @scribu5 years ago

  • Keywords has-patch added

context.15250.patch applies the 'get_tax_sql' filter, passing a context too.

I'm still concerned about what would happen if we decide we also need to alter the JOIN clause, for example.

comment:10 @scribu5 years ago

  • Milestone changed from Awaiting Review to 3.1

comment:11 @scribu5 years ago

Hm... well now with [16414] the terms are first transformed into term_taxonomy_ids, which I guess would need additional handling.

comment:12 @ryan5 years ago

Perhaps we should see how things go in 3.1 before exposing the guts of get_tax_sql().

comment:13 @scribu5 years ago

  • Keywords has-patch removed
  • Milestone changed from 3.1 to Future Release

Agreed.

comment:14 @sc0ttkclark5 years ago

Why would we push this to another 3.x release if it's dependent on code that could change in 3.1, can't we just patch / implement once those dependencies are set in stone?

comment:15 @scribu5 years ago

Which dependencies?

Also, no code is set in stone.

comment:16 @sc0ttkclark5 years ago

Dependencies would be whatever happens to get_tax_sql that @ryan brought up concerns with.

comment:17 @scribu5 years ago

I think what ryan was trying to say is that we should wait to see how people use get_tax_sql() after 3.1 is released, before adding the filter.

comment:18 @sc0ttkclark5 years ago

I see that, but I also believe that losing out on implementing a filter out the gate means waiting a whole new dev cycle to be able to do things with core. The proposed filter applies to many needs by giving quite a bit of information to a function that wishes to extend the SQL generated by the function.

comment:19 @scribu5 years ago

Please take another look at the code inside get_tax_sql(). It's quite different from what you made the patch against.

If you can give me an example that can efficiently leverage the filter you're proposing, i.e. without wasting queries made by _transform_terms(), I will commit it.

comment:20 @sc0ttkclark5 years ago

Will do, the patch was before a lot of things happened so I'll update today.

@sc0ttkclark5 years ago

Updated patch for latest SVN Revision (16692)

comment:21 @scribu3 years ago

  • Owner scribu deleted
  • Status changed from new to assigned

@sc0ttkclark3 years ago

Refreshing this patch w/ 3.5 trunk, updated filter args with ones now used

comment:22 @sc0ttkclark3 years ago

  • Keywords has-patch dev-feedback added

Refreshed this patch with the latest code from 3.5 trunk

comment:23 @scribu3 years ago

Using apply_filters_ref_array() is not necessary anymore, since we dropped support for PHP 4. Can just use apply_filters().

@sc0ttkclark3 years ago

Changing to use of apply_filters

comment:24 @nacin18 months ago

  • Component changed from Plugins to Query

comment:25 @wonderboymusic15 months ago

  • Focuses docs added
  • Keywords needs-docs added; dev-feedback removed
  • Milestone changed from Future Release to 4.0

Refreshed - I am less in love with this one, but needs filter docs, nonetheless. We can at least make a decision one way or another.

@wonderboymusic15 months ago

@DrewAPicture15 months ago

Filter docs

comment:26 @DrewAPicture15 months ago

  • Keywords needs-docs removed

15250.5.patch adds filter docs.

comment:27 @DrewAPicture14 months ago

  • Focuses docs removed

comment:28 @ircbot13 months ago

This ticket was mentioned in IRC in #wordpress-dev by helen. View the logs.

comment:29 @helen13 months ago

  • Milestone changed from 4.0 to Future Release

Pretty late for this one for 4.0, would prefer for this to really get tested and soak. No consensus either.

comment:30 @boonebgorges10 months ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

I'm strongly against adding filters directly to SQL queries. It's terrible for future backward compatibility, and it's an invitation for plugins to do things that end up causing conflicts with other plugins. (If we could remove the filters in Date and Meta queries without breaking existing plugins, I would.) The changes to WP_Tax_Query in WP 4.1 make it so that pretty much any kind of tax query can be expressed in the syntax of the class, reducing the need for this filter; and anyone who *really* needs to filter SQL for some reason can still use filters like 'posts_where' in WP_Query.

In the absence of a compelling use case, I'm going to say wontfix.

Note: See TracTickets for help on using tickets.