🔨 chore: Update Appsero and refactor with Mozart #55
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
dependencies/Appsero/Client.php (1)
223-226: Standardize HTTP header keysIn the headers array, 'user-agent' should be capitalized as 'User-Agent' to comply with HTTP standards.
Apply this diff to correct the header key:
$headers = [ - 'user-agent' => 'Appsero/' . md5( esc_url( home_url() ) ) . ';', + 'User-Agent' => 'Appsero/' . md5( esc_url( home_url() ) ) . ';', 'Accept' => 'application/json', ];dependencies/Appsero/License.php (2)
Line range hint
854-855: Correct typo in error messageThere's a typo in the error message: "Nonce vefification failed." It should be "Nonce verification failed."
Apply this diff:
- wp_send_json_error( 'Nonce vefification failed' ); + wp_send_json_error( 'Nonce verification failed' );
312-345: Improve form data handling and validationIn the
license_form_submit()method, consider improving the handling and validation of form data to prevent potential issues.
- Ensure all
$_POSTvariables are checked withisset()before using them.- Use consistent sanitization functions appropriate for the data type.
- Add error handling for unexpected or missing form data.
dependencies/Appsero/Insights.php (2)
854-855: Correct typo in error messageThere's a typo in the error message: "Nonce vefification failed." It should be "Nonce verification failed."
Apply this diff:
- wp_send_json_error( 'Nonce vefification failed' ); + wp_send_json_error( 'Nonce verification failed' );
365-379: Simplify and secureis_local_server()methodThe
is_local_server()method could be simplified and made more secure:
- Use
filter_var()withFILTER_VALIDATE_IPto check IP addresses.- Use
in_array()with strict comparison.- Sanitize server variables properly.
Consider refactoring the method for clarity and security.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
composer.json(1 hunks)conversion-tracking.php(1 hunks)dependencies/Appsero/Client.php(1 hunks)dependencies/Appsero/Insights.php(1 hunks)dependencies/Appsero/License.php(1 hunks)
🔇 Additional comments (2)
composer.json (1)
60-60: Verify exclusion of necessary packages
In the excluded_packages section, you have excluded psr/container. Ensure that excluding this package does not affect the functionality of other dependencies.
Run the following script to check for any dependencies requiring psr/container:
If any essential packages depend on psr/container, reconsider excluding it.
✅ Verification successful
Excluding psr/container package is safe in this context
Looking at the composer.json, the exclusion of psr/container is in the Mozart configuration section, which is used for dependency isolation. The package is not directly required by the project (not listed in require or require-dev), and the only packages being processed by Mozart are appsero/updater and appsero/client. Since these are simple client/updater libraries, excluding PSR Container interface won't affect the functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if any required packages depend on psr/container
composer why psr/container
Length of output: 127
Script:
#!/bin/bash
# Let's check the content of composer.json to understand the dependencies and exclusions
cat composer.json
Length of output: 2055
Script:
#!/bin/bash
# Search for any direct usage of PSR Container interfaces or classes in the codebase
rg -l "use Psr\\Container" || rg -l "use Psr/Container"
Length of output: 146
Script:
#!/bin/bash
# Look for Container interface implementations
ast-grep --pattern 'implements ContainerInterface'
Length of output: 52
conversion-tracking.php (1)
Line range hint 249-253: Verify autoloading of namespaced Client class
The init_tracker() method now uses the WeDevs_WC_Conversion_Tracking\Dependencies\Appsero\Client class. Ensure that the new namespace is correctly configured in the autoloader.
Run the following script to confirm that the class is autoloaded properly:
If the class is not found, ensure that the autoload section in composer.json is correctly configured and that you've run composer dump-autoload.
✅ Verification successful
Autoloading configuration is correct and class is properly namespaced
The verification shows that:
- The
Clientclass exists atdependencies/Appsero/Client.php - It's correctly namespaced as
WeDevs_WC_Conversion_Tracking\Dependencies\Appsero - The composer.json has proper PSR-4 autoloading configuration for the namespace
- Mozart is correctly configured to prefix the Appsero client package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify autoloading of the Client class
# Test: Check if the class file exists in the autoload paths
grep -r "class Client" vendor/ | grep "WeDevs_WC_Conversion_Tracking/Dependencies/Appsero/Client.php"
Length of output: 142
Script:
#!/bin/bash
# Let's first check if the class file exists in the repository
fd -t f "Client.php"
# Check composer.json for autoloading configuration
fd -t f "composer.json" --exec cat {}
# Check if there are any PHP files that define this namespace
rg -l "namespace.*WeDevs_WC_Conversion_Tracking\\Dependencies\\Appsero"
Length of output: 2200
Script:
#!/bin/bash
# Let's check the content of the Client.php file to verify namespace
cat dependencies/Appsero/Client.php | head -n 10
# Check if Mozart has properly prefixed the namespace
rg -l "use.*WeDevs_WC_Conversion_Tracking\\Dependencies\\Appsero\\Client"
# Check if the class is being loaded correctly in the main plugin file
rg -A 5 "new WeDevs_WC_Conversion_Tracking\\Dependencies\\Appsero\\Client"
Length of output: 767
| $response = wp_remote_post( | ||
| $url, | ||
| [ | ||
| 'method' => 'POST', | ||
| 'timeout' => 30, | ||
| 'redirection' => 5, | ||
| 'httpversion' => '1.0', | ||
| 'blocking' => $blocking, | ||
| 'headers' => $headers, | ||
| 'body' => array_merge( $params, [ 'client' => $this->version ] ), | ||
| 'cookies' => [], | ||
| ] | ||
| ); | ||
|
|
||
| return $response; |
There was a problem hiding this comment.
Add error handling for HTTP requests
In the send_request() method, the response from wp_remote_post() may be a WP_Error on failure. Currently, there is no check for this, which could lead to unexpected behavior.
Add error handling to check if $response is a WP_Error and handle it appropriately.
Apply this diff:
$response = wp_remote_post(
$url,
[
'method' => 'POST',
'timeout' => 30,
'redirection' => 5,
'httpversion' => '1.0',
'blocking' => $blocking,
'headers' => $headers,
'body' => array_merge( $params, [ 'client' => $this->version ] ),
'cookies' => [],
]
);
+ if ( is_wp_error( $response ) ) {
+ return $response;
+ }
return $response;📝 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.
| $response = wp_remote_post( | |
| $url, | |
| [ | |
| 'method' => 'POST', | |
| 'timeout' => 30, | |
| 'redirection' => 5, | |
| 'httpversion' => '1.0', | |
| 'blocking' => $blocking, | |
| 'headers' => $headers, | |
| 'body' => array_merge( $params, [ 'client' => $this->version ] ), | |
| 'cookies' => [], | |
| ] | |
| ); | |
| return $response; | |
| $response = wp_remote_post( | |
| $url, | |
| [ | |
| 'method' => 'POST', | |
| 'timeout' => 30, | |
| 'redirection' => 5, | |
| 'httpversion' => '1.0', | |
| 'blocking' => $blocking, | |
| 'headers' => $headers, | |
| 'body' => array_merge( $params, [ 'client' => $this->version ] ), | |
| 'cookies' => [], | |
| ] | |
| ); | |
| if ( is_wp_error( $response ) ) { | |
| return $response; | |
| } | |
| return $response; |
| list( $this->slug, $mainfile ) = explode( '/', $this->basename ); | ||
|
|
There was a problem hiding this comment.
Handle potential Undefined offset when parsing basename
In the set_basename_and_slug() method, you are using explode( '/', $this->basename ) without verifying if $this->basename contains the '/' character. If $this->basename does not contain '/', this could lead to an Undefined offset notice.
Consider adding a check to ensure $this->basename contains '/' before exploding it.
Apply this diff to handle the case:
if ( strpos( $this->basename, '/' ) !== false ) {
list( $this->slug, $mainfile ) = explode( '/', $this->basename );
} else {
+ $this->slug = $this->basename;
+ $mainfile = '';
}Committable suggestion skipped: line range outside the PR's diff.
| if ( ! isset( $form_data['_nonce'] ) ) { | ||
| return; | ||
| } | ||
|
|
||
| if ( ! wp_verify_nonce( sanitize_key( wp_unslash( $form_data['_nonce'] ) ), $this->client->name ) ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use appropriate sanitization for nonce verification
In the license_form_submit() method, you are sanitizing the nonce using sanitize_key(). Since nonces are strings that may contain characters not allowed in keys, it's better to use sanitize_text_field().
Apply this diff:
if ( ! wp_verify_nonce( sanitize_key( wp_unslash( $_POST['_nonce'] ) ), $this->client->name ) ) {
$this->error = $this->client->__trans( 'Nonce vefification failed.' );
+ if ( ! wp_verify_nonce( sanitize_text_field( wp_unslash( $_POST['_nonce'] ) ), $this->client->name ) ) {
+ $this->error = $this->client->__trans( 'Nonce verification failed.' );Committable suggestion skipped: line range outside the PR's diff.
| $response = wp_remote_get( 'https://icanhazip.com/' ); | ||
|
|
||
| if ( is_wp_error( $response ) ) { | ||
| return ''; | ||
| } | ||
|
|
||
| $ip = trim( wp_remote_retrieve_body( $response ) ); | ||
|
|
||
| if ( ! filter_var( $ip, FILTER_VALIDATE_IP ) ) { | ||
| return ''; | ||
| } | ||
|
|
||
| return $ip; | ||
| } |
There was a problem hiding this comment.
Handle potential failures when retrieving IP address
The get_user_ip_address() method relies on an external service (https://icanhazip.com/) to retrieve the user's IP address. If the service is unavailable or returns an invalid response, it could cause issues.
Consider adding a fallback mechanism or handling for when the remote request fails.
Apply this diff:
$ip = trim( wp_remote_retrieve_body( $response ) );
+ if ( empty( $ip ) || ! filter_var( $ip, FILTER_VALIDATE_IP ) ) {
+ $ip = $_SERVER['REMOTE_ADDR'] ?? '';
+ }📝 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.
| $response = wp_remote_get( 'https://icanhazip.com/' ); | |
| if ( is_wp_error( $response ) ) { | |
| return ''; | |
| } | |
| $ip = trim( wp_remote_retrieve_body( $response ) ); | |
| if ( ! filter_var( $ip, FILTER_VALIDATE_IP ) ) { | |
| return ''; | |
| } | |
| return $ip; | |
| } | |
| $response = wp_remote_get( 'https://icanhazip.com/' ); | |
| if ( is_wp_error( $response ) ) { | |
| return ''; | |
| } | |
| $ip = trim( wp_remote_retrieve_body( $response ) ); | |
| if ( empty( $ip ) || ! filter_var( $ip, FILTER_VALIDATE_IP ) ) { | |
| $ip = $_SERVER['REMOTE_ADDR'] ?? ''; | |
| } | |
| return $ip; | |
| } |
| "[ $COMPOSER_DEV_MODE -eq 0 ] || \"vendor/bin/mozart\" compose", | ||
| "composer dump-autoload" | ||
| ], | ||
| "post-update-cmd": [ | ||
| "[ $COMPOSER_DEV_MODE -eq 0 ] || \"vendor/bin/mozart\" compose", | ||
| "composer dump-autoload" |
There was a problem hiding this comment.
Correct script syntax for Composer hooks
In the post-install-cmd and post-update-cmd scripts, the conditional statements may not execute as intended within Composer's script context.
Consider revising the scripts to ensure they function correctly. Composer scripts don't interpret shell conditionals natively.
Apply this diff:
"post-install-cmd": [
- "[ $COMPOSER_DEV_MODE -eq 0 ] || \"vendor/bin/mozart\" compose",
+ "@php vendor/bin/mozart compose",
"composer dump-autoload"
],
"post-update-cmd": [
- "[ $COMPOSER_DEV_MODE -eq 0 ] || \"vendor/bin/mozart\" compose",
+ "@php vendor/bin/mozart compose",
"composer dump-autoload"
]Alternatively, if you want the script to run only in dev mode, use Composer's scripts-dev section or check the COMPOSER_DEV_MODE environment variable within a PHP script.
📝 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.
| "[ $COMPOSER_DEV_MODE -eq 0 ] || \"vendor/bin/mozart\" compose", | |
| "composer dump-autoload" | |
| ], | |
| "post-update-cmd": [ | |
| "[ $COMPOSER_DEV_MODE -eq 0 ] || \"vendor/bin/mozart\" compose", | |
| "composer dump-autoload" | |
| "@php vendor/bin/mozart compose", | |
| "composer dump-autoload" | |
| ], | |
| "post-update-cmd": [ | |
| "@php vendor/bin/mozart compose", | |
| "composer dump-autoload" |
Updated Appsero and integrated Mozart.
Summary by CodeRabbit
Release Notes
New Features
Clientclass for managing plugin data and updates.Insightsclass for tracking plugin usage with user consent.Licenseclass for managing license activation and validation.Improvements
init_trackermethod for improved encapsulation of dependencies.Bug Fixes