diff --git a/core/lib/Drupal/Core/Workflow/Entity/Workflow.php b/core/lib/Drupal/Core/Workflow/Entity/Workflow.php index b84af09..b57751d 100644 --- a/core/lib/Drupal/Core/Workflow/Entity/Workflow.php +++ b/core/lib/Drupal/Core/Workflow/Entity/Workflow.php @@ -245,10 +245,6 @@ public function addTransition($transition_id, $label, array $from_state_ids, $to if (preg_match('/[^a-z0-9_]+/', $transition_id)) { throw new \InvalidArgumentException("The transition ID '$transition_id' must contain only lowercase letters, numbers, and underscores"); } - // @todo what to about duplicates - I don't think this needs to be in the - // API. - // if (!$this->hasTransitionFromStateToState($from_state_id, $to_state_id)) { - // } if (!$this->hasState($to_state_id)) { throw new \InvalidArgumentException("The state '$to_state_id' does not exist in workflow '{$this->id()}'"); @@ -258,7 +254,13 @@ public function addTransition($transition_id, $label, array $from_state_ids, $to 'label' => $label, ]; - $this->setTransitionFromStates($transition_id, $from_state_ids); + try { + $this->setTransitionFromStates($transition_id, $from_state_ids); + } + catch (\InvalidArgumentException $e) { + unset($this->transitions[$transition_id]); + throw $e; + } return $this; } @@ -330,36 +332,39 @@ public function getTransitionsForState($state_id, $direction = 'from') { /** * {@inheritdoc} */ - public function getTransitionsFromStateToState($from_state_id, $to_state_id) { - $transition_ids = $this->getTransitionIdsFromStateToState($from_state_id, $to_state_id); - if (empty($transition_ids)) { + public function getTransitionFromStateToState($from_state_id, $to_state_id) { + $transition_id = $this->getTransitionIdFromStateToState($from_state_id, $to_state_id); + if (empty($transition_id)) { throw new \InvalidArgumentException("The transition from '$from_state_id' to '$to_state_id' does not exist in workflow '{$this->id()}'"); } - return $this->getTransitions($transition_ids); + return $this->getTransition($transition_id); } /** * {@inheritdoc} */ public function hasTransitionFromStateToState($from_state_id, $to_state_id) { - return !empty($this->getTransitionIdsFromStateToState($from_state_id, $to_state_id)); + return !empty($this->getTransitionIdFromStateToState($from_state_id, $to_state_id)); } /** - * Gets the possible transition IDs from state to state. + * Gets the transition ID from state to state. * * @param string $from_state_id * The state ID to transition from. * @param string $to_state_id * The state ID to transition to. * - * @return string[] - * The transition IDs. + * @return string|null + * The transition ID, or NULL if no transition exists. */ - protected function getTransitionIdsFromStateToState($from_state_id, $to_state_id) { - return array_keys(array_filter($this->transitions, function ($transition) use ($from_state_id, $to_state_id) { - return in_array($from_state_id, $transition['from'], TRUE) && $transition['to'] === $to_state_id; - })); + protected function getTransitionIdFromStateToState($from_state_id, $to_state_id) { + foreach ($this->transitions as $transition_id => $transition) { + if (!empty($transition['from']) && in_array($from_state_id, $transition['from'], TRUE) && $transition['to'] === $to_state_id) { + return $transition_id; + } + } + return NULL; } /** @@ -388,6 +393,12 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) { if (!$this->hasState($from_state_id)) { throw new \InvalidArgumentException("The state '$from_state_id' does not exist in workflow '{$this->id()}'"); } + if ($this->hasTransitionFromStateToState($from_state_id, $this->transitions[$transition_id]['to'])) { + $transition = $this->getTransitionFromStateToState($from_state_id, $this->transitions[$transition_id]['to']); + if ($transition_id !== $transition->id()) { + throw new \InvalidArgumentException("The '{$transition->id()}' transition already allows '$from_state_id' to '{$this->transitions[$transition_id]['to']}' transitions in workflow '{$this->id()}'"); + } + } } // Preserve the order of the state IDs in the from value and don't save any diff --git a/core/lib/Drupal/Core/Workflow/State.php b/core/lib/Drupal/Core/Workflow/State.php index 3a8b6ae..386e52e 100644 --- a/core/lib/Drupal/Core/Workflow/State.php +++ b/core/lib/Drupal/Core/Workflow/State.php @@ -89,11 +89,11 @@ public function canTransitionTo($to_state_id) { /** * {@inheritdoc} */ - public function getTransitionsTo($to_state_id) { + public function getTransitionTo($to_state_id) { if (!$this->canTransitionTo($to_state_id)) { throw new \InvalidArgumentException("Can not transition to '$to_state_id' state"); } - return $this->workflow->getTransitionsFromStateToState($this->id(), $to_state_id); + return $this->workflow->getTransitionFromStateToState($this->id(), $to_state_id); } /** diff --git a/core/lib/Drupal/Core/Workflow/StateInterface.php b/core/lib/Drupal/Core/Workflow/StateInterface.php index 35463f4..524f94b 100644 --- a/core/lib/Drupal/Core/Workflow/StateInterface.php +++ b/core/lib/Drupal/Core/Workflow/StateInterface.php @@ -49,18 +49,18 @@ public function weight(); public function canTransitionTo($to_state_id); /** - * Gets the Transition objects for the provided state ID. + * Gets the Transition object for the provided state ID. * * @param $to_state_id * The state to transition to. * - * @return \Drupal\Core\Workflow\TransitionInterface[] - * The Transition objects for the provided state ID. + * @return \Drupal\Core\Workflow\TransitionInterface + * The Transition object for the provided state ID. * * @throws \InvalidArgumentException() * Exception thrown when the provided state ID can not be transitioned to. */ - public function getTransitionsTo($to_state_id); + public function getTransitionTo($to_state_id); /** * Gets all the possible transition objects for the state. diff --git a/core/lib/Drupal/Core/Workflow/WorkflowInterface.php b/core/lib/Drupal/Core/Workflow/WorkflowInterface.php index 9ec731a..e42baf5 100644 --- a/core/lib/Drupal/Core/Workflow/WorkflowInterface.php +++ b/core/lib/Drupal/Core/Workflow/WorkflowInterface.php @@ -195,13 +195,13 @@ public function getTransitionsForState($state_id, $direction = 'from'); * @param string $to_state_id * The state ID to transition to. * - * @return \Drupal\Core\Workflow\TransitionInterface[] - * The possible transitions. + * @return \Drupal\Core\Workflow\TransitionInterface + * The transitions. * * @throws \InvalidArgumentException * Thrown if the transition does not exist. */ - public function getTransitionsFromStateToState($from_state_id, $to_state_id); + public function getTransitionFromStateToState($from_state_id, $to_state_id); /** * Determines if a transition from state to state exists. diff --git a/core/modules/content_moderation/src/ContentModerationState.php b/core/modules/content_moderation/src/ContentModerationState.php index 6a1d686..265a01c 100644 --- a/core/modules/content_moderation/src/ContentModerationState.php +++ b/core/modules/content_moderation/src/ContentModerationState.php @@ -100,8 +100,8 @@ public function canTransitionTo($to_state_id) { /** * {@inheritdoc} */ - public function getTransitionsTo($to_state_id) { - return $this->state->getTransitionsTo($to_state_id); + public function getTransitionTo($to_state_id) { + return $this->state->getTransitionTo($to_state_id); } /** diff --git a/core/modules/system/tests/modules/workflow_type_test/src/DecoratedState.php b/core/modules/system/tests/modules/workflow_type_test/src/DecoratedState.php index 0e6b567..dfd1169 100644 --- a/core/modules/system/tests/modules/workflow_type_test/src/DecoratedState.php +++ b/core/modules/system/tests/modules/workflow_type_test/src/DecoratedState.php @@ -76,8 +76,8 @@ public function canTransitionTo($to_state_id) { /** * {@inheritdoc} */ - public function getTransitionsTo($to_state_id) { - return $this->state->getTransitionsTo($to_state_id); + public function getTransitionTo($to_state_id) { + return $this->state->getTransitionTo($to_state_id); } /** diff --git a/core/modules/workflow_ui/src/Form/WorkflowTransitionAddForm.php b/core/modules/workflow_ui/src/Form/WorkflowTransitionAddForm.php index be495f9..a33608c 100644 --- a/core/modules/workflow_ui/src/Form/WorkflowTransitionAddForm.php +++ b/core/modules/workflow_ui/src/Form/WorkflowTransitionAddForm.php @@ -110,16 +110,16 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form, */ public function validateForm(array &$form, FormStateInterface $form_state) { /** @var \Drupal\Core\Workflow\WorkflowInterface $workflow */ - // @todo should we only permit single transitions through the UI? - //$workflow = $this->getEntity(); - //$values = $form_state->getValues(); - // If the form is for a new transition it should not exist. - // if ($workflow->hasTransitionFromStateToState($values['from'], $values['to'])) { - // $form_state->setErrorByName('from', $this->t('The transition from %from to %to already exists.', [ - // '%from' => $workflow->getState($values['from'])->label(), - // '%to' => $workflow->getState($values['to'])->label(), - // ])); - //} + $workflow = $this->getEntity(); + $values = $form_state->getValues(); + foreach (array_filter($values['from']) as $from_state_id) { + if ($workflow->hasTransitionFromStateToState($from_state_id, $values['to'])) { + $form_state->setErrorByName('from][' . $from_state_id, $this->t('The transition from %from to %to already exists.', [ + '%from' => $workflow->getState($from_state_id)->label(), + '%to' => $workflow->getState($values['to'])->label(), + ])); + } + } } /** diff --git a/core/modules/workflow_ui/src/Form/WorkflowTransitionDeleteForm.php b/core/modules/workflow_ui/src/Form/WorkflowTransitionDeleteForm.php index b007fa3..f49f41f 100644 --- a/core/modules/workflow_ui/src/Form/WorkflowTransitionDeleteForm.php +++ b/core/modules/workflow_ui/src/Form/WorkflowTransitionDeleteForm.php @@ -44,7 +44,7 @@ public function getFormId() { * {@inheritdoc} */ public function getQuestion() { - return $this->t('Are you sure you want to delete the %transition transition from %workflow?', ['%transition' => $this->transition->label(), '%workflow' => $this->workflow->label()]); + return $this->t('Are you sure you want to delete %transition from %workflow?', ['%transition' => $this->transition->label(), '%workflow' => $this->workflow->label()]); } /** diff --git a/core/modules/workflow_ui/src/Form/WorkflowTransitionEditForm.php b/core/modules/workflow_ui/src/Form/WorkflowTransitionEditForm.php index ed92497..313bda7 100644 --- a/core/modules/workflow_ui/src/Form/WorkflowTransitionEditForm.php +++ b/core/modules/workflow_ui/src/Form/WorkflowTransitionEditForm.php @@ -79,6 +79,26 @@ public function form(array $form, FormStateInterface $form_state) { } /** + * {@inheritdoc} + */ + public function validateForm(array &$form, FormStateInterface $form_state) { + /** @var \Drupal\Core\Workflow\WorkflowInterface $workflow */ + $workflow = $this->getEntity(); + $values = $form_state->getValues(); + foreach (array_filter($values['from']) as $from_state_id) { + if ($workflow->hasTransitionFromStateToState($from_state_id, $values['to'])) { + $transition = $workflow->getTransitionFromStateToState($from_state_id, $values['to']); + if ($transition->id() !== $values['id']) { + $form_state->setErrorByName('from][' . $from_state_id, $this->t('The transition from %from to %to already exists.', [ + '%from' => $workflow->getState($from_state_id)->label(), + '%to' => $workflow->getState($values['to'])->label(), + ])); + } + } + } + } + + /** * Copies top-level form values to entity properties * * This form can only change values for a state, which is part of workflow. diff --git a/core/modules/workflow_ui/tests/src/Functional/WorkflowUiTest.php b/core/modules/workflow_ui/tests/src/Functional/WorkflowUiTest.php index 2db9b64..5f3c1c4 100644 --- a/core/modules/workflow_ui/tests/src/Functional/WorkflowUiTest.php +++ b/core/modules/workflow_ui/tests/src/Functional/WorkflowUiTest.php @@ -123,11 +123,30 @@ public function testWorkflowCreation() { $this->clickLink('Add a new transition'); $this->submitForm(['id' => 'create_new_draft', 'label' => 'Create new draft', 'from[published]' => 'published', 'to' => 'draft'], 'Save'); $this->assertSession()->pageTextContains('The machine-readable name is already in use. It must be unique.'); + // Try creating a transition which duplicates the states of another. + $this->submitForm(['id' => 'create_new_draft2', 'label' => 'Create new draft again', 'from[published]' => 'published', 'to' => 'draft'], 'Save'); + $this->assertSession()->pageTextContains('The transition from Live to Draft already exists.'); + + // Create a new transition. + $this->submitForm(['id' => 'save_and_publish', 'label' => 'Save and publish', 'from[published]' => 'published', 'to' => 'published'], 'Save'); + $this->assertSession()->pageTextContains('Created Save and publish transition.'); + // Edit the new transition and try to add an existing transition. + $this->clickLink('Edit', 3); + $this->submitForm(['from[draft]' => 'draft'], 'Save'); + $this->assertSession()->pageTextContains('The transition from Draft to Live already exists.'); + + // Delete the transition. + $workflow = $workflow_storage->loadUnchanged('test'); + $this->assertTrue($workflow->hasTransitionFromStateToState('published', 'published'), 'Can transition from published to published'); + $this->clickLink('Delete'); + $this->assertSession()->pageTextContains('Are you sure you want to delete Save and publish from Test?'); + $this->submitForm([], 'Delete'); + $workflow = $workflow_storage->loadUnchanged('test'); + $this->assertFalse($workflow->hasTransitionFromStateToState('published', 'published'), 'Cannot transition from published to published'); // Try creating a duplicate state. $this->drupalGet('admin/config/workflow/workflows/manage/test'); $this->clickLink('Add a new state'); - // Don't create a draft to draft transition by default. $this->submitForm(['label' => 'Draft', 'id' => 'draft'], 'Save'); $this->assertSession()->pageTextContains('The machine-readable name is already in use. It must be unique.'); diff --git a/core/tests/Drupal/Tests/Core/Workflow/StateTest.php b/core/tests/Drupal/Tests/Core/Workflow/StateTest.php index 218f809..f2509ed 100644 --- a/core/tests/Drupal/Tests/Core/Workflow/StateTest.php +++ b/core/tests/Drupal/Tests/Core/Workflow/StateTest.php @@ -73,28 +73,28 @@ public function testCanTransitionTo() { } /** - * @covers ::getTransitionsTo + * @covers ::getTransitionTo */ - public function testGetTransitionsTo() { + public function testGetTransitionTo() { $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow'); $workflow ->addState('draft', 'Draft') ->addState('published', 'Published') ->addTransition('publish', 'Publish', ['draft'], 'published'); $state = $workflow->getState('draft'); - $transition = $state->getTransitionsTo('published'); - $this->assertEquals('Publish', $transition['publish']->label()); + $transition = $state->getTransitionTo('published'); + $this->assertEquals('Publish', $transition->label()); } /** - * @covers ::getTransitionsTo + * @covers ::getTransitionTo */ - public function testGetTransitionsToException() { + public function testGetTransitionToException() { $this->setExpectedException(\InvalidArgumentException::class, "Can not transition to 'published' state"); $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow'); $workflow->addState('draft', 'Draft'); $state = $workflow->getState('draft'); - $state->getTransitionsTo('published'); + $state->getTransitionTo('published'); } /** diff --git a/core/tests/Drupal/Tests/Core/Workflow/TransitionTest.php b/core/tests/Drupal/Tests/Core/Workflow/TransitionTest.php index 8225d55..b55ce0f 100644 --- a/core/tests/Drupal/Tests/Core/Workflow/TransitionTest.php +++ b/core/tests/Drupal/Tests/Core/Workflow/TransitionTest.php @@ -63,7 +63,7 @@ public function testFromAndTo() { ->addState('published', 'Published') ->addTransition('publish', 'Publish', ['draft'], 'published'); $state = $workflow->getState('draft'); - $transition = $state->getTransitionsTo('published')['publish']; + $transition = $state->getTransitionTo('published'); $this->assertEquals($state, $transition->from()['draft']); $this->assertEquals($workflow->getState('published'), $transition->to()); } diff --git a/core/tests/Drupal/Tests/Core/Workflow/WorkflowTest.php b/core/tests/Drupal/Tests/Core/Workflow/WorkflowTest.php index dc1f9b0..aeb592d 100644 --- a/core/tests/Drupal/Tests/Core/Workflow/WorkflowTest.php +++ b/core/tests/Drupal/Tests/Core/Workflow/WorkflowTest.php @@ -144,7 +144,7 @@ public function testGetState() { $this->assertTrue($draft->canTransitionTo('draft')); $this->assertTrue($draft->canTransitionTo('published')); $this->assertFalse($draft->canTransitionTo('archived')); - $this->assertEquals('Publish', $draft->getTransitionsTo('published')['publish']->label()); + $this->assertEquals('Publish', $draft->getTransitionTo('published')->label()); $this->assertEquals(0, $draft->weight()); $this->assertEquals(1, $workflow->getState('published')->weight()); $this->assertEquals(2, $workflow->getState('archived')->weight()); @@ -313,6 +313,36 @@ public function testAddTransitionMissingFromException() { /** * @covers ::addTransition */ + public function testAddTransitionDuplicateTransitionStatesException() { + $this->setExpectedException(\InvalidArgumentException::class, "The 'publish' transition already allows 'draft' to 'published' transitions in workflow 'test'"); + $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow'); + $workflow + ->addState('draft', 'Draft') + ->addState('published', 'Published'); + $workflow->addTransition('publish', 'Publish', ['draft', 'published'], 'published'); + $workflow->addTransition('draft_to_published', 'Publish a draft', ['draft'], 'published'); + } + + /** + * @covers ::addTransition + */ + public function testAddTransitionConsistentAfterFromCatch() { + $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow'); + $workflow->addState('published', 'Published'); + try { + $workflow->addTransition('publish', 'Publish', ['draft'], 'published'); + } + catch (\InvalidArgumentException $e) { + } + // Ensure that the workflow is not left in an inconsistent state after an + // exception is thrown from Workflow::setTransitionFromStates() whilst + // calling Workflow::addTransition(). + $this->assertFalse($workflow->hasTransition('publish')); + } + + /** + * @covers ::addTransition + */ public function testAddTransitionMissingToException() { $this->setExpectedException(\InvalidArgumentException::class, "The state 'published' does not exist in workflow 'test'"); $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow'); @@ -417,10 +447,10 @@ public function testGetTransitionsForState() { /** - * @covers ::getTransitionsFromStateToState + * @covers ::getTransitionFromStateToState * @covers ::hasTransitionFromStateToState */ - public function testGetTransitionsFromStateToState() { + public function testGetTransitionFromStateToState() { $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow'); // By default states are ordered in the order added. $workflow @@ -433,14 +463,14 @@ public function testGetTransitionsFromStateToState() { $this->assertTrue($workflow->hasTransitionFromStateToState('draft', 'published')); $this->assertFalse($workflow->hasTransitionFromStateToState('archived', 'archived')); - $transitions = $workflow->getTransitionsFromStateToState('published', 'archived'); - $this->assertEquals('Archive', $transitions['archive']->label()); + $transition = $workflow->getTransitionFromStateToState('published', 'archived'); + $this->assertEquals('Archive', $transition->label()); } /** - * @covers ::getTransitionsFromStateToState + * @covers ::getTransitionFromStateToState */ - public function testGetTransitionsFromStateToStateException() { + public function testGetTransitionFromStateToStateException() { $this->setExpectedException(\InvalidArgumentException::class, "The transition from 'archived' to 'archived' does not exist in workflow 'test'"); $workflow = new Workflow(['id' => 'test', 'type' => 'test_type'], 'workflow'); // By default states are ordered in the order added. @@ -452,7 +482,7 @@ public function testGetTransitionsFromStateToStateException() { ->addTransition('publish', 'Publish', ['draft', 'published'], 'published') ->addTransition('archive', 'Archive', ['published'], 'archived'); - $workflow->getTransitionsFromStateToState('archived', 'archived'); + $workflow->getTransitionFromStateToState('archived', 'archived'); } /**