Tag Processor: Add bookmark invalidation logic #47559

ockham · 2023-01-30T14:04:36Z

What?

Part of #44410.

Invalidate bookmarks in a WP_HTML_Tag_Processor instance if the tags that they're pointing to have been replaced.

Why?

While WP_HTML_Tag_Processor currently only supports changing a given tag's attributes, we plan to also provide methods to make broader changes (e.g. set_content_inside_balanced_tags ) (probably through a subclass of WP_HTML_Tag_Processor).

An API like that will have the potential of replacing a tag that a bookmark points to. As a preparation, we thus need to make sure that all bookmarks affected by a HTML replacement are invalidated (i.e. released).

How?

By extending the existing loop in apply_attributes_updates that adjusts bookmarks' start and end positions upon HTML changes to check if the entire bookmark is within a portion of the HTML that has been replaced.

Note that this PR is extracted from #47068, per this suggestion.

Testing Instructions

As stated above, there aren't currently any methods that actually replace existing bookmarks, so the relevant codepath should never be hit. Furthermore, apply_attributes_updates is a private method, so we cannot directly test it.

However, we have extensive unit test coverage of WP_HTML_Tag_Processor's public methods which guarantee that their desired behavior is still warranted.

Finally, #47036 contains the code from this PR, and has unit test coverage for bookmark invalidation.

github-actions · 2023-01-30T15:23:53Z

Flaky tests detected in 83f274c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4231726230
📝 Reported issues:

[Flaky Test] should always expand single line selection #39787 in specs/editor/various/multi-block-selection.test.js

dmsnell

is this something you consider necessary for 6.1?
is this necessary at all? It appears to change the behavior of bookmarks where if we seek to a bookmark we previously set, but that bookmark is gone, instead of returning false, with WP_DEBUG set it will crash saying "you did it wrong."

is the safety we're after something that belongs in seek instead so that we can still distinguish between "failed to seek to the bookmark you previously set" and "you tried to seek to a non-existent bookmark"? that is, "something failed because of normal situations" vs. "you tried to do something that is guaranteed to fail and should never be done"

ockham · 2023-01-31T11:54:38Z

is this something you consider necessary for 6.1?

You mean 6.2 🙂 No, this isn't needed for the pending WP release.

is this necessary at all?

I kinda think it is 🤔 Since a bookmark is just a WP_HTML_Span -- i.e. a start/end tuple -- it could end up pointing to something totally different.

Take this unit test for example. We start with the following HTML, and set a bookmark to the img tag:

<div>outside</div><section><div><img>inside</div></section>
                                ^   ^

Now we replace the contents between the third and fourth div tags:

<div>outside</div><section><div>This is the new section content.</section>
                                ^   ^

The bookmark isn't even pointing to a tag anymore. I don't think we want the user to proceed assuming that all is well with that bookmark 😬

It's easy enough for us to add logic (like in this PR) to determine which bookmarks (if any) have been invalidated; it seems like it'd be quite cumbersome for the user to do so (without access to class internals).

It appears to change the behavior of bookmarks where if we seek to a bookmark we previously set, but that bookmark is gone, instead of returning false, with WP_DEBUG set it will crash saying "you did it wrong."

Yeah, that's a fair point.

is the safety we're after something that belongs in seek instead so that we can still distinguish between "failed to seek to the bookmark you previously set" and "you tried to seek to a non-existent bookmark"? that is, "something failed because of normal situations" vs. "you tried to do something that is guaranteed to fail and should never be done"

I could see that 🤔 The downside is that we'll need to keep the bookmark around -- probably setting it to null or so -- to differentiate "invalidated" from "never has been there". Since bookmarks are already scarce, I thought it'd be nice to release them upon invalidation 😄

Since the user would have to check the return value of seek() to find out if the bookmark was valid -- how about instead introducing a has_bookmark( $bookmark_name ) check for the user to call before seek()? (They wouldn't have to do that if they know that they're never touching the HTML in question.)

