Make WordPress Core

Opened 14 years ago

Last modified 4 weeks ago

#20899 reviewing defect (bug)

is_home() should be able to be true when is_feed()

Reported by: nacin Owned by:
Priority: normal Milestone: Future Release
Component: Query Version:
Severity: normal Keywords: has-patch has-unit-tests 2nd-opinion needs-test-info
Cc: Focuses:

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)

20899.diff​ (826 bytes ) - added by stevenkword 12 years ago.
20899.2.diff​ (1.4 KB ) - added by jubstuff 11 years ago.
20899.3.diff​ (4.3 KB ) - added by swissspidy 10 years ago.
20899.4.diff​ (6.9 KB ) - added by stevenkword 10 years ago.

Download all attachments as: .zip

Change History (39)

#1 @johnbillion
14 years ago

  • Cc johnbillion added

#2 @batmoo
14 years ago

  • Cc batmoo@… added

#3 @ocean90
14 years ago

#22031 was marked as a duplicate.

#5 @wonderboymusic
13 years ago

  • Keywords needs-patch reporter-feedback added; has-patch removed

How this make you feel now - still needed?

#6 @stevenkword
12 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.

@stevenkword
12 years ago

#8 @stevenkword
11 years ago

Anyone have an argument against this?

#9 @johnbillion
11 years ago

  • Owner set to johnbillion
  • Status new → reviewing

#10 @stevenkword
11 years ago

  • Milestone Future Release → 4.5

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


11 years ago

#12 @johnbillion
11 years ago

  • Keywords needs-unit-tests added; reporter-feedback removed

@jubstuff
11 years ago

#13 @jubstuff
11 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 @johnbillion
11 years ago

  • Keywords has-unit-tests 4.6-early added; needs-unit-tests removed
  • Milestone 4.5 → 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 @swissspidy
10 years ago

  • Keywords 4.6-early removed
  • Milestone Future Release → 4.7

What about getting this into 4.7? I'll try to update the patch next week.

#16 @swissspidy
10 years ago

Feedback from @danielbachhuber:

I think this is going to break things
I've used is_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

#17 @johnbillion
10 years ago

  • Owner johnbillion removed

#18 @jorbin
10 years ago

@swissspidy if this is something you want to try, I think it needs to happen soon.

@swissspidy
10 years ago

#19 @swissspidy
10 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?

#20 @swissspidy
10 years ago

  • Keywords dev-feedback added

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


10 years ago

#22 @swissspidy
10 years ago

  • Milestone 4.7 → Future Release

Punting as per discussion in yesterday's bug scrub.

#23 @stevenkword
10 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 @stevenkword
10 years ago

I'm starting to think that is an even better idea than I did yesterday. See #39157

#25 @stevenkword
10 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.

@stevenkword
10 years ago

#26 @swissspidy
10 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.

#27 @stevenkword
10 years ago

#11053 was marked as a duplicate.

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


10 years ago

#29 @stevenkword
10 years ago

  • Milestone Future Release → 4.8

#30 @dlh
10 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() and is_custom_feed() be mutually exclusive? I ask because if I register add_feed( 'foo', 'do_feed_rss2' ), then my feed will be available at example.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 with is_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 when is_home_feed() or is_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?

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


9 years ago

#32 @obenland
9 years ago

  • Milestone 4.8 → Future Release

#33 @SirLouen
16 months ago

  • Keywords 2nd-opinion added; needs-testing dev-feedback removed

There are a lot of questions open to @stevenkword that were never answered. Nothing to test here for now until they are resolved

#34 @SirLouen
16 months ago

  • Keywords needs-test-info added

This ticket was mentioned in ​PR #13531 on ​WordPress/wordpress-develop by ​skikken.


4 weeks ago
#35

The blog homepage feed could not be identified on its own. is_home() is false on every feed, and is_feed() does not say which feed is being served, so themes and plugins had no supported way to target the homepage feed specifically.

This adds two conditionals rather than changing is_home(). Making is_home() true on feeds was discussed on the ticket and rejected, because plugins that filter the main query with is_home() would begin matching on feeds.

is_home_feed() is true for the blog homepage feed and false for comment feeds. is_custom_feed() is true when the feed is not one of the bundled types registered in WP_Rewrite::$feeds (feed, rdf, rss, rss2, atom). That list is how a feed added through add_feed() is told apart from a bundled one, since add_feed() appends to the same list without marking the entry as custom.

## Use of AI Tools
AI assistance: Yes
Tool(s): Claude
Model(s): Sonnet
Used for: test cases, code review

Note: See TracTickets for help on using tickets.