The default #delta for the weight form element is 10, which gives produces a range of -10...10 with which to order. Even with a modest number of states, the number of possible transitions can quickly exceed 20, leaving you with a range of transitions that become stuck with the same weight and cannot be reordered (at least via the UI).

Comments

kevin.dutra created an issue. See original summary.

kevin.dutra’s picture

Here's a test to help demonstrate the issue.

kevin.dutra’s picture

Assigned: kevin.dutra » Unassigned
Status: Active » Needs review
StatusFileSize
new3.43 KB

And now after augmenting the #delta.

sam152’s picture

Nice catch, bumping the available transition weights from 20 to 200 makes a lot of sense. A couple of things:

  • Do you think we should bump the number of allowed states at the same time?
  • Do we think we can simply assert the weight select boxes have the correct ranges of numbers in them, or use some weights in the earlier test case between -100 and 100? We want to avoid retesting abstractions we are depending on, in this case '#type' => 'weight'.

Maybe something as simple as:

    $this->assertSession()
      ->selectExists('transitions[create_new_draft][weight]')
      ->selectOption('100');
    $this->assertSession()
      ->selectExists('transitions[create_new_draft][weight]')
      ->selectOption('-100');
kevin.dutra’s picture

Assigned: Unassigned » kevin.dutra
Status: Needs review » Needs work

My one reservation with allowing there to be more states is that it causes the number of possible transitions to explode farther, which is why I left that one as-is. Hopefully people aren't trying to create tons of states because it makes for a much more complicated process for the end users who are using it, but I suppose there might be the odd exception that actually requires something that complex. For now I think I'll leave that one alone.

I'm totally on board for the test refactoring. :)

kevin.dutra’s picture

Assigned: kevin.dutra » Unassigned
Status: Needs work » Needs review
StatusFileSize
new1.44 KB
new3.04 KB
sam152’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, thanks.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/workflows/src/Form/WorkflowEditForm.php
@@ -193,6 +193,7 @@ public function form(array $form, FormStateInterface $form_state) {
           '#default_value' => $transition->weight(),
           '#attributes' => ['class' => ['transition-weight']],

Why not count the number of transitions and use that? Any limit seems a bit arbitrary.

In blocks we do:

    // Weights range from -delta to +delta, so delta should be at least half
    // of the amount of blocks present. This makes sure all blocks in the same
    // region get an unique weight.
    $weight_delta = round(count($entities) / 2);
kevin.dutra’s picture

Status: Needs work » Needs review
StatusFileSize
new2.74 KB
new2.42 KB

Fair enough. Now it's dynamic based on the current number of transitions. I've gone ahead and added in a similar adjustment for states, as long as we're there.

runeasgar’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new45.16 KB
new37.68 KB

Testing.

  1. Confirming undesirable behavior by enabling moderation/workflows, adding a bunch of states, getting up to 21 transitions, and attempting to reorder: ...
  2. Actually, you need 22 transitions, because there are 21 weightings including 0. Adding another.
  3. Successfully reproduced. Ended up in a situation where alphabetical ordering took over for my last 2 items.
  4. Applying patch: curl -l https://www.drupal.org/files/issues/2018-04-13/workflow-transition-ordering-2939634-9.patch | git apply -v
  5. Applies clean.
  6. drush cr
  7. Refreshing and resting using my existing states and transitions: success, I am able to reorder my final 2 items, and showing the row weights confirms the new behavior. Providing a screenshot below showing row weights where I moved "Create New Draft" to come before "5 -> 4", even though "5 -> 4" has alphabetical priority. Also providing a screenshot of the "-11" delta that got used in the process.

Moving to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 6a74240ce3 to 8.6.x and ffaf769407 to 8.5.x. Thanks!

Backported to 8.5.x as a bugfix.

  • alexpott committed 6a74240 on 8.6.x
    Issue #2939634 by kevin.dutra, runeasgar, Sam152, alexpott: Unable to...

  • alexpott committed ffaf769 on 8.5.x
    Issue #2939634 by kevin.dutra, runeasgar, Sam152, alexpott: Unable to...

Status: Fixed » Closed (fixed)

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