Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed
MarioBlazek wants to merge1 commit intosymfony:masterfromMarioBlazek:workflow_config_do_not_normalize_24981
Closed

[Workflow] Do not normalize keys for workflow transitions#25049

MarioBlazek wants to merge1 commit intosymfony:masterfromMarioBlazek:workflow_config_do_not_normalize_24981

Conversation

@MarioBlazek
Copy link
Contributor

@MarioBlazekMarioBlazek commentedNov 20, 2017
edited by lyrixx
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#24981
LicenseMIT

Simperfit reacted with thumbs up emoji
@lyrixx
Copy link
Member

Hi@MarioBlazek

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
Copy link
ContributorAuthor

@lyrixx I've added this as bug becausehere it is marked as bug. Should I mark this as new feature ? Thanks.

@backbone87
Copy link
Contributor

if this is not a bug, its a BC break and therefore cant be merged until 5.0,

my opinion is still:

  • keep current transformation behavior
  • deprecate usage of transition names in keys (and use transition names in the name property of the entry)

@lyrixx
Copy link
Member

@backbone87 Your are totally right. So now, I don't know if it's a bugfix or not :/

@nicolas Any advice here?

@chalasr
Copy link
Member

Well, using the transition name as key makes it normalized while it is not using thename: value syntax, that's inconsistent but doesn't qualify as a bugfix.

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.

@chalasrchalasr added this to the4.1 milestoneNov 20, 2017
@backbone87
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Member

I'm sorry, I'm going to close your PR because there are noreally easy way to deal with BC.
More over there is already a way to fix this issue:

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

@lyrixxlyrixx closed thisFeb 7, 2018
@backbone87
Copy link
Contributor

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
Copy link
Member

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 ;)

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

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

6 participants

@MarioBlazek@lyrixx@backbone87@chalasr@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp