Make WordPress Core

Opened 2 months ago

Last modified 2 months ago

#54443 new defect (bug)

Database Error Breaks "custom_css_post_id" Theme Mod

Reported by: domainsupport Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Customize Keywords: needs-patch
Focuses: Cc:

Description

We have now seen this issue on a couple of WordPress installations and it is rare but it happens.

When MySQL falls over during a page load (for RAM issues or whatever) the request on line 1863 of /wp-includes/theme.php ...

<?php
                $post_id = get_theme_mod( 'custom_css_post_id' );

... fails or it might be line 1866 ...

<?php
                        $post = get_post( $post_id );

... that fails.

Either way, when line 1877 is executed and the database has by then recovered ...

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

... this results in the custom_css_post_id theme mod being set to -1 and all the Additional CSS is seemed to be wiped out from the Customizer.

At this point the content of the post with post_type set as custom_css in the database has to be copied back into "Customizer - Additional CSS".

Here is an example of a PHP error around the time that this happened ... please note that we have seen it happen on completely different themes and most recently on the latest version of WordPress (yesterday) ...

[24-Feb-2021 20:46:45 UTC] WordPress database error Server shutdown in progress for query SELECT * FROM wp_posts WHERE ID = 2213700 LIMIT 1 made by require('wp-blog-header.php'), require_once('wp-includes/template-loader.php'), include('/plugins/wp-property-feed/templates/archive-wppf_property.php'), get_header, locate_template, load_template, require_once('/themes/twentyseventeen/header.php'), wp_head, do_action('wp_head'), WP_Hook->do_action, WP_Hook->apply_filters, wp_custom_css_cb, wp_get_custom_css, wp_get_custom_css_post, get_post, WP_Post::get_instance

To fix this issue from re-occuring I think that set_theme_mod on line 1877 is replaced with ...

<?php
                        if ($post) {

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

                        }

... so that the theme mod cannot be broken and CSS cannot be lost when the database has a blip.

What do you think?

Oliver

Change History (6)

#1 @dlh
2 months ago

  • Focuses css administration removed
  • Keywords close added
  • Version changed from trunk to 4.7

Adding the condition at that location would mean, I think, that the "no custom CSS" state wouldn't be cached during normal database operations, causing unnecessary lookups on subsequent requests. If so, I'm not sure that the performance cost is worth the potential benefit.

Additionally, is there something about the custom CSS logic in question that makes it likely to be running when database issues occur? Database blips would be indiscriminate in the code they affect, I would think. I'm not sure that it's clear why this location in particular deserves extra checks.

#2 @domainsupport
2 months ago

Thank you for looking into this and I totally understand why you've marked it as "closed" ... however, if you would take the time to reconsider I would be most grateful.

I've seen this occur now 3 or 4 times on two completely different sites with completely different hosting environments. Bearing in mind that I only look after a few hundred WordPress sites, this leads me to believe that the issue must be more common, worldwide, than one would imagine. After a quick Google, here is another likely victim for example ... https://wordpress.org/support/topic/all-additional-css-is-missing/

The problem with the highlighted code is that if the CSS isn't found, whether it is there or not, it is then marked, as you say "no custom css". Which is great when there really isn't any custom CSS post for the ID in the option (not sure when this would be the case TBH) but not great when the post actually does exist.

Is there a way to confirm that the custom css post was not returned from the database because it didn't exist and not because there was an error ... ?

The reason why this location deserves the extra checks is that the outcome is so destructive to the site. I cannot think of another situation where if a database query is not successful (for whatever reason) then the outcome is so detrimental.

Furthermore, although the css post isn't actually deleted, your average user wouldn't know where to look for it to restore their CSS!

If you don't mind I will add to this thread if I see the problem re-occur.

Many thanks,

Oliver

#3 @dlh
2 months ago

  • Keywords needs-patch added; close removed
  • Milestone changed from Awaiting Review to Future Release

Thanks for providing this context and feedback, @domainsupport!

As a procedural matter, note that the close keyword acts more like a suggestion for other maintainers and for the core developers, as opposed to a resolution.

Your point about the unusually visible effect of the problem is well-taken, so I've switched from close to needs-patch to solicit approaches.

when there really isn't any custom CSS post for the ID in the option (not sure when this would be the case TBH)

The custom CSS post isn't created until custom CSS is actually saved in the Customizer. For sites that never use the Customizer or custom CSS, or don't provide access to either, the expected ID would be -1.

Is there a way to confirm that the custom css post was not returned from the database because it didn't exist and not because there was an error ... ?

Possibly. It does seem a little out of scope to me for that function to try to diagnose potential database problems at the point in the request lifecycle when it usually runs, which is in the middle of rendering a template.

But your question got me thinking: Perhaps a custom CSS post should always be created when a theme is activated. That would remove the need for the query entirely, and if the query did happen, an empty result would be clearly suspect.

Last edited 2 months ago by dlh (previous) (diff)

#4 follow-up: @domainsupport
2 months ago

OK, great!!

Which is great when there really isn't any custom CSS post for the ID in the option (not sure when this would be the case TBH)

Sorry about the double negative ... what I meant was I'm not sure why the value would be anything other than "-1" if CSS was not required. Surely a positive integer would infer that a custom_css custom post type post should exist ... or is there somewhere where this post might be deleted without the custom_css_post_id theme mod being updated to "-1"?

But I agree with you that, on reflection, the theme.php file shouldn't be checking for database errors. It's a shame that get_post() doesn't differentiate between an empty result and a database error (by returning an appropriate WP_Error object rather than null for example).

Perhaps a custom CSS post should always be created when a theme is activated. That would remove the need for the query entirely, and if the query did happen, an empty result would be clearly suspect.

Totally agree with this ... although ... would this require input from the FSE theme component team as I don't believe they've yet decided what to do with the potentially soon-to-be-redundant "Customizer - Additional CSS" AFAIK ... ?

Oliver

#5 @domainsupport
2 months ago

As an aside, I wrote this to restore lost custom CSS ...

<?php
if (-1 === get_theme_mod('custom_css_post_id')) {

   $custom_css_posts = get_posts(array('post_type' => 'custom_css', 'numberposts' => -1));

   if (is_array($custom_css_posts)) {

      foreach ($custom_css_posts as $custom_css_post) {

         if (get_stylesheet() === $custom_css_post->post_title) {

            set_theme_mod('custom_css_post_id', $custom_css_post->ID);

         }

      }

   }

}

#6 in reply to: ↑ 4 @dlh
2 months ago

Replying to domainsupport:

Surely a positive integer would infer that a custom_css custom post type post should exist ... or is there somewhere where this post might be deleted without the custom_css_post_id theme mod being updated to "-1"?

Oh, great point! At a glance, I don't see any logic in core that would allow for deleting the post without updating the theme mod, so that might be a sound assumption to make in wp_get_custom_css_post().

Note: See TracTickets for help on using tickets.