[FrameworkBundle] Allow multiple transitions with the same name

[Padam87]

Q	A
Branch?	3.2
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#20794
License	MIT
Doc PR	-

[xabbuh]

I think we should normalise the values in the Configuration class already to ensure that an element with the name key is set.

[nicolas-grekas]

ping @lyrixx

[lyrixx]

->scalarNode('name')
    ->isRequired()
    ->cannotBeEmpty()
->end()

[lyrixx]

->beforeNormalization()
    ->always()
    ->then(function ($transitions) {
        // It's an indexed array, we let the validation occurs
        if (isset($transitions[0])) {
            return $transitions;
        }

        foreach ($transitions as $name => $transition) {
            if (array_key_exists('name', $transition)) {
                continue;
            }
            $transition['name'] = $name;
            $transitions[$name] = $transition;
        }

        return $transitions;
    })
->end()

[lyrixx]

Hello @Padam87

Thanks for your PR. I added 2 comments about the code. Could you please update your code with my patch?

BTW, with my patch, there is no need to use a key anymore. All the following syntax works:

transitions:
    request_review:
        from: draft
        to:
            - wait_for_journalist
            - wait_for_spellchecker

or

transitions:
    request_review:
        from: draft
        to:
            - wait_for_journalist
            - wait_for_spellchecker
        name: this is the new name, it override request_review

or

transitions:
    -
        from: draft
        to:
            - wait_for_journalist
            - wait_for_spellchecker
        name: here I MUST specify a name

[stof]

We should still validate that you don't have 2 transitions with the same name and a common from node though.

[lyrixx]

We should still validate that you don't have 2 transitions with the same name

Yes, indeed

and a common from node though.

Why? it's possible to have 2 transitions, with the same "from" name.

[stof]

@lyrixx your Yes indeed being in the middle of the and qualifying transitions makes me think that you misunderstood my comment entirely (your Yes indeed looks like you want to forbid exactly what this PR is about allowing to configure)

[lyrixx]

Oups, yes, you are right ;) Sorry.

[lyrixx]

But i'm not sure we should validate the "name + from" here. It should be done in the symfony/component validator instead.

[stof]

@lyrixx it indeed must be done in the workflow validator.
But it would not be bad to have an early validation in the DI class if it is easy to do (forbidding to configure an invalid workflow which would fail only later on). Validation during the compile time of the container would give early feedback without performance impact at runtime (as it happens at build time).

[lyrixx]

@stof All validations are already done in a compiler pass ;)

[stof]

then fine. I forgot that

[lyrixx]

Hello @Padam87

I'm sorry, I totally forgot this PR because it was not tagged with Workflow.

Thanks for this PR!

👍

[fabpot]

Thank you @Padam87.