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 | Owned by: | 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)
Change History (15)
#2
@
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
#4
follow-up:
↓ 5
@
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:
↓ 6
@
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
@
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?
#7
@
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
@
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
@
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
@
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
@
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.
I agree. This, and the lack of flexibility for the
$meta_table
name itself (inget_sql()
), are long term annoyances for me. Could you suggest an approach in a patch?