Allow changing of transition 'to' on edit

I think it'd be good to add a test that edits the "to".

I can't remember for the life of me why we did that. I think it might have been to do with how the ID was being set - but this is no longer done that way. We need tests for this change though.

Didn't look at the tests yet, but the code looks great. One comment:

+++ b/core/modules/workflows/src/Entity/Workflow.php
@@ -427,6 +427,36 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
+    if ($this->transitions[$transition_id]['from']) {
+      foreach ($this->transitions[$transition_id]['from'] as $from_state_id) {
+        if ($this->hasTransitionFromStateToState($from_state_id, $to_state_id)) {
+          $transition = $this->getTransitionFromStateToState($from_state_id, $to_state_id);
+          if ($transition_id !== $transition->id()) {
+            throw new \InvalidArgumentException("The '{$transition->id()}' transition already allows '$from_state_id' to '$to_state_id' transitions in workflow '{$this->id()}'");
+          }
+        }
+      }
+    }

As far as I can tell this validation is not performed in the form, so it's possible to trigger this from the UI, which we should not allow.

Oh that's absolutely correct. Sorry, I hadn't even thought to look.

I'm not sure that \Drupal\workflows\Form\WorkflowTransitionAddForm::validateForm() is correct. It might be that you want to duplicate transitions - but have attached different actions and permissions to each transitions. We discussed this a bit in #2779647: Add a workflow component, ui module, and implement it in content moderation. It is tricky - flexibility vs making things simple.

I think the validation is a separate issue.

The patch in #12 looks good.

Looks good. Making it as RTBC!

We need to add test coverage of creating a duplication transition by changing a to state in UI.

