Make WordPress Core

Opened 7 years ago

Closed 6 months ago

Last modified 6 months ago

#39537 closed enhancement (fixed)

When switching themes, set previous theme mods as autoload = no

Reported by: igmoweb's profile igmoweb Owned by: flixos90's profile flixos90
Milestone: 6.5 Priority: normal
Severity: normal Version: 4.8
Component: Themes Keywords: has-patch has-unit-tests commit
Focuses: performance Cc:

Description

When a theme is switched, previous theme mods are yet autoloaded. This will start filling up the autoloaded options list if you change of Theme often. Normally this won't happen often but we had an issue like this in a big multisite where a user had many sites and he switched themes many times and PHP got out of memory when loading the user in wp-admin.

In any case, it's a good idea to:

  • Set autoload = no for previous theme mods
  • Set autoload = yes again for the new switched theme

Attachments (2)

39537.diff (1.3 KB) - added by igmoweb 7 years ago.
Patch
39537-unit-tests.diff (3.1 KB) - added by igmoweb 7 years ago.
Unit Tests

Download all attachments as: .zip

Change History (26)

@igmoweb
7 years ago

Patch

@igmoweb
7 years ago

Unit Tests

This ticket was mentioned in Slack in #themereview by williampatton. View the logs.


5 years ago

#2 @westonruter
8 months ago

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

This will now be greatly facilitated by #58964 which introduced the wp_set_option_autoload() function which will avoid having to delete and add the option as in the current patch.

#3 @flixos90
7 months ago

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

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


7 months ago

#5 @mukesh27
7 months ago

  • Keywords needs-unit-tests added

This ticket was mentioned in Slack in #core-performance by mukeshpanchal27. View the logs.


6 months ago

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


6 months ago
#7

  • Keywords has-patch added; needs-patch removed

Using wp_set_option_autoload() to set no to the old theme's autoload option.

Trac ticket: https://core.trac.wordpress.org/ticket/39537

#8 @rajinsharwar
6 months ago

Used the wp_set_option_autoload() function for setting the Autoload to No for the old theme.

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


6 months ago
#9

  • Keywords has-unit-tests added; needs-unit-tests removed

Trac ticket: https://core.trac.wordpress.org/ticket/39537

This pull request (PR) sets autoload=no for the previous themes after switching between different themes, utilizing the wp_set_option_autoload function.

Within the switch_theme function, we retrieve two themes: the current theme and the previous theme. However, there isn't a direct method to obtain all previously activated themes. For instance, if a user has used 4-5 themes and all of these themes have theme_mods_* options autoloaded, this method involves querying to retrieve all the theme_mods_* and subsequently setting autoload=no for all themes except the current one.

#10 @mukesh27
6 months ago

  • Owner changed from flixos90 to mukesh27

@swissspidy commented on PR #5706:


6 months ago
#11

However, there isn't a direct method to obtain all previously activated themes. For instance, if a user has used 4-5 themes and all of these themes have theme_mods_* options autoloaded, this method involves querying to retrieve all the theme_mods_* and subsequently setting autoload=no for all themes except the current one.

This (multiple previous theme mods with autoload=yes) is basically just a one time scenario, right? After that, only updating the old theme's option would be enough.

@mukesh27 commented on PR #5706:


6 months ago
#12

This (multiple previous theme mods with autoload=yes) is basically just a one time scenario, right? After that, only updating the old theme's option would be enough.

Yes. Could we add new function in src/wp-admin/includes/upgrade.php for 6.5 and set autoload=no for all stored theme options except current theme? and change the PR code to set autoload option for old theme only?

@swissspidy commented on PR #5706:


6 months ago
#13

Yeah we could. This would have the benefit that it will apply this change immediately upon upgrade, and not only upon the next theme change. And the change in switch_theme would become very straightforward.

@mukesh27 commented on PR #5706:


6 months ago
#14

Thanks @swissspidy @joemcgill and @westonruter for the review. I have updates PR so it will only updates the old and new theme autoload option not for other olds, for that i will open followup issue and raise PR for it.

@mukesh27 commented on PR #5706:


6 months ago
#15

I have invested some time in examining the behavior of the cache that requires reloading or not. To address this issue, we can consider several options:

Option 1: Utilize the wp_set_option_autoload_values function to reset the autoload value. This function deletes all caches in this section of the code. Therefore, we should update the autoload option after deleting the cache within the same function.

Option 2: Remove the autoload option cache assertions from unit tests. Theoretically, these checks might not be necessary.

Option 3: Revert https://github.com/WordPress/wordpress-develop/pull/5706/commits/041eaad9ebae0dbeb5d5bd1af4f59b9aa19d83ac to ensure that the autoload option cache reloads after a theme switch.

In my opinion, the second option seems preferable. When the cache is deleted, it seems logical to run a query to retrieve that option and then reload the cache.

@felixarntz @joemcgill @swissspidy @westonruter, I would greatly appreciate your thoughts on this matter. Thank you!

@westonruter commented on PR #5706:


6 months ago
#16

Option 4: Add wp_load_alloptions() to the test case.

// Make sure that autoloaded options are cached properly.
+               wp_load_alloptions();
                $autoloaded_options = wp_cache_get( 'alloptions', 'options' );
                $this->assertArrayNotHasKey( "theme_mods_$new_theme_stylesheet", $autoloaded_options, "Option theme_mods_$new_theme_stylesheet not deleted from alloptions cache" );
                $this->assertArrayHasKey( "theme_mods_$current_theme_stylesheet", $autoloaded_options, "Option theme_mods_$current_theme_stylesheet unexpectedly deleted from alloptions cache" );

@flixos90 commented on PR #5706:


6 months ago
#17

@mukeshpanchal27 I think we should go with option 2. The assertions for whether the cache is refreshed IMO aren't suitable here, as they are related to the wp_set_option_autoload_values() function itself, which already has sufficient coverage.

For this change, the only thing we need to test is that the autoload values have been correctly updated, which is the main purpose of the function and this change.

@joemcgill commented on PR #5706:


6 months ago
#18

I agree with @mukeshpanchal27 here:

In my opinion, the second option seems preferable. When the cache is deleted, it seems logical to run a query to retrieve that option and then reload the cache.

The only reason the assertions testing the cache values are useful are if the behavior we want to confirm is that the cache is refreshed when the theme mod's autoload values are updated. I don't think that should be the responsibility of the switch_theme() function, and instead will naturally take place the next time that option is requested.

@mukesh27 commented on PR #5706:


6 months ago
#19

Thanks for thoughts and feedback. The PR is ready for final review and commit.

#20 @flixos90
6 months ago

  • Keywords commit added
  • Owner changed from mukesh27 to flixos90
  • Status changed from assigned to reviewing

@flixos90 commented on PR #5692:


6 months ago
#21

Closing in favor of #5706. Thanks for working on this!

#23 @flixos90
6 months ago

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

Fixed via [57153] (wrong ticket reference 🤦‍♂️)

#24 @flixos90
6 months ago

In 57155:

Themes: Clean up inactive themes' theme mods from being autoloaded as part of upcoming 6.5 upgrade routine.

Alongside [57153], this reduces unnecessarily autoloaded data from inactive themes, which can contribute to slow database performance as part of excessive autoloading. This changeset specifically resolves the issue for existing sites.

Props mukesh27, joemcgill, swissspidy, westonruter, flixos90.
Fixes #59975.
See #39537.

Note: See TracTickets for help on using tickets.