Footnotes: store in revisions #52686
Conversation
ellatrix
added
[Type] Bug
ellatrix
force-pushed
the
try/fn-revision
branch
from
553ed6c
to
276d336
Compare
|
Flaky tests detected in a924b4b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5597363864
|
|
@ellatrix - you may need to also add a filter on |
|
@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 Any ideas on what to do here? |
Ah, ok
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 |
|
I took a new pass at the core patch and will test to verify 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 (I see you update meta before updating the post in the unit tests, which is the opposite of what the REST API does.) |
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 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.
I tend to agree: it is a serious bug, unless we plan to backport a fix to 6.3.1 |
That doesn't work because revisions are created on
We should get this PR in 6.3 RC2. |
|
@ellatrix can you test with the latest version on WordPress/wordpress-develop#4859? |
There was a problem hiding this comment.
Thanks for the PR @ellatrix ! I'm not fully across how revisions work so am probably not the best person to review this change, but one thing I'm noticing is the function naming: because PHP files from block-library are auto-generated from the package in core, it's better to prefix the functions with wp instead of gutenberg, and rename them at build time. This avoids collisions when running the plugin once the functions are in core. See #51989 for a recent example.
Edit: actually #51978 might be a better example 😄
Did you add REST API support? |
|
Size Change: +1.29 kB (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
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 |
tellthemachines
removed
the
Backport to WP 6.7 Beta/RC
* Filter out patterns that are not allowed in the inserter (#52675) * Remove autofocus and improve placeholder text consistency. (#52634) * Do not navigate to the styles pages unless you're in a random listing page (#52728) * Patterns: Don't override the rootClientID in create menu - only set if undefined (#52713) * Footnotes: store in revisions (#52686) * Fix: Block toolbar obscuring document tools when Top Toolbar is enabled (#52722) * Update toolbar width * Site editor needs specific width * fixes top toolbar width for post editor when not in fullscreen * remove the body rule --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> * Site Editor: Fix site link accessibility issues (#52744) * Add id to pattern inserted notice to stop multiple notices stacking (#52746) * Global Styles: Don't use named arguments for 'sprintf' (#52782) * Footnotes: Use static closures when not using '' (#52781) * removes check for active preview device type to enable the fixed toolbar preference (#52770) * Parser / Site Editor: Ensure autop is not run when freeform block is set to core/html (#52716) * Parse / Site Editor: Ensure autop is not run when freeform block is set to core/html * Switch to equals freeform instead of not equals core/html * Rename core/test-freeform to core/freeform in tests * Adding @SInCE annotation for relevant 6.3 changes. (#52820) * Navigation: Load the raw property on the navigation fallback (#52758) * Navigation: Load the raw property on the navigation fallback * Update lib/compat/wordpress-6.3/navigation-fallback.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update lib/compat/wordpress-6.3/navigation-fallback.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Add a test for these properties * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php Co-authored-by: Dave Smith <getdavemail@gmail.com> * add more comments * add necessary method * Fix php coding standards error --------- Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> * Allow styles to be changed dynamically through editor settings (#52767) * ResizableFrame: Fix styling in Firefox (#52700) * ResizableFrame: Fix styling in Firefox * Remove unused class * Patterns: Fix empty general template parts category (#52747) --------- Co-authored-by: Glen Davies <glen.davies@automattic.com> --------- Co-authored-by: Carolina Nymark <myazalea@hotmail.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Riad Benguella <benguella@gmail.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Lena Morita <lena@jaguchi.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Glen Davies <glen.davies@automattic.com>
|
|
||
| if ( $_gutenberg_revision_id ) { | ||
| $revision = get_post( $_gutenberg_revision_id ); | ||
| $post_id = $revision->post_parent; |
There was a problem hiding this comment.
This line is causing PHP test failures on the core backport. Not 100% sure why that's happening but might be good to check if the revision exists/is an object before trying to access one of its properties?
There was a problem hiding this comment.
I think @tellthemachines already created PR for this #52870
|
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.
What do you mean? |
It doesn't work with published posts. Testing Instructions
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 |
What?
Fixes #52541. Currently footnotes are not stored in revisions because it's post meta.
Why?
We should store them, otherwise footnotes are lost and links become broken.
How?
When a revision is added, update the revision meta. When restoring a revision, also restore the meta.
Testing Instructions
Create a post with footnotes. Then update the content and update a footnote or add/remove one. Now restore a previous revision.
Testing Instructions for Keyboard
Screenshots or screencast