Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust the WC_Admin_Notices to support multisite setups #45349

Merged
merged 3 commits into from Mar 7, 2024

Conversation

Konamiman
Copy link
Contributor

@Konamiman Konamiman commented Mar 6, 2024

Changes proposed in this Pull Request:

The WC_Admin_Notices uses a static array to cache the names of the existing notices locally (the $notices field). In multisite setups this will cause the sequence of calls WC_Admin_Notices::add_custom_notice() - switch_to_blog() - WC_Admin_Notices::store_notices() to generate a woocommerce_admin_notices option in the second site with the values of the option names for the first site.

While this doesn't cause notices to actually appear in wrong sites (each notice gets an additional woocommerce_admin_notice_<name> that is created in the proper site and is needed for the notice to actually be displayed), it causes the woocommerce_admin_notices option to get unnecessary values, which can lead to confussion (and in extreme cases degrade the site performance).

This pull request fixes this by transforming the $notices field into an associative array where keys are site ids and values are arrays of notice names (for multisite only: for single site setups the original behavior is kept, so $notices is still a plain names array).

How to test the changes in this Pull Request:

Create a site in multisite mode, add one additional site, run wp shell in the command line and execute this:

WC_Admin_Notices::add_custom_notice('site_1', 'Notice in site 1');
WC_Admin_Notices::store_notices();
switch_to_blog(2);
WC_Admin_Notices::add_custom_notice('site_2', 'Notice in site 2');
WC_Admin_Notices::store_notices();
var_dump(get_option('woocommerce_admin_notices'));

Without this fix you'll get an array with two entries, site_1 and site_2. With the fix you'll get an array with only site_2.

You can also browse the admin area of both sites and verify that each site is still displaying its own notice only.

Changelog entry

  • Automatically create a changelog entry from the details below.

Significance

  • Patch
  • Minor
  • Major

Type

  • Fix - Fixes an existing bug
  • Add - Adds functionality
  • Update - Update existing functionality
  • Dev - Development related task
  • Tweak - A minor adjustment to the codebase
  • Performance - Address performance issues
  • Enhancement - Improvement to existing functionality

Message

Comment

Previously there was one single locally cached instance of the
notice names array that wasn't aware of switch_to_blog executions.
Now in multisite setups one separate array is kept for each
existing blog id.
@Konamiman Konamiman self-assigned this Mar 6, 2024
@Konamiman Konamiman requested review from a team and lsinger and removed request for a team March 6, 2024 13:28
@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label Mar 6, 2024
Copy link
Contributor

github-actions bot commented Mar 6, 2024

Hi @lsinger,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Contributor

github-actions bot commented Mar 6, 2024

Test Results Summary

Commit SHA: 202375b

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 39s
E2E Tests341001003517m 36s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Copy link
Contributor

@lsinger lsinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and tests as intended. Also tested with a non-multisite blog to make sure that's not affected. Merge when CI has passed. :shipit:

@Konamiman Konamiman merged commit 564fd47 into trunk Mar 7, 2024
33 checks passed
@Konamiman Konamiman deleted the improve_admin_notices_processing_in_multisite branch March 7, 2024 09:54
@github-actions github-actions bot added this to the 8.8.0 milestone Mar 7, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label Mar 7, 2024
@alopezari alopezari added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels Mar 11, 2024
Konamiman added a commit that referenced this pull request Mar 13, 2024
Previously there was one single locally cached instance of the notice names array that wasn't aware of switch_to_blog executions. Now in multisite setups one separate array is kept for each existing blog id.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants