Adding a workflow state or transition with an integer ID results in unrecoverable fatal errors

This also happens if you add a transition.
If you do happen to create a state or transition with a numeric machine name then the workflow edit page just shows you a blank page: The website encountered an unexpected error. Please try again later.

Adding a workflow with a numeric machine name is fine.

Looks good @timmillwood - this currently prevents the user from breaking things.

+++ b/core/modules/workflows/tests/src/Functional/WorkflowUiTest.php
@@ -111,6 +111,12 @@ public function testWorkflowCreation() {
+    $this->assertSession()->pageTextContains('The machine-readable name must contain only lowercase letters, numbers, and underscores.');

@@ -118,6 +124,11 @@ public function testWorkflowCreation() {
+    $this->assertSession()->pageTextContains('The machine-readable name must contain only lowercase letters, numbers, and underscores.');

We'll need to update this text in the add forms and the tests to reflect the new restriction.

Thanks @timmillwood, errors look fine now.
We should change the #description for the Machine-readable name fields as well:

Perfect thanks @timmillwood, from my perspective this is good to go now.

So the try-catch is explained in #4 with:

In doing this I found the exceptions we throw in addState and addTransition actually prevent the validation from running, so fixing that too.

Not 100% sure about this, though. Over in #2842193: Exception in Workflow::addState when an invalid machine name is given we came to the agreement, that, although it is ugly, we should explicitly duplicate the validation ::copyFormValuesToEntity(). This seems to go against that. I am definitely against it, but even if we do go with a try-catch:

1. I think we should remove the validation introduced in #2842193: Exception in Workflow::addState when an invalid machine name is given as it is then obsolete.
2. We should be catching something more specific than \Exception. If the database server dies in the middle of the request,
   it should not be handled equivalently to someone inputting an invalid value in a form.

Marking needs review, though, to get some more opinions on this.

Thanks @tstoeckler for the review - tackling #10.1 and #10.2 here.

+++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
@@ -89,7 +89,7 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
       // Replicate the validation that Workflow::addState() does internally as the
       // form values have not been validated at this point.

This can also be removed then.

Again, though, I am not endorsing the try-catch. I would actually prefer duplicating the validation. But let's hear what others think.

Argh - good catch... removing the comments as well.

Yup, we should have more input on this... just making the changes you suggested because they make sense, in case we do want to go the try/catch route =)

Patch rerolled

@timmillwood Thanks for fixing tests.

Patch re-rolled.

Looking good!

+++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
@@ -82,17 +86,20 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
+          // @todo move to submitStateConfigurationForm. #2849827.

#2849827: Move workflow "settings" setters and getters to WorkflowTypeInterface has landed now :) - Im guessing this affects WorkflowTransitionAddForm as well.

I think we should explore supporting integer IDs, any reason we abandon that so quickly? Seems like it'd be good for consistency, especially given even the workflow entity itself can have numeric IDs. I've opened #2899187: For consistency, workflow states and transitions should support numeric machine names to have a look at this.

One thing I found looking into this is that we previously took the approach in #2842193: Exception in Workflow::addState when an invalid machine name is given to duplicate the machine name validation logic in copyFormValuesToEntity so we didn't trigger the exception. Why didn't we apply the same treatment to transitions? Because the transition forms already had a pretty good solution for this.

I think we should slightly rejig things, so this issue becomes "Make validation in transition and state forms consistent" and the prior issue I linked dedicated to making ID based machine name handling consistent. That way we avoid our snow-flake error messages about not containing only numbers etc.

If we decide to do that, here is a patch which introduces that consistency. Proposed issue name:

Don't reimplement validation rules for workflow state add/edit forms in ::copyFormValuesToEntity and make them consistent with transition forms

Ah, I logged #2899187: For consistency, workflow states and transitions should support numeric machine names to sort the numeric IDs, but makes sense to use this issue as well.

I've logged #2899248: Don't reimplement validation rules for workflow state add/edit forms in ::copyFormValuesToEntity to capture the validation rejig, looking into numeric IDs now.

Attached brings id handling into line, by allowing numeric IDs.

Removing screenshot of the error message introduced in earlier patches from the IS.

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -282,14 +282,17 @@ public function getTransitions(array $transition_ids = NULL) {
-        $labels, SORT_NATURAL, SORT_ASC
+        $labels, SORT_NATURAL, SORT_ASC,
+        $keys

This is kind of weird formatting for the parameters; it doesn't really follow our coding standards. Either the parameters should be all on one line, or they should be each on a separate line. I'd suggest the former.

Also, let's add inline comments explaining the sort logic, since it's totally not obvious by reading the method itself why we're doing this. The test coverage proves the fix nicely, but we should also make it clear in the code.

Thanks!

I think this is a patch-safe backport as it should be BC for the normal non-integer ID case.

Tested patch #35. Tested add new state & add new transition, works without any error.

but this is not to be confused by the sorting for display and use within Drupal,

Indeed, #2900700: Add orderby to content_moderation schema sequences is conceptually related, but very different.

Promoting to major according to the criteria in https://www.drupal.org/core/issue-priority#major-bug; this results in a fatal error and renders the workflow module unusable based on actions taken only in the UI and does not have a workaround.

So a couple of concerns here still:

The test-only patch doesn't show the fatal error; it just shows the invalid sort. This is presumably because it's a unit test. This means we don't have any test coverage exercising the case where the fatal error is thrown.

Even with the added comments I'm still struggling to see how the fatal is triggered and how the change fixes it; this could be a combination of array_multisort() being evil and insufficient caffeine.

Edit: Left at RTBC because this is most likely still a me-problem.

Okay, so part of my confusion is with the documentation of labelWeightMultisort(). This method is passed a list of state or transition keyed by state or transition ID, but the documentation for the method does not suggest a meaningful key/value association.

I'm wondering if we need a deeper fix; I keep coming back to words like "brittle" and "code smell". Going to get a second opinion since it might be worthwhile to fix the major with the quickfix here and then improve it more in a followup, except I haven't quite decided yet what that followup would be exactly so it's not very actionable feedback.

Thanks, and sorry for the delay reviewing this and other issues.

Okay, discussed with @catch; we agreed it at least needs an additional functional test case that exercises the fatal. The inline comments also could spell out what's happening a bit more explicitly; always a good idea when there's lots of array manipulation.

I did manually test and confirm that this patch does indeed fix the fatal.

I realize now why the array was formatted as it was in #30; it was grouping each array with its flags with the array_multisort(). Sorry for the kneejerk about that. While I think it may violate coding standards, it also does make the function bit more readable. I also realized it was out of scope, because the array is already sorted this way. So we can revert those changes and it was a mistake on my part to push back on them here.

This is reminding me of all the reasons I hate array_multisort(). :P

Another thing I'd want to see (probably also out of scope) is fixing the documentation for this method to indicate that the key/value association is important. However, that plus the need for inline comments to explain what's going wrong (the current comments still aren't quite enough for me to understand what's going to break just by reading the code) plus the existence of the bug in the first place makes me wonder if these methods shouldn't just be refactored. I don't have a concrete suggestion right now on how to improve them, and I don't want to block what is a nasty major on a vague request for refactoring, but maybe @Sam152 or @timmillwood has ideas? Are there past issues related to this?

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -277,15 +277,23 @@ public function getTransitions(array $transition_ids = NULL) {
         $labels[$id] = $object->label();
...
-        $labels, SORT_NATURAL, SORT_ASC
+        $labels, SORT_NATURAL, SORT_ASC,

So $labels is never used again after this... why is it there?

Edit: Because it's the secondary sort for the weights which we do return, disregard. But really inline comments would be good.

@timmillwood, yeah, saw that on second read; just edited my comment. Sorry, I hate array_multisort(). :P Inline comments would really help. Like:

      // Sort weights, labels, and keys in the same order as each other.
       array_multisort(
         // Use the numerical weight as the primary sort.
         $weights, SORT_NUMERIC, SORT_ASC,
         // When objects have the same weight, sort them alphabetically by label.
        $labels, SORT_NATURAL, SORT_ASC
        // Ensure that the keys (the object IDs) are sorted in the same order as the weights.
       );
      // Combine the sorted keys and weights to make sure the weights are keyed with
      // the correct keys.
      $weights = array_combine($keys, $weights);
      // Return the objects sorted by weight and label.

And there should be test cases for sorting with duplicate weights, duplicate labels, etc.

Posted #2906879: WorkflowTypeBase:: labelWeightMultisort() has meaningful param/return keys, but docs do not indicate this for the out-of-scope improvements I suggested for the docblock.

+++ b/core/modules/workflows/src/Plugin/WorkflowTypeBase.php
@@ -277,15 +277,26 @@ public function getTransitions(array $transition_ids = NULL) {
+        // Ensure that the keys (the object IDs) are sorted in the same order as the weights.

Minor issue here with the comment being over the 80 char line.

On a quick scan, the added tests and comments look great. Thanks @timmillwood.

Alright, this looks good to me. Thanks @timmillwood for adding the tests. I keep thinking there must be a better way but that's not actually helpful and no reason to hold back a major bugfix.

No comments other than what xjm said in #62, so:

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!