-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[FrameworkBundle] Perform-no-deep-merging on workflow transitions' from/to configs #61757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
955a0ba
to
376d0a9
Compare
Status: needs work While adding tests, I figured out the normalization logic is broken, as highlighted by the failures when using XML. |
edd1296
to
3745417
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Status: needs review
</xsd:sequence> | ||
<xsd:attribute name="name" type="xsd:string" use="required" /> | ||
<xsd:attribute name="name" type="xsd:string" /> | ||
<xsd:attribute name="key" type="xsd:string" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In XML, we have to differentiate the name and the key of transitions, so that deep-merging them works the same way as in other config formats (appending numeric keys and recursing into associative ones).
/cc @jakubtobiasz FYI this is how you'll be able to leverage this fix - use attribute "key" instead of "name" if you use the XML format to define workflows.
3745417
to
688a045
Compare
688a045
to
66fff01
Compare
$guardsConfiguration[$eventName][] = new Definition( | ||
Workflow\EventListener\GuardExpression::class, | ||
[new Reference($transitionId), $transition['guard']] | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too many intermediary variables made me wonder about this piece of code for too long
Similar to #61596 but for transitions.
Also replaces #57873 per @stof's comment at #57873 (comment)