Make WordPress Core

Opened 7 years ago

Last modified 3 years ago

#41266 assigned enhancement

Not hard coding the table alias prefix in WP_Meta_Query would make class more extendable

Reported by: thomaslhotta's profile thomaslhotta Owned by: noisysocks's profile noisysocks
Milestone: Future Release Priority: normal
Severity: normal Version: 4.8
Component: Query Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description

Hi,

I am extending the WP_Meta_Query class for my own uses with custom tables that have the same structure as meta tables. It is an incredibly useful class for this. However the fact that the table alias prefix mt on line 535 is hard coded forces me to do awkward and complex string replaces if I want to use my extensions alongside built in meta queries.

I think changing the prefix from being hard coded to a property/function of the meta query object would make this class much more versatile for plugin developers.

Attachments (3)

41266-approach-1.diff (1.3 KB) - added by andizer 7 years ago.
41266-approach-2.diff (653 bytes) - added by andizer 7 years ago.
41266.patch (2.3 KB) - added by noisysocks 7 years ago.
A patch based on Approach 1 that includes unit tests

Download all attachments as: .zip

Change History (15)

#1 @boonebgorges
7 years ago

  • Keywords needs-patch good-first-bug added

I agree. This, and the lack of flexibility for the $meta_table name itself (in get_sql()), are long term annoyances for me. Could you suggest an approach in a patch?

#2 @andizer
7 years ago

I've added two patches both containing a different approach. The first patch works with a property having a default value being mt and a setter for overriding that value. The second patch works with a hook to override the default value.

If one of those patches will be merged, please also gives props to ireneyoast

#3 @janw.oostendorp
7 years ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed

#4 follow-up: @ryotsun
7 years ago

@janw.oostendorp
Hello, it's just a quick question.
Which ticket are you going to choose?

#5 in reply to: ↑ 4 ; follow-up: @janw.oostendorp
7 years ago

Replying to ryotsun:

@janw.oostendorp
Hello, it's just a quick question.
Which ticket are you going to choose?

I don't understand what you are asking. Did I miss something?

#6 in reply to: ↑ 5 @ryotsun
7 years ago

Ahhh, I'm sorry.
I wanted to say "approach". Not "ticket".

2 patches are uploaded, so I just wanna know which one would be chosen.

Replying to janw.oostendorp:

Replying to ryotsun:

@janw.oostendorp
Hello, it's just a quick question.
Which ticket are you going to choose?

I don't understand what you are asking. Did I miss something?

@noisysocks
7 years ago

A patch based on Approach 1 that includes unit tests

#7 @noisysocks
7 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Personally, I think Approach 1 is the cleaner solution. WP_Meta_Query is a class and we may as well take advantage of an object oriented approach to solving this problem.

I've added a unit test to 41266-approach-1.diff and uploaded it as 41266.diff.

#8 @DrewAPicture
7 years ago

  • Owner set to noisysocks
  • Status changed from new to assigned

Assigning to mark the good-first-bug as "claimed".

This ticket was mentioned in Slack in #core by sergey. View the logs.


4 years ago

#10 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.8

This appears to have some initial agreement, moving to the milestone.

For consistency, should a similar change be applied to WP_Tax_Query, which also currently has a hardcoded tt table alias prefix?

#11 @desrosj
3 years ago

  • Milestone changed from 5.8 to 5.9

Since this has not seen any movement during the 5.8 cycle, I'm going to punt.

#12 @hellofromTonya
3 years ago

  • Milestone changed from 5.9 to Future Release

Query changes by design need careful consideration and plenty of soak time to ensure nothing breaks. With 5.9 feature freeze ~2 days away, it's too late in the release cycle for a change to query. Punting this out of 5.9. As the next milestone is not yet available, moving to Future Release. Once available, feel free to move into the next release.

Note: See TracTickets for help on using tickets.