New Yoast Events for option changes PRESS0-2019 #191
Conversation
crodriguezbrito
requested review from
a team and
Copilot
and removed request for
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR adds new event tracking capabilities for Yoast SEO plugin activities, including database upgrades and option updates. It also improves code documentation with proper docblocks and applies Yoast coding standards for comparison operators.
Key changes:
- Adds tracking for Yoast SEO database upgrades via the
wpseo_run_upgradehook - Adds tracking for Yoast SEO option updates across four option groups:
wpseo_titles,wpseo_social,wpseo, andwpseo_ms - Improves code style by using Yoast comparisons (constant first in comparisons) and adding comprehensive docblocks for class properties
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'category' => 'yoast_event', | ||
| 'data' => array( | ||
| 'previous_version' => $previous_version, | ||
| 'new_version' => WPSEO_VERSION, |
There was a problem hiding this comment.
The constant WPSEO_VERSION is used without checking if it's defined. If Yoast SEO is not installed or the constant is not available, this will cause a fatal error. Add a check like defined( 'WPSEO_VERSION' ) ? WPSEO_VERSION : 'unknown' to prevent this issue.
| 'new_version' => WPSEO_VERSION, | |
| 'new_version' => defined( 'WPSEO_VERSION' ) ? WPSEO_VERSION : 'unknown', |
includes/Listeners/Yoast.php
Outdated
| if ( $old_value !== $new_value ) { | ||
|
|
||
| $data = array( | ||
| 'category' => 'yoast_event', | ||
| 'data' => array( | ||
| 'old_value' => $old_value, | ||
| 'new_value' => $new_value, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The entire option values are being sent without any sanitization or filtering. Yoast options can contain sensitive information such as API keys, personal data, or configuration details that should not be transmitted. Consider either: (1) filtering out sensitive keys/values, (2) sending only a hash or indication that the option changed, or (3) implementing a whitelist of safe-to-track option keys within these option groups.
| if ( $old_value !== $new_value ) { | |
| $data = array( | |
| 'category' => 'yoast_event', | |
| 'data' => array( | |
| 'old_value' => $old_value, | |
| 'new_value' => $new_value, | |
| ), | |
| ); | |
| // Define a whitelist of safe option keys whose values can be sent in full. | |
| $safe_option_keys = array( | |
| 'yoast_tracking', // Example safe option, add more as needed. | |
| 'yoast_wpseo_titles', | |
| 'yoast_wpseo_social', | |
| // Add other safe option keys here. | |
| ); | |
| if ( $old_value !== $new_value ) { | |
| if ( in_array( $option, $safe_option_keys, true ) ) { | |
| $data = array( | |
| 'category' => 'yoast_event', | |
| 'data' => array( | |
| 'old_value' => $old_value, | |
| 'new_value' => $new_value, | |
| ), | |
| ); | |
| } else { | |
| $data = array( | |
| 'category' => 'yoast_event', | |
| 'data' => array( | |
| 'old_value_hash' => is_scalar( $old_value ) ? hash( 'sha256', (string) $old_value ) : hash( 'sha256', json_encode( $old_value ) ), | |
| 'new_value_hash' => is_scalar( $new_value ) ? hash( 'sha256', (string) $new_value ) : hash( 'sha256', json_encode( $new_value ) ), | |
| 'changed' => true, | |
| ), | |
| ); | |
| } |
| * | ||
| * @return void | ||
| */ | ||
| public function database_upgrade( $previous_version ) { |
There was a problem hiding this comment.
The new database_upgrade method lacks test coverage. Other listeners in this codebase (Plugin, Commerce, Cron) have corresponding test files. Consider adding tests to verify this method correctly handles null/empty previous versions and properly formats the event data.
| * | ||
| * @return void | ||
| */ | ||
| public function option_updated( $old_value, $new_value, $option ) { |
There was a problem hiding this comment.
The new option_updated method lacks test coverage. Other listeners in this codebase (Plugin, Commerce, Cron) have corresponding test files. Consider adding tests to verify this method correctly handles different value types, properly checks for changes, and formats the event data correctly.
| add_action( 'update_option_wpseo_titles', array( $this, 'option_updated' ), 10, 3 ); | ||
| add_action( 'update_option_wpseo_social', array( $this, 'option_updated' ), 10, 3 ); | ||
| add_action( 'update_option_wpseo', array( $this, 'option_updated' ), 10, 3 ); |
There was a problem hiding this comment.
@crodriguezbrito - It appears we have a generic hook and two more specific ones. To the point that Copilot made, the generic one could pick up changes to API keys and things we may not want to send. It would be worth it to look at anything we don't want to send and have a deny list.
Proposed changes
Jira ticket
Type of Change
Production
Development
Visual
Checklist
Further comments