Problem/Motivation

Followup to #2779647: Add a workflow component, ui module, and implement it in content moderation.

When a state is deleted an entity continues to use that state causing an error such as Uncaught PHP Exception InvalidArgumentException: "The state 'test' does not exist in workflow 'editorial'" at /home/timmillwood/Code/drupal1/core/modules/workflows/src/Entity/Workflow.php line 205 when trying to edit the entity.

Proposed resolution

- Show an error (and hide the buttons) on the delete confirmation page for states and workflows which are in use.
- Throw an exception if trying to delete a state or workflow via the API methods.
- Set a config validation error if config imports remove states or workflows which are in use.

Remaining tasks

Review.

User interface changes

None

API changes

- It's up to the Workflow plugin to handle what happens.
- The ContentModeration workflow plugin updates the content_moderation_state tables to switch out the moderation state id.

Data model changes

None

CommentFileSizeAuthor
#112 interdiff.txt3.39 KBamateescu
#112 2830740-112.patch17.65 KBamateescu
#110 2830740-110.patch18.03 KBtimmillwood
#110 interidff-2830740-110.txt1.37 KBtimmillwood
#106 interdiff.txt1.12 KBamateescu
#106 2830740-106.patch17.98 KBamateescu
#105 2830740-105.patch17.88 KBSam152
#105 2830740-interdiff.txt1.14 KBSam152
#101 2830740-101.patch17.63 KBtimmillwood
#101 interdiff-2830740-101.txt1.14 KBtimmillwood
#98 interdiff.txt1.94 KBamateescu
#98 2830740-98.patch17.29 KBamateescu
#94 interdiff-workflows-8.4.x.txt6.58 KBSam152
#94 interdiff.txt13.28 KBSam152
#83 interdiff-2830740-81-83.txt2.15 KBmartin107
#83 2830740-83.patch21.31 KBmartin107
#81 2830740-81.patch21.32 KBtimmillwood
#81 interdiff-2830740-81.txt1.69 KBtimmillwood
#79 2830740-79.patch19.63 KBtimmillwood
#79 interdiff-2830740-79.txt2.93 KBtimmillwood
#74 2830740-74.patch17.2 KBtimmillwood
#74 interdiff-2830740-74.txt1.67 KBtimmillwood
#72 2830740-72.patch17.03 KBtimmillwood
#72 interdiff-2830740-72.txt10.54 KBtimmillwood
#69 2830740-69.patch14.91 KBtimmillwood
#69 interdiff-2830740-69.txt11.62 KBtimmillwood
#64 2830740-64.patch14.57 KBtimmillwood
#64 interdiff-2830740-64.txt7.9 KBtimmillwood
#62 2830740-61.patch6.79 KBtimmillwood
#53 2830740-53.patch13.25 KBtimmillwood
#53 interdiff-2830740-53.txt1.17 KBtimmillwood
#38 2830740-38.patch12.93 KBSam152
#38 interdiff.txt11.91 KBSam152
#35 Screenshot from 2017-06-20 14-46-51.png28.31 KBtimmillwood
#35 2830740-35.patch8.43 KBtimmillwood
#35 interdiff-2830740-35.txt1.42 KBtimmillwood
#32 2830740-32.patch7.01 KBtimmillwood
#32 interdiff-2830740-32.txt729 bytestimmillwood
#31 2830740-31.patch7.73 KBtimmillwood
#31 interdiff-2830740-31.txt6.94 KBtimmillwood
#30 2830740-30.patch7.1 KBtimmillwood
#30 interdiff-2830740-30.txt1.81 KBtimmillwood
#28 2830740-28.patch7.23 KBtimmillwood
#28 interdiff-2830740-28.txt3.29 KBtimmillwood
#26 interdiff-2830740.txt3.79 KBtimmillwood
#25 2830740-25.patch5.07 KBtimmillwood
#23 2830740-23.patch3.86 KBtimmillwood
#4 2830740-4.patch10.51 KBalexpott
#4 2-4-interdiff.txt11.04 KBalexpott
#2 2830740-2.patch1014 bytesalexpott
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Sam152 created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1014 bytes

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