adamziel

This looks great @ockham 🚢

Is the safety we're after something that belongs in seek instead

I'm not sure if we need to distinguish between these two situations. If the bookmark is gone, it's gone – the entire code branch relying upon that bookmark will no longer work and needs to perform careful checks. I'm not sure how to exactly use the ability to distinguish between the two failure modes.

adamziel · 2023-02-07T14:54:51Z

how about instead introducing a has_bookmark( $bookmark_name ) check for the user to call before seek()? (They wouldn't have to do that if they know that they're never touching the HTML in question.)

has_bookmark sounds great to me @ockham. Does this PR need anything beyond that?

ockham · 2023-02-09T10:58:36Z

Thank you for reviewing and approving, Adam! (Apologies for not replying any sooner -- I was AFK from Friday until yesterday!)

how about instead introducing a has_bookmark( $bookmark_name ) check for the user to call before seek()? (They wouldn't have to do that if they know that they're never touching the HTML in question.)

has_bookmark sounds great to me @ockham. Does this PR need anything beyond that?

No, IMO, that should be all that's needed! 😄 Does that work for you @dmsnell?

ockham · 2023-02-09T11:05:35Z

One thing we need to be mindful of here is that this patch will change the code of the tag processor with regard to what we merged into Core 🤔 While the user is never supposed to encounter any practical consequences from this (since it's currently impossible to make any changes that would actually invalidate bookmakrs), we might want to retain the code in the 6.2 compat layer identical to what is in Core.

So maybe this should go into a different file and/or folder 😬 lib/compat/wordpress-6.3/ anyone?

ockham · 2023-02-09T14:11:10Z

Rebased on #47933 and modified to add the bookmark invalidation logic to the 6.3 compat layer version of this class.

ockham · 2023-02-09T15:10:00Z

I've added has_bookmark() and basic unit test coverage. (Note that we cannot yet test if the invalidation logic work since we simply don't have any method yet that would invalidate bookmarks 😬)

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php

dmsnell · 2023-02-10T19:18:12Z

phpunit/html-api/class-gutenberg-html-tag-processor-6-3-test.php

+		$p->set_bookmark( 'my-bookmark' );
+		$p->release_bookmark( 'my-bookmark' );
+		$this->assertFalse( $p->has_bookmark( 'my-bookmark' ) );
+	}


not we can verify already in a test if a bookmark exists or not, because $p->seek( 'place' ) will return false if it's unavailable.

$this->assertFalse( $p->seek( 'nonexistent' ) ); $this->assertTrue( $p->seek( 'existent' ) ); // wipe out bookmark with some HTML modifications $this->assertFalse( $p->seek( 'existent' ) );

Are you suggesting to add a separate test to cover that behavior of seek? (Happy to do so 😊 )

Ah, I missed the // wipe out bookmark with some HTML modifications part. We don't have any API to do that yet though, do we? 🤔

Are you suggesting to add a separate test to cover that behavior of seek?

No; that's a valid thing to talk about, but I was only responding to what I now realize could be a misreading? I thought you had said that it wasn't possible to test before this patch if we have a bookmark or not, and I was suggesting that we do, it's just a method that also impacts the processor state.

We don't have any API to do that yet though, do we? 🤔

That's correct. It's intentionally possible to mess this up though, hopefully in quiet dark corners with blatant warnings to look away. So yes, we could actually write some tests of this behavior before it's necessary, for the purpose of writing tests.

A simpler alternative is simply to assert that a seek to a nonexistent bookmark doesn't exist. I was trying to provide more context in that snippet.

No; that's a valid thing to talk about, but I was only responding to what I now realize could be a misreading? I thought you had said that it wasn't possible to test before this patch if we have a bookmark or not, and I was suggesting that we do, it's just a method that also impacts the processor state.

Right, thanks for clarifying!

