WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 26 hours 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 (3)

20899.diff (826 bytes) - added by stevenkword 21 months ago.
20899.2.diff (1.4 KB) - added by jubstuff 10 months ago.
20899.3.diff (4.3 KB) - added by swissspidy 3 months ago.

Download all attachments as: .zip

Change History (27)

#1 @johnbillion
4 years ago

  • Cc johnbillion added

#2 @batmoo
4 years ago

  • Cc batmoo@… added

#3 @ocean90
4 years ago

#22031 was marked as a duplicate.

#5 @wonderboymusic
3 years ago

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

How this make you feel now - still needed?

#6 @stevenkword
21 months 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
21 months ago

#8 @stevenkword
16 months ago

Anyone have an argument against this?

#9 @johnbillion
15 months ago

  • Owner set to johnbillion
  • Status changed from new to reviewing

#10 @stevenkword
12 months ago

  • Milestone changed from Future Release to 4.5

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


11 months ago

#12 @johnbillion
11 months ago

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

@jubstuff
10 months ago

#13 @jubstuff
10 months 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
10 months 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 @swissspidy
3 months 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 @swissspidy
3 months 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
3 months ago

  • Owner johnbillion deleted

#18 @jorbin
3 months ago

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

@swissspidy
3 months ago

#19 @swissspidy
3 months 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
2 months ago

  • Keywords dev-feedback added

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


2 months ago

#22 @swissspidy
2 months ago

  • Milestone changed from 4.7 to Future Release

Punting as per discussion in yesterday's bug scrub.

#23 @stevenkword
2 days 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
26 hours ago

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

Note: See TracTickets for help on using tickets.