timmillwood’s picture

Issue tags: +Workflow Initiative
alexpott’s picture

Issue tags: -Needs tests
FileSize
11.04 KB
10.51 KB

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

alexpott’s picture

Issue summary: View changes

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.

alexpott’s picture

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

@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 :(

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

catch’s picture

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.

alexpott’s picture

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

alexpott’s picture

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.

catch’s picture

Could go for that as well yeah.

timmillwood’s picture

Sam152’s picture

What about doing what the image style system does and implementing a method like \Drupal\image\ImageStyleStorageInterface::setReplacementId? That way the UI asks you which state replaces the one you've deleted?

Sam152’s picture

Note, I think closing this issue resolves a @todo in \Drupal\content_moderation\Plugin\Validation\Constraint\ModerationStateConstraintValidator::validate.

alexpott’s picture

@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?

alexpott’s picture

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

Sam152’s picture

Good point re: #18, image styles got away with it because they were updating configuration objects that depended on styles that were exported when the style was deleted. Since this updates content, the replacement would have to be stored in config also.

Sam152’s picture

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

timmillwood’s picture

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.

timmillwood’s picture

FileSize
3.86 KB

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.

timmillwood’s picture

Status: Needs work » Needs review
timmillwood’s picture

FileSize
5.07 KB

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)

timmillwood’s picture

FileSize
3.79 KB

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

timmillwood’s picture

Title: Allow workflow types to lock certain changes to workflows once things are in use » Revert to one of the required states when another state is deleted
Issue summary: View changes

Updating issue summary to reflect the new direction.

timmillwood’s picture

FileSize
3.29 KB
7.23 KB

Switching to injecting the database.

Sam152’s picture

Love it when a plan comes together :)

