Improved: Cloaked Affiliate Links and Query Params now have their own settings sections. Enhanced Measurements is now display in 2 columns.#315
Open
Dan0sz wants to merge 29 commits into
Open
Conversation
… settings section.
…hecking if the EM is enabled.
…ttings are empty or emptied.
…Links were empty.
…om enhanced measurements array.
Contributor
📝 WalkthroughWalkthroughThe PR replaces EnhancedMeasurements-flag-based handling for cloaked affiliate links and query params with dedicated settings values, updates the settings UI and asset gates, adds a 2.6.0 upgrade path, and aligns helpers and tests with the new settings shape. ChangesSettings-driven affiliate links and query params
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Admin/Settings/Page.php (2)
478-481: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
OPTION_DISABLED_BY_MISSING_API_TOKEN_HOOKshould be appended togeneral[5], notgeneral[3].
general[3]isquery_paramsafter the new groups, while the API-token-dependent dashboard option lives in View Stats. As written, the notice lands in the wrong section.🤖 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 `@src/Admin/Settings/Page.php` around lines 478 - 481, The API-token missing notice is being attached to the wrong settings group in Page::maybeAddApiTokenMissingHooks, so move OPTION_DISABLED_BY_MISSING_API_TOKEN_HOOK from the general[3] entry to general[5] where the View Stats dashboard option lives. Keep API_TOKEN_MISSING_HOOK on general[0], and update the append target so the disabled-by-missing-token message appears under the correct section.
128-206: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdjust the CE notice splice offset (
src/Admin/Settings/Page.php:453).array_splice( $fields, 7, ... )now inserts beforeSEARCH_QUERIES; the notice is meant forECOMMERCE_REVENUE, which is now at index4.🤖 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 `@src/Admin/Settings/Page.php` around lines 128 - 206, The CE notice is being inserted at the wrong position because the hardcoded array_splice offset in Page::settings fields no longer matches the reordered enhanced measurement entries. Update the splice in the settings-building logic so the notice is placed relative to EnhancedMeasurements::ECOMMERCE_REVENUE (now at index 4) instead of the old index, using the field keys/order in Page to locate the correct insertion point.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/Admin/Upgrades.php`:
- Around line 411-417: The legacy-entry removal logic in the upgrade flow is
using a falsy check on the result of array_search in upgrade_to_200, so entries
at index 0 are skipped and loose matching can remove the wrong item. Update the
checks in the upgrade routine that handles EnhancedMeasurements::QUERY_PARAMS
and EnhancedMeasurements::CLOAKED_AFFILIATE_LINKS to explicitly test for a found
key and use strict comparison in array_search, then unset the matching element
only when a real match is returned.
- Around line 224-229: The migration in Upgrades::update_option flow is
persisting stale data after Helpers::update_setting() already saves
plausible_analytics_settings, which can overwrite the migrated shared_link
state. Update the local $settings array to reflect the self_hosted_shared_link
migration before saving, or remove the extra helper writes and persist once with
update_option so the final stored settings match the migrated values.
---
Outside diff comments:
In `@src/Admin/Settings/Page.php`:
- Around line 478-481: The API-token missing notice is being attached to the
wrong settings group in Page::maybeAddApiTokenMissingHooks, so move
OPTION_DISABLED_BY_MISSING_API_TOKEN_HOOK from the general[3] entry to
general[5] where the View Stats dashboard option lives. Keep
API_TOKEN_MISSING_HOOK on general[0], and update the append target so the
disabled-by-missing-token message appears under the correct section.
- Around line 128-206: The CE notice is being inserted at the wrong position
because the hardcoded array_splice offset in Page::settings fields no longer
matches the reordered enhanced measurement entries. Update the splice in the
settings-building logic so the notice is placed relative to
EnhancedMeasurements::ECOMMERCE_REVENUE (now at index 4) instead of the old
index, using the field keys/order in Page to locate the correct insertion point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb54ad49-9929-4f58-8796-88d2e7d55abe
📒 Files selected for processing (8)
npm-shrinkwrap.jsonsrc/Admin/Provisioning.phpsrc/Admin/Settings/API.phpsrc/Admin/Settings/Page.phpsrc/Admin/Upgrades.phpsrc/Assets.phpsrc/EnhancedMeasurements.phpsrc/Helpers.php
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Admin/Settings/API.php (1)
102-107: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNormalize clonable values before iterating.
Helpers::get_settings()defaults these settings to arrays, but it does not normalize stored/filter-provided scalar values foraffiliate_linksorquery_params. A scalar here will make the settings page emit aforeachwarning.Proposed fix
- $values = $group['value'] ?: [ 0 => '' ]; - $slug = $group['slug'] ?? ''; + $raw_values = $group['value'] ?? []; + if ( ! is_array( $raw_values ) ) { + $raw_values = $raw_values === '' ? [] : [ $raw_values ]; + } + $values = $raw_values ?: [ 0 => '' ]; + $slug = $group['slug'] ?? '';🤖 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 `@src/Admin/Settings/API.php` around lines 102 - 107, The settings render in API.php assumes $group['value'] is iterable, but affiliate_links/query_params can still arrive as scalars from stored or filtered values and trigger a foreach warning. Normalize $group['value'] to an array before assigning $values in the settings section, using the existing rendering flow around $group, $slug, and the foreach loop so single values are wrapped consistently before iteration.src/Admin/Upgrades.php (1)
221-226: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCopy the old shared link into
self_hosted_shared_linkbefore clearing it.
upgrade_to_200()clearsshared_linkfor self-hosted installs, but the value is never written toself_hosted_shared_link, which is what the self-hosted dashboard reads later. Upgrading from<2.0.0drops the saved link instead of migrating it.
[src/Admin/Upgrades.php:221-226]🤖 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 `@src/Admin/Upgrades.php` around lines 221 - 226, The shared link migration in upgrade_to_200() currently clears shared_link for self-hosted installs without preserving it, so move the existing value into self_hosted_shared_link before resetting shared_link. Update the migration logic in Upgrades::upgrade_to_200() so the old shared link is copied to the self-hosted field first, then cleared from shared_link, keeping the dashboard-facing value available after upgrade.
🧹 Nitpick comments (3)
src/Helpers.php (1)
427-438: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing
@paramfor$settings.Both new wrappers omit a
@param array $settingstag, unlike the similarproxy_enabled()pattern elsewhere in this file.📝 Suggested docblock addition
/** * Wrapper to check if the Cloaked Affiliate Links option contains any values. * + * `@param` array $settings Allows passing a current settings object. * `@return` bool */ public static function is_cloaked_affiliate_links_enabled( $settings = [] ) {Also applies to: 461-472
🤖 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 `@src/Helpers.php` around lines 427 - 438, The new wrapper methods are missing the `@param array $settings` docblock entry, unlike the existing `proxy_enabled()`-style helpers. Update the PHPDoc for `is_cloaked_affiliate_links_enabled()` and the other affected wrapper to document the optional `$settings` parameter, keeping the docblocks consistent with the rest of `Helpers` and matching the method signatures.tests/TestCase.php (1)
208-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnused loop variable
$callback_data.Static analysis flags this. Simple fix using
array_keys()instead of destructuring the value.🧹 Suggested fix
- foreach ( $callbacks as $callback_key => $callback_data ) { + foreach ( array_keys( $callbacks ) as $callback_key ) { if ( str_contains( $callback_key, $callback ) ) { unset( $wp_filter[ $hook ]->callbacks[ $priority ][ $callback_key ] ); } }🤖 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/TestCase.php` around lines 208 - 223, The removeAction helper in TestCase has an unused loop value ($callback_data), which triggers static analysis. Update the foreach in removeAction to iterate over only the callback keys (for example by looping over the result of array_keys on the priority callback list) and keep the existing unset logic intact using the callback key and callback match.Source: Linters/SAST tools
src/Admin/Provisioning.php (1)
366-376: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the key→option mapping explicit The current constants match the underscore convention (
affiliate-links→affiliate_links,query-params→query_params), so this works today. An explicit map would make the contract obvious and avoid silent misses if new measurements ever diverge.🤖 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 `@src/Admin/Provisioning.php` around lines 366 - 376, The key-to-option conversion in maybe_add() is currently implicit via str_replace('-', '_', $key), which hides the contract between measurement keys and option names. Replace this with an explicit mapping inside maybe_add() (or a dedicated helper it calls) so each key like affiliate-links and query-params resolves to its intended option name directly, and use that mapping before calling Helpers::setting_has_values().
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/Admin/Settings/API.php`:
- Around line 57-61: The Administrator disabled-check special case is currently
keyed off the generated $id, which depends on the display label and can break
for localized or renamed roles. Update the logic in the Settings API rendering
path to use the canonical role value passed from Page::build_user_roles_array()
(the stable $field['value']) when determining the Administrator case, and apply
the same change anywhere the same check is duplicated, including the related
branch referenced in the later occurrence.
---
Outside diff comments:
In `@src/Admin/Settings/API.php`:
- Around line 102-107: The settings render in API.php assumes $group['value'] is
iterable, but affiliate_links/query_params can still arrive as scalars from
stored or filtered values and trigger a foreach warning. Normalize
$group['value'] to an array before assigning $values in the settings section,
using the existing rendering flow around $group, $slug, and the foreach loop so
single values are wrapped consistently before iteration.
In `@src/Admin/Upgrades.php`:
- Around line 221-226: The shared link migration in upgrade_to_200() currently
clears shared_link for self-hosted installs without preserving it, so move the
existing value into self_hosted_shared_link before resetting shared_link. Update
the migration logic in Upgrades::upgrade_to_200() so the old shared link is
copied to the self-hosted field first, then cleared from shared_link, keeping
the dashboard-facing value available after upgrade.
---
Nitpick comments:
In `@src/Admin/Provisioning.php`:
- Around line 366-376: The key-to-option conversion in maybe_add() is currently
implicit via str_replace('-', '_', $key), which hides the contract between
measurement keys and option names. Replace this with an explicit mapping inside
maybe_add() (or a dedicated helper it calls) so each key like affiliate-links
and query-params resolves to its intended option name directly, and use that
mapping before calling Helpers::setting_has_values().
In `@src/Helpers.php`:
- Around line 427-438: The new wrapper methods are missing the `@param array
$settings` docblock entry, unlike the existing `proxy_enabled()`-style helpers.
Update the PHPDoc for `is_cloaked_affiliate_links_enabled()` and the other
affected wrapper to document the optional `$settings` parameter, keeping the
docblocks consistent with the rest of `Helpers` and matching the method
signatures.
In `@tests/TestCase.php`:
- Around line 208-223: The removeAction helper in TestCase has an unused loop
value ($callback_data), which triggers static analysis. Update the foreach in
removeAction to iterate over only the callback keys (for example by looping over
the result of array_keys on the priority callback list) and keep the existing
unset logic intact using the callback key and callback match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a29a3a2a-c266-4868-a9c5-09c63a5c7fe6
📒 Files selected for processing (8)
src/Admin/Provisioning.phpsrc/Admin/Settings/API.phpsrc/Admin/Settings/Page.phpsrc/Admin/Upgrades.phpsrc/Assets.phpsrc/Helpers.phptests/TestCase.phptests/integration/Admin/ProvisioningTest.php
Comment on lines
+57
to
+61
| $id = $field['slug'] . '_' . str_replace( '-', '_', sanitize_title( $field['label'] ) ); | ||
| $checked = ! empty( $field['checked'] ) ? 'checked="checked"' : | ||
| ( is_array( $slug ) ? checked( $value, in_array( $value, $slug, false ) ? $value : false, false ) : checked( $value, $slug, false ) ); | ||
| $disabled = ! empty( $field['disabled'] ) ? 'disabled' : ''; | ||
| $caps = ! empty( $field['caps'] ) ? $field['caps'] : []; | ||
| $addtl_opts = ! empty( $field['addtl_opts'] ); | ||
| $disabled = ! empty( $field['disabled'] ) ? 'disabled' : ''; | ||
| $check_when_disabled = $id === 'expand_dashboard_access_administrator'; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the canonical role value for the disabled Administrator check.
$id is derived from the display label, so localized/renamed role labels can miss this special case. Page::build_user_roles_array() already passes the stable role ID as value.
Proposed fix
- $check_when_disabled = $id === 'expand_dashboard_access_administrator';
+ $check_when_disabled = $field['slug'] === 'expand_dashboard_access' && $value === 'administrator';Also applies to: 76-76
🤖 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 `@src/Admin/Settings/API.php` around lines 57 - 61, The Administrator
disabled-check special case is currently keyed off the generated $id, which
depends on the display label and can break for localized or renamed roles.
Update the logic in the Settings API rendering path to use the canonical role
value passed from Page::build_user_roles_array() (the stable $field['value'])
when determining the Administrator case, and apply the same change anywhere the
same check is duplicated, including the related branch referenced in the later
occurrence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests