Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/wp-includes/theme.php
Original file line number Diff line number Diff line change
Expand Up @@ -2018,9 +2018,12 @@ function wp_get_custom_css_post( $stylesheet = '' ) {
$post = $query->post;
/*
* Cache the lookup. See wp_update_custom_css_post().
* A stored post ID is kept when the lookup fails, as the post is expected to exist.
* @todo This should get cleared if a custom_css post is added/removed.
*/
set_theme_mod( 'custom_css_post_id', $post ? $post->ID : -1 );
if ( $post || empty( $post_id ) ) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should fix: This changes behaviour but has no test yet, as @webd-uk also asked. A DB failure is easy to simulate with the query filter. The test below fails on trunk (Failed asserting that -1 is identical to <ID>) and passes with this PR. I ran it locally, so it could go into tests/phpunit/tests/customize/custom-css-setting.php or a new tests/phpunit/tests/theme/wpGetCustomCssPost.php:

/**
 * @ticket 54443
 *
 * @covers ::wp_get_custom_css_post
 */
public function test_stored_post_id_is_kept_when_lookup_fails() {
	global $wpdb;

	$post = wp_update_custom_css_post( 'body { color: red; }' );

	$suppress = $wpdb->suppress_errors( true );
	$break    = static function ( $query ) use ( $wpdb ) {
		return str_contains( $query, $wpdb->posts ) ? 'SELECT broken FROM missing_table' : $query;
	};
	add_filter( 'query', $break );
	clean_post_cache( $post->ID );

	$this->assertNull( wp_get_custom_css_post(), 'The lookup should fail.' );

	remove_filter( 'query', $break );
	$wpdb->suppress_errors( $suppress );

	$this->assertSame( $post->ID, get_theme_mod( 'custom_css_post_id' ), 'The stored post ID should be kept.' );
}

A second test for a site with no custom CSS post caching -1 would cover the other branch.

set_theme_mod( 'custom_css_post_id', $post ? $post->ID : -1 );

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Would checking $wpdb->last_error right after the WP_Query work here instead? In the failure simulation above, last_error is set at that point. Skipping set_theme_mod() only when the query errored would protect the stored ID and still let a really deleted post fall back to -1, so there are no repeated lookups. I know @dlh found diagnosing DB errors here a bit out of scope in comment:3, so this is only an option for whoever commits it. The current approach is fine for the case in the ticket. Roughly (with global $wpdb; added to the function):

if ( ! $post && $wpdb->last_error ) {
	// The lookup failed; keep the stored ID rather than caching "no post".
} else {
	set_theme_mod( 'custom_css_post_id', $post ? $post->ID : -1 );
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to add some real-world context to this trade-off.

I've encountered orphaned custom CSS posts dozens of times over the years across both my own and clients' WordPress installations. I can't 100% say every instance was caused by this particular bug, but it's certainly a recurring problem.

On the other hand, I've never encountered anyone manually deleting a custom CSS post directly from the database. Doing so bypasses WordPress's normal housekeeping anyway, so some resulting inconsistencies are to be expected.

Personally, I'd favour keeping the fix simple rather than introducing additional complexity to accommodate an edge case that, in my experience at least, is considerably less likely than the problem we're trying to solve.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the manually deleted case, I agree with @webd-uk. Deleting the custom_css post directly in the database skips WordPress's own housekeeping, so it's already "doing it wrong". Once the CSS is saved again it fixes itself, until then the cost is a few extra queries per request. I think that's a better trade-off than a site losing its CSS completely.

I originally thought about checking $wpdb->last_error first and decided against it for the following reasons:

  1. @dlh pointed out in comment:3 that diagnosing database errors in this function is out of scope, and his comment:6 points to the approach this PR takes instead.
  2. It doesn't catch every failure. If the connection drops and reconnecting fails after template_redirect (custom CSS loads on wp_head), wpdb::query() returns false without setting last_error. -1 would still be cached in that case.
  3. last_error only describes the most recent query. A plugin running its own query from posts_results or the_posts would overwrite it before the check.

}
}
} else {
$query = new WP_Query( $custom_css_query_vars );
Expand Down
Loading