-
Notifications
You must be signed in to change notification settings - Fork 843
Verbum Comments: optimize block filtering performance with Block_Scanner pre-filtering #44996
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
Verbum Comments: optimize block filtering performance with Block_Scanner pre-filtering #44996
Conversation
- Added the "automattic/block-delimiter" package to composer.json. - Enhanced the Verbum_Block_Utils class to utilize Block_Scanner for faster detection of disallowed blocks, improving performance by avoiding unnecessary parsing in cases where all blocks are allowed. Introduced fallback methods for handling disallowed blocks when detected.
|
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! Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 1 file.
|
- Updated the Verbum_Block_Utils class to return unslashed content for consistency across processing paths. - Added validation for $allowedtags to prevent override warnings. - Introduced comprehensive tests for handling malformed block delimiters, Unicode edge cases, block type spoofing, and resource limits to ensure robust security and consistent input validation.
- Introduced the "automattic/block-delimiter" package with its dependencies and autoload configurations. - Updated the reference for "automattic/jetpack-mu-wpcom" to ensure compatibility with the new package.
- Removed redundant comments and simplified the logic for returning unslashed content. - Updated the method documentation for clarity. - Enhanced the handling of innerHTML to ensure it is set correctly when not defined.
dilirity
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.
Nice work on this! I added some questions but they are in no way blockers, more like wonderings.
![]()
| * @param string $content Unslashed content to scan. | ||
| * @return bool True if disallowed blocks found, false if all blocks are allowed. | ||
| */ | ||
| private static function has_disallowed_blocks_fast( $content ) { |
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.
Nitpick: I think the fast part is not necessary. There's no slow equivalent :D
| */ | ||
| private static function has_disallowed_blocks_fast( $content ) { | ||
| if ( ! class_exists( '\\Automattic\\Block_Scanner' ) ) { | ||
| return true; |
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.
I was going back and forth about the function returning true (has disallowed blocks) but I think it makes sense. This way we default back to always using parse blocks if we can't use the Block_Scanner.
| // Validate $allowedtags integrity - use local variable to avoid override warning | ||
| $validated_allowedtags = $allowedtags; | ||
| if ( ! is_array( $validated_allowedtags ) ) { | ||
| $validated_allowedtags = wp_kses_allowed_html( 'post' ); | ||
| } |
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.
This is to just make sure that we're working with correct data, right? Just making sure 😅
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.
Yup! Correct, just an extra defensive measure.
…larity and performance. Updated method name from `has_disallowed_blocks_fast` to `has_disallowed_blocks` and adjusted documentation accordingly.
dilirity
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.
![]()
Fixes HOG-297: Investigate usability of Block_Scanner within Verbum_Block_Utils
Proposed changes:
Other information:
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
N/A
Testing instructions:
This needs to be tested in both Atomic and Simple.
/wp-comments-post.php%3C!--%20wp%3Alatest-posts%20%2F--%3E(which isencodeURIComponent())bodykey to betweencomment=and&hc_post_as=tocomment=%3C!--%20wp%3Alatest-posts%20%2F--%3E&hc_post_as=