Review as follows.

  1. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -129,6 +139,25 @@ public function decorateState(StateInterface $state) {
    +   * @inheritDoc
    

    nit, {@inheritdoc}

  2. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -129,6 +139,25 @@ public function decorateState(StateInterface $state) {
    +  public function deleteState($state_id) {
    

    I wonder if this method covers deploying config between environments. I suspect it's not called during a config import, thus we'd probably need to listen to entity_update too.

  3. +++ b/core/modules/content_moderation/src/Plugin/WorkflowType/ContentModeration.php
    @@ -129,6 +139,25 @@ public function decorateState(StateInterface $state) {
    +    $this->database
    +      ->update($content_moderation_state_type->getDataTable())
    +      ->condition('moderation_state', $state_id)
    +      ->fields(['moderation_state' => $new_state])
    +      ->execute();
    +    $this->database
    +      ->update($content_moderation_state_type->getRevisionDataTable())
    +      ->condition('moderation_state', $state_id)
    +      ->fields(['moderation_state' => $new_state])
    +      ->execute();
    

    It kind of sucks we have to do this low level fiddling, we're essentially bypassing entity api rewriting history, but of course when integrity it at stake in the first place, I don't think there is an alternative approach which makes more sense though.

    Might be good to get some more input on this though.

  4. +++ b/core/modules/content_moderation/tests/src/Kernel/DeleteWorkflowStateTest.php
    @@ -0,0 +1,121 @@
    +    $node = Node::create([
    +      'type' => 'example',
    +      'title' => 'Test title',
    +    ]);
    +    $node->setUnpublished()->save();
    +    $this->assertEquals('draft', $node->moderation_state->value);
    +    $this->assertFalse($node->isPublished());
    +
    +    $node->moderation_state->value = 'test';
    +    $node->save();
    +    $this->assertEquals('test', $node->moderation_state->value);
    +    $this->assertTrue($node->isPublished());
    

    Is it relevant to this feature that the content started out as 'draft'? Possibly to just go 'moderation_state' => 'test' in the factory?

  5. +++ b/core/modules/content_moderation/tests/src/Kernel/DeleteWorkflowStateTest.php
    @@ -0,0 +1,121 @@
    +    $node2 = Node::create([
    +      'type' => 'example',
    +      'title' => 'Test title 2',
    +    ]);
    +    $node2->setUnpublished()->save();
    +    $this->assertEquals('draft', $node2->moderation_state->value);
    +    $this->assertFalse($node2->isPublished());
    +
    +    $node2->moderation_state->value = 'unpub_test';
    +    $node2->save();
    

    Same here? Doesn't 'moderation_state' => 'unpub_test' work?

timmillwood’s picture

FileSize
1.81 KB
7.1 KB

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.

timmillwood’s picture

FileSize
6.94 KB
7.73 KB

Moving things to entity_update as suggested in #29.2

timmillwood’s picture

FileSize
729 bytes
7.01 KB

Should've removed the use statement.

pauloamgomes’s picture

@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."

Manuel Garcia’s picture

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?

timmillwood’s picture

Updating the message shown on the confirmation page.

Manuel Garcia’s picture

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

Sam152’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/content_moderation/src/EntityOperations.php
    @@ -145,6 +157,25 @@ public function entityUpdate(EntityInterface $entity) {
    +    if ($entity->getEntityTypeId() == 'workflow') {
    +      $deleted_states = array_diff_key($entity->original->getStates(), $entity->getStates());
    +      /** @var \Drupal\content_moderation\ContentModerationState $deleted_state */
    +      foreach ($deleted_states as $deleted_state) {
    +        $new_state = $deleted_state->isPublishedState() ? 'published' : 'draft';
    +        $content_moderation_state_type = $this->entityTypeManager->getDefinition('content_moderation_state');
    +        $this->database
    +          ->update($content_moderation_state_type->getDataTable())
    +          ->condition('moderation_state', $deleted_state->id())
    +          ->fields(['moderation_state' => $new_state])
    +          ->execute();
    +        $this->database
    +          ->update($content_moderation_state_type->getRevisionDataTable())
    +          ->condition('moderation_state', $deleted_state->id())
    +          ->fields(['moderation_state' => $new_state])
    +          ->execute();
    +      }
    +    }
    

    I wonder if we can test that is is necessary this by using $workflow->set('states', [...]) to simulate a config import, without calling deleteState?

    I'm sure someone would think to open a follow up to move this to the more logical (but incorrect) deleteState method.

  2. +++ b/core/modules/content_moderation/src/EntityOperations.php
    index 8f6ac4d79e..a199f6fd1c 100644
    --- a/core/modules/content_moderation/src/EntityTypeInfo.php
    
    +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -292,6 +292,27 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
    +    if ($form_id == 'workflow_state_delete_form') {
    +      $args = $form_state->getBuildInfo()['args'];
    +      /** @var \Drupal\workflows\WorkflowInterface $workflow */
    +      $workflow = $args[0];
    +      $state = $args[1];
    +      $count = $this->entityTypeManager
    +        ->getStorage('content_moderation_state')
    +        ->getQuery()
    +        ->count()
    +        ->condition('moderation_state', $state)
    +        ->execute();
    +      if ($count > 0) {
    +        $state = $workflow->getState($state);
    +        $new_state = $state->isPublishedState() ? $workflow->getState('published') : $workflow->getState('draft');
    +        $form['description']['#markup'] = $this->t('This moderation state is assigned to @count. Deleting it will revert these to @new_state.', [
    +          '@count' => $this->stringTranslation->formatPlural($count, '1 item', '@count items'),
    +          '@new_state' => $new_state->label()
    +        ]);
    +      }
    +    }
    

    This will affect all workflows when content_moderation is enabled, not just workflows of type content_moderation.

    Would also be good to test this.

Sam152’s picture

Updated patch:

  • Ensures the functionality only affects workflows of type content moderation.
  • Tests why we need to do this in a hook_entity_update vs deleteState.
  • Adds a test for the state delete warning in the UI.
Sam152’s picture

Status: Needs work » Needs review
timmillwood’s picture

Looks awesome, thanks @Sam152.

Manuel Garcia’s picture

Status: Needs review » Reviewed & tested by the community

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

plach’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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?

timmillwood’s picture

@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.

plach’s picture

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?

plach’s picture

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?

jibran’s picture

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.

timmillwood’s picture

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.

plach’s picture

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

Sam152’s picture

Status: Needs review » Needs work

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

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. This approach has been in the works since #2844594: Default workflow states and transitions.

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

ContentModerationState is kinda an internal entity type that no one should be interacting with

@timmillwood is right here, I feel like we have some liberty with this entity type. If we need to clear the entity cache, lets do that, but we do not need to hold this entity type to the same standards as node in terms of integration and api-completeness. It's a storage mechanism for a computed field, slated to be @internal, that nobody should really have to every think about/interact with directly (see #2876419: Review content_moderation module and mark code with @internal where necessary).

Personally, I think this should only be NW for clearing the entity cache after the update is done.

timmillwood’s picture

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.

plach’s picture

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

Sam152’s picture

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
13.25 KB

Adding a comment and a cache reset.

catch’s picture

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.

Sam152’s picture

Ah okay, I haven't seen #54 implemented before. Is the only cost that you potentially create config you can't import? We don't have tools for bulk changing states, if we went down the route of making the site administrator update the content.

plach’s picture

Priority: Normal » Major
Issue tags: +WI critical

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

catch’s picture

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.

Sam152’s picture

It'd be great to keep all the config importable at all times anyway IMO, even if we add tooks for bulk resolving the issue. The closest thing to that is #2797583: Actions for content_moderation.

The interdiff looks good to me, but will let others comment #53 if we're happy with that solution.

plach’s picture

Status: Needs review » Needs work
Related issues: +#2797583: Actions for content_moderation

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: Actions for content_moderation 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.

timmillwood’s picture

Assigned: Unassigned » timmillwood

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

catch’s picture

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

timmillwood’s picture

Title: Revert to one of the required states when another state is deleted » Allow workflow types to lock certain changes to workflows once things are in use
Status: Needs work » Needs review
FileSize
6.79 KB

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.

plach’s picture

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 :)

timmillwood’s picture

FileSize
7.9 KB
14.57 KB

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

Review and feedback really welcome.

Sam152’s picture

  1. +++ b/core/modules/content_moderation/content_moderation.services.yml
    @@ -19,4 +19,9 @@ services:
         tags:
    -     - { name: backend_overridable }
    

    Unrelated change, but looks correct so +1.

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

    Do we have to cater to the whole workflow being deleted here as well?

  3. +++ b/core/modules/content_moderation/src/EventSubscriber/ConfigImportSubscriber.php
    @@ -0,0 +1,72 @@
    +        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()]));
    +        }
    

    Should we mention which workflow here, or do we get that for free?

  4. +++ 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) {
    

    I wonder if "used" is the right verb. You can probably have states and workflows people would consider being "used" but are completely safe to delete. The fact that a state is in active use and can't be deleted seems kinda content-moderation specific.

  5. +++ b/core/modules/workflows/src/WorkflowAccessControlHandler.php
    @@ -65,11 +65,15 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
    +        ->andIf(AccessResult::allowedIf(!$workflow_type->isWorkflowStateUsed($entity, $entity->getState($state_id))))
    ...
    +      $admin_access = $admin_access->orIf(AccessResult::forbiddenIf($workflow_type->isWorkflowUsed($entity)))->setCacheMaxAge(0);
    

    Do we need the max age on both access checks?

plach’s picture

+++ 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?

amateescu’s picture

  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.

plach’s picture

Status: Needs review » Needs work

Looks like this needs work :)

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.62 KB
14.91 KB

