Add an async __unstablePreSavePost hook; resolving with false prevents saving
#58022
Conversation
adamsilverstein
added
[Feature] Extensibility
packages/editor/src/store/actions.js
Outdated
| ); | ||
| if ( ! preSaveOK ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Thank you for starting to implement this! 🚀
Some further improvements would be nice:
- The hook should be able to return/throw an error. Like
Error( 'ACF validation failed' ). The code further down thesavePostfunction will then display a notice with that error. - The hook should be able to inspect also the
edits, and maybe even change them. In addition to theoptionsthat are already passed to it (in practice we inspectoptions.isAutosave).
The result would look like:
try {
filteredEdits = await applyFilters(
'editor.__unstablePreSavePost',
Promise.resolve( edits ),
options
);
} catch ( err ) {
error = err;
}
if ( ! error ) {
// do save and the SavePost hook
}What do you think?
There was a problem hiding this comment.
👋 Thanks a lot for working on this, Adam! I was pointed to this PR by Jarda, as I have a somewhat related use case:
I have an entity (a wp_navigation postType) that is loaded via useEntityRecords(). The entity has a post meta field registered (which is returned by the endpoint). When saving the entity (after it has had some edits), I’d like to include the post meta, even if the latter doesn't have any edits. (See the bottom of this comment for more context.)
It seems that an __unstablePreSavePost filter -- if Jarda's suggestion of allowing modifications to edits is applied -- could solve that problem (although I'd need it to work in the Site Editor rather than in the Post Editor).
There was a problem hiding this comment.
although I'd need it to work in the Site Editor rather than in the Post Editor
Do we need to add it there separately @ockham?
There was a problem hiding this comment.
The hook should be able to inspect also the edits, and maybe even change them. In addition to the options that are already passed to it (in practice we inspect options.isAutosave).
My only goal here is to prevent saving, I don't thing we should enable changing the edits at this point, what would the use case for that feature be?
I have an entity (a wp_navigation postType) that is loaded via useEntityRecords(). The entity has a post meta field registered (which is returned by the endpoint). When saving the entity (after it has had some edits), I’d like to include the post meta, even if the latter doesn't have any edits.
@ockham Ah, this does sound like a valid use case for being able to modify the edits before a save, have you already solved this elsewhere?
This would complicate the use of the proposed preSave hook as I intended - simply returning an error message to hook into the existing error handling logic to prevent saving.
Since this proposal changes the use case for the hook, I would prefer to consider it in a follow up issue, what do you think?
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @aduth, @lex127, @audiovisuel-uqam, @eballeste, @jenilk, @zhitaryksergey, @christianMiguez. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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 Getting back to this after a break, thanks for the feedback - I like the idea of using an Error and of being able to modify the edits. I applied your suggestions as I understood them, can you take another look? |
|
@jsnajdr can you take a look at this please when you have a moment? |
|
Hi @adamsilverstein 👋 Do you plan to work on this PR? It's very close to being finished and it would be nice to get it into WordPress 6.6. I could take it over from you and get it merged if you agree. |
|
Excited for this PR |
Thanks for the ping @jsnajdr (and @bph). I will get back to updating this soon! |
|
@jsnajdr I have tried to address all your feedback, please review again when you have a moment! |
|
I have updated the PR so functionality is restored and tests pass. Appreciate any reviews/feedback. |
|
Sorry for the late feedback, I only just saw this. I might be missing something, but I don't think this should ship with the Regardless, |
|
Sure, let's remove the |
|
I'm proposing the stabilization and some followup changes in #64198. |
| try { | ||
| error = await applyFilters( | ||
| 'editor.__unstablePreSavePost', | ||
| Promise.resolve( false ), |
There was a problem hiding this comment.
If I'm not wrong we use this technique (a promise filter) because we don't support async actions yet right? Can we implement async action instead? await doAsyncAction( 'editor.__unstablePreSavePost' )
I have another use-case for this and maybe it's time to fix that once and for all.
There was a problem hiding this comment.
Good idea. I'm working on this and one little obstacle is that currentFilter()/currentAction() don't make sense any more, because there can be multiple hooks running at the same time.
If you use the promise filter technique with sync hooks this problem is invisible. Because the hook merely sets up a promise chain and returns. Only later the actual data will flow through the chain. This will change when runHook will start waiting for each promise returned from a handler to resolve.
There was a problem hiding this comment.
This is a draft PR for async hooks: #64204
Thanks for spinning up a PR quickly. I'll give it a review. It's mostly core releases that are a concern, though ideally it also wouldn't ship in a plugin release. If it does I think the usual rule is deprecate for two plugin versions and then remove. It's a bit of hassle that's usually better to avoid where possible. Next plugin release (19.0) is on 14th August, with the RC on 7th August, so there should be time to rectify. |
|
@lex127 mind sharing your WordPress.org username so I can ensure its included in the 6.7 credits listing? |
|
@audiovisuel-uqam mind sharing your WordPress.org username so I can ensure its included in the 6.7 credits listing? |
What?
__unstablePreSavePost. resolving with false will prevent save from occurring.Fixes #13413
Why?
Useful for pre-save validation where a plugin wants to validate a set of requirements before allowing the post to save. Instead of trying to disable the save button itself, this hook provides a validation opportunity _right before the post save is actually triggered. When validation fails, plugins can add a visual indication of the issue in their UI and resolve to false here to prevent the save from occurring.
See #13413 for additional discussion of use cases.
How?
savePostaction right before the save actually occurs.isEditedPostSaveablereturning falseTesting Instructions
wp.hooks.addFilter('editor.__unstablePreSavePost', 'editor', function(){ return Promise.resolve( { message:"This is the error message." } ) } );wp.hooks.removeAllFilters( 'editor.__unstablePreSavePost' ); wp.hooks.addFilter('editor.__unstablePreSavePost', 'editor', function(){ return Promise.resolve(false); } );