Commit b92859d2aac for woocommerce
commit b92859d2aac972f0eb264a9d34965e786e328a61
Author: Oleksandr Aratovskyi <79862886+oaratovskyi@users.noreply.github.com>
Date: Tue Aug 18 14:45:48 2026 +0300
Replace the settings registration guard with idempotent registrars (#67718)
* Replace the settings registration guard with idempotent registrars
The doing_action( 'rest_api_init' ) && did_action( 'admin_init' ) guard
shipped in WC 11.0 is order-dependent: a plugin booting the REST server
before admin_init duplicates every settings group, while a REST dispatch
early in admin_init skips registration for the request that needed it.
It also made rest_api_init a dead path on admin pages, and moved the
woocommerce_email_classes latch to admin_init priority 10, silently
dropping email classes that extensions register from their own
admin_init callbacks.
Root cause of the duplication: every registration pass created new
WC_Register_WP_Admin_Settings instances, so WordPress could not
deduplicate their callbacks. Memoize one registrar per settings page or
email object on the WooCommerce singleton and re-attach the same
callbacks on repeat calls: add_filter() overwrites instead of stacking,
which makes registration safe from any hook, any number of times, with
no context guard at all. Register on admin_init at PHP_INT_MAX so that
extension callbacks adding settings pages or emails on admin_init are
captured before the first-use latch freezes both filters.
* Add changelog entry for the idempotent settings registration fix
* Guard non-object settings sources before keying the registrar memo
register_wp_admin_settings_for() computed spl_object_id() before constructing
the registrar, which moved the key computation ahead of the is_object() guard
WC_Register_WP_Admin_Settings has held since 3.0.
woocommerce_get_settings_pages and woocommerce_email_classes are third-party
writable, so a callback returning a non-object went from being silently skipped
to raising an uncaught TypeError on admin_init and rest_api_init, taking down
wp-admin and every REST request.
Check is_object() before computing the key, restoring the previous tolerance.
Refs #67461
* Use priority 999 instead of PHP_INT_MAX for admin_init registration
999 is the dominant convention in Woo core for execute-last semantics (34
occurrences against 3 for PHP_INT_MAX), raised in review on #67494.
Verified it is high enough: the highest explicit admin_init priority is 20 in
core and 100 across the installed extension set (Stripe, Square, PayPal,
Amazon, Mercado Pago, Subscriptions, WooPayments). Unlike PHP_INT_MAX, 999 also
leaves room for an extension that deliberately needs to run after registration.
Refs #67461
* Reword changelog entry to describe the released-version impact
The previous message advertised fixing duplicated settings groups, but the
guard that caused the duplication shipped in #67494, which is itself unreleased
(git tag --contains returns 11.1.0-dev only). No released version ever
duplicated them, so the entry described a bug users never saw.
The dropped-registration half is real relative to 11.0.x, where registration
ran only on rest_api_init, so keep that and drop the duplication claim.
Refs #67461
* Retarget @since annotations at 11.2.0
The PR now targets 11.2 rather than 11.1, and trunk is at 11.2.0-dev.
Refs #67461
* Restore the duplicated-settings-groups note in the changelog entry
The entry dropped that symptom on the reasoning that no released version
duplicated settings groups. That no longer holds: #67494 is already an ancestor
of release/11.1, so 11.1 ships the duplication, and this fix now lands in 11.2.
Both symptoms are real against a released version, so the entry names both.
Refs #67461
* Add changelog entry for the admin_init registration priority move
The existing entry names the merchant-visible symptoms: duplicated settings
groups, and settings pages or email classes dropped when registered from
admin_init callbacks. It says nothing about how registration itself moved.
#67494 shipped the admin_init binding at the default priority 10 in 11.0.1,
so a remove_action() call with no priority argument unhooks it on current
stable and silently no-ops once this lands. That is a developer-facing break
with no trace in the release notes.
Add a second entry typed dev, which is where release notes render hook and
deprecation advisories, so extension authors get the priority to match.
Refs #67461
* Reword the priority changelog entry to state ordering, not outcome
The dev entry promised that extensions adding settings pages or email
classes from their own admin_init callbacks "are no longer dropped".
That guarantee is false two independent ways. Callbacks registered at or
after priority 999 still lose the race. And with the Block Email Editor
enabled, Integration::init_hooks() calls WC_Emails::instance() from
woocommerce_init, which fires at init priority 0, so the email list
latches before admin_init runs at any priority: an extension registering
at priority 20 is ahead of our callback and still dropped. Narrowing the
wording to "before WooCommerce's registration callback", as review
suggested, leaves that second case wrong.
State the ordering change instead of an outcome, since ordering is what
the priority move actually controls, and name the releases the
remove_action guidance matters for: the binding ships at the default
priority in 11.0.1 and 11.1.
Refs #67461
* Fix hook-context test not reproducing the removed registration guard
The test called register_wp_admin_settings() directly, where
doing_action( 'rest_api_init' ) is always false, so the guard this branch
removes could never have fired inside it. The pre-PR code passed the test
unchanged, leaving the hook-context axis unverified.
Reproduce the guard's exact condition instead, by setting the two globals
doing_action() and did_action() read and restoring both afterwards.
Driving the hooks with do_action() would fire every core callback bound
to admin_init and rest_api_init, which this test class does not otherwise
do.
Confirmed by reinstating the guard locally: the test fails with it and
passes without it.
diff --git a/plugins/woocommerce/changelog/dev-settings-registration-admin-init-priority b/plugins/woocommerce/changelog/dev-settings-registration-admin-init-priority
new file mode 100644
index 00000000000..0bb8d3d1459
--- /dev/null
+++ b/plugins/woocommerce/changelog/dev-settings-registration-admin-init-priority
@@ -0,0 +1,4 @@
+Significance: patch
+Type: dev
+
+Settings registration now runs on `admin_init` at priority 999 instead of the default 10, so it runs after other callbacks on that hook rather than before. Code that unhooks it must match the new priority: `remove_action( 'admin_init', array( WC(), 'register_wp_admin_settings' ), 999 )`; the no-priority form that matches in 11.0.1 and 11.1 silently stops matching.
diff --git a/plugins/woocommerce/changelog/fix-idempotent-settings-registration b/plugins/woocommerce/changelog/fix-idempotent-settings-registration
new file mode 100644
index 00000000000..1201003e514
--- /dev/null
+++ b/plugins/woocommerce/changelog/fix-idempotent-settings-registration
@@ -0,0 +1,4 @@
+Significance: patch
+Type: fix
+
+Fix settings groups being registered twice, and settings pages or email classes registered from admin_init callbacks being dropped.
diff --git a/plugins/woocommerce/includes/class-wc-register-wp-admin-settings.php b/plugins/woocommerce/includes/class-wc-register-wp-admin-settings.php
index 442cfb8d1fd..26545c760e8 100644
--- a/plugins/woocommerce/includes/class-wc-register-wp-admin-settings.php
+++ b/plugins/woocommerce/includes/class-wc-register-wp-admin-settings.php
@@ -22,6 +22,14 @@ class WC_Register_WP_Admin_Settings {
*/
protected $object;
+ /**
+ * Type of settings the wrapped object holds ('page' or 'email').
+ *
+ * @since 11.2.0
+ * @var string
+ */
+ protected $type;
+
/**
* Hooks into the settings API and starts registering our settings.
*
@@ -35,13 +43,40 @@ class WC_Register_WP_Admin_Settings {
}
$this->object = $object;
+ $this->type = $type;
+
+ $this->register();
+ }
- if ( 'page' === $type ) {
+ /**
+ * Attaches the filters that expose the wrapped object's settings to the Settings REST API.
+ *
+ * Called from the constructor; safe to call again to re-attach after hook state has been
+ * reset. Repeat calls re-add the same callbacks, which WordPress stores under the same
+ * identifiers, so they never duplicate registered groups or settings.
+ *
+ * @since 11.2.0
+ * @return void
+ */
+ public function register() {
+ if ( 'page' === $this->type ) {
+ /**
+ * A settings page (or an object with a compatible shape).
+ *
+ * @var WC_Settings_Page $page
+ */
+ $page = $this->object;
add_filter( 'woocommerce_settings_groups', array( $this, 'register_page_group' ) );
- add_filter( 'woocommerce_settings-' . $this->object->get_id(), array( $this, 'register_page_settings' ) );
- } elseif ( 'email' === $type ) {
+ add_filter( 'woocommerce_settings-' . $page->get_id(), array( $this, 'register_page_settings' ) );
+ } elseif ( 'email' === $this->type ) {
+ /**
+ * An email (or an object with a compatible shape).
+ *
+ * @var WC_Email $email
+ */
+ $email = $this->object;
add_filter( 'woocommerce_settings_groups', array( $this, 'register_email_group' ) );
- add_filter( 'woocommerce_settings-email_' . $this->object->id, array( $this, 'register_email_settings' ) );
+ add_filter( 'woocommerce_settings-email_' . $email->id, array( $this, 'register_email_settings' ) );
}
}
diff --git a/plugins/woocommerce/includes/class-woocommerce.php b/plugins/woocommerce/includes/class-woocommerce.php
index 27414314dfb..7d59f7b405d 100644
--- a/plugins/woocommerce/includes/class-woocommerce.php
+++ b/plugins/woocommerce/includes/class-woocommerce.php
@@ -102,6 +102,15 @@ final class WooCommerce {
*/
private $api;
+ /**
+ * WP admin settings registrars created by register_wp_admin_settings(), memoized per
+ * settings page or email object (keyed by spl_object_id) so that repeat registrations
+ * reuse the same callback identities instead of stacking duplicate filters.
+ *
+ * @var WC_Register_WP_Admin_Settings[]
+ */
+ private $wp_admin_settings_registrars = array();
+
/**
* Product factory instance.
*
@@ -349,7 +358,10 @@ final class WooCommerce {
// Originating from https://github.com/woocommerce/woocommerce/pull/11082 (WC_API::register_wp_admin_settings(), July 2016),
// the settings were intended for REST context only. By chance, admin pages relied on unconditional rest_preload_api_request calls,
// that triggered rest_api_init as a side effect, and over time admin code bound to these settings as well.
- add_action( 'admin_init', array( $this, 'register_wp_admin_settings' ) );
+ // Registration runs late on admin_init because it latches woocommerce_get_settings_pages and
+ // woocommerce_email_classes on first use: extensions adding their callbacks from their own
+ // admin_init handlers would otherwise be silently dropped for the whole request.
+ add_action( 'admin_init', array( $this, 'register_wp_admin_settings' ), 999 );
add_action( 'rest_api_init', array( $this, 'register_wp_admin_settings' ) );
add_action( 'woocommerce_installed', array( $this, 'add_woocommerce_remote_variant' ) );
@@ -1535,6 +1547,12 @@ final class WooCommerce {
*
* This method used to be part of the now removed Legacy REST API.
*
+ * It is idempotent and independent of the hook it runs from: it fires on whichever of
+ * admin_init / rest_api_init comes first and is safe to run again on the other, or on
+ * any repeat invocation (plugins can boot the REST server at any point of an admin
+ * request). Repeat calls re-attach the same registrar callbacks, which WordPress
+ * deduplicates, so settings groups and settings are never registered twice.
+ *
* @since 9.0.0
*
* @internal For exclusive usage of WooCommerce core, backwards compatibility not guaranteed.
@@ -1542,20 +1560,48 @@ final class WooCommerce {
* @return void
*/
public function register_wp_admin_settings() {
- // Avoid double-loading in admin caused by rest_preload_api_request calls (e.g. analytics page).
- if ( doing_action( 'rest_api_init' ) && did_action( 'admin_init' ) ) {
- return;
- }
-
$pages = WC_Admin_Settings::get_settings_pages();
foreach ( $pages as $page ) {
- new WC_Register_WP_Admin_Settings( $page, 'page' );
+ $this->register_wp_admin_settings_for( $page, 'page' );
}
$emails = WC_Emails::instance();
foreach ( $emails->get_emails() as $email ) {
- new WC_Register_WP_Admin_Settings( $email, 'email' );
+ $this->register_wp_admin_settings_for( $email, 'email' );
+ }
+ }
+
+ /**
+ * Creates (or re-attaches) the settings registrar for one settings page or email.
+ *
+ * The registrar is memoized per settings object so that every registration pass reuses
+ * the same callback identities: add_filter() then overwrites instead of stacking
+ * duplicates, which is what makes register_wp_admin_settings() idempotent.
+ *
+ * @param WC_Settings_Page|WC_Email $settings_source The object holding the settings to register.
+ * Non-objects are ignored, matching the guard
+ * WC_Register_WP_Admin_Settings has held since 3.0.
+ * @param string $type Type of settings to register ('page' or 'email').
+ * @return void
+ */
+ private function register_wp_admin_settings_for( $settings_source, $type ) {
+ // woocommerce_get_settings_pages and woocommerce_email_classes are third-party writable, and
+ // non-object entries have been skipped rather than fatal since 3.0. Check before keying:
+ // spl_object_id() throws a TypeError, which here would take down every admin and REST request.
+ if ( ! is_object( $settings_source ) ) {
+ return;
}
+
+ $key = spl_object_id( $settings_source );
+
+ if ( isset( $this->wp_admin_settings_registrars[ $key ] ) ) {
+ // Re-attach in case hook state was reset since the first registration (e.g. between tests).
+ $this->wp_admin_settings_registrars[ $key ]->register();
+ return;
+ }
+
+ // The constructor attaches the filters.
+ $this->wp_admin_settings_registrars[ $key ] = new WC_Register_WP_Admin_Settings( $settings_source, $type );
}
/**
diff --git a/plugins/woocommerce/phpstan-baseline.neon b/plugins/woocommerce/phpstan-baseline.neon
index 8205602bfeb..d07aefbf59c 100644
--- a/plugins/woocommerce/phpstan-baseline.neon
+++ b/plugins/woocommerce/phpstan-baseline.neon
@@ -14202,18 +14202,6 @@ parameters:
count: 1
path: includes/class-wc-register-wp-admin-settings.php
- -
- message: '#^Access to protected property WC_Email\|WC_Settings_Page\:\:\$id\.$#'
- identifier: property.protected
- count: 1
- path: includes/class-wc-register-wp-admin-settings.php
-
- -
- message: '#^Call to an undefined method WC_Email\|WC_Settings_Page\:\:get_id\(\)\.$#'
- identifier: method.notFound
- count: 1
- path: includes/class-wc-register-wp-admin-settings.php
-
-
message: '#^Call to an undefined method WC_Register_WP_Admin_Settings\:\:get_id\(\)\.$#'
identifier: method.notFound
diff --git a/plugins/woocommerce/tests/legacy/unit-tests/settings/register-wp-admin-settings.php b/plugins/woocommerce/tests/legacy/unit-tests/settings/register-wp-admin-settings.php
index 2e29350bfb6..6af41295f9f 100644
--- a/plugins/woocommerce/tests/legacy/unit-tests/settings/register-wp-admin-settings.php
+++ b/plugins/woocommerce/tests/legacy/unit-tests/settings/register-wp-admin-settings.php
@@ -52,6 +52,24 @@ class WC_Tests_Register_WP_Admin_Settings extends WC_Unit_Test_Case {
$this->assertEquals( has_filter( 'woocommerce_settings-' . $this->page->get_id(), array( $settings, 'register_page_settings' ) ), 10 );
}
+ /**
+ * @since 11.2.0
+ * @covers WC_Register_WP_Admin_Settings::register
+ */
+ public function test_register_is_idempotent() {
+ $settings = new WC_Register_WP_Admin_Settings( $this->page, 'page' );
+
+ $settings->register();
+ $settings->register();
+
+ $this->assertEquals( has_filter( 'woocommerce_settings_groups', array( $settings, 'register_page_group' ) ), 10 );
+
+ // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment -- Reading the registered groups under test.
+ $groups = apply_filters( 'woocommerce_settings_groups', array() );
+ $page_group_ids = array_keys( array_column( $groups, 'id' ), $this->page->get_id(), true );
+ $this->assertCount( 1, $page_group_ids, 'Repeat register() calls should not duplicate the settings group.' );
+ }
+
/**
* @since 3.0.0
* @covers WC_Register_WP_Admin_Settings::register_page_group
diff --git a/plugins/woocommerce/tests/php/includes/class-woocommerce-test.php b/plugins/woocommerce/tests/php/includes/class-woocommerce-test.php
index 96dc694396c..9bd14dd3985 100644
--- a/plugins/woocommerce/tests/php/includes/class-woocommerce-test.php
+++ b/plugins/woocommerce/tests/php/includes/class-woocommerce-test.php
@@ -171,10 +171,109 @@ class WooCommerce_Test extends \WC_Unit_Test_Case {
* @testdox Settings registration is hooked to both admin_init and rest_api_init to support direct PHP and REST consumption.
*/
public function test_register_wp_admin_settings_hooked_to_admin_init_and_rest_api_init(): void {
- $this->assertSame( 10, has_action( 'admin_init', array( WC(), 'register_wp_admin_settings' ) ) );
+ // admin_init runs last so extensions registering settings pages or email classes on
+ // admin_init are still captured; see https://github.com/woocommerce/woocommerce/pull/67494.
+ $this->assertSame( 999, has_action( 'admin_init', array( WC(), 'register_wp_admin_settings' ) ) );
$this->assertSame( 10, has_action( 'rest_api_init', array( WC(), 'register_wp_admin_settings' ) ) );
}
+ /**
+ * @testdox Settings registration is idempotent, so no hook ordering duplicates the settings groups.
+ */
+ public function test_register_wp_admin_settings_is_idempotent(): void {
+ remove_all_filters( 'woocommerce_settings_groups' );
+
+ WC()->register_wp_admin_settings();
+ // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment -- Reading the registered groups under test.
+ $after_first = apply_filters( 'woocommerce_settings_groups', array() );
+
+ WC()->register_wp_admin_settings();
+ WC()->register_wp_admin_settings();
+ // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment -- Reading the registered groups under test.
+ $after_repeats = apply_filters( 'woocommerce_settings_groups', array() );
+
+ $this->assertNotEmpty( $after_first, 'The first call should register the settings groups.' );
+ $this->assertSame(
+ count( $after_first ),
+ count( $after_repeats ),
+ 'Repeat calls should not add duplicate settings groups.'
+ );
+
+ $ids = array_column( $after_repeats, 'id' );
+ $this->assertSame( array_values( array_unique( $ids ) ), array_values( $ids ), 'Settings group ids should be unique.' );
+ }
+
+ /**
+ * @testdox Settings registration is not conditional on the hook it runs from.
+ */
+ public function test_register_wp_admin_settings_does_not_depend_on_hook_context(): void {
+ global $wp_current_filter, $wp_actions;
+
+ // The previous guard keyed off doing_action( 'rest_api_init' ) && did_action( 'admin_init' ),
+ // which made the rest_api_init path unreachable on admin requests. Reproduce that exact state
+ // rather than calling the method bare: admin_init has already run, and registration is now
+ // invoked from inside rest_api_init. Setting the globals doing_action()/did_action() read is
+ // enough, and avoids firing every core callback bound to those two hooks.
+ $current_filter_backup = $wp_current_filter;
+ $admin_init_backup = $wp_actions['admin_init'] ?? null;
+
+ $wp_actions['admin_init'] = ( $admin_init_backup ?? 0 ) + 1; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
+ $wp_current_filter[] = 'rest_api_init'; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
+
+ try {
+ $this->assertTrue( doing_action( 'rest_api_init' ), 'The removed guard condition should be reproduced.' );
+ $this->assertNotEmpty( did_action( 'admin_init' ), 'The removed guard condition should be reproduced.' );
+
+ remove_all_filters( 'woocommerce_settings_groups' );
+
+ WC()->register_wp_admin_settings();
+
+ // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment -- Reading the registered groups under test.
+ $groups = apply_filters( 'woocommerce_settings_groups', array() );
+ $ids = array_column( $groups, 'id' );
+ } finally {
+ $wp_current_filter = $current_filter_backup; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
+
+ if ( null === $admin_init_backup ) {
+ unset( $wp_actions['admin_init'] );
+ } else {
+ $wp_actions['admin_init'] = $admin_init_backup; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
+ }
+ }
+
+ $this->assertContains( 'general', $ids, 'Registration should happen regardless of which hook is running.' );
+ }
+
+ /**
+ * @testdox Settings registration recovers from a hook state reset without duplicating per-group settings.
+ */
+ public function test_register_wp_admin_settings_recovers_from_hook_reset(): void {
+ WC()->register_wp_admin_settings();
+ // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment, WordPress.NamingConventions.ValidHookName.UseUnderscores -- Reading the registered settings under test; the hook name is a core legacy one.
+ $general_before = apply_filters( 'woocommerce_settings-general', array() );
+
+ // Wipe the groups filter only, leaving the per-group filters attached. This mirrors
+ // what the WP test framework does between tests (hook state restored, singletons kept).
+ remove_all_filters( 'woocommerce_settings_groups' );
+ WC()->register_wp_admin_settings();
+
+ // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment -- Reading the registered groups under test.
+ $groups = apply_filters( 'woocommerce_settings_groups', array() );
+ $this->assertContains( 'general', array_column( $groups, 'id' ), 'Registration should re-attach after hook state is reset.' );
+
+ $ids = array_column( $groups, 'id' );
+ $this->assertSame( array_values( array_unique( $ids ) ), array_values( $ids ), 'Settings group ids should be unique after re-registration.' );
+
+ // phpcs:ignore WooCommerce.Commenting.CommentHooks.MissingHookComment, WordPress.NamingConventions.ValidHookName.UseUnderscores -- Reading the registered settings under test; the hook name is a core legacy one.
+ $general_after = apply_filters( 'woocommerce_settings-general', array() );
+ $this->assertNotEmpty( $general_before, 'The general settings group should have settings registered.' );
+ $this->assertSame(
+ count( $general_before ),
+ count( $general_after ),
+ 'Per-group settings should not be duplicated by re-registration.'
+ );
+ }
+
/**
* @testdox Should load WooCommerce includes in post editor load actions.
*/