Opened 12 years ago
Last modified 7 years ago
#20899 reviewing defect (bug)
is_home() should be able to be true when is_feed()
Reported by: | nacin | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Query | Keywords: | has-patch needs-testing has-unit-tests dev-feedback |
Focuses: | Cc: |
Description
is_feed() is a special query flag that can be combined with other query flags — for example, is_author() && is_feed()
for /author/admin/feed/.
But it can't be combined with is_home(), because is_home() is the fallback that is only set to true when a lot of other things are true — including is_feed(). This appears to be incorrect — is_home() should still be able to be true despite is_feed().
I tracked this down to [1449]. What kind of breakage could occur with this?
Attachments (4)
Change History (36)
#5
@
11 years ago
- Keywords needs-patch reporter-feedback added; has-patch removed
How this make you feel now - still needed?
#6
@
10 years ago
- Keywords has-patch needs-testing added; needs-patch removed
Patch 20899.diff removes the is_feed
check when assigning the value for the is_home
query argument.
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
9 years ago
#13
@
9 years ago
The patch was not applying correctly anymore.
I refreshed it and added a unit test, even if I'm not sure about its location.
#14
@
9 years ago
- Keywords has-unit-tests 4.6-early added; needs-unit-tests removed
- Milestone changed from 4.5 to Future Release
This smells like something that needs to go in earlier in the cycle, so we have more time to discover hard-to-spot bugs.
@jubstuff FYI, $this->assertQueryTrue()
can be used for asserting conditional functions.
#15
@
8 years ago
- Keywords 4.6-early removed
- Milestone changed from Future Release to 4.7
What about getting this into 4.7? I'll try to update the patch next week.
#16
@
8 years ago
Feedback from @danielbachhuber:
I think this is going to break things
I've usedis_home()
a fair number of times to conditionally modify the main query where changing core's behavior would cause unintended behavior in the main feed
Source: https://wordpress.slack.com/archives/core/p1472220220006226
#18
@
8 years ago
@swissspidy if this is something you want to try, I think it needs to happen soon.
#19
@
8 years ago
Regarding my previous comment, Daniel did not exaggerate.
20899.3.diff makes the patch apply cleanly against trunk again. As you can see from the changed tests, this affects a few queries.
So if anyone's simply checking for is_home()
to modify the query or something, they would likely need to add an && is_feed()
check to it. But we don't know without more testing, right?
Is anyone not comfortable with testing this in trunk to see how this affects things?
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
8 years ago
#22
@
8 years ago
- Milestone changed from 4.7 to Future Release
Punting as per discussion in yesterday's bug scrub.
#23
@
8 years ago
Another possible solution would be to add a new set of keys to WP_Query. e.g.) is_home_feed, is_author_feed, is_archive_feed, etc.
#24
@
8 years ago
I'm starting to think that is an even better idea than I did yesterday. See #39157
#25
@
8 years ago
20899.4.diff builds upon a patch created in #39157 as an attempt to solve this problem by adding two new getter methods and modifying the wp_query conditionals. This allows for the ability to distinguish between a home page and a home feed as well as between a custom feed and a regular feed. This solution should protect against the regression mentioned by @danielbachhuber and @swissspidy above. Unit test pass as of 4.7.2.
#26
@
8 years ago
Thanks @stevenkword!
The patch works as expected and makes sense from a logical point of view. Especially when used in combination with #30210. Because of backward compatibility, these query tags are a bit of an outlier and we'd need to make sure they're documented accordingly. No objections from me.
This ticket was mentioned in Slack in #core by stevenkword. View the logs.
8 years ago
#30
@
8 years ago
Hi @stevenkword,
I tested 20899.4.diff and found it generally worked as expected, but I had a few questions and comments along the way. My apologies if I've missed previous discussions about these.
- Should
is_home_feed()
andis_custom_feed()
be mutually exclusive? I ask because if I registeradd_feed( 'foo', 'do_feed_rss2' )
, then my feed will be available atexample.com/foo/
, and I think I might then assume I was still looking at a "home feed."
- Should
is_custom_feed()
be true for invalid feeds? Currently it looks like it is. I suppose that's consistent withis_feed()
?
- Along those lines, I was curious about the benefits of having an "is custom feed" conditional as opposed to "is bundled feed"? Not that "is custom feed" is unhelpful, but it seems like it currently assumes that something is a custom feed if it isn't a core feed. An "is bundled feed" conditional might be able to tell you roughly the same thing but with more certainty because it could test against a known list of core feeds.
- Currently,
is_home()
will be false whenis_home_feed()
oris_custom_feed()
is true. I think this is expected behavior, but I'm noting it just in case.
- Lastly, should the new
_doing_it_wrong()
calls use 4.8.0 for the version?
#22031 was marked as a duplicate.