8000 [Workflow] Introduce a Workflow interface, type-hint against it in Registry · Issue #23910 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Workflow] Introduce a Workflow interface, type-hint against it in Registry #23910

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.

Sign up for GitHub

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

Jump to bottom
Closed
dkarlovi opened this issue Aug 17, 2017 · 21 comments
Closed

[Workflow] Introduce a Workflow interface, type-hint against it in Registry #23910

dkarlovi opened this issue Aug 17, 2017 · 21 comments
Labels
Feature Workflow

Comments

@dkarlovi
Copy link
Contributor
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version latest

Workflow\Registry accepts only instances of Worflow\Workflow which makes decorating workflows awkward. It would allow for more flexibility if an interface was introduced here.
@lyrixx
Copy link
Member
lyrixx commented Aug 18, 2017

The decoration argument make sens ; I'm 👍

Do you want to create the PR?

@dkarlovi
Copy link
Contributor Author

Do you want to create the PR?

Sure, not a problem. Would this count as a BC?

@lyrixx
Copy link
Member
lyrixx commented Aug 18, 2017 •
edited
Loading

It's not a BC break.

@xabbuh
Copy link
Member
xabbuh commented Aug 18, 2017

In fact, changing some of the type hints to accept WorkflowInterface instance instead of Workflow would be a BC break for child classes.

@lyrixx
Copy link
Member
lyrixx commented Aug 18, 2017

Ouch. You are right :( How can we do ?

@chalasr
Copy link
Member
chalasr commented Aug 18, 2017 •
edited
Loading

We would have to first deprecate passing a Workflow that doesn't implement WorkflowInterface everywhere the typehint should change, or flag Registry as @final.

@dkarlovi
Copy link
Contributor Author

OK, what's the proper action here? I'll create the PR.

@chalasr
Copy link
Member
chalasr commented Aug 18, 2017

Ah... nevermind, we are talking about a method, I thought a constructor. Then a possible path would be to deprecate the whole add() method in favor of a constructor. Question is: is add() useful?

@Simperfit
Copy link
Contributor
Simperfit commented Aug 24, 2017 •
edited
Loading

@chalasr, can't we accept both in the add method and add a deprecation when we pass a Workflow that does not implement the interface ?

Just like the supportStrategy.

@chalasr
Copy link
Member
chalasr commented Aug 24, 2017 •
edited
Loading

@Simperfit nope, because removing the current typehint to allow both would break any subclass of Registry overriding the add() method (fatal)

@dkarlovi
Copy link
Contributor Author
dkarlovi commented Aug 28, 2017 •
edited
Loading

So, how do we tackle this? I'm guessing introducing the interface, attaching to workflow and testing the instance in all places that accept Workflow?

@chalasr
Copy link
Member
chalasr commented Aug 28, 2017

@dkarlovi We can't remove/change the Workflow typehint in public/protected methods, that would break existing implementations and subclasses.
The only possible path is to let the method unchanged and deprecate it for removing it in 4.0, but actually there are 2 such methods: SupportsStrategyInterface::supports() and Registry::add(). I think add() could be deprecated, but supports() cannot since it is part of an interface and is useful.
About your use case, the decorating Workflow could extend Workflow and override its methods. An interface would give more flexibility, but that's not worth breaking BC.

@chalasr
Copy link
Member
chalasr commented Sep 6, 2017

Unless we want to break BC for this, I think we can close.

@Simperfit
8000 Copy link
Contributor

IMO it could be worth the little BC.

@dkarlovi
Copy link
Contributor Author
dkarlovi commented Sep 6, 2017

Agree with @Simperfit, otherwise you're locked-in to current state "forever" without a clear way forward in the future, and we're talking about only two usage occurrences here AFAIK. If decided to do it in the future, it'll only be as complex or more so, not less.

@lyrixx
Copy link
Member
lyrixx commented Oct 5, 2017

Another approach is to rename the method. What do you think ?

@chalasr
Copy link
Member
chalasr commented Oct 5, 2017 •
edited
Loading

@lyrixx 👍 The most annoying is SupportsStrategyInterface though, as we would need to deprecate it entirely (renamed) for changing the typehint in supports().
Imho it's worth changing both Registry and SupportsStrategyInterface, doing that in 3.4 would be best.

@lyrixx
Copy link
Member
lyrixx commented Oct 5, 2017

Wahoo, that so much work :/
Do someone want to do it ?

@dkarlovi
Copy link
Contributor Author
dkarlovi commented Oct 6, 2017

I can do it, what's the scope exactly?

@chalasr
Copy link
Member
chalasr commented Oct 6, 2017
  • Deprecate Registry::add(Workflow $workflow)
  • Add Registry::addWorkflow(WorklowInterface $workflow) (or so)
  • Duplicate Registry::add() tests and use Registry::addWorkflow() into
  • Mark Registry::add() tests as legacy, asserting the deprecation
  • Deprecate SupportsStrategyInterface
  • Add a replacement interface, changing supports(Workflow $workflow) to supports(WorkflowInterface $workflow)
  • Duplicate tests relying on this interface to have new ones using the new interface
  • Mark the tests relying on this interface as legacy
  • Document the changes in Workflow/CHANGELOG and UPGRADE files

I think it's too late for 3.4. If I'm right, PR should be submitted against master.

@dkarlovi
Copy link
Contributor Author
dkarlovi commented Oct 8, 2017

Thanks for the list @chalasr, I'll start on it this week.

@Simperfit Simperfit mentioned this issue Oct 30, 2017
Merged
@dkarlovi dkarlovi mentioned this issue Oct 30, 2017
Closed
lyrixx added a commit that referenced this issue Dec 7, 2017
@lyrixx
feature #24751 [Workflow] Introduce a Workflow interface (Simperfit)
7c0b1cd 
This PR was merged into the 4.1-dev branch.

Discussion
----------

[Workflow] Introduce a Workflow interface

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #23910
| License       | MIT
| Doc PR        | todo

@chalasr I think all the points you made in 23910 has been done. Needs to update the docs too.

Commits
-------

e8351d8 [Workflow] Introduce a Workflow interface
@lyrixx lyrixx closed this as completed Dec 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Workflow
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dkarlovi @lyrixx @xabbuh @Simperfit @chalasr
0