Flaky tests detected in a924b4b.
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/5597363864
📝 Reported issues:

• [Flaky Test] Animation selector should NOT appear if Behavior is None #52560 in /test/e2e/specs/editor/blocks/image.spec.js
• [Flaky Test] should close and focus appropriately using enter key on close button #52564 in /test/e2e/specs/editor/blocks/image.spec.js
• [Flaky Test] should open and focus appropriately using enter key #52397 in /test/e2e/specs/editor/blocks/image.spec.js
• [Flaky Test] zoom animation #52381 in /test/e2e/specs/editor/blocks/image.spec.js

@ellatrix - you may need to also add a filter on wp_save_post_revision_post_has_changed - otherwise changes to only meta (without content changes) will not create a revision. I have a pretty refined version of this type of revision auto-saving in https://github.com/adamsilverstein/wp-post-meta-revisions/blob/main/wp-post-meta-revisions.php please use what you can from there!

@adamsilverstein Yes, I know, right now you need to change content as well to trigger a revision. When you add/remove or change footnotes order it will create a revision, because it affects post content. But this is the least of our problems. Currently we need to do this ugly hack:

remove_action( 'post_updated', 'wp_save_post_revision', 10, 1 );
add_action( 'wp_after_insert_post', 'wp_save_post_revision', 10, 1 );

Because the REST API first updates the post, then updates the meta. And revisions are created on post_updated, before the meta is inserted. If the REST API were using the meta_input field, it would be no problem.

Any ideas on what to do here?

When you add/remove or change footnotes order it will create a revision, because it affects post content. But this is the least of our problems.

Ah, ok

Any ideas on what to do here?

This sounds very similar to the "off by one" issue raised several times on the plugin (eg. adamsilverstein/wp-post-meta-revisions#17 (comment) - review the fix there). I think moving the hook to wp_after_insert_post will have some unanticipated side effects so I would be hesitant to change that! Ultimately, I think we can fix this in core in a way that won't be as hacky because we can copy over the revised meta fields at the correct time. Now that GB footnotes actually utilizes post meta we have a stronger argument for getting a fix into core; I would be happy to work on trying to land this in 6.4. I can work on updating the core patch with the latest approach from the plugin

I took a new pass at the core patch and will test to verify footnotes work correctly (which I included by default now):

WordPress/wordpress-develop#4859

note the additional feature there that makes previews also work correctly with revisioned meta which I believe will correct other reported issues as well.

Is this something we can publish as a plugin in the directory so we have something to point people to until 6.4? Though this bug makes me think that footnotes is not ready for 6.3. It would not be cool if we had to point people to a plugin for a new feature to work properly.

To be clear, here's what happens now:

Whatever the current footnotes are, those will be preserved. But you might not realise that footnotes are not being tracked in revision history, and only realise after you restore a revision that footnotes are not restored. The fact that it’s heavily linked to the post content makes matter worse: the links will become broken with anchors pointing to nonexistent footnotes, footnotes being there that are not relevant, and the list numbering will not match the numbering in the content. That’s the case until you edit the post and the entity provider onChange is called so the order is fixed and irrelevant notes are deleted. But old footnotes will remain empty.

@adamsilverstein How do you address (or plan to address) the fact that the REST API updates meta separately, while the only hook it corrects is wp_after_insert_post, not post_updated, save_post etc. As long as creating a revision is done on the post_updated hook, this will be a problem, unless the REST API's hook order is corrected.

(I see you update meta before updating the post in the unit tests, which is the opposite of what the REST API does.)

@adamsilverstein How do you address (or plan to address) the fact that the REST API updates meta separately, while the only hook it corrects is wp_after_insert_post, not post_updated, save_post etc. As long as creating a revision is done on the post_updated hook, this will be a problem, unless the REST API's hook order is corrected.

I noticed that as well in my manual testing. I'll need to expand the unit tests to cover actual REST API saves. maybe here you could use the wp_after_insert_post action?

I think the best bet will be to integrate the revisioning meta update into the REST API endpoint directly, right after the post meta is updated, we can copy it over to the newly created revision. I started working on this in the PR and plan to continue on that direction. I will also try integrating to the autosaves endpoint which we use for previews.

this bug makes me think that footnotes is not ready for 6.3

I tend to agree: it is a serious bug, unless we plan to backport a fix to 6.3.1

I noticed that as well in my manual testing. I'll need to expand the unit tests to cover actual REST API saves. maybe here you could use the wp_after_insert_post action?

That doesn't work because revisions are created on post_updated. A couple of solutions:

• Move creating revisions to wp_after_insert_post. That will at least break plugins relying on the fact that it's added to the post_updated action, for example for removing the hook.
• Fix the REST API so post_updated is fired after meta is updated, just like it is for wp_insert/update_post. I think this is my preference because it make the behaviour the same as wp_insert/update_post.
• Maybe update post meta before updating the post in the REST API, but I think that's more risky and widens the difference between the REST API and wp_insert/update_post even more.

I tend to agree: it is a serious bug, unless we plan to backport a fix to 6.3.1

We should get this PR in 6.3 RC2.

@ellatrix can you test with the latest version on WordPress/wordpress-develop#4859?

@ellatrix can you test with the latest version on WordPress/wordpress-develop#4859?

Did you add REST API support?

Size Change: +1.29 kB (0%)

Total Size: 1.44 MB

_{compressed-size-action}

@ellatrix can you test with the latest version on WordPress/wordpress-develop#4859?

Did you add REST API support?

Yes, it supports the rest API correctly, I just haven't added it to the tests

I created a small follow-up PR #52781.

I just cherry-picked this PR to the update/packages-RC2 branch to get it included in the next release: 5a3ac19

Does this fix also work for previews?

I realize this is needed to land footnotes in 6.3, however I feel we should generalize the code so any post meta can be revisioned, which is what my core PR does. I will continue to work on that code with a plan to remove what was added here once the more generalized solution is available.

Yes, it would be good to replace this with a general solution.

Does this fix also work for previews?

What do you mean?

Does this fix also work for previews?

It doesn't work with published posts.

Testing Instructions

1. Create a new post.
2. Add text and a few footnotes.
3. Publish the post.
4. Add another footnote.
5. Preview the post.
6. Notice that the new footnote isn't displayed in the block, but the anchor is in the content.

Screenshot

Looking into it

What does this have to do with revisions though?

The previews are stored as a temp revision - autosaves.

Oh, ok. Since there's no content loss, I'm not sure if we should fix it this late.

@adamsilverstein Yes, your PR adds e2e test for the revisions endpoint, but it doesn't add tests for the posts endpoint, which is what GB is using. GB is not managing revisions.