I think this addresses #66 and #67.

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

timmillwood’s picture

Status: Needs review » Needs work

Needs work on #65.2.

timmillwood’s picture

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: Actions for content_moderation would help to allow users to bulk update the moderation state.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.54 KB
17.03 KB

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?

plach’s picture

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) :)

timmillwood’s picture

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.

plach’s picture

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.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Tests green

amateescu’s picture

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 :)

timmillwood’s picture

Status: Reviewed & tested by the community » Needs work

Switching to needs work on #77.

timmillwood’s picture

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

Status: Needs review » Needs work

The last submitted patch, 79: 2830740-79.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
21.32 KB

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.

timmillwood’s picture

Assigned: timmillwood » Unassigned

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

martin107’s picture

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

timmillwood’s picture

Thanks @martin107 LGTM.

Sam152’s picture

Sam152’s picture

+++ b/core/modules/workflows/src/Entity/Workflow.php
@@ -265,6 +269,18 @@ public function deleteState($state_id) {
+      if ($entity->getTypePlugin()->workflowHasData($entity)) {
+        throw new EntityStorageException("This '{$entity->label()}' workflow is in use. You cannot remove this workflow until you have removed all content using it.");
+      }

+++ b/core/modules/workflows/src/Form/WorkflowDeleteForm.php
@@ -14,6 +14,19 @@ class WorkflowDeleteForm extends EntityConfirmFormBase {
+    if ($this->entity->getTypePlugin()->workflowHasData($this->entity)) {
+      $form['#title'] = $this->getQuestion();
+      $form['description'] = ['#markup' => $this->t('This workflow is in use. You cannot remove this workflow until you have removed all content using it.')];
+      return $form;
+    }

+++ b/core/modules/workflows/src/Form/WorkflowStateDeleteForm.php
@@ -75,6 +75,13 @@ public function buildForm(array $form, FormStateInterface $form_state, WorkflowI
+    if ($this->workflow->getTypePlugin()->workflowStateHasData($this->workflow, $this->workflow->getState($this->stateId))) {
+      $form['#title'] = $this->getQuestion();
+      $form['description'] = ['#markup' => $this->t('This workflow state is in use. You cannot remove this workflow state until you have removed all content using it.')];
+      return $form;
+    }

+++ b/core/modules/workflows/src/WorkflowTypeInterface.php
@@ -57,6 +57,30 @@ public function label();
   /**
+   * Determines if the workflow is being has data associated with it.
+   *
+   * @param \Drupal\workflows\WorkflowInterface $workflow
+   *   The workflow to check.
+   *
+   * @return bool
+   *   TRUE if the workflow is being used, FALSE if not.
+   */
+  public function workflowHasData(WorkflowInterface $workflow);
+
+  /**
+   * Determines if the workflow state has data associated with it.
+   *
+   * @param \Drupal\workflows\WorkflowInterface $workflow
+   *   The workflow to check.
+   * @param \Drupal\workflows\StateInterface $state
+   *   The workflow state to check.
+   *
+   * @return bool
+   *   TRUE if the workflow state is being used, FALSE if not.
+   */
+  public function workflowStateHasData(WorkflowInterface $workflow, StateInterface $state);

I really wonder if these methods names are describing their behaviour clearly enough. Their functional purpose is checking if a state/workflow can be deleted, which sounds very familiar to the access control handler. Is there a reason instead of creating two new state/workflow methods concerned with "can I delete this thing", that we can't leverage an existing method like \Drupal\workflows\Plugin\WorkflowTypeBase::checkWorkflowAccess?

This will help protect the states/workflow from the UI, then I think content moderation could hook in separately to provide the api level protection in preDelete, much like it does with the config validator.

Possibly related #2896726: Review the entity access model for workflow states and transitions..

Sam152’s picture

Come to think of it, access results even have a "reason" field. We might be able to bubble those up to the UI, so the user gets the same experience of knowing why a state can't be removed.

plach’s picture

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.

timmillwood’s picture

Issue summary: View changes

Updating the resolution in the Issue Summary.

Sam152’s picture

I think my primary hesitation is, when you diff the patch in the workflows module, it's mainly adding checks and protection in the UI. Technically REST is another UI which we aren't providing the same nice messages for, we should then probably then duplicate the same workflowHasData() checks in the access controller, which leads me back to, why isn't it just check there in the first place?

plach’s picture

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.

Sam152’s picture

We already have the pattern of denying access to delete states by removing the button, for "required states" of a workflow type (draft and published for content_moderation).

Do you agree this will have to go in the access control handler for proper REST support? After that it'll take extra work to allow users to access the delete confirm route, so unless there is a really good reason we probably shouldn't introduce the pattern in the first place.

If we do decide to do this in the access check instead, I filed #2896881: Workflow type plugins have no chance to act during access checks if a user is an admin to make it possible.

timmillwood’s picture

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.

Sam152’s picture

FWIW, if it's only the message on the delete confirm form I've prototyped this (see interdiff.txt). Seems pretty easy and the ContentModeration workflow type can provide a much more specific error message about how to actually resolve the problem.

Based on this, it reduces the changes between workflows in HEAD vs this patch to the contents of interdiff-workflows-8.4.x.txt. Instead of adding 2 methods to the plugin interface, we can actually delete a custom route access controller. On top of this, REST support is now built in. We also use this same mechanism for protecting the deletion of our "required" states (publish/draft) which are also essential for the moderation workflow to work.

As @plach mentioned in #88, if these really are two totally different concepts, happy to drop this and help review/RTBC it, I can see heaps of good work has been put in so far. If anyone can see merit in this approach, I'm happy to work on it until it has full parity with what was proposed in #83.

Not meaning to be a stick in the mud here either, sorry if this is seen as needless bikeshedding, I am just conscious of the amount of methods we're adding to the workflow type plugin interface.

plach’s picture

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.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

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.

Sam152’s picture

We agreed to remove the api-based delete protection from the latest patch, so lets at least get that done. It's inconsistent with how other critical items of config are handled and also doesn't cover all the ways to delete a state (you could simply use Workflow::set or the config api, deleteState doesn't have to be called).

Other than that, I'm fine with moving this to an access check going in a follow up.

How would you feel about both methods becoming protected in the follow-up? A bit like how \Drupal\entity\EntityAccessControlHandler::checkAccess is used in core. That way we have a single way to check if something can be deleted and you don't have to know about both methods before deciding if a state can be deleted.

amateescu’s picture

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.

timmillwood’s picture

Assigned: Unassigned » timmillwood
Status: Reviewed & tested by the community » Needs work

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.

Sam152’s picture

I've filed #2897148: Explore if it makes sense to use the access system for disallowing users to delete "locked" states for the access based follow-up. I think once we can get consensus on #97, that will be ready to start.

timmillwood’s picture

Assigned: timmillwood » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
1.14 KB
17.63 KB

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.

amateescu’s picture

@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’s picture

The patch in question uses the access system to block states/workflows from being used. The main part of my proposal is not calling workflowHasData/workflowStateHasData() directly to make decisions at the UI level about if a user can delete a workflow or state, this seems like a job for the access controller. While I think it'd be nice to make protected, so it isn't part of the public API, I have no issue with the method names.

+++ b/core/modules/workflows/src/WorkflowAccessControlHandler.php
@@ -66,6 +66,10 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
+    $type = $entity->getTypePlugin();
+    if ($operation === 'delete' || $operation === 'delete-state') {
+      $admin_access = $admin_access->orIf(AccessResult::forbiddenIf($type->isWorkflowUsed($entity)))->setCacheMaxAge(0);
+    }
plach’s picture

@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?

Sam152’s picture

Making these @internal. I think in the follow-up we can either decide to remove the tag, thus making them part of the public API, or they can be safely removed or be made protected if not.

Happy with this approach, lets get this fixed!

amateescu’s picture

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).

plach’s picture

RTBC +1

timmillwood’s picture

Works for me! +1

larowlan’s picture

+++ 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

timmillwood’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 110: 2830740-110.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
17.65 KB
3.39 KB
plach’s picture

RTBC +1

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 832d769 and pushed to 8.4.x.

Great use of the config import events and api.

  • larowlan committed 832d769 on 8.4.x
    Issue #2830740 by timmillwood, amateescu, Sam152, alexpott, martin107,...
webchick’s picture

Issue tags: +8.4.0 release notes

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.