[Workflow] Added new validator to make sure each place has unique translation names #21271

Closed
wants to merge 4 commits into
from

Projects

None yet

7 participants

@Nyholm
Contributor
Nyholm commented Jan 13, 2017
Q A
Branch? 3.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR n/a

A definition where a place that has transitions with the same name is an invalid definition.

invalid1: 
  places: ['a', 'b', 'c']
  transitions: 
    t1: 
      from: a
      to: b
    t1: 
      from: a
      to: c

valid1: 
  places: ['a', 'b', 'c']
  transitions: 
    t1: 
      from: a
      to: b
    t2: 
      from: a
      to: c

valid2: 
  places: ['a', 'b', 'c']
  transitions: 
    t1: 
      from: a
      to: b
    t1: 
      from: b
      to: c

valid3: 
  places: ['a', 'b', 'c', 'd']
  transitions: 
    t1: 
      from: ['a', 'b']
      to: d
    t2: 
      from: ['a', 'b']
      to: c

FYI @lyrixx

@Nyholm Nyholm Added new validator to make sure each place has unique translation names
9115a07
@Nyholm Nyholm changed the title from Added new validator to make sure each place has unique translation names to [Workflow] Added new validator to make sure each place has unique translation names Jan 13, 2017
@Nyholm Nyholm Stylefix
65d4c5e
@lyrixx
lyrixx approved these changes Jan 13, 2017 View changes

👍

@lyrixx lyrixx added this to the 3.2 milestone Jan 13, 2017
@lyrixx
Member
lyrixx commented Jan 13, 2017

Note: It should be merge in 3.2 and not master.

@@ -16,6 +16,7 @@
use Symfony\Component\DependencyInjection\Exception\RuntimeException;
use Symfony\Component\Workflow\Validator\DefinitionValidatorInterface;
use Symfony\Component\Workflow\Validator\StateMachineValidator;
+use Symfony\Component\Workflow\Validator\UniqueTransitionNameValidator;
@nicolas-grekas
nicolas-grekas Jan 13, 2017 Member

missing composer.json update I guess, for ^3.2.3

@Nyholm
Nyholm Jan 14, 2017 Contributor

There is no reference to symfony/workflow at all in the composer.json of the framework bundle. Should I still add one?

@ro0NL
Contributor
ro0NL commented Jan 14, 2017

What about moving it to WorkflowValidator? The StateMachineValidator already enforces this right?

@Nyholm
Contributor
Nyholm commented Jan 14, 2017

Thank you @ro0NL. You are correct. The StateMachineValidator has this test already but it is missing in the WorkflowValidator.

I will merge this test to the Workflow validator.

@fabpot
Member
fabpot commented Jan 15, 2017

Looks like a new feature to me, not a bug fix. I think it should be merged in 3.3 instead.

Nyholm added some commits Jan 16, 2017
@Nyholm Nyholm Moved unique name check to the WorkflowValidator
88a22c3
@Nyholm Nyholm typo
3f539b3
@Nyholm
Contributor
Nyholm commented Jan 16, 2017

Looks like a new feature to me, not a bug fix.

This PR makes sure the Definition is not invalid. That is why I marked it as a "bugfix". But one could argue that all PRs to the Validator as bugfixes... Anyhow.

Since this check exist for the StateMachineValidator, I've updated the PR to include this check in the Workflow validator only.

@nicolas-grekas
Member

Generally : any new visible interface => feature.
Now that this just tweaks an edge case behavior only, it may much likely qualify as bug fix :)

@lyrixx
Member
lyrixx commented Jan 16, 2017

For me it's a bug fix too, because if the definition is not valid, the workflow will not work correctly.

@fabpot
Member
fabpot commented Jan 18, 2017

Thank you @Nyholm.

@fabpot fabpot added a commit that referenced this pull request Jan 18, 2017
@fabpot fabpot bug #21271 [Workflow] Added new validator to make sure each place has…
… unique translation names (Nyholm)

This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes #21271).

Discussion
----------

[Workflow] Added new validator to make sure each place has unique translation names

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        | n/a

A definition where a place that has transitions with the same name is an invalid definition.

```yaml
invalid1:
  places: ['a', 'b', 'c']
  transitions:
    t1:
      from: a
      to: b
    t1:
      from: a
      to: c

valid1:
  places: ['a', 'b', 'c']
  transitions:
    t1:
      from: a
      to: b
    t2:
      from: a
      to: c

valid2:
  places: ['a', 'b', 'c']
  transitions:
    t1:
      from: a
      to: b
    t1:
      from: b
      to: c

valid3:
  places: ['a', 'b', 'c', 'd']
  transitions:
    t1:
      from: ['a', 'b']
      to: d
    t2:
      from: ['a', 'b']
      to: c
```

FYI @lyrixx

Commits
-------

eece8ad [Workflow] Added new validator to make sure each place has unique translation names
30a0143
@fabpot fabpot closed this Jan 18, 2017
@Nyholm Nyholm deleted the Nyholm:workflow-unique-name branch Jan 18, 2017
@fabpot fabpot referenced this pull request Feb 6, 2017
Merged

Release v3.2.3 #21544

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment