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

Switch the entity to one of the required moderation states.

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

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.