Stabilize the PreSavePost and SavePost filters #64198
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsnajdr
added
[Type] Enhancement
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jsnajdr
commented
Aug 2, 2024
| 'editor.__unstableSavePost', | ||
| Promise.resolve(), | ||
| options | ||
| ); |
There was a problem hiding this comment.
If anyone ever registers an editor.__unstableSavePost legacy filter, I'd like to log a deprecated warning. But not sure how to do that. Did anyone do something similar before?
There was a problem hiding this comment.
Doesn't seem like it'd be as effective as a deprecation message on a call to addFilter, but core has this PHP function, so replicating it might be the way to go - https://developer.wordpress.org/reference/functions/apply_filters_deprecated/
There's no JS equivalent unfortunately, so it'd need to be implemented in the hooks package, which makes this a bit more involved. 😅
I guess the reason it's not handled on add_filter in core is the deprecation messages would have to be added within the add_filter function?
There was a problem hiding this comment.
Yes, apply_filters_deprecated is exactly what I need 🙂 It does nothing when no filter is registered, but complains otherwise.
It needs to be added to @wordpress/hooks first, but why not, I'm already adding async filters anyway. Seems that the hooks package will get a nice upgrade in 6.7.
There was a problem hiding this comment.
I think a apply_filters_deprecated port to JS will make a lot of sense in the package, especially as we deprecate filters/actions in the future. Work for another PR of course 😉 It doesn't seem like the unstable hook was used anywhere (I've checked extensively on wpdirectory.net and a few more places)
|
Size Change: +5 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 19c6326. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10214362037
|
talldan
reviewed
Aug 5, 2024
packages/editor/src/store/actions.js
Outdated
| 'editor.__unstablePreSavePost', | ||
| Promise.resolve( false ), | ||
| edits = await applyFilters( | ||
| 'editor.PreSavePost', |
There was a problem hiding this comment.
Most examples in the codebase are lower case first letter:
Suggested change
| 'editor.PreSavePost', | |
| 'editor.preSavePost', |
There was a problem hiding this comment.
OK, I'll change that to lowercase. I've been looking at prior art for editor filters, and there are names like editor.BlockListBlock, editor.BlockEdit or editor.MediaUpload, but these turn out to be exceptions: they are filters that wrap React components of the same name. By default filters should start with lowercase.
packages/editor/src/store/actions.js
Outdated
| if ( ! error ) { | ||
| try { | ||
| await applyFilters( | ||
| 'editor.SavePost', |
There was a problem hiding this comment.
Suggested change
| 'editor.SavePost', | |
| 'editor.savePost', |
|
What are the blockers on this PR? We should probably ship stabilized names before WP 6.7. |
|
@Mamaduka We're experimenting with support for async hooks in #64204. See the discussion in #58022 (comment) I also plan that all this is finished in time for 6.7. |
jsnajdr
force-pushed
the
stabilize/save-post-hooks
branch
from
830664f to
f6c7cf5
Compare
|
Async hooks were just merged in #64204, so I amended this PR into a final form: it now gives the Please have a look. The next big question is whether we can still backport this into 6.7. |
tyxla
reviewed
Sep 27, 2024
There was a problem hiding this comment.
This makes a lot of sense and I like the new way it works 👍
I wonder if we should get an approval from @adamsilverstein who worked on these hooks before shipping and attempting to get it into 6.7 though.
| } ) | ||
| async ( options ) => { | ||
| if ( ! options.isAutosave && select.hasMetaBoxes() ) { | ||
| await dispatch.requestMetaBoxUpdates(); |
There was a problem hiding this comment.
Returning here previously didn't make much sense, and that's why we're removing it, right?
There was a problem hiding this comment.
Previously we were returning a promise, so that the next filter can .then on it. Now this is all handled by the async runner: it will ensure that the next handler runs only after the previous one finished and succeeded.
That's also why this hook is no longer a filter, but an action. Action handlers don't return anything.
| 'editor.__unstableSavePost', | ||
| Promise.resolve(), | ||
| options | ||
| ); |
There was a problem hiding this comment.
I think a apply_filters_deprecated port to JS will make a lot of sense in the package, especially as we deprecate filters/actions in the future. Work for another PR of course 😉 It doesn't seem like the unstable hook was used anywhere (I've checked extensively on wpdirectory.net and a few more places)
tyxla
approved these changes
Sep 27, 2024
Yes, this is going to be a very nice and simple addition 🙂 Too bad I didn't get to finish it for 6.7. |
jsnajdr
added
the
Backport to WP 6.7 Beta/RC
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Sep 29, 2024
* Stabilize the PreSavePost and SavePost filters * Use lowercase names * Use async hooks
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: fbd2119 |
huubl
pushed a commit
to huubl/gutenberg
that referenced
this pull request
Oct 2, 2024
* Stabilize the PreSavePost and SavePost filters * Use lowercase names * Use async hooks
huubl
added a commit
to huubl/gutenberg
that referenced
this pull request
Oct 2, 2024
…)" This reverts commit 4f97ebb.
fabiankaegy
added
the
Needs Dev Note
|
@fabiankaegy Yes it's true, this PR is also a good candidate for a dev note. I'll take care of it. |
97 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup to #58022 which stabilizes the filter names as
editor.preSavePostandeditor.savePost, removing the__unstableprefixes. The oldeditor.__unstableSavePostfilter continues to work because it's been there for some time.I'm also changing the behavior a little bit:
editsas the first parameter of the filteredits, not an error. You need to throw or return a rejected promise to signal error.This example from #58022 no longer works:
The right thing to do is now:
or (the same thing with different syntax:
Now when the
hookspackage supports async hooks, I also turned thepreSavePostfilter into an async filter, andsavePostcan now be an action. Because it doesn't need to pass the promise from the previous handler and wait for it, it's all handled by the framework indoAsyncAction.