+++ b/core/modules/workflows/tests/src/Functional/WorkflowUiTest.php
@@ -155,6 +155,34 @@ public function testWorkflowCreation() {
+    $this->assertSession()->pageTextContains('Created ToTestState state.');
+    // Create a new transition to test changing the transition 'to' value.
...
+    // Edit the new transition to test changing the transition 'to' value.
+    $this->drupalGet('admin/config/workflow/workflows/manage/test/transition/totesttransition');
...
+    // Delete transition use in test changing the transition 'to' value.
+    $this->drupalGet('admin/config/workflow/workflows/manage/test/transition/totesttransition');
...
+    $this->submitForm([], 'Delete');
+    // Delete state use in test changing the transition 'to' value.

If there was a new line before each comment the test would be easier to read.

Re #15 - I was wrong we decided to prevent such duplicate transitions in UI and API so the approach in the patch is correct. Just need the missing test coverage. @scott_euser++ nice one.

The test looks looks good. As suggested in #19 adding some newlines between distinct sections of the test would make it seem like less of a wall of text and make it easier to read, but given that's a style thing not a standard RTBCing. Can RTBC any follow-up patches if you feel like resolving that :)

Given I only reviewed the interdiff, here are a few things I would probably include in a full review if --uber-nit-mode was on. These comments are totally subjective, so not changing issue status.

1. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -427,6 +427,36 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
   +          $transition = $this->getTransitionFromStateToState($from_state_id, $to_state_id);
   +          if ($transition_id !== $transition->id()) {
   

   s/$transition/$existing_transition/ is perhaps more explicit?

2. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -427,6 +427,36 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
   +    // Update the transitions.
   +    $this->transitions[$transition_id]['to'] = $to_state_id;
   

   Another style thing, but the comment should describe the "why" more than the "what". The latter is useful when it's not 100% apparent just by glancing at the code, but we can see here transitions are being updated without the comment.

3. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -427,6 +427,36 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
   +    // Ensure that the states exist.
   +    if (!$this->hasState($to_state_id)) {
   

   Some deal with the comment here. If a comment was necessary, this could describe why it's important the to_state exists, not that the check is happening. The comment reads exactly like a synonym of the code.

Hint: giving interdiffs a .txt extension will stop the bot testing them.

@scott_euser - @Sam152 makes some good points in #26 - given that this is an API/UI loosening for an experimental module it can go into a patch release. Therefore there's no rush - let's make the patch perfect :) Readable tests really help future-us.

@Sam152 are there any other nits you'd pick up if "--uber-nit-mode" was on?

If a comment adds no value, no reason to keep it.

+++ b/core/modules/workflows/src/Entity/Workflow.php
@@ -427,6 +427,36 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
+    if ($this->transitions[$transition_id]['from']) {
...
+    }

I wonder if the schema definition enforces this as an empty array, making this check not required?

That's all I got, rest LGTM.

Opened #2851708: Workflow entity getters should return FALSE or NULL instead of throwing an exception if their data doesn't exist as a follow up to discuss being required to call hasTransitionFromStateToState before getTransitionFromStateToState.

We already call isset, so the question is, can a transition be initialized without 'from' as an empty array. FWIW, the tests pass without it.

@scott_euser Re #2851708: Workflow entity getters should return FALSE or NULL instead of throwing an exception if their data doesn't exist you could just call getTransitionFromStateToState and catch the exception - then less nesting and no call to hasTransitionFromStateToState

1. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -427,6 +427,35 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
   +    if ($this->transitions[$transition_id]['from']) {
   

   @Sam152 is right this is initialized to be an empty array it will always be one.

2. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -427,6 +427,35 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
   +      foreach ($this->transitions[$transition_id]['from'] as $from_state_id) {
   +        if ($this->hasTransitionFromStateToState($from_state_id, $to_state_id)) {
   +          $transition = $this->getTransitionFromStateToState($from_state_id, $to_state_id);
   +          if ($transition_id !== $transition->id()) {
   +            throw new \InvalidArgumentException("The '{$transition->id()}' transition already allows '$from_state_id' to '$to_state_id' transitions in workflow '{$this->id()}'");
   +          }
   +        }
   +      }
   

   You actually only need only need to do this if the to state is changing. And if the to state is changing and $this->hasTransitionFromStateToState($from_state_id, $to_state_id) returns true then you know that the transition is not your transition.

In fact there is a method already to help us get the problem - see \Drupal\workflows\Entity\Workflow::getTransitionIdFromStateToState()...

So the method could look as simple as this:

  /**
   * {@inheritdoc}
   */
  public function setTransitionToState($transition_id, $to_state_id) {
    if (!isset($this->transitions[$transition_id])) {
      throw new \InvalidArgumentException("The transition '$transition_id' does not exist in workflow '{$this->id()}'");
    }

    // Ensure that the state the transition is to be changed to actually exists.
    if (!$this->hasState($to_state_id)) {
      throw new \InvalidArgumentException("The state '$to_state_id' does not exist in workflow '{$this->id()}'");
    }
    
    // If there is no change there is no need to validate the transitions.
    if ($this->transitions[$transition_id]['to'] === $to_state_id) {
      return $this;
    }

    foreach ($this->transitions[$transition_id]['from'] as $from_state_id) {
      if ($transition_id = $this->getTransitionIdFromStateToState($from_state_id, $to_state_id)) {
        throw new \InvalidArgumentException("The '{$transition_id}' transition already allows '$from_state_id' to '$to_state_id' transitions in workflow '{$this->id()}'");
      }
    }

    $this->transitions[$transition_id]['to'] = $to_state_id;
    return $this;
  }

So I'm going to close #2851708: Workflow entity getters should return FALSE or NULL instead of throwing an exception if their data doesn't exist

In my suggested update I did

     if ($transition_id = $this->getTransitionIdFromStateToState($from_state_id, $to_state_id)) {
        throw new \InvalidArgumentException("The '{$transition_id}' transition already allows '$from_state_id' to '$to_state_id' transitions in workflow '{$this->id()}'");
      }

That's not good because it clashes with $transition_id passed in ... this could be something like:

     if ($existing_transition_id = $this->getTransitionIdFromStateToState($from_state_id, $to_state_id)) {
        throw new \InvalidArgumentException("The '{$existing_transition_id}' transition already allows '$from_state_id' to '$to_state_id' transitions in workflow '{$this->id()}'");
      }

RTBCing with the last unresolved nit fixed.

1. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -422,9 +422,9 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
   -        $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()}'");
   +        $existing_transition = $this->getTransitionFromStateToState($from_state_id, $this->transitions[$transition_id]['to']);
   +        if ($transition_id !== $existing_transition->id()) {
   +          throw new \InvalidArgumentException("The '{$existing_transition->id()}' transition already allows '$from_state_id' to '{$this->transitions[$transition_id]['to']}' transitions in workflow '{$this->id()}'");
   

   Hm it looks like the only change in this hunk is that a local variable is being renamed. That's out-of-scope-ish. I checked this with a word diff. I guess it's for consistency with the added method implementation.

2. +++ b/core/modules/workflows/src/Entity/Workflow.php
   @@ -442,6 +442,36 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
      /**
       * {@inheritdoc}
       */
   +  public function setTransitionToState($transition_id, $to_state_id) {
   

   What docs is this inheriting? I couldn't find any existing method named this?

Edit: Nope, this was wrong. Removing to avoid confusion.

Hm nope, #45 is not the origin. I thought the change for the machine name had overshot but looks like it was not #36 that introduced the other; bad merge on my part.

Aha. So it first appears as this in #2779647-61: Add a workflow component, ui module, and implement it in content moderation:

1.    $form['to'] = [
   +      '#type' => 'select',
   +      '#title' => $this->t('To'),
   +      '#required' => TRUE,
   +      '#default_value' => isset($transition) ? $transition->to()->id() : '',
   +      '#options' => $states,
   +      '#disabled' => isset($transition),
   +    ];
   +

   Comment was:

   The patch attached separates the transitions from states so that workflow types can also decorate transitions and do special things. The separation of concerns takes us to a config that looks very like Symfony and what @bojanz was saying about other state machine implementations.

   The comment of @bojanz' being referenced is apparently in #48:

   This is a great improvement! I'm very happy to see it.

   A few questions:
   1) What's your strategy for editing workflows after they're in use? Disable the whole form if there are any entities?
   Right now there are no safeguards, which means that an edited workflow could result in invalid entities (non-existent states, etc).

   2) Why store the workflow ID inside the bundle info, and bless it as the "one true workflow"? Why not simply have a state_item field type that holds the state, and references the workflow? It would be convenient, and more natural when allowing multiple workflows.
   This also makes sense when formatting the state, we currently store only the state ID, but will need to show the state label in various places. There's also a use case for a formatter that renders transition buttons (Jira-style), which would benefit from a field type.

   3) ModerationStateFieldItemList appears to be unused?

   4) I'm not a big fan of keying transitions by from_state, and found it a bit difficult to parse.
   There are cases when a transition can have multiple from states, and most state machines make from_state an array because of that, for example cancelling an order that's in various states. Here that would require duplication the transition definition for each from state. Guessing this was done to avoid having a transition ID, even though that's usually a more common implementation.

2. Then it turns into TRUE in #67. Comment is:

   More work on the UI and form classes for workflow states and transitions.

So maybe that will jog memories. If @alexpott still does not remember why, though, then I think the why is lost to history and we should just do what makes the most sense. I guess we could check with @bojanz if this makes any difference for his usecases.

API changes are allowed as this is an experimental module, this should indeed be added to the interface, nice catch @xjm.

There is only one implementation of the interface in core, so this looks good to me. I believe that's all of the feedback resolved.

Looks like my RTBC clashed with @Sam152, so +1 to RTBC.

+++ b/core/modules/workflows/src/Entity/Workflow.php
@@ -422,9 +422,9 @@ public function setTransitionFromStates($transition_id, array $from_state_ids) {
-        $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()}'");
+        $existing_transition = $this->getTransitionFromStateToState($from_state_id, $this->transitions[$transition_id]['to']);
+        if ($transition_id !== $existing_transition->id()) {
+          throw new \InvalidArgumentException("The '{$existing_transition->id()}' transition already allows '$from_state_id' to '{$this->transitions[$transition_id]['to']}' transitions in workflow '{$this->id()}'");

I'm a little confused by the variable name change, it doesn't seem in scope. Not sure that should block things though.

Yep @timmillwood, I said the same in #44. In #35 alexpott did not suggest changing the variable name in a completely different method, only in the method added by this patch. So let's go ahead and remove that hunk.

+++ b/core/modules/workflows/src/WorkflowInterface.php
@@ -265,6 +265,23 @@ public function setTransitionWeight($transition_id, $weight);
+   * Sets a transition's to state.

I think 'to' should be in quotes here as it is elsewhere; otherwise, it's hard to parse the sentence.

Thanks!

Fixes everything in #56.

So the reason the 'to' was disabled is this. When you create a transition you label it and this creates a machine name. The label you chose nearly always has the 'to' state in mind - consider the core examples:

  archive:
    label: Archive
    from:
      - published
    to: archived
    weight: 2
  archived_draft:
    label: 'Restore to Draft'
    from:
      - archived
    to: draft
    weight: 3
  archived_published:
    label: Restore
    from:
      - archived
    to: published
    weight: 4
  create_new_draft:
    label: 'Create New Draft'
    from:
      - draft
      - published
    to: draft
    weight: 0
  publish:
    label: Publish
    from:
      - draft
      - published
    to: published
    weight: 1

@scott_euser Why did you want to change the 'to'? Was there a practical reason - or were you just surprised you couldn't?

I'm ambivalent about the change - ie. I can see that it helps when people make a mistake - but also I can see that the extra freedom gives people more ways to make something confusing when they come back in 6 months time.

I guess #59 and #60 need further discussion. Thanks!

I'm also ambivalent about the change.

Do we want to give more flexibility or prevent people from creating confusing systems?

I think if I was to sway one way or another it would be flexibility, thus RTBCing this again.

Patch at #57still applies to HEAD cleanly. As there is no straightforward impact of allowing this (except the transition machine name represent 'to' field value, which kinda same case with any other entity machine name as explained by @scott_euser in #60), RTBC assuming it's OK to allow. If not we eventually close this issue as 'won't fix' or 'works as designed' anyway.

Looking at the patch I was wondering if the user facing error handling needs updating but the possibility to change the from state already required validation on transition duplicity and indeed, it does still work when changing the destination on my testing.

@Gábor Hojtsy and @vijaycs85 - I still think we haven't really answered #59. Sure we can make this change but does it really buy us anything? Or does it just potentially make things more confusing? I'd love the UI work to be done before this - ie. #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait. Shall we postpone based on that one?

Indeed it is UI related and more of usability improvement than functional change. I am happy to postpone this on #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait

So let's postpone on #2830584: Use modals for creating, updating, and deleting workflows, with a new DialogFormTrait