FWIW, part of the reason for adding has_bookmark was to give the user a way to find out without triggering a PHP notice when WP_DEBUG is set (as you'd pointed out).

I'll add a test for seek( 'nonexistent' ) though!

I'll add a test for seek( 'nonexistent' ) though!

Hmm, this is a bit more involved. Since the PHP notice is being thrown in unit tests, I need the test to "expect" it.

I just haven't found the right exception type yet 🤔

$this->expectException( 'PHPUnit_Framework_Error_Notice' ); // Nope, not that one.

dmsnell · 2023-02-10T19:19:20Z

Looks very good, and thanks for your patience. I left some questions about the naming, which at first seemed helpful, and then confusing.

Since this is targeting the compat PR can we make sure not to merge it until that one merges? I would rather avoid mixing this preventative fix with that code shuffle.

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php

Co-authored-by: Dennis Snell <dennis.snell@automattic.com>

ockham · 2023-02-21T10:35:35Z

Rebased, now that #47933 has landed 😊

@adamziel @dmsnell Care to give this another look (and ideally approval)? 😊 🙏

gziolo · 2023-02-21T11:06:29Z

Do we have a WordPress Trac ticket we could reference in the PR? I think it will become an official recommendation soon for PHP changes that we know require backporting to WordPress core.

It would also allow us to include * @ticket NUMBER in PHPDoc for newly added tests.

adamziel · 2023-02-21T11:35:25Z

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php

-				$update_tail = $bookmark->end >= $diff->start;
-
-				if ( ! $update_head && ! $update_tail ) {
+				if ( $bookmark->start < $diff->start && $bookmark->end < $diff->start ) {


I still think this would read better as:

Suggested change

if ( $bookmark->start < $diff->start && $bookmark->end < $diff->start ) {

if ( $bookmark->end < $diff->start ) {

If $bookmark->start is ever after $bookmark->end, I'd rather make it fail in some way rather than try to salvage it with a defensive condition.

While we did inline $update_head and $update_tail here, we didn't otherwise change the condition -- it existed like that prior to this PR. For that reason, I'd like to keep it as is 😬 I know it shouldn't make a difference outside of any pathological cases, but since this PR has been sitting for a while, I'd like to de-risk it as much as possible.

Aside: I kinda like that it's symmetrical to the condition right below:

if ( $bookmark->start >= $diff->start && $bookmark->end < $diff->end ) {

I feel it helps me visualize better that the bookmark is either fully past the diff, or enclosed by it.

I'm open to changing this in a follow-up though 😊

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php

adamziel

I left a nit but it looks great overall. I like how simple it turned!

ockham · 2023-02-21T13:49:28Z

Do we have a WordPress Trac ticket we could reference in the PR? I think it will become an official recommendation soon for PHP changes that we know require backporting to WordPress core.

We don't have a Trac ticket for this specific change. However, since the general plan is to move development of the HTML (Tag) Processor to wordpress-develop (see the conversation in the 6.3 compat layer PR that starts about here), we might file a PR against wordpress-develop soon; that would be a good opportunity to also file a Trac ticket, I think.

That ticket might be a bit broader in scope than just this PR, since there's strictly speaking no need for bookmark invalidation as long as there isn't any API in WP_HTML_Tag_Processor that could actually cause bookmarks to become invalid. So maybe it'd make more sense to file a ticket to request the ability to make non-attribute lexical changes, which would amount to this PR plus #47068.

It would also allow us to include * @ticket NUMBER in PHPDoc for newly added tests.

That's a good point; but maybe we can do that after the fact? 😊 Especially since the idea is to port changes from wordpress-develop back to Gutenberg.

gziolo · 2023-02-21T14:06:27Z

That ticket might be a bit broader in scope than just this PR, since there's strictly speaking no need for bookmark invalidation as long as there isn't any API in WP_HTML_Tag_Processor that could actually cause bookmarks to become invalid. So maybe it'd make more sense to file a ticket to request the ability to make non-attribute lexical changes, which would amount to this PR plus #47068.

Yes, one Trac ticket to use as a reference for all work planned in WordPress 6.3 cycle would be perfect. It can cover non-attribute lexical changes for now, but we can extend the scope if that makes sense later.

It would also allow us to include * @ticket NUMBER in PHPDoc for newly added tests.

That's a good point; but maybe we can do that after the fact? 😊 Especially since the idea is to port changes from wordpress-develop back to Gutenberg.

I'm not sure I understand the reasoning here. If the ticket blocks merging the PR now, then definitely ignore my comment. We will have to include that annotation in WordPress core anyway, but it's the least important thing today.

ockham · 2023-02-21T14:38:13Z

Thanks @dmsnell @adamziel @gziolo!

It's mergin' time! 💥

dmsnell

this is definitely a good example of why I think developing in Core is a good idea. can we prioritize getting this merged into Core and then remove this code ASAP. right now we have introduced divergent codebases and will need to continue to resolve discrepancies introduced on both sides (something we already did once during the merge of #47933)

that is, remove the unit tests by merging them into Core's suite, then the compat file probably won't need updating (or maybe it must since we'll need to reference a Trac ticket), but may anyway, depending on whether the class in Core received updates as well.

thanks for the work on this.

ockham · 2023-02-22T15:12:22Z

I'll file a PR against wordpress-develop later today!

ockham · 2023-02-22T16:52:42Z

I'll file a PR against wordpress-develop later today!

WordPress/wordpress-develop#4116

While `WP_HTML_Tag_Processor` currently only supports changing a given tag's attributes, the plan is to provide methods to make broader changes (possibly through a subclass of `WP_HTML_Tag_Processor`). The API will have the potential of replacing a tag that a bookmark points to. To prepare, this changeset makes sure that all bookmarks affected by a HTML replacement are invalidated (i.e. released). Changes: * Extends the existing loop in `WP_HTML_Tag_Processor::apply_attributes_updates()` that adjusts bookmarks' start and end positions upon HTML changes to check if the entire bookmark is within a portion of the HTML that has been replaced. * Adds `WP_HTML_Tag_Processor::has_bookmark() to check whether the given bookmark name exists. References: * [WordPress/gutenberg#47559 Gutenberg PR 47559] * [https://github.com/WordPress/gutenberg/releases/tag/v15.3.0 Released in Gutenberg 15.3.0] Follow-up to [55203]. Props bernhard-reiter, dmsnell, zieladam. Fixes #57788. git-svn-id: https://develop.svn.wordpress.org/trunk@55555 602fd350-edb4-49c9-b593-d223f7449a82

While `WP_HTML_Tag_Processor` currently only supports changing a given tag's attributes, the plan is to provide methods to make broader changes (possibly through a subclass of `WP_HTML_Tag_Processor`). The API will have the potential of replacing a tag that a bookmark points to. To prepare, this changeset makes sure that all bookmarks affected by a HTML replacement are invalidated (i.e. released). Changes: * Extends the existing loop in `WP_HTML_Tag_Processor::apply_attributes_updates()` that adjusts bookmarks' start and end positions upon HTML changes to check if the entire bookmark is within a portion of the HTML that has been replaced. * Adds `WP_HTML_Tag_Processor::has_bookmark() to check whether the given bookmark name exists. References: * [WordPress/gutenberg#47559 Gutenberg PR 47559] * [https://github.com/WordPress/gutenberg/releases/tag/v15.3.0 Released in Gutenberg 15.3.0] Follow-up to [55203]. Props bernhard-reiter, dmsnell, zieladam. Fixes #57788. Built from https://develop.svn.wordpress.org/trunk@55555 git-svn-id: http://core.svn.wordpress.org/trunk@55067 1a063a9b-81f0-0310-95a4-ce76da25c4cd

While `WP_HTML_Tag_Processor` currently only supports changing a given tag's attributes, the plan is to provide methods to make broader changes (possibly through a subclass of `WP_HTML_Tag_Processor`). The API will have the potential of replacing a tag that a bookmark points to. To prepare, this changeset makes sure that all bookmarks affected by a HTML replacement are invalidated (i.e. released). Changes: * Extends the existing loop in `WP_HTML_Tag_Processor::apply_attributes_updates()` that adjusts bookmarks' start and end positions upon HTML changes to check if the entire bookmark is within a portion of the HTML that has been replaced. * Adds `WP_HTML_Tag_Processor::has_bookmark() to check whether the given bookmark name exists. References: * [WordPress/gutenberg#47559 Gutenberg PR 47559] * [https://github.com/WordPress/gutenberg/releases/tag/v15.3.0 Released in Gutenberg 15.3.0] Follow-up to [55203]. Props bernhard-reiter, dmsnell, zieladam. Fixes #57788. Built from https://develop.svn.wordpress.org/trunk@55555 git-svn-id: https://core.svn.wordpress.org/trunk@55067 1a063a9b-81f0-0310-95a4-ce76da25c4cd

ockham added the [Type] Experimental Experimental feature or API. label Jan 30, 2023

ockham requested review from adamziel and dmsnell January 30, 2023 14:04

ockham self-assigned this Jan 30, 2023

ockham marked this pull request as ready for review January 30, 2023 14:37

ockham requested a review from spacedmonkey as a code owner January 30, 2023 14:37

dmsnell reviewed Jan 30, 2023

View reviewed changes

adamziel approved these changes Feb 1, 2023

View reviewed changes

ockham force-pushed the add/tag-processor-bookmark-invalidation-logic branch from 6895965 to b334e23 Compare February 9, 2023 11:02

ockham force-pushed the add/tag-processor-bookmark-invalidation-logic branch from b334e23 to ce6c644 Compare February 9, 2023 13:35

ockham mentioned this pull request Feb 9, 2023

HTML Tag Processor: Add WP 6.3 compat layer #47933

Merged

ockham force-pushed the add/tag-processor-bookmark-invalidation-logic branch from ce6c644 to 6d223e5 Compare February 9, 2023 14:09

ockham changed the base branch from trunk to add/html-tag-processor-wp-6-3-compat-layer February 9, 2023 14:10

ockham force-pushed the add/tag-processor-bookmark-invalidation-logic branch from dc3e4ce to 50f4305 Compare February 9, 2023 15:06

adamziel mentioned this pull request Feb 9, 2023

WP_HTML_Tag_Processor overview issue #44410

Closed

26 tasks

dmsnell reviewed Feb 10, 2023

View reviewed changes

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php Outdated Show resolved Hide resolved

dmsnell reviewed Feb 10, 2023

View reviewed changes

ockham added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Feb 13, 2023

ockham force-pushed the add/tag-processor-bookmark-invalidation-logic branch from 4204f21 to f960d1e Compare February 13, 2023 10:32

dmsnell reviewed Feb 13, 2023

View reviewed changes

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php Outdated Show resolved Hide resolved

dmsnell reviewed Feb 13, 2023

View reviewed changes

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php Show resolved Hide resolved

dmsnell reviewed Feb 13, 2023

View reviewed changes

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php Outdated Show resolved Hide resolved

ockham and others added 7 commits February 21, 2023 11:33

Add since PHPDoc

90fef0a

Add has_bookmark method

8d35826

Add basic unit test coverage for has_bookmark

0503eb6

Re-write bookmark updating logic

7e2a5bd

Co-authored-by: Dennis Snell <dennis.snell@automattic.com>

Avoid double negation

1d17cb5

Rename test file to include -bookmark suffix

ecee379

Verbing weirds language

83f274c

ockham force-pushed the add/tag-processor-bookmark-invalidation-logic branch from 6d8d626 to 83f274c Compare February 21, 2023 10:34

adamziel reviewed Feb 21, 2023

View reviewed changes

lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php Show resolved Hide resolved

adamziel approved these changes Feb 21, 2023

View reviewed changes

gziolo removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Feb 21, 2023

ockham merged commit 0ff570f into trunk Feb 21, 2023

ockham deleted the add/tag-processor-bookmark-invalidation-logic branch February 21, 2023 14:39

github-actions bot added this to the Gutenberg 15.3 milestone Feb 21, 2023

ockham mentioned this pull request Feb 21, 2023

WP_HTML_Tag_Processor: Allow non-attribute lexical updates #47068

Closed

2 tasks

dmsnell reviewed Feb 21, 2023

View reviewed changes

ockham mentioned this pull request Feb 22, 2023

Tag Processor: Add Bookmark Invalidation Logic WordPress/wordpress-develop#4116

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tag Processor: Add bookmark invalidation logic #47559

Tag Processor: Add bookmark invalidation logic #47559

ockham commented Jan 30, 2023 •

edited by gziolo

Loading

github-actions bot commented Jan 30, 2023 •

edited

Loading

dmsnell left a comment

ockham commented Jan 31, 2023

adamziel left a comment •

edited

Loading

adamziel commented Feb 7, 2023

ockham commented Feb 9, 2023

ockham commented Feb 9, 2023

ockham commented Feb 9, 2023

ockham commented Feb 9, 2023

dmsnell Feb 10, 2023

ockham Feb 13, 2023

ockham Feb 13, 2023

dmsnell Feb 13, 2023

ockham Feb 14, 2023

ockham Feb 14, 2023

dmsnell commented Feb 10, 2023

ockham commented Feb 21, 2023

gziolo commented Feb 21, 2023

adamziel Feb 21, 2023 •

edited

Loading

ockham Feb 21, 2023

adamziel left a comment

ockham commented Feb 21, 2023

gziolo commented Feb 21, 2023 •

edited

Loading

ockham commented Feb 21, 2023

dmsnell left a comment

ockham commented Feb 22, 2023

ockham commented Feb 22, 2023

	if ( $bookmark->start < $diff->start && $bookmark->end < $diff->start ) {
	if ( $bookmark->end < $diff->start ) {

Tag Processor: Add bookmark invalidation logic #47559

Tag Processor: Add bookmark invalidation logic #47559

Conversation

ockham commented Jan 30, 2023 • edited by gziolo Loading

What?

Why?

How?

Testing Instructions

github-actions bot commented Jan 30, 2023 • edited Loading

dmsnell left a comment

Choose a reason for hiding this comment

ockham commented Jan 31, 2023

adamziel left a comment • edited Loading

Choose a reason for hiding this comment

adamziel commented Feb 7, 2023

ockham commented Feb 9, 2023

ockham commented Feb 9, 2023

ockham commented Feb 9, 2023

ockham commented Feb 9, 2023

dmsnell Feb 10, 2023

Choose a reason for hiding this comment

ockham Feb 13, 2023

Choose a reason for hiding this comment

ockham Feb 13, 2023

Choose a reason for hiding this comment

dmsnell Feb 13, 2023

Choose a reason for hiding this comment

ockham Feb 14, 2023

Choose a reason for hiding this comment

ockham Feb 14, 2023

Choose a reason for hiding this comment

dmsnell commented Feb 10, 2023

ockham commented Feb 21, 2023

gziolo commented Feb 21, 2023

adamziel Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

ockham Feb 21, 2023

Choose a reason for hiding this comment

adamziel left a comment

Choose a reason for hiding this comment

ockham commented Feb 21, 2023

gziolo commented Feb 21, 2023 • edited Loading

ockham commented Feb 21, 2023

dmsnell left a comment

Choose a reason for hiding this comment

ockham commented Feb 22, 2023

ockham commented Feb 22, 2023

ockham commented Jan 30, 2023 •

edited by gziolo

Loading

github-actions bot commented Jan 30, 2023 •

edited

Loading

adamziel left a comment •

edited

Loading

adamziel Feb 21, 2023 •

edited

Loading

gziolo commented Feb 21, 2023 •

edited

Loading