Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#38866 closed defect (bug) (fixed)

Custom CSS generates an update SQL query on every front end request

Reported by: bradyvercher's profile bradyvercher Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch
Focuses: Cc:

Description

The wp_get_custom_css_post() can generate an UPDATE query on every front end request in some cases.

On sites that haven't saved the custom CSS setting in the customizer, a custom_css CPT won't exist, so that function runs set_theme_mod( 'custom_css_post_id', -1 ); on every request. Normally, update_option() would short circuit the update query if the new value is the same as the old, but when a header image is set, it saves an object in the header_image_data theme mod.

I didn't dig too deep, but in update_option(), the header_image_data objects have different identifiers, so the strict equality check fails and the update goes through.

Here's a reduced theme mod for the new $value in update_option():

array(3) {
  ["custom_css_post_id"]=>
  int(-1)
  ["header_image"]=>
  string(85) "http://src.wordpress-develop.dev/wp-content/uploads/2016/10/cropped-Blurry-Lights.jpg"
  ["header_image_data"]=>
  object(stdClass)#370 (5) {
    ["attachment_id"]=>
    int(292)
    ["url"]=>
    string(85) "http://src.wordpress-develop.dev/wp-content/uploads/2016/10/cropped-Blurry-Lights.jpg"
    ["thumbnail_url"]=>
    string(85) "http://src.wordpress-develop.dev/wp-content/uploads/2016/10/cropped-Blurry-Lights.jpg"
    ["height"]=>
    int(708)
    ["width"]=>
    int(1260)
  }
}

And the var dump for the $old_value:

array(3) {
  ["custom_css_post_id"]=>
  int(-1)
  ["header_image"]=>
  string(85) "http://src.wordpress-develop.dev/wp-content/uploads/2016/10/cropped-Blurry-Lights.jpg"
  ["header_image_data"]=>
  object(stdClass)#626 (5) {
    ["attachment_id"]=>
    int(292)
    ["url"]=>
    string(85) "http://src.wordpress-develop.dev/wp-content/uploads/2016/10/cropped-Blurry-Lights.jpg"
    ["thumbnail_url"]=>
    string(85) "http://src.wordpress-develop.dev/wp-content/uploads/2016/10/cropped-Blurry-Lights.jpg"
    ["height"]=>
    int(708)
    ["width"]=>
    int(1260)
  }
}

Attachments (3)

38866.diff (1.3 KB) - added by bradyvercher 8 years ago.
38866.2.diff (1.6 KB) - added by peterwilsoncc 8 years ago.
38866.3.diff (1.4 KB) - added by helen 8 years ago.

Download all attachments as: .zip

Change History (10)

@bradyvercher
8 years ago

#1 @bradyvercher
8 years ago

  • Keywords has-patch added

38866.diff does a few things:

  • Checks to make sure $post_id is not -1 before calling get_post() to eliminate an extra SELECT query.
  • Checks to be sure the cached post ID hasn't changed before calling set_theme_mod(), which eliminates the UPDATE query and a SHOW FULL COLUMNS query.
  • Changes the number query arg to posts_per_page. Neither seem to do anything when is_singular is true, but I don't think number is valid.

To test the patch:

  1. Set a header image and ensure the header_image_data theme mod exists.
  2. Delete the custom_css post.

Every request on the front end should execute an UPDATE query. From what I can tell, this would affect any install that upgrades to 4.7 with a header image, as well as new installs that set a header image. The extra queries would continue running until the custom CSS setting is modified in the customizer.

After applying the patch, the first request will execute the UPDATE query if the custom_css_post_id doesn't exist or has an invalid post ID, but subsequent requests should have 3 fewer queries.

#2 @dd32
8 years ago

  • Milestone changed from Awaiting Review to 4.7

Pushing into 4.7 for visibility and testing

#3 @peterwilsoncc
8 years ago

Tested Brady's patch and it behaves as described.

In 38866.2.diff I've attempted to fix underlying problem in update_option() by comparing old an new values as serialised data. I'd like some feedback on the perf implications here.

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


8 years ago

#5 @peterwilsoncc
8 years ago

  • Owner set to peterwilsoncc
  • Status changed from new to assigned

@helen
8 years ago

#6 @helen
8 years ago

Running serialize() on every update_option() call is something for 4.8-early, probably.

In 38866.3.diff I flipped and split some of the logic to make it easier to understand and maintain in the future. Do test to make sure I got it right, though :)

#7 @peterwilsoncc
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 39338:

Themes: Prevent unneeded database updates in wp_get_custom_css_post().

When a custom header image was set but custom CSS was not, wp_get_custom_css_post() was generating an UPDATE query on every frontend request.

In theme options the header image meta data is stored as an object. In update_option() this hits an edge case as the resource IDs of the old and new values never match.

This changes the logic of wp_get_custom_css_post() to ensure set_theme_mod() is only called when the custom CSS has changed.

Props bradyvercher, helen.
Fixes #38866.

Note: See TracTickets for help on using tickets.