WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 23 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 6 years ago.
Initial Patch including the new filter
context.15250.patch (1.4 KB) - added by scribu 6 years ago.
'get_tax_sql' filter with context
15250.2.patch (785 bytes) - added by sc0ttkclark 6 years ago.
Updated patch for latest SVN Revision (16692)
15250.3.patch (596 bytes) - added by sc0ttkclark 4 years ago.
Refreshing this patch w/ 3.5 trunk, updated filter args with ones now used
15250.4.patch (577 bytes) - added by sc0ttkclark 4 years ago.
Changing to use of apply_filters
15250.diff (701 bytes) - added by wonderboymusic 2 years ago.
15250.5.patch (1.3 KB) - added by DrewAPicture 2 years ago.
Filter docs

Download all attachments as: .zip

Change History (37)

#1 @sc0ttkclark
6 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.

#2 @scribu
6 years ago

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

#3 @scribu
6 years ago

  • Type changed from feature request to enhancement

@sc0ttkclark
6 years ago

Initial Patch including the new filter

#5 @sc0ttkclark
6 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)

#6 @scribu
6 years ago

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

#7 @scribu
6 years ago

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

#8 @sc0ttkclark
6 years ago

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

@scribu
6 years ago

'get_tax_sql' filter with context

#9 @scribu
6 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.

#10 @scribu
6 years ago

  • Milestone changed from Awaiting Review to 3.1

#11 @scribu
6 years ago

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

#12 @ryan
6 years ago

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

#13 @scribu
6 years ago

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

Agreed.

#14 @sc0ttkclark
6 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?

#15 @scribu
6 years ago

Which dependencies?

Also, no code is set in stone.

#16 @sc0ttkclark
6 years ago

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

#17 @scribu
6 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.

#18 @sc0ttkclark
6 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.

#19 @scribu
6 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.

#20 @sc0ttkclark
6 years ago

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

@sc0ttkclark
6 years ago

Updated patch for latest SVN Revision (16692)

#21 @scribu
4 years ago

  • Owner scribu deleted
  • Status changed from new to assigned

@sc0ttkclark
4 years ago

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

#22 @sc0ttkclark
4 years ago

  • Keywords has-patch dev-feedback added

Refreshed this patch with the latest code from 3.5 trunk

#23 @scribu
4 years ago

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

@sc0ttkclark
4 years ago

Changing to use of apply_filters

#24 @nacin
3 years ago

  • Component changed from Plugins to Query

#25 @wonderboymusic
2 years 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.

@wonderboymusic
2 years ago

@DrewAPicture
2 years ago

Filter docs

#26 @DrewAPicture
2 years ago

  • Keywords needs-docs removed

15250.5.patch adds filter docs.

#27 @DrewAPicture
2 years ago

  • Focuses docs removed

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


2 years ago

#29 @helen
2 years 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.

#30 @boonebgorges
23 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.