Make WordPress Core

Opened 4 weeks ago

Last modified 4 weeks ago

#65853 new defect (bug)

Using is_feed() does not always work

Reported by: josephscott Owned by:
Priority: normal Milestone: Awaiting Review
Component: Feeds Version:
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses: performance

Description

There are code paths where is_feed() is being used, but it does not do what was intended because the query hasn't been parsed yet. That puts is_feed() into a state where $wp_query exists, so _doing_it_wrong() does not get called, but it still incorrectly returns false for is_feed(). Because $wp_query->is_feed( $feeds ); defaults to false.

This can be an issue for things that run via init.

I've got a PR that will add a new function ( and tests ) to check if the request looks like a one for a feed. It will also replace a few is_feed() calls that were not working correctly.

Change History (5)

This ticket was mentioned in PR #12976 on WordPress/wordpress-develop by @josephscott.


4 weeks ago
#1

  • Keywords has-patch has-unit-tests added

https://core.trac.wordpress.org/ticket/65853

This adds the wp_is_serving_feed_request() function, which can be used instead of is_feed() in cases where the query parser has not run yet. Code that runs off of the init action falls into that category.

Included are updates to wp_should_load_separate_core_block_assets() and wp_should_load_block_assets_on_demand() to replace their use of is_feed(). Because they were both using is_feed() before the query was parsed is_feed() always returned false. Which means the early return didn't actually happen.

All this results in WordPress feed requests doing more work than they needed to. In my local testing this reduced TTFB for feed requests 39.3 ms to 37.8 ms at p75. That is a savings of ~1.5 ms or 3.9%.

See https://core.trac.wordpress.org/changeset/50836 for the original work on should_load_separate_core_block_assets() - more than 5 years ago.

AI assistance: Yes
Tool(s): Claude
Model(s): Fable 5
Used for: The code was a joint effort between myself and Claude. The tests were written by Claude.

#2 in reply to: ↑ description @westonruter
4 weeks ago

Replying to josephscott:

There are code paths where is_feed() is being used, but it does not do what was intended because the query hasn't been parsed yet. That puts is_feed() into a state where $wp_query exists, so _doing_it_wrong() does not get called, but it still incorrectly returns false for is_feed(). Because $wp_query->is_feed( $feeds ); defaults to false.

What about just modifying the _doing_it_wrong() code so that it fires if ! did_action( 'parse_query' ) in addition to when ! isset( $wp_query )?

#3 @westonruter
4 weeks ago

For a parallel, the wp_enqueue_script() function calls _wp_scripts_maybe_doing_it_wrong() which checks:

<?php
if ( did_action( 'init' ) || did_action( 'wp_enqueue_scripts' )
        || did_action( 'admin_enqueue_scripts' ) || did_action( 'login_enqueue_scripts' )
) {

#4 @josephscott
4 weeks ago

WordPress has tried to avoid backwards compatibility issues, so changing the behavior of an existing function - one that is more than 20 years old - wasn't my first option. And none of the comments or documentation that I saw on it gave any indication that it only worked correctly after the query had been parsed. If there was something to indicate that is_feed() was only reliable after request parsing then I think there would be an argument for changing how it worked to comply with that description.

It certainly would work for the cases that I was looking at, but I don't know what assumptions themes and plugins have made about it. Anything that expects the existing behavior has the potential to break ( or at least get different results ) after a change like that.

Given the large usage of WordPress and how long this function has been around it runs the risk of hitting Hyrum's Law.

#5 @josephscott
4 weeks ago

The other angle is that triggering _doing_it_wrong() doesn't actually fix the issues with wp_should_load_separate_core_block_assets() and wp_should_load_block_assets_on_demand() - they still need a way to determine if WP is processing a feed request or not before the request parser has happened.

Note: See TracTickets for help on using tickets.