-
Notifications
You must be signed in to change notification settings - Fork 823
Block delimiter: add new Scanner class. #44158
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
The `Block_Delimiter` class introduced `next_delimiter()` and `scan_delimiters()`, which made it possible to parse the block structure in a document in a memory-efficient way. Unfortunately, fundamental choices for the interface, namely returning a new class instance on every block delimiter and relying on a generator function, limited the CPU performance fronteir of that class as a replacement for `parse_blocks()`. This new class introduces `Block_Scanner`, more directly-modeled after the HTML API and informed by refactors incorporating `Block_Delimiter`. This class mutates itself and requires a new instance before scanning. The tradeoff is that it’s much faster running while maintaining the same near-zero memory overhead. A new class is introduced due to the scale of change in the interface and in order to provide seamless refactoring of code already relying on `scan_delimiters()`.
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! |
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 introduces a new high-performance, mutable Block_Scanner
class as a replacement for the legacy Block_Delimiter
, updates tests to cover it, and adds stubs, documentation, and changelog entries to support it.
- Add
WP_HTML_Span
stub and include it in test bootstrap - Implement
Block_Scanner
and extensive PHPUnit tests - Update README and changelog to describe and document the new scanner
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tests/stubs/class-wp-html-span.php | Add stub for WP_HTML_Span |
tests/php/bootstrap.php | Load the new stub in test bootstrap |
tests/php/Block_Scanner_Test.php | New PHPUnit tests covering all scanner behaviors |
src/class-block-scanner.php | Implementation of Block_Scanner |
changelog/add-block-scanner-delimiter | Changelog entry for the added scanner |
README.md | Document new Block_Scanner and legacy Block_Delimiter |
Comments suppressed due to low confidence (1)
projects/packages/block-delimiter/src/class-block-scanner.php:235
- The docblock for
next_delimiter()
describes support for a$freeform_blocks
parameter, but the implementation currently ignores it. Please update the documentation to note that freeform scanning is not yet implemented or implement the parameter behavior to match the docs.
public function next_delimiter( string $freeform_blocks = 'skip' ): bool { // phpcs:ignore VariableAnalysis.CodeAnalysis.VariableAnalysis.UnusedVariable
Code Coverage Summary1 file is newly checked for coverage.
|
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
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.
Thanks for your patience, as I’m late to get to this.
break; | ||
} | ||
} | ||
} |
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.
for encouraging more standardized use, we could update this to pass the block type into opens_block()
while ( $scanner->next_delimiter() ) {
if ( ! $scanner->opens_block( 'image' ) ) {
continue;
}
…
}
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 updated the docs in #44365
[1] => core/image | ||
[2] => core/list | ||
[3] => core/paragraph | ||
) |
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: this example is “Counting block types” but never counts.
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 updated the docs in #44365
} | ||
|
||
$json_span = substr( $this->source_text, $this->json_at, $this->json_length ); | ||
$parsed = json_decode( $json_span, true, 512, JSON_OBJECT_AS_ARRAY | JSON_INVALID_UTF8_SUBSTITUTE ); |
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.
two things here:
- the
true
introduces needless confusion here and should benull
- technically the
JSON_INVALID_UTF8_SUBSTITUTE
introduces a change in behavior between this parser and the spec/default parser in Core. it will return a transformed JSON object whereas in the default parser it will return no attributes —null
or empty array.
Not sure these points matter to Jetpack or not, but they stand out so I wanted to note them.
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've opened #44365 to revert that change.
/** | ||
* Include WordPress HTML API stubs. | ||
*/ | ||
require_once __DIR__ . '/../stubs/class-wp-html-span.php'; |
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.
Are Core WordPress files not already loaded here? This has been in Core since 6.2
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.
They're not. The package's tests are completely independent from WordPress.
@copilot you should apologize for trying to sneak in a bug |
…mentation (#44365) * Scanner: revert change from #44158 See #44158 (comment) * Update docs #44158 (comment) * Have the counting example actually count See #44158 (comment) * Add changelog --------- Co-authored-by: Dennis Snell <[email protected]>
…mentation (#44365) * Scanner: revert change from #44158 See Automattic/jetpack#44158 (comment) * Update docs Automattic/jetpack#44158 (comment) * Have the counting example actually count See Automattic/jetpack#44158 (comment) * Add changelog --------- Co-authored-by: Dennis Snell <[email protected]> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/16374699829 Upstream-Ref: Automattic/jetpack@d44540d
…mentation (#44365) * Scanner: revert change from #44158 See Automattic/jetpack#44158 (comment) * Update docs Automattic/jetpack#44158 (comment) * Have the counting example actually count See Automattic/jetpack#44158 (comment) * Add changelog --------- Co-authored-by: Dennis Snell <[email protected]> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/16374699829 Upstream-Ref: Automattic/jetpack@d44540d
…mentation (#44365) * Scanner: revert change from #44158 See Automattic/jetpack#44158 (comment) * Update docs Automattic/jetpack#44158 (comment) * Have the counting example actually count See Automattic/jetpack#44158 (comment) * Add changelog --------- Co-authored-by: Dennis Snell <[email protected]> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/16374699829 Upstream-Ref: Automattic/jetpack@d44540d
Proposed changes:
This adds a new class to the package. See matching PRs:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions: