Exception in Workflow::addState when an invalid machine name is given

I have taken a look at your patch, it took a bit to figure out how to reproduce the error but once I did I can confirm your patch solves it.

Steps followed:

1. Go to admin > config > workflows
2. Edit the Editorial workflow
3. Add a new state
4. Type a new label
5. Edit the machine name that is generated and add special characters to invalidate

Saving before patch, white screen with fatal error as you described.
Saving after patch, drupal form error message as is desirable.

Nice work!

This kind of an override is very problematic as it can lead to unexpected behavior when calling buildEntity() in other places.

I am providing an alternate patch that simply duplicates the validation in copyFormValuesToEntity().

If you do not like duplication and you see the fact that unvalidated form values are put into entity objects as problematic, see you in #2813073: Don't update $this->entity in EntityForm::afterBuild(). For the scope of this issue, I think this is all we can do.

Oops, should have included the test in the patch as well.

Hm, this (the previous patch at least, this is a crosspost with @tstoeckler) is problematic, because if we don't build the entity then this prevents us from ever relying on using the entity validation/constraint API for config entities as well.

IMHO a better fix would be to not let the workflow entity explode like this. This doesn't happen with any other config entity AFAIK..

As I said, crosspost.

Here is a different approach, it appears to save and work as expected but fatal error is avoided by waiting until form state validation is completed first (perhaps has something to do with the 'This is fired twice', but I couldn't see why it is fired twice).

Well currently it is not right that the entity is built twice on submission as it might happen that we save an entity that is not validated because of the entity builders. There is an issue to avoid this for content entities, but the approach might be used for config entities as well - #2833682: In submit use the entity built during the validation instead of building it again - for performance and consistency ensuring we are saving the entity that has been validated.

I am not sure if there is anything relying on that or further consequences of that if any, but hopefully someone who knows the ins and outs of workflow can review that. Otherwise my patch in #15 should solve the issue without knock-on effects.

Perhaps preventing double build can be addressed in a separate issue so solving this one isn't held up?

I agree with @hchonov that it is not right that the entity is built twice on submission, but I don't think that is for this issue. To fix the issue in hand I like the solution in #15, but would also be happy if #10 were committed.

I don't think we need to fix all the underlying issues here, just prevent the user from seeing a WSOD.

Well if we fix all the issues by a way where the entity builders are checking that they've already run and that the result from the previous execution is already there then we are only curing the symptoms but not the problem itself and we have to start at some point when there is an issue really showing the problem...

Resubmitting #15 so it's the latest for review to at least fix the WSOD.

Would help if I attached it...

Mhh, I guess we need a maintainer's input about this. With the three patches and @Berdir's comment we have 4 different directions to go in...

Ah I thought from your comment in #21 I'd resubmit to keep things moving, sorry!

Discussed this with @timmillwood and @Berdir in IRC (see attached log). We (at least Berdir and I) agreed that while my patch is ugly in that it duplicates the validation logic it is the best we can do given the current state of form validation vs. entity validation vs. form building etc.

Re-uploading my patch.

Sounds good. I don't know my regular expressions very well; do the @ signs before and after that machine name uses in its validation make a difference? If so, perhaps we should match that?
!preg_match('@[^a-z0-9_]+@', $values['id'])

Re #28, no it really doesn't make a difference. The version I chose is the one used in Workflow::addState() so I think it should be fine.

Nit: Can we have a failing test-only patch? That'll make it easier for committers.

+1 to RTBC

+++ b/core/modules/workflows/src/Form/WorkflowStateAddForm.php
@@ -82,9 +82,9 @@ protected function copyFormValuesToEntity(EntityInterface $entity, array $form,
-    // This is fired twice so we have to check that the entity does not already
-    // have the state.
-    if (!$entity->hasState($values['id'])) {
+    // Replicate the validation that Workflow::addState() does internally as the
+    // form values have not been validated at this point.
+    if (!$entity->hasState($values['id']) && !preg_match('/[^a-z0-9_]+/', $values['id'])) {

Should we link this issue to #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … because we're adding validation that only runs in a form? I.e. to move to validation constraints once they're supported for config entities?

Given what we have to work with, I think this is the best we can do.

Added related items, including #2845517: Enforce allowed characters of machine names in config entities. which is directly related.

Updating issue credits

Reviewed this.

We have buy in from the maintainer (@timmillwood) and entity subsystem maintainers (@Berdir, @tstoeckler) and we have a follow-up for cleaning this up - #2845517: Enforce allowed characters of machine names in config entities.

This fixes the fatal/WSOD seen in the UI.

Committed 6d68ffd and pushed to 8.4.x. Thanks!

This seems related to #2886567: Adding a workflow state or transition with an integer ID results in unrecoverable fatal errors.