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

[Workflow] Introduce a Workflow interface #24751

Merged
merged 1 commit into from Dec 7, 2017

Conversation

@Simperfit
Contributor

Simperfit commented Oct 30, 2017

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.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 30, 2017

Note that changing a public interface is a BC break, so this cannot be merged as is, isn't it?

@chalasr

This comment has been minimized.

Member

chalasr commented Oct 30, 2017

@nicolas-grekas the upgrade note is misleading, this replaces the whole interface.

@chalasr chalasr self-requested a review Oct 30, 2017

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Oct 30, 2017

@lyrixx

Some minor comments, but it's good.
Thanks.

* Deprecated the `add` method in favor of the `addWorkflow` method in `Workflow\Registry`
* Deprecated the interface `SupportStrategyInterface` in favor of the interface `SupportStrategryWorkflowInterface`

This comment has been minimized.

@lyrixx

lyrixx Nov 2, 2017

Member

Looks like there is a space here

@@ -5,6 +5,9 @@ CHANGELOG
-----
* Removed class name support in `WorkflowRegistry::add()` as second parameter.
* Deprecate the usage of `add(Workflow $workflow, $supportStrategy) in `Workflow/Registry` use `addWorkflow(WorkflowInterface, $supportStrategy)` instead

This comment has been minimized.

@lyrixx

lyrixx Nov 2, 2017

Member

missing backquote at the end of add(Workflow $workflow, $supportStrategy)

@@ -28,8 +29,17 @@ class Registry
*/
public function add(Workflow $workflow, $supportStrategy)
{
if (!$supportStrategy instanceof SupportStrategyInterface) {
throw new \InvalidArgumentException('The "supportStrategy" is not an instance of SupportStrategyInterface.');
@trigger_error('add is deprecated since Symfony 4.0. Use addWorkflow instead', E_USER_DEPRECATED);

This comment has been minimized.

@lyrixx

lyrixx Nov 2, 2017

Member

'add' method is ... use 'addWorkflow' insteand.
(don't miss the final dot)

public function addWorkflow(WorkflowInterface $workflow, $supportStrategy)
{
if ($supportStrategy instanceof SupportStrategyInterface) {
@trigger_error('Passing a SupportStrategyInterface in Registry::addWorkflow is deprecated since 4.0. Use SupportStrategyWorkflowInterface instead', E_USER_DEPRECATED);

This comment has been minimized.

@lyrixx

lyrixx Nov 2, 2017

Member

use quote to surround class name and add a final dot.

if ($supportStrategy instanceof SupportStrategyInterface) {
@trigger_error('Passing a SupportStrategyInterface in Registry::addWorkflow is deprecated since 4.0. Use SupportStrategyWorkflowInterface instead', E_USER_DEPRECATED);
}
if (!($supportStrategy instanceof SupportStrategyWorkflowInterface || $supportStrategy instanceof SupportStrategyInterface)) {

This comment has been minimized.

@lyrixx

lyrixx Nov 2, 2017

Member

this if could be transformed in an elseif (!$supportStrategy instanceof SupportStrategyWorkflowInterface)

@@ -11,6 +11,8 @@
namespace Symfony\Component\Workflow\SupportStrategy;
@trigger_error('SupportStrategyInterface is deprecated since Symfony 4.0. Use SupportStrategyWorkflowInterface instead', E_USER_DEPRECATED);

This comment has been minimized.

@lyrixx

lyrixx Nov 2, 2017

Member

please add a final dot

*
* @return bool true if the transition is enabled
*/
public function can($subject, $transitionName);

This comment has been minimized.

@lyrixx

lyrixx Nov 2, 2017

Member

Hm, if getMarking can throw an exception, then can and apply may throw an exception too

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 2, 2017

@lyrixx Thanks for the review :)

@@ -913,7 +913,9 @@ Workflow
--------
* Removed class name support in `WorkflowRegistry::add()` as second parameter.
* Deprecated the `add` method in favor of the `addWorkflow` method in `Workflow\Registry`
* Deprecated the interface `SupportStrategyInterface` in favor of the interface `SupportStrategryWorkflowInterface`

This comment has been minimized.

@xabbuh

xabbuh Nov 2, 2017

Member

typo here (strategry)

* Deprecated the `add` method in favor of the `addWorkflow` method in `Workflow\Registry`
* Deprecated the interface `SupportStrategyInterface` in favor of the interface `SupportStrategyWorkflowInterface`

This comment has been minimized.

@lyrixx

lyrixx Nov 3, 2017

Member

And now there are two spaces here. You need to configure your editor to remove automatically all trailing spaces :D

This comment has been minimized.

@Simperfit

Simperfit Nov 3, 2017

Contributor

Yeah, you are right, I didn't configure it to do so !

{
if ($supportStrategy instanceof SupportStrategyInterface) {
@trigger_error('Passing a "SupportStrategyInterface" in "Registry::addWorkflow" is deprecated since 4.0. Use "SupportStrategyWorkflowInterface" instead.', E_USER_DEPRECATED);
} elseif (!($supportStrategy instanceof SupportStrategyWorkflowInterface || $supportStrategy instanceof SupportStrategyInterface)) {

This comment has been minimized.

@lyrixx

lyrixx Nov 3, 2017

Member

It looks like you did not simplified the condition.

This comment has been minimized.

@Simperfit

Simperfit Nov 3, 2017

Contributor

Yeah, sorry, that's modified.

public function addWorkflow(WorkflowInterface $workflow, $supportStrategy)
{
if ($supportStrategy instanceof SupportStrategyInterface) {
@trigger_error('Passing a "SupportStrategyInterface" in "Registry::addWorkflow" is deprecated since 4.0. Use "SupportStrategyWorkflowInterface" instead.', E_USER_DEPRECATED);

This comment has been minimized.

@chalasr

chalasr Nov 3, 2017

Member

strings should be replaced SupportsStrategyInterface::class, __METHOD and SupportStrategyWorkflowInterface::class in this order, we always use FQCNs

This comment has been minimized.

@chalasr

chalasr Nov 3, 2017

Member

same for the exception below

/**
* @author Andreas Kleemann <akleemann@inviqa.com>
*/
final class ClassInstanceSupportStrategy implements SupportStrategyInterface
final class ClassInstanceSupportStrategy implements SupportStrategyWorkflowInterface

This comment has been minimized.

@chalasr

chalasr Nov 3, 2017

Member

Not implementing the old interface is a BC break ($strategy instanceof SupportsStrategyInterface returning false). Personally I wouldn't care, this could be internal

This comment has been minimized.

@Simperfit

Simperfit Nov 4, 2017

Contributor

I think this is internal too.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 4, 2017

Member

Internal should be hinted by an annotation, we can't have a policy of telling "that's internal" and break BC.
I suggest to open a PR against 3.4 to really mark them internal so that we're free to change in 4.1.

This comment has been minimized.

@Simperfit

Simperfit Nov 4, 2017

Contributor

Ill open that PR.

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 4, 2017

Status: Needs Review

travis failure is unrelated

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 4, 2017

@nicolas-grekas @xabbuh PR changed with the new behaviour deprecating ClassInstanceSupportStrategy and creating 4.1 files, since this will be in 4.1 and not in 4.0.

Thanks for the review.

@lyrixx

lyrixx approved these changes Nov 6, 2017

👍 (but there is one tiny comment)

@@ -913,7 +913,7 @@ Workflow
--------
* Removed class name support in `WorkflowRegistry::add()` as second parameter.

This comment has been minimized.

@lyrixx

lyrixx Nov 6, 2017

Member

oups ;)

@xabbuh

This comment has been minimized.

Member

xabbuh commented Nov 6, 2017

We should already start with an UPGRADE-5.0.md guide which explains the consequences this will have in 5.0 (removed methods and interfaces).

--------
* Deprecated the `add` method in favor of the `addWorkflow` method in `Workflow\Registry`
* Deprecated the interface `SupportStrategyInterface` in favor of the interface `SupportStrategyWorkflowInterface`

This comment has been minimized.

@xabbuh

xabbuh Nov 6, 2017

Member

we could omit the word "interface" (it's already part of the interface name)

@@ -38,7 +38,7 @@ protected function setUp()
$workflow = new Workflow($definition);
$registry = new Registry();
$registry->add($workflow, new ClassInstanceSupportStrategy(\stdClass::class));
$registry->addWorkflow($workflow, new ClassInstanceSupportStrategyWorkflow(\stdClass::class));

This comment has been minimized.

@xabbuh

xabbuh Nov 6, 2017

Member

I think we should keep a test case for the deprecated method to make sure we don't break it (flagging it as legacy then of course)

@@ -28,8 +29,16 @@ class Registry
*/
public function add(Workflow $workflow, $supportStrategy)
{
if (!$supportStrategy instanceof SupportStrategyInterface) {
throw new \InvalidArgumentException('The "supportStrategy" is not an instance of SupportStrategyInterface.');
@trigger_error(sprintf('%s is deprecated since Symfony 4.0. Use addWorkflow instead.', __METHOD__), E_USER_DEPRECATED);

This comment has been minimized.

@xabbuh

xabbuh Nov 6, 2017

Member

missing parentheses after the method name

This comment has been minimized.

@xabbuh

xabbuh Nov 6, 2017

Member

[...] since Symfony 4.1. [...]

This comment has been minimized.

@xabbuh

xabbuh Nov 6, 2017

Member

[...] Use the addWorkflow() method instead.

public function addWorkflow(WorkflowInterface $workflow, $supportStrategy)
{
if ($supportStrategy instanceof SupportStrategyInterface) {
@trigger_error(sprintf('Passing a "%s" in "%s" is deprecated since 4.0. Use "%s" instead.', SupportStrategyInterface::class, __METHOD__, SupportStrategyWorkflowInterface::class), E_USER_DEPRECATED);

This comment has been minimized.

@xabbuh

This comment has been minimized.

@xabbuh

xabbuh Nov 6, 2017

Member

Actually, IMO the new method can easily just require the new interface.

This comment has been minimized.

@Simperfit

Simperfit Nov 7, 2017

Contributor

I guess if we do that we need to tu dupplicate the logic in addWorkflow (there is not much). Maybe we should do that when going to 5.0 and removing this deprecation ?

This comment has been minimized.

@chalasr

chalasr Nov 7, 2017

Member

I agree with @xabbuh.
Removing the deprecation + exception there lets only $this->workflows[] = array($workflow, $supportStrategy); to duplicate, not a big deal IMHO.

public function testSupportsIfNotClassInstance()
{
$strategy = new ClassInstanceSupportStrategy('Symfony\Component\Workflow\Tests\SupportStrategy\Subject2');
$strategy = new ClassInstanceSupportStrategy('Symfony\Component\Workflow\Tests\SupportStrategy\Subject4');

This comment has been minimized.

@chalasr

chalasr Nov 7, 2017

Member

Could use ::class too since the line is updated, same above

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 8, 2017

@chalasr @xabbuh I've taken care of the comments

* `add` method has been removed use `addWorkflow` method in `Workflow\Registry` instead
* `SupportStrategyInterface` has been removed use `WorkflowSupportStrategyInterface` instead
* `ClassInstanceSupportStrategy` has been removed use `InstanceOfSupportStrategy` instead

This comment has been minimized.

@chalasr

chalasr Nov 8, 2017

Member

, before use for the last two lines

* @param Workflow $workflow
* @param SupportStrategyInterface $supportStrategy
* @param Workflow $workflow
* @param SupportStrategyInterface|WorkflowSupportStrategyInterface $supportStrategy

This comment has been minimized.

@chalasr

chalasr Nov 8, 2017

Member

we usually make phpdoc reflect the new feature, WorkflowSupportStrategyInterface only

*/
final class ClassInstanceSupportStrategy implements SupportStrategyInterface
{
private $className;
public function __construct(string $className)
{
@trigger_error(sprintf('"%s" is deprecated since Symfony 4.1. Use "%s" instead.', self::class, InstanceOfSupportStrategy::class), E_USER_DEPRECATED);

This comment has been minimized.

@chalasr

chalasr Nov 8, 2017

Member

should be outside of the class, just after the namespace

This comment has been minimized.

@chalasr

chalasr Nov 8, 2017

Member

and will be removed in 5.0.

namespace Symfony\Component\Workflow\SupportStrategy;
use Symfony\Component\Workflow\Workflow;
/**
* @author Andreas Kleemann <akleemann@inviqa.com>
*
* @deprecated

This comment has been minimized.

@chalasr

chalasr Nov 8, 2017

Member

... since ...

$this->addWorkflow($workflow, $supportStrategy);
}
public function addWorkflow(WorkflowInterface $workflow, $supportStrategy)

This comment has been minimized.

@chalasr

chalasr Nov 8, 2017

Member

It's a new method, it does not have to support the legacy class. Needs WorkflowSupportStrategyInterface typehint, removing exception+deprec below and keeping only $this->workflows[] = array($workflow, $supportStrategy); in both add() and `addWorkflow() (#24751 (comment)).

This comment has been minimized.

@Simperfit

Simperfit Nov 8, 2017

Contributor

Yes sorry I forgot about that. Ill modify it

4.1.0
-----
* Deprecate the usage of `add(Workflow $workflow, $supportStrategy)` in `Workflow/Registry` use `addWorkflow(WorkflowInterface, $supportStrategy)` instead

This comment has been minimized.

@chalasr

chalasr Nov 8, 2017

Member

, before use, same below

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 8, 2017

Thanks @chalasr done ;)

*
* @return bool
*/
public function supports(WorkflowInterface $workflow, $subject);

This comment has been minimized.

@chalasr

chalasr Nov 10, 2017

Member

Bool return type?

}
/**
* @return string

This comment has been minimized.

@ro0NL

ro0NL Nov 10, 2017

Contributor

aren't we trying to get rid of these kind docblocks?

This comment has been minimized.

@chalasr

chalasr Nov 10, 2017

Member

👍 useless

@@ -6,20 +6,26 @@
use Symfony\Component\Workflow\SupportStrategy\ClassInstanceSupportStrategy;
use Symfony\Component\Workflow\Workflow;
/**
* @group legacy

This comment has been minimized.

@ro0NL

ro0NL Nov 10, 2017

Contributor

this file basically only needs this change.. what about SubjectA + SubjectB for the new test?

This comment has been minimized.

@Simperfit

Simperfit Nov 12, 2017

Contributor

done

This comment has been minimized.

@ro0NL

ro0NL Nov 12, 2017

Contributor

still.. no real reason to optimize/tweak a legacy test (::class notation, class removals). Doesnt hurt; just extra diff :)

This comment has been minimized.

@chalasr

chalasr Nov 12, 2017

Member

indeed, adding the ::class notation was ok because the line was actually changed. Now it just gives merge conflicts, should be reverted

* @param Workflow $workflow
* @param SupportStrategyInterface $supportStrategy
* @param Workflow $workflow
* @param WorkflowSupportStrategyInterface $supportStrategy

This comment has been minimized.

@ro0NL

ro0NL Nov 10, 2017

Contributor

the BC layer actually expects a SupportStrategyInterface? Using the new strategy interface should only go via addWorkflow IMHO.

This comment has been minimized.

@chalasr

chalasr Nov 10, 2017

Member

Agree

@ro0NL

nice work :) i also asked @nicolas-grekas if this is really the desired upgrade path.

Basically i see one other option; to make Registry final since 3.4, so we can keep favoring add(); meaning less upgrade hassle for users as the common usecase would keep working, given Workflow implements WorkflowInterface tomorrow.

Also means we close extensibility on Registry, which we can open by introducing a RegistryInterface, when needed.

@@ -25,13 +26,17 @@ class Registry
/**
* @param Workflow $workflow
* @param SupportStrategyInterface $supportStrategy
*
* @deprecated since Symfony 4.1. Use addWorkflow() instead.

This comment has been minimized.

@ro0NL

ro0NL Nov 12, 2017

Contributor

@deprecated since version 4.1, to be removed in 5.0. Use addWorkflow() instead.

*/
public function add(Workflow $workflow, $supportStrategy)
{
if (!$supportStrategy instanceof SupportStrategyInterface) {

This comment has been minimized.

@ro0NL

ro0NL Nov 12, 2017

Contributor

doesnt hurt to keep right? or why shouldnt it?

@@ -6,20 +6,26 @@
use Symfony\Component\Workflow\SupportStrategy\ClassInstanceSupportStrategy;
use Symfony\Component\Workflow\Workflow;
/**
* @group legacy

This comment has been minimized.

@ro0NL

ro0NL Nov 12, 2017

Contributor

still.. no real reason to optimize/tweak a legacy test (::class notation, class removals). Doesnt hurt; just extra diff :)

*/
public function getEnabledTransitions($subject);
public function getName();

This comment has been minimized.

@ro0NL

ro0NL Nov 12, 2017

Contributor

missing type info + {@inheritdoc} in sub class (id prefer a @return string here).

This comment has been minimized.

@Simperfit

Simperfit Dec 1, 2017

Contributor

maybe we could typehint it instead of phpdoc ?

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 14, 2017

WDYT of @ro0NL's path ?

It's not too late to say that Registry is final since 3.4 @nicolas-grekas ?

I guess it could be a thing too to be able to re-implement the registry, and users won't need to change alot of thing. I think it's a good idea @ro0NL. (i'll take care of the comments, I just want to wait a little for others' thoughts).

@chalasr

This comment has been minimized.

Member

chalasr commented Nov 15, 2017

It's not too late to say that Registry if final since 3.4

It is to me, 3.4 should not receive new deprecations anymore. I think this is fine.

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Nov 16, 2017

Ill take care of the 2 comments.

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Nov 16, 2017

👍 lets go with addWorkflow. It's not like that's super bad or so.. but im not sure which method comes after that ;-) in case we need to patch this signature one day.

@dunglas

This comment has been minimized.

Member

dunglas commented Nov 30, 2017

Needs a rebase.

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 1, 2017

The PR has been rebased

*/
public function getEnabledTransitions($subject);
public function getName(): string;

This comment has been minimized.

@chalasr

chalasr Dec 1, 2017

Member

return type should be removed, otherwise it requires to add it to Workflow::getName() which would break BC

This comment has been minimized.

@Simperfit

Simperfit Dec 1, 2017

Contributor

removed

@chalasr

This comment has been minimized.

Member

chalasr commented Dec 4, 2017

Looks good to me.

@lyrixx

Sorry, I left one (two) extra comment.
I'm 👍 after the changes.

* @param WorkflowInterface $workflow
* @param object $subject
*
* @return bool

This comment has been minimized.

@lyrixx

lyrixx Dec 4, 2017

Member

Sorry about asking again some change, but we don't need this PHP doc. Could you remove it?
Thanks

This comment has been minimized.

@Simperfit

Simperfit Dec 4, 2017

Contributor

Don't be sorry, that's why the PR are made for ;)

Workflow
--------
* Deprecated the `add` method in favor of the `addWorkflow` method in `Workflow\Registry`

This comment has been minimized.

@lyrixx

lyrixx Dec 4, 2017

Member

As I asked a mandatory change, Can I also ask you to add a final dot on each line of each .md updated?

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 4, 2017

done @lyrixx

@lyrixx

lyrixx approved these changes Dec 4, 2017

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 4, 2017

This PR is ready.

@xabbuh

xabbuh approved these changes Dec 7, 2017

@lyrixx

This comment has been minimized.

Member

lyrixx commented Dec 7, 2017

It took time, but here we go, this is in now. Thank you very much Hamza.

@lyrixx lyrixx merged commit e8351d8 into symfony:master Dec 7, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

lyrixx added a commit that referenced this pull request Dec 7, 2017

feature #24751 [Workflow] Introduce a Workflow interface (Simperfit)
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
@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 7, 2017

🎉 Thanks for your time @lyrixx @ro0NL @xabbuh @chalasr @nicolas-grekas

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