Filter response from isEditedPostSaveable, enabling developer control

[adamsilverstein]

Description

This PR adds a filter editor.selectors.isEditedPostSavable to the value returned by select( 'core/editor' ).isEditedPostSaveable()

Partially resolves #7020.

How has this been tested?

Add the following code to your console:

wp.hooks.addFilter( 'editor.selectors.isEditedPostSavable', 'myplugin', function( isSavable ) {
    return false;
} );

Note that returning false here disables both the (initial) publish button and the update button for publish post, because the post is no longer savable.

Removing the filter with following code restores the default functionality:

wp.hooks.removeFilter(  'editor.selectors.isEditedPostSavable', 'myplugin' );

Note I chose not to filter the early return of false if isSaving is true. This should probably stay as is.

This completely disables the publish button, in our use case we would wait for the prepublish panel to show

Screenshots

Disabled state when filter is applied:

new post:

on publish panel:

published post:

Types of changes

• Filter the return value from select( 'core/editor' ).isEditedPostSaveable with a new filter editor.selectors.isEditedPostSavable.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[adamsilverstein]

@WordPress/gutenberg-core what do you think of this proposal?

If you like it, where shall I document it? If you don't like it, can you share any ideas how to enable developer control of the ability to publish a post?

[gziolo]

I bet @aduth and @youknowriad have some thoughts on this topic. We discussed in one of the other PR (which I can't find now) how developers could override stores - the conclusion was to allow overriding resolvers. I think this use case is a special case so maybe we should identify all places where a button depends on this selector and explore if there could be an alternative solution offered which is component based.

[adamsilverstein]

the conclusion was to allow overriding resolvers

By overriding resolvers, you would change what actually gets in the store, right? That won't help here since i want to alter the selector temporarily. Maybe I am misunderstanding how this could be used.

[aduth]

An issue here is that there's nothing which notifies subscribers that the return value of the function will have been changed. I expect that this can be seen in the branch if, while the publish button is active, a filter is added to override the return value to false. In that case, I assume that the button will not be made disabled. Additionally, I have concerns about the ad hoc usage of filters on return values of some but not all selector functions.

Instead, I wonder if we should track as part of state a value representing whether there is a save blocker. We have similar needs for things like preventing save while an image is still being uploaded. In that case, preventing a save as a matter of including the save blocker value as part of the condition. This also ensures that when the state value changes, any subscribers will automatically re-call the selector and get the updated result.

[adamsilverstein]

@aduth thanks for the detailed feedback.

An issue here is that there's nothing which notifies subscribers that the return value of the function will have been changed

Possibly the plugin code would need to accommodate that. Worth noting that in my testing the button updated immediately when I applied the filter via the console and refocused the editor.

Instead, I wonder if we should track as part of state a value representing whether there is a save blocker.

So do I understand this right: plugins would dispatch actions to enable/disable publishing? One potential issue with this approach is the binary nature. I wonder if we could still use a filter here?

Imagine a scenario where there are several plugins that all want to control the publish-ability of the post. plugin a requires that you have a featured image. plugin b requires that you choose a category. plugin c requires that your content include a copyright attribution. using a filter approach you can easily imagine all plugins applying their conditional logic and only having the button get enabled when all conditions are met. if plugins dispatch on/off actions, only the last one to run will matter.

We have similar needs for things like preventing save while an image is still being uploaded. In that case, preventing a save as a matter of including the save blocker value as part of the condition.

This is a little bit of a different need/use case because its a condition that occurs in response to a known action, and has a specific duration or timeout. This PR handles unknown conditions monitored by third party plugins that can occur at any time (you can't publish after midnight!).

This also ensures that when the state value changes, any subscribers will automatically re-call the selector and get the updated result.

Can you think of any other ways we can notify subscribers? withFilters handles this, maybe a similar HOC could be leveraged here?

Additionally, I have concerns about the ad hoc usage of filters on return values of some but not all selector functions.

We had decided previously with some use cases of filters it is better to take an ad hoc approach, that said I'm open to exploring your idea of integrating this into the store/reducers somehow if it can still meet the original goals.

[adamsilverstein]

I created an a PR with an alternate approach in #10195. This PR adds filter to props in withFitlers which would enable developer control over any filtered components props. This also neatly provides forced updates when filters are added or removed (although not if their return value changes - maybe we should add an action trigger to withFilters that would enable that?).

I tried adding filtering into the reducers but faced the similar issue in that React doesn't recognize there has been a change and re-render

[aduth]

Everywhere else (including the function itself) we use the spelling "saveable"

[adamsilverstein]

old habits die hard

[aduth]

Imagine a scenario where there are several plugins that all want to control the publish-ability of the post. plugin a requires that you have a featured image. plugin b requires that you choose a category. plugin c requires that your content include a copyright attribution. using a filter approach you can easily imagine all plugins applying their conditional logic and only having the button get enabled when all conditions are met. if plugins dispatch on/off actions, only the last one to run will matter.

This is a good one to raise, and where filters are particularly well-suited.

In Calypso, we had addressed it by using named unique keys to associate the reason for blocking:

https://github.com/Automattic/wp-calypso/blob/master/client/state/ui/editor/save-blockers/actions.js

[aduth]

Can you think of any other ways we can notify subscribers? withFilters handles this, maybe a similar HOC could be leveraged here?

I think it'd need to occur at the store level, because the same issue could occur for a consumer via subscribe (of which Gutenberg already has an unfortunate few).

The loose idea that pops to mind, though inelegant as I view it, is an action which is dispatched with the name of a filter callback, where callbacks are tracked in a reducer and applied during the call of the filterable selector.

[aduth]

This is a little bit of a different need/use case because its a condition that occurs in response to a known action, and has a specific duration or timeout. This PR handles unknown conditions monitored by third party plugins that can occur at any time (you can't publish after midnight!).

I don't totally agree that even in this example it's an unknown, since time is an observable thing.

[gziolo]

I came to conclusion that instead of using filters in this case, we should compose isEditedPostSaveable as an internal state of the editor composed with the external state controlled by plugin developers through some action toggle. In effect:

const isEditedPostSaveable = ( state ) =>
    isEditedPostSaveable( state ) &&
    isPostSavingLocked( state );

which would be controlled by two actions:
lockPostSaving( 'my-feature' ) and unlockPostSaving( 'my-feature' )

This way we can maintain multiple locks and only activate Publish button when all checks are unlocked. What do you think?

[aduth]

@gziolo Yeah, in effect that's how Calypso handles it. It won't reflect the "unknown-til-time-we-try-save" cases, but those are (a) not compellingly demonstrated as an unserved need, and (b) perhaps still future-compatible if we grant some pre-save opportunity to initiate a lock before the save (and lock check) continues.

[adamsilverstein]

👍 Sounds good to me, thanks for the review and feedback.

[adamsilverstein]

Closing in favor of #10649 which takes the approach suggested by @aduth and @gziolo above.