Make WordPress Core

Opened 2 years ago

Closed 19 months ago

Last modified 18 months ago

#54677 closed defect (bug) (fixed)

Default widgets with alternative option name causing unnecessary database queries

Reported by: chouby's profile Chouby Owned by: peterwilsoncc's profile peterwilsoncc
Milestone: 6.1 Priority: normal
Severity: normal Version: 5.8
Component: Widgets Keywords: has-patch needs-testing has-unit-tests add-to-field-guide
Focuses: performance Cc:

Description

This ticket is a follow up to #26876, as the same bug still occurs for the widget recent comments and the widget recent posts because both have an alternative option name.

The bug was not detectable at that time because both widgets were used by default, and thus options were always filled. However, this is no more the case since the version 5.8. Now if no option have been saved, then we can see that 4 useless database queries are triggered:

SELECT option_value
FROM wp_options
WHERE option_name = 'widget_recent-posts'
LIMIT 1	

SELECT option_value
FROM wp_options
WHERE option_name = 'widget_recent_entries'
LIMIT 1	

SELECT option_value
FROM wp_options
WHERE option_name = 'widget_recent-comments'
LIMIT 1	

SELECT option_value
FROM wp_options
WHERE option_name = 'widget_recent_comments'
LIMIT 1

The bug can easily reproduced on a fresh WP 5.8 install with Query Monitor to see the useless queries triggered by WP_Widget->get_settings().

Attachments (1)

54677.patch (1.3 KB) - added by mvraghavan 2 years ago.
Created patch

Download all attachments as: .zip

Change History (17)

#1 @audrasjb
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 6.0

@mvraghavan
2 years ago

Created patch

#2 @mvraghavan
2 years ago

  • Keywords has-patch added; needs-patch removed

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


23 months ago

#4 @costdev
23 months ago

  • Keywords needs-testing added

Per the discussion in the bug scrub, I'm adding the needs-testing keyword in hopes that this might be tested ahead of RC1 on Tuesday.

#5 @costdev
23 months ago

  • Milestone changed from 6.0 to 6.1

With 6.0 RC1 starting soon, I'm moving this to the 6.1 milestone.

#6 @peterwilsoncc
21 months ago

It looks like 54677.patch prioritizes $this->alt_option_name over the current option name $this->option_name. My reading of the code is that if both options exist, then option_name is likely to hold the correct data.

I am still testing but I think WP_Widget::save_settings() should remain unchanged while WP_Widget::get_settings() should start with something along the lines of the following code.

<?php
$settings = get_option( $this->option_name );

if ( false === $settings ) {
        $settings = array();
        if ( isset( $this->alt_option_name ) ) {
                // Get from legacy (alternative) option name.
                $settings = get_option( $this->alt_option_name, array() );

                // Delete the legacy option as the new option will be created.
                delete_option( $this->alt_option_name );
        }
        // Save an option so it can be autoloaded next time.
        $this->save_settings( $settings );
}

This ticket was mentioned in PR #2951 on WordPress/wordpress-develop by peterwilsoncc.


21 months ago
#7

  • Keywords has-unit-tests added

#8 @peterwilsoncc
21 months ago

In the linked pull request:

  • no change to WP_Widget::save_settings() it always uses $this->option_name.
  • WP_Widget::get_settings() uses $this->option_name if it it exists
  • WP_Widget::get_settings() falls back $this->alt_option_name if it it exists
  • WP_Widget::get_settings() initializes $this->option_name if neither exist
  • WP_Widget::get_settings() migrates $this->alt_option_name to $this->option_name only the alt option is set
  • If both options exist then no changes are made to the database -- doing so would create extra db queries if the alt option was not set.

Testing notes:

  • no options creates options_name
  • alt option exists and is migrated to option_name
  • new option exists does not create alt_option_name
  • if both options exist then both remain in the database

#9 @spacedmonkey
21 months ago

Someone from my team also looked into this issue. Here is their PR #12

#10 @peterwilsoncc
20 months ago

At @spacedmonkey's suggestion I investigated using an upgrade routine https://github.com/WordPress/wordpress-develop/pull/2951 the linked pull request].

On a single site install the upgrade routine is pointless, as wp_widgets_init() is run on the init hook and fires prior to the redirect to the upgrade screen.

An upgrade routine may be beneficial for Multisite installs (I'm not sure either way) but, given the options will be upgraded regardless, I am inclined not to add one.

The presence of an upgrade routine causes a complete cache flush, so any benefit to MS installs needs to be weighed against the impact for single site installs.

#11 @mukesh27
19 months ago

@peterwilsoncc I left some small feedback on PR.

mukeshpanchal27 commented on PR #2951:


19 months ago
#12

Thanks @peterwilsoncc for the update. PR look good to me.

@costdev Can you please review it when you get the time so it can be merged?

peterwilsoncc commented on PR #2951:


19 months ago
#13

I've applied many of @costdev's suggestions in 8ae2130a493e5ae210a1eabe7901df630740086b

  • test uses initializes
  • consistent reference to "alternative (legacy) option" on first appearance in a comment, from then on "alternative option"

#14 @peterwilsoncc
19 months ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from new to closed

In 54112:

Widgets: Store default options for uninitialized widgets.

Prevent unnecessary database queries on page load by initializing widget options. On sites with uninitialized widgets, this prevents one or two database queries per uninitialized widget on each page load.

Props Chouby, mvraghavan, costdev, peterwilsoncc, spacedmonkey, mukesh27.
Fixes #54677.

#16 @milana_cap
18 months ago

  • Keywords add-to-field-guide added
Note: See TracTickets for help on using tickets.