Opened 10 years ago
Last modified 8 days ago
#35913 new defect (bug)
`is_()` conditional methods should share their logic
| Reported by: |
|
Owned by: | |
|---|---|---|---|
| Milestone: | 7.1 | Priority: | normal |
| Severity: | normal | Version: | |
| Component: | Query | Keywords: | has-patch 2nd-opinion early |
| Focuses: | Cc: |
Description
Many of the is_() methods in WP_Query share the nearly the exact same logic. As such, they share the exact same bugs, and fixing those bugs requires many parallel changes and huge numbers of tests. See #35902 and #24674 for some recent examples.
It's possible to move all the shared logic to a single protected utility method. It's a bit more abstract, but is much easier to maintain and test.
Attachments (3)
Change History (16)
#3
@
9 months ago
- Keywords needs-refresh added
- Milestone changed from Future Release to 6.9
I like the red!
I concur!
This patch doesn't apply clean since WP_Query is now in a different file though. Would love to see if it's still possible for this duplicate code to be removed in 6.9.
This ticket was mentioned in PR #9161 on WordPress/wordpress-develop by @sukhendu2002.
9 months ago
#4
- Keywords needs-refresh removed
Trac ticket: https://core.trac.wordpress.org/ticket/35913
#5
@
8 months ago
Refactor: Introduce shared helper for is_*() conditional methods to reduce code duplication in WP_Query
#6
@
5 months ago
- Keywords needs-testing added
I think this needs to be punted. The current PR hasn't been reviewed/tested.
This ticket was mentioned in Slack in #core-test by krupajnanda. View the logs.
5 months ago
#8
@
5 months ago
- Milestone changed from 6.9 to 7.0
I agree, this ran out of time for 6.9. Moving to 7.0
#9
@
7 weeks ago
- Keywords needs-refresh added; needs-testing removed
The recent patch and doesn't apply cleanly against trunk. Removing need-testing and adding needs-refresh until we have a refreshed patch to test.
#10
@
4 weeks ago
- Keywords needs-refresh removed
@mukeshpanchal27 reviewed PR #9161 and shared suggestions about 3 weeks ago. I think we should continue working on that PR and address the existing feedback there.
@sachinrajcp123 thanks for your efforts, However, multiple refreshed patches have been uploaded without follow-up discussion, which is making it a bit confusing to verify which implementation should be reviewed.
Removing needs-refresh since the current PR applies cleanly.
#11
@
4 weeks ago
For context, while working on Ticket #35913 earlier, I had explored
introducing a shared helper for object-based is_*() conditional
methods, similar to the is_object_type() abstraction used here.
Glad to see this direction moving forward. Happy to assist with
additional testing or refinements if needed.
35913.diff demonstrates how it can be done. (Look at all that red! <3)
The one slight functional change from the current behavior, made to simplify the new
is_object_type(), is that *all*$objectproperties are cast to strings before thein_array()checks, instead of just the numerical ones. This will be redundant, since all of these properties are already strings.Anyone have thoughts about the wisdom, or lack thereof, of this kind of abstraction?