-
Notifications
You must be signed in to change notification settings - Fork 841
Fix backslash character stripping in Copy Post module #46151
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
base: trunk
Are you sure you want to change the base?
Conversation
- Add wp_slash() wrapper in copy-post.php update_content() to preserve backslash characters when duplicating posts - Add wp_slash() wrapper in markdown module's wp_restore_post_revision() and fix_latest_revision_on_restore() to preserve backslash characters when restoring revisions - Add unit tests for copy-post backslash preservation - Add changelog entry Co-authored-by: sirreal <[email protected]>
Co-authored-by: sirreal <[email protected]>
|
Thanks for sharing that context. I reviewed the history and see that PR #1703's fix was reverted because it modified the My changes are different in scope - I'm only adding The copy-post.php change is straightforward and safe. For the markdown module changes, the context is narrower - we're just ensuring DB data is properly slashed before passing to these WordPress functions, not modifying the transformation pipeline that caused issues in the old PR. That said, given the module's complexity, if you'd prefer I revert the markdown changes and keep only the copy-post fix, let me know. |
@copilot Since this appears to be a separate issue, you should create a separate PR for that and remove the changes from this PR. |
Keep only the copy-post fix. The markdown module changes should be addressed in a separate PR given the module's complexity and historical issues with slash handling. Co-authored-by: sirreal <[email protected]>
| /** | ||
| * Class Copy_Post_Test for unit testing the Copy Post module. | ||
| * | ||
| * @package automattic/jetpack | ||
| */ | ||
|
|
||
| use PHPUnit\Framework\Attributes\CoversClass; | ||
| use PHPUnit\Framework\Attributes\DataProvider; | ||
| use PHPUnit\Framework\Attributes\Group; | ||
|
|
||
| require_once JETPACK__PLUGIN_DIR . 'modules/copy-post.php'; | ||
|
|
||
| /** | ||
| * @group copy-post | ||
| * @covers Jetpack_Copy_Post | ||
| */ | ||
| #[Group( 'copy-post' )] | ||
| #[CoversClass( Jetpack_Copy_Post::class )] | ||
| class Copy_Post_Test extends WP_UnitTestCase { |
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.
@copilot Is this consistent with how other unit tests are authored in this repository?
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.
Good point. I reviewed existing tests like Jetpack_Sitemap_Finder_Test.php and GFM_Markdown_Test.php and simplified the test to follow the same patterns. Removed the redundant @group/#[Group] annotations and updated docblocks to match (8fd9c8b).
- Remove redundant @group and #[Group] annotations - Update docblocks to match patterns from other test files - Keep core test functionality unchanged Co-authored-by: sirreal <[email protected]>
| * @covers Jetpack_Copy_Post | ||
| */ | ||
| #[CoversClass( Jetpack_Copy_Post::class )] |
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.
There are a lot of duplicated annotations (PHPdoc and PHP annotations) in this test file.
Is that standard practice for this repository?
Review them carefully @copilot. Let's adhere to best practices for this project.
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.
Reviewed the patterns and simplified the test in 83516a2. Removed the redundant @covers/#[CoversClass] annotations, following the simpler pattern used in Jetpack_Google_Font_Face_Test.php. The @dataProvider/#[DataProvider] duplication is kept as that's standard for this repo to support both older and newer PHPUnit versions.
Follow simpler pattern used in Jetpack_Google_Font_Face_Test.php: - Remove @Covers and #[CoversClass] annotations - Keep @dataProvider/#[DataProvider] which is the standard pattern for this repo Co-authored-by: sirreal <[email protected]>
|
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! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Fixes #46020
Proposed changes:
Backslash characters (
\) were being stripped from post content when duplicating posts. For example,\t is a tabbecamet is a tab.Root cause:
wp_update_post()internally callswp_unslash(), expecting slashed data (like from$_POST). When passing unslashed database data directly, backslashes get stripped.Fix: Wrap data with
wp_slash()before passing towp_update_post(), matching the pattern in WordPress Core'swp_restore_post_revision()(revision.php:485).Changes:
copy-post.php: Addwp_slash()inupdate_content()Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
\t is a tab\t is a tab(nott is a tab)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.