Added: WPML "domain per language" compatibility.#312
Merged
Conversation
… Domain name are changed.
…omains. Some re-factors and simplifications.
…o Client being instantiated as a singleton.
… configuration, otherwise goals aren't created.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Ajax.php (1)
139-141: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the nonce lookup before verification.
A request without
_noncewill emit an undefined-index notice before returning the 403 response. Use the existingclean()path and anempty()check here.Suggested fix
public function dismiss_multilang_notice() { - if ( ! current_user_can( 'manage_options' ) || wp_verify_nonce( $_REQUEST['_nonce'], 'plausible_analytics_dismiss_multilang_notice' ) < 1 ) { + $request_data = $this->clean( $_REQUEST ); + + if ( ! current_user_can( 'manage_options' ) || empty( $request_data['_nonce'] ) || wp_verify_nonce( $request_data['_nonce'], 'plausible_analytics_dismiss_multilang_notice' ) < 1 ) { wp_send_json_error( __( 'Not allowed.', 'plausible-analytics' ), 403 ); }🤖 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/Ajax.php` around lines 139 - 141, In dismiss_multilang_notice(), the nonce is read from $_REQUEST['_nonce'] before confirming it exists, which can trigger an undefined-index notice. Update the guard to use the existing clean() handling with an empty() check before calling wp_verify_nonce(), while keeping the current current_user_can('manage_options') permission check and wp_send_json_error() response intact.
🤖 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.
Outside diff comments:
In `@src/Ajax.php`:
- Around line 139-141: In dismiss_multilang_notice(), the nonce is read from
$_REQUEST['_nonce'] before confirming it exists, which can trigger an
undefined-index notice. Update the guard to use the existing clean() handling
with an empty() check before calling wp_verify_nonce(), while keeping the
current current_user_can('manage_options') permission check and
wp_send_json_error() response intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffd89d29-9a74-4da3-b753-e7d45fde63d3
📒 Files selected for processing (13)
src/Admin/Provisioning.phpsrc/Admin/Provisioning/Integrations.phpsrc/Admin/Settings/API.phpsrc/Admin/Settings/OptionsParser.phpsrc/Admin/Settings/Page.phpsrc/Ajax.phpsrc/Client.phpsrc/Cron.phpsrc/Helpers.phptests/TestableHelpers.phptests/bootstrap.phptests/integration/ClientFactoryTest.phptests/integration/HelpersTest.php
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Cron.php
- src/Admin/Settings/OptionsParser.php
- src/Admin/Provisioning/Integrations.php
- src/Admin/Settings/API.php
- src/Admin/Settings/Page.php
- src/Client.php
- src/Helpers.php
- src/Admin/Provisioning.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.
🧹 Nitpick comments (1)
tests/integration/AjaxTest.php (1)
45-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for keyed/multilang option submission.
All three tests exercise only the unkeyed (
default) option path. None assert that a bracketed option name such asdomain_name[german.dev.local]/api_token[german.dev.local]is correctly parsed byOptionsParser::parse_keyed_options()and routed through$language_domain_keyinAjax::save_options()— this is the core new behavior for this cohort. A regression here (e.g. token saved under the wrong domain key) would silently break the WPML feature without failing any existing test.✅ Example additional test
public function testSaveOptionsWithLanguageDomainKey() { $options = [ [ 'name' => 'domain_name[german.dev.local]', 'value' => 'german.dev.local' ], [ 'name' => 'api_token[german.dev.local]', 'value' => 'fake-token' ], ]; $_POST['_nonce'] = wp_create_nonce( 'plausible_analytics_toggle_option' ); $_POST['options'] = wp_json_encode( $options ); try { $this->ajax->save_options(); } catch ( \Exception $e ) { } $settings = Helpers::get_settings(); $this->assertEquals( 'german.dev.local', $settings['domain_name']['german.dev.local'] ); $this->assertArrayHasKey( 'german.dev.local', $settings['api_token'] ); }🤖 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/integration/AjaxTest.php` around lines 45 - 110, Add a test that covers keyed/multilang option submission through Ajax::save_options and OptionsParser::parse_keyed_options, not just the default option path. Create a new integration test similar to testSaveOptionsSuccess that posts bracketed names like domain_name[german.dev.local] and api_token[german.dev.local], then assert the values are stored under the language domain key in Helpers::get_settings(). Use the existing Ajax test setup and reference save_options, parse_keyed_options, and $language_domain_key behavior to verify the keyed data is routed correctly.
🤖 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.
Nitpick comments:
In `@tests/integration/AjaxTest.php`:
- Around line 45-110: Add a test that covers keyed/multilang option submission
through Ajax::save_options and OptionsParser::parse_keyed_options, not just the
default option path. Create a new integration test similar to
testSaveOptionsSuccess that posts bracketed names like
domain_name[german.dev.local] and api_token[german.dev.local], then assert the
values are stored under the language domain key in Helpers::get_settings(). Use
the existing Ajax test setup and reference save_options, parse_keyed_options,
and $language_domain_key behavior to verify the keyed data is routed correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 295d9903-8eb3-45bf-8450-4dc3d7668b5d
📒 Files selected for processing (2)
src/Admin/Provisioning.phptests/integration/AjaxTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Admin/Provisioning.php
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Admin/Provisioning.php (2)
438-461: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNormalize
enhanced_measurementsbefore flag checks.These methods read
$settings['enhanced_measurements']directly, unlike the surrounding handlers. A partial settings save or malformed value can emit warnings or breakEnhancedMeasurements::is_enabled()before the no-op path is reached.Proposed fix
public function update_tracker_script_config( $_, $settings ) { + $enhanced_measurements = $settings['enhanced_measurements'] ?? []; + + if ( ! is_array( $enhanced_measurements ) ) { + $enhanced_measurements = []; + } + $config = [ @@ - if ( EnhancedMeasurements::is_enabled( EnhancedMeasurements::FILE_DOWNLOADS, $settings['enhanced_measurements'] ) ) { + if ( EnhancedMeasurements::is_enabled( EnhancedMeasurements::FILE_DOWNLOADS, $enhanced_measurements ) ) { @@ - if ( EnhancedMeasurements::is_enabled( EnhancedMeasurements::FORM_COMPLETIONS, $settings['enhanced_measurements'] ) ) { + if ( EnhancedMeasurements::is_enabled( EnhancedMeasurements::FORM_COMPLETIONS, $enhanced_measurements ) ) { @@ - if ( EnhancedMeasurements::is_enabled( EnhancedMeasurements::HASH_BASED_ROUTING, $settings['enhanced_measurements'] ) ) { + if ( EnhancedMeasurements::is_enabled( EnhancedMeasurements::HASH_BASED_ROUTING, $enhanced_measurements ) ) { @@ - if ( EnhancedMeasurements::is_enabled( EnhancedMeasurements::OUTBOUND_LINKS, $settings['enhanced_measurements'] ) ) { + if ( EnhancedMeasurements::is_enabled( EnhancedMeasurements::OUTBOUND_LINKS, $enhanced_measurements ) ) { @@ public function maybe_create_custom_properties( $_, $settings ) { - $enhanced_measurements = $settings['enhanced_measurements']; + $enhanced_measurements = $settings['enhanced_measurements'] ?? []; + + if ( ! is_array( $enhanced_measurements ) ) { + $enhanced_measurements = []; + }Also applies to: 599-605
🤖 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 438 - 461, Normalize or default `$settings['enhanced_measurements']` before the `EnhancedMeasurements::is_enabled()` checks in `update_tracker_script_config`, since reading it directly can trigger warnings or invalid input handling on partial saves. Follow the same safe pattern used by the surrounding settings handlers: extract the value once, ensure it is an array or fallback to an empty/default value, then pass that normalized value into each flag check.
101-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegister update hooks before clients exist.
Line 105 returns during construction when no token/client is currently configured. For an existing install whose settings option already exists but has no token yet, the first Connect save fires
update_option_plausible_analytics_settings, notadd_option_plausible_analytics_settings, so provisioning/shared-link/tracker hooks are never registered for that first connection.Proposed fix
private function init() { /** This hook should always be registered because it handles fresh installs. */ add_action( 'add_option_plausible_analytics_settings', [ $this, 'provision_on_connect' ], 10, 2 ); - - if ( empty( $this->get_clients() ) ) { - return; // `@codeCoverageIgnore` - } add_action( 'update_option_plausible_analytics_settings', [ $this, 'maybe_provision_on_connect' ], 10, 2 );🤖 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 101 - 115, The init() method in Provisioning is returning early when get_clients() is empty, which prevents the update_option_plausible_analytics_settings hooks from being registered on existing installs before a token/client exists. Move the add_action/add_filter registrations for provision_on_connect, maybe_provision_on_connect, maybe_create_shared_link, maybe_create_goals, maybe_delete_goals, maybe_create_custom_properties, maybe_enable_customer_user_roles, and update_tracker_script_config so they are always registered, and keep only any client-dependent logic guarded separately if needed.src/Client.php (1)
111-113: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist capabilities using the client’s domain key
src/Client.php
get_clients()is the only place that forcesplausible_analytics_current_language_domain_key, and that filter is removed before the per-client calls run.update_capabilities()still reads the ambient key, so an exception fromcreate_goals()/create_shared_link()can write the capabilities snapshot under the wrong domain. Pass the domain key intoClientor intoupdate_capabilities()instead.🤖 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/Client.php` around lines 111 - 113, The capabilities snapshot is being saved using whatever domain key is currently in the ambient filter state, so `update_capabilities()` can persist under the wrong client when `create_goals()` or `create_shared_link()` fails. Update `Client` so the client’s domain key is stored on the instance or passed directly into `update_capabilities()`, and use that explicit key when writing the capabilities cache instead of reading the ambient `plausible_analytics_current_language_domain_key`.
🤖 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.
Outside diff comments:
In `@src/Admin/Provisioning.php`:
- Around line 438-461: Normalize or default `$settings['enhanced_measurements']`
before the `EnhancedMeasurements::is_enabled()` checks in
`update_tracker_script_config`, since reading it directly can trigger warnings
or invalid input handling on partial saves. Follow the same safe pattern used by
the surrounding settings handlers: extract the value once, ensure it is an array
or fallback to an empty/default value, then pass that normalized value into each
flag check.
- Around line 101-115: The init() method in Provisioning is returning early when
get_clients() is empty, which prevents the
update_option_plausible_analytics_settings hooks from being registered on
existing installs before a token/client exists. Move the add_action/add_filter
registrations for provision_on_connect, maybe_provision_on_connect,
maybe_create_shared_link, maybe_create_goals, maybe_delete_goals,
maybe_create_custom_properties, maybe_enable_customer_user_roles, and
update_tracker_script_config so they are always registered, and keep only any
client-dependent logic guarded separately if needed.
In `@src/Client.php`:
- Around line 111-113: The capabilities snapshot is being saved using whatever
domain key is currently in the ambient filter state, so `update_capabilities()`
can persist under the wrong client when `create_goals()` or
`create_shared_link()` fails. Update `Client` so the client’s domain key is
stored on the instance or passed directly into `update_capabilities()`, and use
that explicit key when writing the capabilities cache instead of reading the
ambient `plausible_analytics_current_language_domain_key`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 028f6d3e-09a7-407d-bf6a-e54fa5335f5f
📒 Files selected for processing (8)
src/Admin/Provisioning.phpsrc/Admin/Provisioning/Integrations.phpsrc/Admin/Settings/Page.phpsrc/Ajax.phpsrc/Client.phpsrc/Helpers.phptests/integration/Admin/ProvisioningTest.phptests/integration/AjaxTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Admin/Provisioning/Integrations.php
- src/Helpers.php
- src/Admin/Settings/Page.php
- tests/integration/Admin/ProvisioningTest.php
- src/Ajax.php
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.
This PR adds native compatibility with WPML's "a different per language" compatibility (which currently isn't available for Multisite) by adding a Language Domains pulldown menu, which allows the user to map each configured Language Domain in WPML to a Plausible Analytics dashboard:
Like in previous versions, the default value of the Domain Name for each Language Domain is the same value of the selected Language Domain:
The Create Token link and Connect button behave the same as they did before UX-wise. This PR also fixes an issue where the "Connect" button was sometimes mislabeled as "Connected".
The configuration wizard remains unchanged to not overcomplicate it. If a new Plausible user configured WPML to use its language per domain feature, the user will be notified of the new feature:
The same goes for users right after updating:
For every configured Language Domain it adds a link to the Admin Bar menu when "View stats in WordPress" is enabled:
It also adds a filter for the Admin Bar menu that allows devs to filter out items/links to stats they don't want/need:
plausible_analytics_admin_bar_view_analytics.Summary by CodeRabbit