Command Palette: Use WP_HTML_Processor and WP_HTML_Decoder to generate menu label and menu URL #10480
Conversation
…e menu label and menu URL
|
I'll prepare the commit message now, ready for when I commit this pull request. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
@dmsnell @mukeshpanchal27 @peterwilsoncc If there are no other blockers, I'd like to commit this before the RC1 release, but what do you think? This PR can be considered a code quality improvement, so if there are any blockers, we can punt it to a future release. |
| } | ||
|
|
||
| return trim( implode( '', $text_parts ) ); | ||
| }; |
There was a problem hiding this comment.
hey nice job @t-hamano getting this built. I hope it wasn’t too obscure to figure out.
this looks like it should be solid, but I can share a couple of points of feedback.
Finding root-level text nodes
when creating a fragment (with the default <body> context) we will always have an open HTML element and BODY element, meaning that root-level text will always have a depth of 3 (and likewise, the breadcrumb depth will be three).
this means we can eliminate the nested loop and directly check if the depth is 3. we don’t have to capture the root depth. that open HTML and BODY are guarantees with how it works.
On the other hand, we can also test this via the breadcrumbs. I found an issue that we should probably change/fix on matches_breadcrumbs(), because that won’t work here, but for the time being this would.
while ( $processor->next_token() ) {
if ( array( 'HTML', 'BODY', '#text' ) !== $processor->get_breadcrumbs() ) {
continue;
}
$text_parts…
}Efficiency and reliability
The use of the HTML Processor is particularly convenient because it provides depth automatically. On the other hand, if you find that it’s too slow or fails too frequently (because it receives the fraction of input documents it can’t parse) then we can still adjust the lever on the reliability/practicality spectrum. The Tag Processor will not fail with the same parsing issues the PCRE matches did, even though that can lead to some kinds of parsing failures (with, for example, mismatched tags).
Still, the Tag Processor won’t fail a parse each token and is considerably faster than the fully-fledged HTML Processor. If we were to choose this approach, we’d want to manually track depth, which again, could be wrong because HTML is so wonderfully complex (vs. the HTML Processor which will not be wrong here).
$processor = new WP_HTML_Tag_Processor( $label );
$depth = 0;
while ( $processor->next_token() ) {
$token_name = $processor->get_token_name();
if ( '#text' === $token_name && 0 === $depth ) {
$text_parts…
continue;
}
if ( $processor->is_closing_tag() ) {
--$depth;
} else if ( ! WP_HTML_Processor::is_void( $token_name ) ) {
++$depth;
}
}The choice is up to you. The only thing I’d watch out for is that occasionally we get things like “nested” A tags, and those can cause the HTML Processor to abort out of caution.
|
@t-hamano let me know your preferences on this based on my feedback. I’m happy to approve the work if we want to get it in still, understanding that it’s now after the RC1 deadline. I believe that with a couple of sign-offs we can still do so. |
4 participants
Trac ticket: https://core.trac.wordpress.org/ticket/64233
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.