Skip to content

fix(tours): honour stripped legacy tour setting keys #1271

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.

Sign up for GitHub

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

Merged
superdav42 merged 1 commit into from
May 26, 2026
Merged

fix(tours): honour stripped legacy tour setting keys #1271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 69 additions & 1 deletion inc/ui/class-tours.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,68 @@ protected function get_meta_key($id) {
return 'wu_tour_finished_' . str_replace('-', '_', $id);
}

/**
* Returns legacy user-settings keys that may hold a finished tour flag.
*
* Older tour persistence used WordPress user settings directly. Depending
* on the WordPress version and whether the value travelled through the
* wp-settings-* cookie sanitizer, hyphenated tour IDs may have been stored
* either with underscores or with hyphens stripped entirely. Check both
* shapes so users who already dismissed those tours do not see them again.
*
* @since 2.5.2
*
* @param string $id The tour ID.
* @return array
*/
protected function get_legacy_setting_keys($id) {

return array_values(
array_unique(
[
$this->get_setting_key($id),
'wu_tour_' . preg_replace('/[^A-Za-z0-9_]/', '', $id),
]
)
);
}

/**
* Whether a legacy user-settings value marks the tour as finished.
*
* @since 2.5.2
*
* @param string $id The tour ID.
* @param int $user_id User ID.
* @return bool
*/
protected function is_legacy_tour_finished($id, $user_id) {

foreach ($this->get_legacy_setting_keys($id) as $setting_key) {
if (get_user_setting($setting_key, false)) {
return true;
}
}

$stored_settings = get_user_option('user-settings', $user_id);
Comment on lines +130 to +136

@coderabbitai coderabbitai Bot May 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

In WordPress core, does get_user_setting( $name, $default ) read only the current user's wp-settings cookie/cache, or can it resolve settings for an arbitrary user ID? Please include the function signature and a short summary of its lookup behavior.

💡 Result:

Function signature: get_user_setting( $name, $default_value = false) [1] Short summary of lookup behavior (WordPress core): - get_user_setting calls get_all_user_settings, then returns the setting value from the returned array for key $name, or returns $default_value if the key is not present. [1][2] - get_all_user_settings only resolves settings for the current request’s “current user” (it uses get_current_user_id). If there is no current user ID, it returns an empty array. [2] - When a cookie is available, get_all_user_settings reads the user-settings cookie named wp-settings-{user_id} from $_COOKIE (and if not present, it falls back to the user option user-settings in the database for that same $user_id). [2] Answer to your question: In WordPress core, get_user_setting($name, $default_value) does not let you resolve settings for an arbitrary user ID. It looks up settings only for the current user ID for the current request, and it prefers the current browser’s wp-settings-{user_id} cookie (with a database fallback if the cookie is absent) rather than an arbitrary user ID argument. [2][1]

Citations:

Scope get_user_setting() to the requested user to prevent cross-user legacy backfill

get_user_setting( $name, $default_value = false ) is backed by get_all_user_settings(), which resolves settings for the current request’s current user (cookie wp-settings-{current_user_id} with DB fallback). Since is_legacy_tour_finished($id, $user_id) calls get_user_setting() without using $user_id, it can read the current browser user’s legacy cookie while then backfilling meta for the passed $user_id.

Proposed fix 
 	protected function is_legacy_tour_finished($id, $user_id) {
 
-		foreach ($this->get_legacy_setting_keys($id) as $setting_key) {
-			if (get_user_setting($setting_key, false)) {
-				return true;
-			}
-		}
+		if (get_current_user_id() === (int) $user_id) {
+			foreach ($this->get_legacy_setting_keys($id) as $setting_key) {
+				if (get_user_setting($setting_key, false)) {
+					return true;
+				}
+			}
+		}
 
 		$stored_settings = get_user_option('user-settings', $user_id);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
foreach ($this->get_legacy_setting_keys($id) as $setting_key) {
if (get_user_setting($setting_key, false)) {
return true;
}
}
$stored_settings = get_user_option('user-settings', $user_id);
if (get_current_user_id() === (int) $user_id) {
foreach ($this->get_legacy_setting_keys($id) as $setting_key) {
if (get_user_setting($setting_key, false)) {
return true;
}
}
}
$stored_settings = get_user_option('user-settings', $user_id);
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@inc/ui/class-tours.php` around lines 130 - 136, The loop in
is_legacy_tour_finished($id, $user_id) calls get_user_setting($setting_key)
which reads the current browser user’s settings; change the check to read
settings for the passed $user_id instead (e.g. use get_user_option($setting_key,
$user_id) or get_user_meta($user_id, $setting_key, true)) so legacy backfill
uses the intended $user_id; update the loop that iterates
get_legacy_setting_keys($id) and also ensure subsequent retrieval of
$stored_settings continues to use get_user_option('user-settings', $user_id).


if ( ! is_string($stored_settings) || '' === $stored_settings) {
return false;
}

$parsed_settings = [];

parse_str($stored_settings, $parsed_settings);

foreach ($this->get_legacy_setting_keys($id) as $setting_key) {
if ( ! empty($parsed_settings[ $setting_key ])) {
return true;
}
}

return false;
}

/**
* Whether the tour has been finished for the current user.
*
Expand Down Expand Up @@ -117,7 +179,13 @@ protected function is_tour_finished($id, $user_id = 0) {
return true;
}

return (bool) get_user_setting($this->get_setting_key($id), false);
$legacy_finished = $this->is_legacy_tour_finished($id, $user_id);

if ($legacy_finished) {
update_user_meta($user_id, $this->get_meta_key($id), 1);
}

return $legacy_finished;
}

/**
Expand Down
35 changes: 35 additions & 0 deletions tests/WP_Ultimo/UI/Tours_Test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,41 @@
}
}

/**
* Test is_tour_finished reads legacy stripped keys from user settings meta.
*
* Regression test for users who dismissed hyphenated tours before the tour ID
* normalisation fix. WordPress could persist keys with hyphens stripped (for
* example, wu_tour_checkoutformlist), while newer code looks for the
* underscore-normalised key (wu_tour_checkout_form_list). The meta fallback
* must recognise the stripped legacy shape and backfill the new user meta flag.
*/
public function test_is_tour_finished_reads_stripped_legacy_user_settings_meta(): void {

$instance = $this->get_instance();

$user_id = self::factory()->user->create(['role' => 'administrator']);
wp_set_current_user($user_id);

$reflection = new \ReflectionClass($instance);
$is_finished = $reflection->getMethod('is_tour_finished');
$is_finished->setAccessible(true);
$get_meta_key = $reflection->getMethod('get_meta_key');
$get_meta_key->setAccessible(true);
$get_legacy_keys = $reflection->getMethod('get_legacy_setting_keys');
$get_legacy_keys->setAccessible(true);

$meta_key = $get_meta_key->invoke($instance, 'checkout-form-list');

update_user_option($user_id, 'user-settings', 'wu_tour_checkoutformlist=1', false);

$this->assertSame('wu_tour_checkoutformlist=1', get_user_option('user-settings', $user_id));
$this->assertContains('wu_tour_checkoutformlist', $get_legacy_keys->invoke($instance, 'checkout-form-list'));

$this->assertTrue($is_finished->invoke($instance, 'checkout-form-list', $user_id));
$this->assertSame('1', get_user_meta($user_id, $meta_key, true));
Comment on lines +350 to +356

@coderabbitai coderabbitai Bot May 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n -C3 "wp-settings-|_updated_user_settings|test_is_tour_finished_falls_back_to_legacy_user_setting|test_is_tour_finished_reads_stripped_legacy_user_settings_meta" tests/WP_Ultimo/UI/Tours_Test.php

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 3005

🏁 Script executed:

#!/bin/bash
set -euo pipefail
nl -ba tests/WP_Ultimo/UI/Tours_Test.php | sed -n '320,390p'

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 124

🏁 Script executed:

#!/bin/bash
set -euo pipefail
cat -n tests/WP_Ultimo/UI/Tours_Test.php | sed -n '320,390p'

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 3274

🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n "function is_tour_finished" -S . --glob='*.php'

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 168

🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '130,260p' inc/ui/class-tours.php

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 3570

🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n "\$_COOKIE\[['\"]wp-settings-'|_updated_user_settings|wp-settings-" tests/WP_Ultimo/UI/Tours_Test.php

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 637

🏁 Script executed:

#!/bin/bash
set -euo pipefail
cat -n tests/WP_Ultimo/UI/Tours_Test.php | sed -n '260,340p'

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 3999

🏁 Script executed:

#!/bin/bash
rg -n -C3 "wp-settings-|_updated_user_settings|test_is_tour_finished_falls_back_to_legacy_user_setting|test_is_tour_finished_reads_stripped_legacy_user_settings_meta" tests/WP_Ultimo/UI/Tours_Test.php

Repository: Ultimate-Multisite/ultimate-multisite

Length of output: 3005

Clear wp-settings-* cookie/cache in stripped legacy meta test

test_is_tour_finished_reads_stripped_legacy_user_settings_meta() doesn’t isolate cookie-backed get_user_setting() state. Since is_legacy_tour_finished() checks get_user_setting() before parsing get_user_option('user-settings', ...), stale $_COOKIE['wp-settings-{user_id}'] / $GLOBALS['_updated_user_settings'] can make the test pass without exercising the stripped-key parsing path.

Proposed fix 
+		$cookie_name                      = 'wp-settings-' . $user_id;
+		$prior_cookie                     = $_COOKIE[ $cookie_name ] ?? null; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput -- test stash, no user input.
+		$prior_updated_settings           = $GLOBALS['_updated_user_settings'] ?? null;
+		unset($_COOKIE[ $cookie_name ]);
+		$GLOBALS['_updated_user_settings'] = null;
+
-		update_user_option($user_id, 'user-settings', 'wu_tour_checkoutformlist=1', false);
-
-		$this->assertSame('wu_tour_checkoutformlist=1', get_user_option('user-settings', $user_id));
-		$this->assertContains('wu_tour_checkoutformlist', $get_legacy_keys->invoke($instance, 'checkout-form-list'));
-
-		$this->assertTrue($is_finished->invoke($instance, 'checkout-form-list', $user_id));
-		$this->assertSame('1', get_user_meta($user_id, $meta_key, true));
+		try {
+			update_user_option($user_id, 'user-settings', 'wu_tour_checkoutformlist=1', false);
+
+			$this->assertSame('wu_tour_checkoutformlist=1', get_user_option('user-settings', $user_id));
+			$this->assertContains('wu_tour_checkoutformlist', $get_legacy_keys->invoke($instance, 'checkout-form-list'));
+
+			$this->assertTrue($is_finished->invoke($instance, 'checkout-form-list', $user_id));
+			$this->assertSame('1', get_user_meta($user_id, $meta_key, true));
+		} finally {
+			if (null === $prior_cookie) {
+				unset($_COOKIE[ $cookie_name ]);
+			} else {
+				$_COOKIE[ $cookie_name ] = $prior_cookie;
+			}
+			$GLOBALS['_updated_user_settings'] = $prior_updated_settings;
+		}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/WP_Ultimo/UI/Tours_Test.php` around lines 350 - 356, The test must
clear cookie/global user-setting state so is_legacy_tour_finished() actually
parses stripped keys: before calling $get_legacy_keys->invoke(...) and
$is_finished->invoke($instance, 'checkout-form-list', $user_id) clear any
wp-settings cookie and updated-user cache (unset
$_COOKIE['wp-settings-'.$user_id] and unset
$GLOBALS['_updated_user_settings'][$user_id]) so get_user_setting() won't return
stale values; update the
test_is_tour_finished_reads_stripped_legacy_user_settings_meta() sequence to
unset those entries immediately after update_user_option(...) and before
invoking the reflection helpers ($get_legacy_keys and $is_finished).

}

/**
* Test enqueue_scripts uses wp_add_inline_script on 'underscore', not wu-admin.
*
Expand All @@ -336,7 +371,7 @@

// Register 'underscore' if not already registered (test environment may not have it).
if ( ! wp_script_is('underscore', 'registered')) {
wp_register_script('underscore', false, [], false, false);

Check warning on line 374 in tests/WP_Ultimo/UI/Tours_Test.php

View workflow job for this annotation

GitHub Actions / Code Quality Checks 

Version parameter is not explicitly set or has been set to an equivalent of "false" for wp_register_script; This means that the WordPress core version will be used which is not recommended for plugin or theme development.
}

// Inject a tour so enqueue_scripts() proceeds.
Expand Down
Loading