Allow workflow types to lock certain changes to workflows once things are in use

Here's a start. Very pleased with how simple this is.

I think we should do something more generic so that all workflow types have to think about this problem.

Updated the issue summary. I've added some decisions to make to it. I'm not sure about adding the same protection to the workflow entity API.

Looks like a good solution to the problem.

I wonder if we need to make use of isWorkflowUsed() anywhere other than checkAcess(), but I guess this can be done in a follow up.

Also another good follow up might be to look at UX. I'm not sure throwing a 403 is that friendly, maybe it'd be better to redirect back and display a warning such as "There are N Nodes and N Blocks using the FOO moderation state."

These should not block this important update though.

@timmillwood well the user doesn't ever get a delete link. So they don't see the 403 unless they hack together the URL. Late in 8.x cycle we added \Drupal\Core\Access\AccessResultReasonInterface we should leverage that here I guess. But the problem is the orIf() not handling merging the reason :(

+++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
@@ -163,4 +204,14 @@ public function calculateDependencies() {
+   */
+  public function isWorkflowUsed(WorkflowInterface $workflow) {
+    return (bool) $this->queryFactory->get('content_moderation_state')
+      ->count()
+      ->condition('workflow', $workflow->id())
+      ->execute();
+  }

Doesn't this need an accessCheck(FALSE) just in case?

Can't imagine any situation where you'd query alter content_moderation_state entities depending on user, but sets a good example at least.

Decide if we want to add protection to the API as well.

We need this because the database you delete from the UI on dev is not in the same state as the one you deploy the change to via the API on production.

@catch good point - although that's not an argument for API protection - that's an argument for a config import validator!

So whilst writing this code to prevent importing a deleted workflow that is in used i started to ponder if we've got this right. Perhaps what we should be doing is confirming with the user and saying that this will delete the states. And perhaps this means we need to allow the content moderation workflow type to always provide a published and unpublished fallback? So if we delete a state in use we just move them to the fallback state and we never allow the fallback states to be deleted.

Could go for that as well yeah.

#2817835: When enabling moderation apply a relative state and #2813723: What happens when you enable / disable moderation? both talk about a similar thing to fallback states.

@Sam152 That's a pretty good idea. But we need to maintain a list of replacement IDs - ie. what happens if the replacement gets deleted too?

Also I'm not sure how replacement IDs in image styles work with configuration deployment.

So... trying to get this issue back on track as it's a "must-have" in #2843494: WI: Workflows module roadmap.

Is the current thinking that we don't lock changes to a workflow, but if a state is deleted all entities with that state get moved to another state?

In Content Moderation we currently have draft and published as a requires states, so we could use one of them. This also means we'll have to depend on the workflow plugin to do the state changes. Content Moderation could move all "unpublished" entities to draft, and all "published" entities to published?

This then makes this issue heavily related to #2817835: When enabling moderation apply a relative state and #2813723: What happens when you enable / disable moderation? and use very much the same code / batch process.

This code builds on the suggestion in #12. If a "published" moderation state is deleted we fall back to the published state, if an "unpublished" moderation state is deleted we fall back to the draft state.

The current code results in resaving the ContentModerationState and moderated entity (such as Node), which is going to be slow on big sites. I'm wondering if a better approach would be to just swap out the moderation state string in the content_moderation_state_field_revision and content_moderation_state_field_data tables.

This does the same but doesn't load any entities to improve performance, and also adds to the test to check an unpublished moderation state.

(sorry forgot to generate an interdiff)

Here's the interdiff between #23 and #25.

Updating issue summary to reflect the new direction.

Switching to injecting the database.

This patch addresses 1, 4, and 5, from #29.

2. This doesn't get called when doing a config import, and neither does \Drupal\workflows\Entity\Workflow::deleteState so related transitions don't get deleted. Maybe that should be a follow up or dependency for this.

3. I've gone back and forth on this. We could call \Drupal\content_moderation\Entity\ContentModerationState::updateOrCreateFromEntity which will just save the ContentModerationState entity and not the parent entity, thus firing all entity API things, but it will not recursively go back through all ContentModerationState revisions updating the states, should it? It will also be a performance it. Just updating the database tables seems much nicer, but means there are no entity hooks fired.

Moving things to entity_update as suggested in #29.2

Should've removed the use statement.

@timmillwood, patch seems good, after applying was able to validate both cases:

1. A custom state (published) is deleted, moderation is set to Published state
2. A custom state (non published) is deleted, moderation is set to Draft state

However I believe that a prompt shall be presented to the user informing about the change, something like "There are X items of content with this state, these will be moved to Y state."

I agree with @pauloamgomes - probably a confirmation form should be put in place so the user can see what would be happening before hand. That said, we could do that as a follow up perhaps?

Updating the message shown on the confirmation page.

Brilliant @timmillwood - works for me!
+1 to rtbc

Looks awesome, thanks @Sam152.

#37 has been addressed, as well as #33 & #34, looks good to me.

+++ b/core/modules/content_moderation/src/EntityOperations.php
@@ -145,6 +157,25 @@ public function entityUpdate(EntityInterface $entity) {
+        $this->database
+          ->update($content_moderation_state_definition->getDataTable())
+          ->condition('moderation_state', $deleted_state->id())
+          ->fields(['moderation_state' => $new_state])
+          ->execute();
+        $this->database
+          ->update($content_moderation_state_definition->getRevisionDataTable())
+          ->condition('moderation_state', $deleted_state->id())
+          ->fields(['moderation_state' => $new_state])
+          ->execute();

I don't think we should be manually fiddling with the database here. This is a bad practice when dealing with content entities. Why can't we just load/edit/save the various state definitions?

@plach - My thought was if we're update hundreds or thousands of ContentModerationState entities, a simple database update would be quicker and easier than loading/editing/saving each entity.

Sorry, I misread the code, I thought this was only updating a few records, now I realized we can have one record per entity. It's unfortunate but I guess there's no way around doing that until we support bulk updates via entity query or something fancy like that. Unless we want to use a batch.

However I guess we need at least to clear entity cache after updating the DB, right?

Are we sure it's fine to skip hook invocation when performing this change? Won't it be more correct to prevent state deletion until all entities were moved out of it?

Unless we want to use a batch.

Any reason we are not using batch+entity API? Using the entity API will ensure that all the hooks will fire.

I guess we should really use batch+entity API, I was just worried of the performance hit and as ContentModerationState is kinda an internal entity type that no one should be interacting with, this felt like a good way to get a really good performance boost when deleting a state.

Also, as this is deleting a state, and not like a mandatory upgrade path, I think it's a slightly different use case to to standard bulk updates.

Although, happy to look at a batch entity load/edit/save if that's the better option.

For the record @catch suggested in Slack to block the state deletion altogether if there are entities in the state to be deleted.

I guess we should give more reasoning in the code as to why we're doing it this way, core is often used as an example for others, so we want to make it clear this is a special case.

Yep, adding an inline comment would definitely help. I also think we should mark this stuff as @internal ASAP, ideally before introducing this change.

Adding a comment and a cache reset.

This is inadequate, because you can delete the state in an environment where there are no corresponding entities and deploy the config to an environment where there is.

Then config validation would prevent you from deploying the change to production and you'd have to sort it out though. This is the same as other cases where we have the same checks.

This is marked as a MUST-HAVE in the roadmap.

Is the only cost that you potentially create config you can't import?

Yes you can end up with config that's deleted on your dev site, which you then can't import into the production site because either new content has been put in the state, or because working with a different db.

We don't have tools for bulk changing states, if we went down the route of making the site administrator update the content.

That seems like a generally useful feature to me, but shouldn't block this issue - good to open a follow-up to add it though if there isn't one already.

Well, according to #48 and #54 the current approach is inconsistent with what happens normally in core, which also forces it to "abuse" the Entity API. It's unfortunate but for that reason I think we need to abandon the current route and implement what @catch is suggesting.

#2797583: Dynamically provide action plugins for every moderation state change can definitely alleviate the problem of not being able to deploy the configuration change, and given that changing the state of many entities has the potential to introduce unexpected and undesirable side-effects, I think in the end it makes more sense to leave that as an explicit choice for administrators rather than doing it automatically.

ok, let me get an initial patch done for @catch's suggested direction.

For reference #232327: Deleting node type leaves orphan nodes was the original issue (or close to it) where we introduced the pattern.

This is a rework of #4 which throws an AccessDenied result if the workflow is being used, or if the state is being used.

Next to implement a config import validator.

As we're going back to the original direction I've changed the issue title back too.

I manually tested #62 and it was working as expected, but I discovered this meanwhile:

#2893888: Content moderation state entity field data not removed when the host field data is

Reviews welcome :)

Initial implementation of the config import validation, and a test to go along with it.

Review and feedback really welcome.

+++ b/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
@@ -0,0 +1,72 @@
+        $diff = array_diff_key($target_storage['states'], $source_storage['states']);
+        $entity_id = ConfigEntityStorage::getIDFromConfigName($unprocessed_configuration, $entity_type->getConfigPrefix());
+        /** @var \Drupal\workflows\WorkflowInterface $workflow */
+        $workflow = $this->entityTypeManager->getStorage($entity_type_id)->load($entity_id);
+        $state = $workflow->getState(key($diff));
+        if ($workflow->getTypePlugin()->isWorkflowStateUsed($workflow, $state)) {
+          $importer->logError($this->t('The moderation state @state_label is being used, but is not in the source storage.', ['@state_label' => $state->label()]));
+        }

Won't this check only the first deleted state? Shouldn't we iterate over $diff?

1. +++ b/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
   @@ -0,0 +1,72 @@
   +   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
   +   *   The entity manager.
   

   Minor nitpick: The entity type manager.

2. +++ b/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
   @@ -0,0 +1,72 @@
   +        $importer = $event->getConfigImporter();
   

   This variable is only used twice and it's not instantiating something expensive to compute, so for better code clarity I would skip this assignment and use the method directly in the two places below.

3. +++ b/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
   @@ -0,0 +1,72 @@
   +        $source_storage = $importer->getStorageComparer()
   ...
   +        $target_storage = $importer->getStorageComparer()
   

   Also for code clarity, how about renaming these variables to $original_workflow_config and $workflow_config?

4. +++ b/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
   @@ -0,0 +1,72 @@
   +        $state = $workflow->getState(key($diff));
   +        if ($workflow->getTypePlugin()->isWorkflowStateUsed($workflow, $state)) {
   

   This seems to handle only the case where one state is deleted in the diff. How about when multiple states are deleted?

5. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
   @@ -131,6 +131,31 @@ public function decorateState(StateInterface $state) {
   +  public function isWorkflowUsed(WorkflowInterface $workflow) {
   ...
   +  public function isWorkflowStateUsed(WorkflowInterface $workflow, StateInterface $state) {
   

   As for the question above from @Sam152, how about renaming these to workflowHasData and workflowStateHasData?

   It's just a suggestion though, this needs more opinions.

6. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -205,4 +205,41 @@ public function testModerationFormSetsRevisionAuthor() {
   +   * @see \Drupal\content_moderation\Plugin\WorkflowType\ContentModeration::isWorkflowUsed()
   

   It would be better to use @covers than @see here. Also, for @covers we need to remove the trailing '()' after the method name.

7. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -205,4 +205,41 @@ public function testModerationFormSetsRevisionAuthor() {
   +    $edit_path = sprintf('node/%d/edit', $node->id());
   

   I don't we need to use sprintf() here, let's just concatenate the string in the drupalPostForm() calls.

8. +++ b/core/modules/content_moderation/tests/src/Functional/ModerationFormTest.php
   @@ -205,4 +205,41 @@ public function testModerationFormSetsRevisionAuthor() {
   +  }
    }
   

   Missing an empty line here ;)

9. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowConfigTest.php
   @@ -0,0 +1,125 @@
   +    $this->copyConfig($this->container->get('config.storage'), $this->container->get('config.storage.sync'));
   +
   +  }
   

   And this one needs to go.

Looks like this needs work :)

I think this addresses #66 and #67.

Re: #65.2 - I think we do need to cover deleting the whole workflow.

Needs work on #65.2.

We discussed this issue on a triage call today. @xjm brought up the point that if we use access control to prevent users from deleting Workflows and Workflow states the user will have no idea why they can't delete them. We should show a message to explain, then link to a list of entities which are causing the issue. the problem is there's no list of entities in core per moderation state however we could add this to Content Moderation. #2862041: Provide useful Views filters for Content Moderation State fields would help by providing a filter for this view. Then #2797583: Dynamically provide action plugins for every moderation state change would help to allow users to bulk update the moderation state.

This patch now handles whole workflows deleted during a config import.
It also, in a slight change of direction, shows the delete button for workflows and workflow states, but then displays a message. Exactly the same as deleting content types which are in use.

This still allows deleting of workflows and workflow states via the API, is this something we want to cover (content types don't cover this yet either)? Do we simply throw an exception in the delete() methods, or use entity validation API, can't entity validation be bypassed too?

Manually tested this and the UI change makes total sense to me and is fully consistent with what we do with content types, so +1 for that. Code is looking great, I found only the minor things below.

This still allows deleting of workflows and workflow states via the API, is this something we want to cover (content types don't cover this yet either)? Do we simply throw an exception in the delete() methods, or use entity validation API, can't entity validation be bypassed too?

If we already have an issue outlining a clear way forward for this approach for content types, then I'd say to stick to that, otherwise I think it's fine to ignore that case an be consistent with core.

1. +++ b/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
   @@ -0,0 +1,94 @@
   +   * @param string $config_name
   +   *
   +   * @return \Drupal\Core\Entity\EntityInterface|null
   

   Incomplete PHP docs

2. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationWorkflowConfigTest.php
   @@ -0,0 +1,147 @@
   +    \Drupal::service('config.storage.sync')->delete('workflows.workflow.editorial');
   +
   +
   

   Surplus newline(s) :)

Fixing #73.

Been thinking about preventing deletion via the API, and not sure entity validation would work, as it's possible to save or delete an entity without running validation. The only option might be littering the Workflow entity with exceptions if the workflow or state is in use. So maybe @plach has the best suggestion, to ignore this case.

So maybe @plach has the best suggestion, to ignore this case.

To clarify: probably we'll want to address this in the future, however starting from the same state as content types should allow a generic solution to be applied also in this case.

Tests green

As discussed in the call, let's implement the preDelete() method in the Workflow entity and prevent deleting stuff via the API if they are in use :)

Switching to needs work on #77.

Adding API coverage for deleting states and workflows which are in use.

First test fail was \Drupal\Tests\content_moderation\Kernel\EntityStateChangeValidationTest::testInvalidStateWithoutExisting where we were checking node validation after deleting a moderation state via the API, but now that's not possible, so removed the test.

The second was \Drupal\Tests\workflows\Unit\WorkflowTest::testDeleteState. This unit test didn't expect workflowStateHaseData to be called. It now does.

Unassigned from me as this needs review from others, no work remaining to be done.

Just fixed a couple of trivial nits found while following along

1) In the new ModerationFormTest::testWorkflowInUse()

Unless explicitly testing translation there is no need to invoke t()

2) Fixed a trivial coding standard nit in ConfigImportSubscriber

Thanks @martin107 LGTM.

From an outsider perspective those names are consistent with what is provided by the Entity API: they have a very specific and well understood meaning and that meaning is useful to determine whether workflow/states can be deleted. The two concepts seem independent to me, although related, I don't think we should merge them.

Updating the resolution in the Issue Summary.

Correct if I'm wrong, but if we added an access check we would go back to the situation where the delete drop-button is not displayed in the UI because you dan't have access to the delete route, right? We could definitely add some access checks on top of those methods but they should not be the route access checks for deletion otherwise wouldn't be able to provide the better UX we came up with in the latest iterations.

I believe the latest patch is a good solution for a stable release of Workflows module.

Access control would remove the delete button and cause confusion. The latest patch is also consistent with deleting bundles which are in use in core. Maybe we need to review that too, and maybe we need to review what happens when you delete a field which is in use.

The delete button is removed for required states, so maybe we need a follow up to discuss UX for that.

To allow flexibility we should keep the method names related to what they are doing, checking if the workflow or state has data, it's not strictly checking if it can be deleted.

This is only of the remaining blockers for Workflows becoming stable, if we get a good solution in we can always iterate on it.

I had a look at #94 and it makes sense to me, however it would still be cleaner if the access checks relied on "hasData" methods like the ones implemented in the latest patches.

I think it would be worth moving this back to RTBC and open a related issue to explore your approach. If we can have it ready before this is committed, we can replace the latest patch with yours, otherwise it can be a (non-blocker) follow-up.

Works for me.

I think the "hasData" method are pretty handy, and we can always deprecate them if we find they're not the best direction.

Removing the API-based delete protection basically means we want #74 + the interdiff from #83, so here's a patch for that with some cosmetic fixes for the new test as well.

Regarding the debate whether we want/need the new "has data" public methods or not, I'm still mulling on it and I don't have any opinions or suggestion so far. However, I do agree that #94 looks pretty good.

I created a user with just the "Administer workflows" permission, and I can delete a workflow and a workflow which are in use. As the admin user (user/1) I see the "This workflow state is in use. You cannot remove this workflow state until you have removed all content using it." message.

Investigating further.

Clearing the cache fixed the issue I saw in #99.

Updating the test to make the permissions more specific just to cover any possible issues like #99.

@Sam152, I re-read the entire issue and it seems that @alexpott wanted the generic public methods way back in #4, "so that all workflow types have to think about this problem".

Do you disagree with that part or just with the new method names?

@Sam152:

I think we all agree that would be a sane goal, but we are running out of time: what do you think about my proposal in #95?

After all these comments, we still need to address #9 :) And we can also improve the performance of the query with a ->range(0, 1).

RTBC +1

Works for me! +1

+++ b/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
@@ -0,0 +1,96 @@
+          if ($op == 'update') {
...
+          if ($op == 'delete') {

nit === can be fixed on commit

looks good

updating issue credit to include @plach and @catch who provided reviews that shaped the patch

As this has not been committed yet, here's a patch to fix #109.

Rerolled after #2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface.

RTBC +1

Committed as 832d769 and pushed to 8.4.x.

Great use of the config import events and api.

Calling out in the release notes, since this feels like pretty important new functionality.