-
Notifications
You must be signed in to change notification settings - Fork 841
WooCommerce Analytics: Fix proxy speed module installation with proper error/path handling #45801
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.
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
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the WooCommerce Analytics proxy speed module installation by transitioning to the WP_Filesystem API and adding robust error handling throughout the file copy process.
Key Changes:
- Migrated from native PHP
copy()to WP_Filesystem API for better WordPress compatibility - Added validation checks for directory writability and source file existence
- Moved version option update to occur only after successful file copy
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| projects/packages/woocommerce-analytics/src/class-woocommerce-analytics.php | Refactored file copy operations to use WP_Filesystem API with additional error handling and path normalization using trailingslashit() |
| projects/packages/woocommerce-analytics/changelog/fix-woocomerce-analytics-proxy-speed-copy | Added changelog entry documenting the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/packages/woocommerce-analytics/src/class-woocommerce-analytics.php
Outdated
Show resolved
Hide resolved
projects/packages/woocommerce-analytics/src/class-woocommerce-analytics.php
Outdated
Show resolved
Hide resolved
projects/packages/woocommerce-analytics/src/class-woocommerce-analytics.php
Outdated
Show resolved
Hide resolved
projects/packages/woocommerce-analytics/changelog/fix-woocomerce-analytics-proxy-speed-copy
Outdated
Show resolved
Hide resolved
036cd16 to
02fb505
Compare
…tability and source file existence, improve error logging, and ensure proper file copying.
…ce-analytics-proxy-speed-copy Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
02fb505 to
99c84f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/packages/woocommerce-analytics/src/class-woocommerce-analytics.php
Outdated
Show resolved
Hide resolved
projects/packages/woocommerce-analytics/src/class-woocommerce-analytics.php
Outdated
Show resolved
Hide resolved
kangzj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed feedback from Copilot and tried to make it more robust too. And now I think it's ready to go!
PS: we might want to work with Atomic team to add the module?
Thanks for reviewing and merging this! @kangzj Yeah, I think we need to find a way to add the module for Atomic sites. I’ll reach out to the Atomic team to see if they have any suggestions. |
Fixes p1762444618534419-slack-CK365S85V
Proposed changes:
This PR fixes issues with the proxy speed module installation introduced in #45243:
trailingslashit()for proper path handling across different environmentsOther information:
Jetpack product discussion
n/a
Does this pull request change what data or activity we track or use?
n/a
Testing instructions:
Follow the same testing instructions as #45243 (steps 1~6) to verify the proxy speed module is still correctly installed/removed when Jetpack is activated/deactivated.
Additional edge case testing:
Test with read-only mu-plugins directory:
wp-content/mu-pluginsdirectory read-only:chmod 555 wp-content/mu-pluginswp-content/debug.logand verify no errors are logged related to woocommerce-analytics packagechmod 755 wp-content/mu-pluginsVerify file operations work correctly on different server configurations with various filesystem permissions