Set autoload=no for previous theme after switching themes

[mukeshpanchal27]

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

This PR sets autoload=no for the old themes after switching between different themes, utilizing the wp_set_option_autoload_values 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.

This will be done using separate ticket: 59975

This PR only updates the autoload value for old and new themes.

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[swissspidy]

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.

[mukeshpanchal27]

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]

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.

[joemcgill]

I like the idea you and @swissspidy are discussing about making the initial change part of an upgrade routine and then only changing the autoload values for the old theme and new theme. I left a few nitpicks in the mean time for you to consider as part of the updated implementation, but neither blocking.

[mukeshpanchal27]

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.

[joemcgill]

This looks good to me, @mukeshpanchal27!

[felixarntz]

@mukeshpanchal27 Thanks for the PR, it looks solid so far. However, two things are missing as far as I can tell.

Another question: I first saw there was #5692, how does this PR relate to that one?

[felixarntz]

@mukeshpanchal27 Per #5706 (comment), most of my feedback can be ignored.

The only thing I'd agree we should change is https://github.com/WordPress/wordpress-develop/pull/5706/files#r1407978384.

[mukeshpanchal27]

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 041eaad 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]

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" );

[felixarntz]

@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.

[felixarntz]

See #5706 (comment).

[joemcgill]

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.

[mukeshpanchal27]

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

[joemcgill]

The updates look good to me. The only thing I would consider changing is moving the new test method to the Test_Themes class in /tests/phpunit/tests/theme.php rather than creating a new class for just this one method, unless we think there will be more tests that need to be added here. Not a blocker, though.

[felixarntz]

Committed in https://core.trac.wordpress.org/changeset/57153