Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Workflow] Do not normalize keys for workflow transitions#25049
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Workflow] Do not normalize keys for workflow transitions#25049
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lyrixx commentedNov 20, 2017
Thanks for working on this. I'm 👍 with this patch. But for me it's not a bugfix. @fabpot As this is a new feature and the 4.0 branch is not yet created, this should not be merged, right ? |
MarioBlazek commentedNov 20, 2017
backbone87 commentedNov 20, 2017
if this is not a bug, its a BC break and therefore cant be merged until 5.0, my opinion is still:
|
lyrixx commentedNov 20, 2017
@backbone87 Your are totally right. So now, I don't know if it's a bugfix or not :/ @nicolas Any advice here? |
chalasr commentedNov 20, 2017
Well, using the transition name as key makes it normalized while it is not using the About BC, could we register a transition for the normalized name and another for the raw name? I don't know much about the component, at least not enough to say if it would be ok functionally talking, at first sight I don't think so (would give an extra transition right?). We already merged a similar PR by documenting the BC break#21718, but if we can provide a smooth upgrade path by triggering notices where it makes sense, that would be best. |
backbone87 commentedNov 20, 2017
Registering a new transition isnt possible as it would change your business cirtical workflow (think of automated transitions, if only one transition is available). |
fabpot commentedNov 20, 2017
@lyrixx Indeed. 4.1 feature cannot be merged before the release of 4.0RC1 which will be the time when we create the 4.0 branch. |
lyrixx commentedFeb 7, 2018
I'm sorry, I'm going to close your PR because there are noreally easy way to deal with BC. before: transitions:request-review:# Does not work because it's normalizedfrom:draftto: -wait_for_journalist -wait_for_spellchecker after: transitions: -name:request-reviewfrom:draftto: -wait_for_journalist -wait_for_spellchecker |
backbone87 commentedFeb 9, 2018
While i get the reasoning, this kind of change was done for similar problems in other components. My original purpose was to deprecate the first example (transition names as keys) and only allow the second example. is this a change you are willing to consider? |
lyrixx commentedFeb 9, 2018
Actually, I don't like to break BC (even if there is a migration path, with deprecation notices) for the sake of cleanness. It's always boring to update code that works. So I really prefer to keep the BC here. But I totally understand and accept your point of view. It's really legit. But again, I prefer to provide a very nice upgrade path without doing anything. I like when it "just works". I hope you finally agree with me ;) |
Uh oh!
There was an error while loading.Please reload this page.