[Workflow] Introduce concept of SupportStrategyInterface #20751

andesk · 2016-12-04T22:24:55Z

Q	A
Branch?	3.2
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	-

As discussed during #symfonycon with @Nyholm and @xabbuh , in it's first implementation, the Workflow component's Registry class is checking on a subject's class instance to decide, if it is supported or not. In our current project we are using a CMS which uses a generic class to hold content of any content type. But we would want to be able to use the registry to identify the supported workflow for a content by checking the class's content type. As this is a very propriatary use case, but I believe similar requirements might be coming for other projects, I decided to introduce a strategy pattern to generalize it.

Disclaimer: Current state is PoC to indicate the need, feel free to find/suggest a different approach or give me directions to adjust/change it.

As the Registry:add() method expected a $className (string) attribute and I changed it to accept any SupportStrategyInterface instead, this PR introduces a BC break. As the component is quite fresh and the Registry is kind of a helper construct, we might be ok to introduce this BC nevertheless? I tried to keep the "supports" configuration key as it is to avoid conflicts with existing workflow service definitions and keep the changes as local as possible. Other ideas to avoid this are welcome of course :)

While adjusting the existing tests for FrameworkBundle I realized that probably not every Workflow related code is covered by tests yet. I decided to skip adding more tests (at least for now) as we first need to agree on if we introduce a generalized approach at all and if the current one is acceptable to others.

…r support checks than class instance

andesk · 2016-12-04T22:26:20Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ValidateWorkflowsPass.php

@@ -38,6 +38,9 @@ public function process(ContainerBuilder $container)
                if (!array_key_exists('marking_store', $tag)) {
                    throw new RuntimeException(sprintf('The "marking_store" for the tag "workflow.definition" of service "%s" must be set.', $id));
                }
+                if (!array_key_exists('supports', $tag) && !array_key_exists('support_strategy', $tag)) {


Before this, "supports" was marked as required in Configuration.php, see below.

This kind of validation should be done in the Configuration class

K, will fix that.

andesk · 2016-12-04T22:27:40Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                        Workflow\SupportStrategy\ClassInstanceSupportStrategy::class, array($supportedClassName)
+                    );
+                    $strategyDefinition->setPublic(false);
+                    $registryDefinition->addMethodCall('add', array(new Reference($workflowId), $supportedClassName));


So instead of using the supported class name directly, I am more or less wrapping it in a support strategy service definition.

andesk · 2016-12-04T22:28:42Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                }
+            }
+            if (isset($workflow['support_strategy'])) {
+                $registryDefinition->addMethodCall('add', array(new Reference($workflowId), new Reference($workflow['support_strategy'])));


As a second option, "support_strategy" expects to be a reference to a registered service implementing the introduced SupportStrategyInterface.

andesk · 2016-12-04T22:29:15Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

@@ -165,6 +165,10 @@ public function testWorkflows()
        $this->assertSame(array('workflow.definition' => array(array('name' => 'pull_request', 'type' => 'state_machine', 'marking_store' => 'single_state'))), $stateMachineDefinition->getTags());
        $this->assertCount(9, $stateMachineDefinition->getArgument(1));
        $this->assertSame('start', $stateMachineDefinition->getArgument(2));
+
+        $this->assertTrue($container->hasDefinition('workflow.registry', 'Workflow registry is registered as a service'));


Until now, there was not coverage for registry at all. I did not want to introduce too much, but the following should we ok?

andesk · 2016-12-04T22:29:35Z

src/Symfony/Component/Workflow/Event/Event.php

@@ -37,16 +37,25 @@ public function __construct($subject, Marking $marking, Transition $transition)
        $this->transition = $transition;
    }

+    /**
+     * @return Marking


Added some docblocks, never bad to have them :)

It does not bring anything IMHO. and it's not related to your PR.

So should I revert it? Or create a separate PR? For me, adding some of these pointed out some details. Sure I might see that looking at the code, but...

Yes, should be revert. IDEs should be able to infer the return type from code (as the $marking property is private and is set during initialisation).

Yes, should be reverted. IDEs should be able to infer the return type from code (as the $marking property is private and is set during initialisation).

andesk · 2016-12-04T22:31:06Z

src/Symfony/Component/Workflow/Registry.php

     */
-    public function add(Workflow $workflow, $className)
+    public function add(Workflow $workflow, SupportStrategyInterface $supportStrategy)


So here is the mentioned BC break... Alternatively we could intrdoduce a second method or keep $className and introduce an additional optional parameter for support strategy? Would be ok with my use case and probably with most others as well?

Regardless of architecture (not familiar with the workings), you don't implement an interface here. This means you can safely remove the typehint and do a type check:

@param SupportStrategyInterface|string public function add(Workflow $workflow, $supportStrategy) if not instance of SupportStrategyInterface if not string throw invalid argument exception trigger deprecation, argument must be a SupportStrategyInterface as of 4.0

True, one could do that to avoid BC, but it's introducing kind of magic below, isn't it?

I ike the deprecation idea though 👍

It's the standard way of introducing a BC layer in symfony. It's kinda like method overloading

So should I change my code accordingly? ping @lyrixx

@andesk yes. Your current proposal is a BC break, and cannot be accepted for Symfony 3.3 (and would not even be acceptable for Symfony 4.0 as is, as it does not allow to have a progressive upgrade path)

K, will do :)

Should I revert changes in Extension back to inject class name string only instead of wrapping it into the ClassInstanceSupportStrategy? Or keep them as that might be the target for 4.0 anyways?

no, don't revert this one, as injecting class names directly will trigger a deprecation warning. We don't want the normal configuration of the bundle to trigger a deprecation warning for our users (as they would not be in control of fixing it anyway)

andesk · 2016-12-04T22:32:28Z

src/Symfony/Component/Workflow/Tests/RegistryTest.php

@@ -64,6 +65,17 @@ public function testGetWithNoMatch()
        $this->assertInstanceOf(Workflow::class, $w1);
        $this->assertSame('workflow1', $w1->getName());
    }
+
+    private function getSupportStrategy($supportedClassName)


The following might not be the best idea, but keeps the changes minimal...

lyrixx · 2016-12-05T09:31:51Z

Hello @andesk

Why can't you use the name of workflow to be able to get it? (the second argument of Registry::get)

andesk · 2016-12-05T09:53:17Z

Hi @lyrixx

Well, we don't want to hardcode the workflow's name to fetch it at all. It's kind of a generic approach to allow having different workflows for different content types in CMS (articles might need extended workflow, images a shorter one, etc), but rely on workflow configuration and the registry to identify the correct one. Is that use case so unexpected? :)

andesk · 2016-12-05T15:27:41Z

src/Symfony/Component/Workflow/Registry.php

    {
-        $this->workflows[] = array($workflow, $className);
+        if (!$supportStrategy instanceof SupportStrategyInterface) {


As mentioned in previous comments, to avoid BC break we are allowing string as a fallback, checking it and otherwise throw deprecation warning.

No BC break anymore after doing suggested changes thanks to @iltar Though it seems triggering the deprecated error leads to CI integrations to fail... Any ideas how to fix this?

Thanks to @xabbuh I figured out the @group legacy annotation. Should be fine now :)

andesk · 2016-12-05T15:29:22Z

Updated the PR introducing suggested handling to avoid BC break. Anything else?

andesk · 2016-12-10T10:43:24Z

Hmmm, fabbot.io is pending since days now. Any ideas how to find out what went wrong there? @nicolas-grekas maybe?

xabbuh · 2016-12-10T10:53:14Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -324,6 +326,10 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode)
                                ->end()
                            ->end()
                        ->end()
+                        ->validate()
+                            ->ifTrue(function ($v) { return isset($v['supports']) && isset($v['support_strategy']); })
+                            ->thenInvalid('"supports" and "support_strategy" cannot be used together.')


Don't we also need to make sure that one of supports or support_strategy is configured?

Have not seen other examples for that yet, but of course I can add a second validation for both being !isset()?

something like empty('supports') && empty('support_strategy')

xabbuh · 2016-12-10T10:57:36Z

src/Symfony/Component/Workflow/Registry.php

-        $this->workflows[] = array($workflow, $className);
+        if (!$supportStrategy instanceof SupportStrategyInterface) {
+            if (!is_string($supportStrategy)) {
+                throw new InvalidArgumentException('Expecting instance of SupportStrategyInterface or class name as a string.');


I don't think we need to have this check. The docblock is pretty about what types you are allowed to pass.

That was suggested by @iltar and I think it's fine to keep it as a check?

If I am not mistaken, we don't do similar checks anywhere else, but rely on the fact that we claimed through the docblock which types we expect.

Any other oppinion on that? Should I remove it? ping @lyrixx @Nyholm

Indeed this should be removed.

xabbuh · 2016-12-10T10:58:26Z

src/Symfony/Component/Workflow/Registry.php

@@ -23,19 +24,34 @@ class Registry

    /**
     * @param Workflow $workflow


this parameter needs to be aligned with the following line

Sorry, don't get your point. What do you mean?

Ah, Symfony uses formatting of docblocks with aligned intendions?

xabbuh · 2016-12-10T11:03:51Z

src/Symfony/Component/Workflow/Registry.php

+     * @param null|string $workflowName
+     *
+     * @return bool
+     */


no need for this docblock IMO as the method is private

Several other private methods (e.g. in Workflow.php) have a docblock as well. Is there any rule of thumb when to add it? I personally like having docblocks, but if they are not common in Symfony core I am fine with removing those.

xabbuh · 2016-12-10T11:03:51Z

src/Symfony/Component/Workflow/SupportStrategy/SupportStrategyInterface.php

+interface SupportStrategyInterface
+{
+    /**
+     * @param \Symfony\Component\Workflow\Workflow $workflow


no need for the FQCN

# Conflicts: # src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

lyrixx · 2016-12-13T17:01:24Z

How would you use it from a twig template?

andesk · 2016-12-13T17:52:12Z

@lyrixx Not sure what you mean. This change does not break anything in twig usage. And as there was no support to call registry from twig before, I did not have anything to adjust. My current use case: I use the registry service in a REST endpoint. Introducing a twig function is out of scope to me, no?

lyrixx · 2016-12-13T18:05:49Z

If you update the Registry, you have to update this file. And, I don't know how to use these twig functions after that.

andesk · 2016-12-13T18:48:38Z

@lyrixx This should work in the same way as before? No changes made to this behavior. Or maybe I don't get your point? The public "API" of registry class stays exactly as it is. The only change is how to decide, which workflow is supposed to be used for a certain object. Until now, only instanceof class was supported, now with introduction of the strategy, anything can be used internally. From the outside, nothing changes. Only change is the (for now) optional configuration of a Strategy service in workflow configuration.

lyrixx · 2016-12-13T20:19:43Z

You are right. I missed something. I'm OK with this PR. But Could you rebase before the merge. Thanks.

Nyholm

I like this. I just had a minor comment.

Nyholm · 2016-12-13T20:45:27Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

-                $registryDefinition->addMethodCall('add', array(new Reference($workflowId), $supportedClass));
+            if (isset($workflow['supports'])) {
+                foreach ($workflow['supports'] as $supportedClassName) {
+                    $strategyDefinition = new Definition(


You never register the $strategyDefinition in the container.

Should be put on one line.

Nyholm · 2016-12-13T20:48:06Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                        Workflow\SupportStrategy\ClassInstanceSupportStrategy::class, array($supportedClassName)
+                    );
+                    $strategyDefinition->setPublic(false);
+                    $registryDefinition->addMethodCall('add', array(new Reference($workflowId), $supportedClassName));


You should use $strategyDefinition here or deprecation notice will always trigger.

Oh wow, how could I not realize that?! Thank you @Nyholm , will fix

# Conflicts: # src/Symfony/Component/Workflow/Registry.php

…k bundle

fabpot · 2016-12-15T06:54:55Z

src/Symfony/Component/Workflow/Registry.php

     *
     * @return Workflow
+     * @throws InvalidArgumentException


missing empty line before this line

fabpot · 2016-12-15T06:55:19Z

src/Symfony/Component/Workflow/Tests/RegistryTest.php

+    {
+        $strategy = $this->getMockBuilder(SupportStrategyInterface::class)->getMock();
+        $strategy->expects($this->any())->method('supports')
+            ->will($this->returnCallback(function($workflow, $subject) use ($supportedClassName) {


missing space after function

fabpot · 2016-12-15T06:55:55Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -436,7 +436,7 @@ private function registerWorkflowConfiguration(array $workflows, ContainerBuilde

            // Create MarkingStore
            if (isset($workflow['marking_store']['type'])) {
-                $markingStoreDefinition = new DefinitionDecorator('workflow.marking_store.'.$workflow['marking_store']['type']);
+                $markingStoreDefinition = new DefinitionDecorator('workflow.marking_store.' . $workflow['marking_store']['type']);


to be reverted

fabpot · 2016-12-15T06:56:41Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                    $registryDefinition->addMethodCall('add', array(new Reference($workflowId), $strategyDefinition));
+                }
+            }
+            if (isset($workflow['support_strategy'])) {


could probably be an elseif as the two options are exclusive

fabpot · 2016-12-15T06:57:04Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+            if (isset($workflow['supports'])) {
+                foreach ($workflow['supports'] as $supportedClassName) {
+                    $strategyDefinition = new Definition(
+                        Workflow\SupportStrategy\ClassInstanceSupportStrategy::class, array($supportedClassName)


You should use a use statement instead of using the FQCN here.

fabpot · 2016-12-15T06:58:17Z

src/Symfony/Component/Workflow/Registry.php

-        $this->workflows[] = array($workflow, $className);
+        if (!$supportStrategy instanceof SupportStrategyInterface) {
+            if (!is_string($supportStrategy)) {
+                throw new InvalidArgumentException('Expecting instance of SupportStrategyInterface or class name as a string.');


Indeed this should be removed.

fabpot · 2016-12-15T06:58:29Z

src/Symfony/Component/Workflow/Registry.php

    }

    /**
     * @param object      $subject
-     * @param string|null $workflowName
+     * @param null|string $workflowName


should be reverted

fabpot · 2016-12-15T06:59:07Z

src/Symfony/Component/Workflow/SupportStrategy/ClassInstanceSupportStrategy.php

+
+class ClassInstanceSupportStrategy implements SupportStrategyInterface
+{
+    /** @var string */


Comment should be removed

fabpot · 2016-12-15T06:59:22Z

src/Symfony/Component/Workflow/SupportStrategy/ClassInstanceSupportStrategy.php

+
+    /**
+     * @param string $className
+     */


docblock should be removed

andesk · 2016-12-17T10:58:31Z

@fabpot Fixed proposed changes, fabbot.io still complaining about code style in FrameworkExtension class not related to this story. Anything I can do about it?

…xception

lyrixx · 2016-12-19T09:36:31Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

-                $registryDefinition->addMethodCall('add', array(new Reference($workflowId), $supportedClass));
+            if (isset($workflow['supports'])) {
+                foreach ($workflow['supports'] as $supportedClassName) {
+                    $strategyDefinition = new Definition(


Should be put on one line.

lyrixx · 2016-12-19T09:46:24Z

src/Symfony/Component/Workflow/Tests/RegistryTest.php

+        $this->assertSame('workflow2', $workflow->getName());
+    }
+
+    private function getSupportStrategy($supportedClassName)


Should be named createSupportStrategy

lyrixx · 2016-12-19T09:46:55Z

src/Symfony/Component/Workflow/Tests/RegistryTest.php

+
+    private function getSupportStrategy($supportedClassName)
+    {
+        $strategy = $this->getMockBuilder(SupportStrategyInterface::class)->getMock();


$this->getMock(SupportStrategyInterface::class) is enough

According to #20984 the current syntax should be kept

lyrixx · 2016-12-19T09:49:55Z

src/Symfony/Component/Workflow/Registry.php

    {
-        if (!$subject instanceof $className) {
+        if (is_string($supportStrategy) && !$subject instanceof $supportStrategy) {


Could you add a comment before this line:

// Keep BC. To be removed in 4.0

Rather than dealing with BC here, I would convert the string to a SupportStrategy when setting it instead.

So @stof , you mean that inside the add() method I would create a ClassInstanceSupportStrategy in the deprecation case?

Should I still add a BC comment somewhere? ping @lyrixx ?

The two conditions could then be simplified to:

if (!$supportStrategy->supports($workflow, $subject)) { return false; }

lyrixx · 2016-12-19T09:54:24Z

src/Symfony/Component/Workflow/Registry.php

            return true;
        }

-        return $name === $workflow->getName();
+        return $workflowName === $workflow->getName();


All this method could be simplified:

if (null !== $workflowName && $workflowName !== $workflow->getName()) { return false; } // Keep BC. To be removed in 4.0 if (is_string($supportStrategy)) { return $subject instanceof $supportStrategy; } return $supportStrategy->supports($workflow, $subject);

So if @stof 's idea above will be implemented, this can even be more simplified, right?

indeed.

(Please, ping me when you're ready. Thanks)

lyrixx · 2016-12-19T09:54:59Z

src/Symfony/Component/Workflow/Tests/RegistryTest.php

+    {
+        $registry = new Registry();
+
+        $registry->add(new Workflow(new Definition(array(), array()), $this->getMockBuilder(MarkingStoreInterface::class)->getMock(), $this->getMockBuilder(EventDispatcherInterface::class)->getMock(), 'workflow1'), Subject1::class);


$this->getMock(SupportStrategyInterface::class) is enough
(same bellow)

According to #20984 the current syntax should be kept

lyrixx · 2016-12-19T09:56:02Z

src/Symfony/Component/Workflow/Tests/RegistryTest.php

+        $strategy = $this->getMockBuilder(SupportStrategyInterface::class)->getMock();
+        $strategy->expects($this->any())->method('supports')
+            ->will($this->returnCallback(function ($workflow, $subject) use ($supportedClassName) {
+                return $subject instanceof $supportedClassName;


Why do you use a mock here when you can use the ClassInstanceSupportStrategy ?

Don't know how this is handled in Symfony environment, but as I want to test the registry alone I personally prefer mocking all other collaborators. Should I change it?

lyrixx · 2016-12-26T18:47:05Z

Hello @andesk

Could you take time to finish this PR? If not, I could finish it for you if you allow me to push in your fork.

Thanks ;)

stof · 2016-12-26T21:30:47Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -287,6 +286,9 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode)
                                    ->end()
                                ->end()
                            ->end()
+                            ->scalarNode('support_strategy')
+                                ->cannotBeEmpty()


this forbids resetting it to null in a later file to use supports again

So should I simply remove it?

stof · 2016-12-26T21:31:39Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -325,6 +327,10 @@ private function addWorkflowSection(ArrayNodeDefinition $rootNode)
                                ->end()
                            ->end()
                        ->end()
+                        ->validate()
+                            ->ifTrue(function ($v) { return isset($v['supports']) && isset($v['support_strategy']); })


supports will always be set as it is a prototyped array node and so has a default. You need to check whether it is the empty array instead.

So return !empty($v['supports']) && isset($v['support_strategy']); ?

stof · 2016-12-26T21:33:02Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -458,8 +459,16 @@ private function registerWorkflowConfiguration(array $workflows, ContainerBuilde
            $container->setDefinition(sprintf('%s.definition', $workflowId), $definitionDefinition);

            // Add workflow to Registry
-            foreach ($workflow['supports'] as $supportedClass) {
-                $registryDefinition->addMethodCall('add', array(new Reference($workflowId), $supportedClass));
+            if (isset($workflow['supports'])) {


it is always set here. You should check for the array not being empty

So if (!empty($workflow['supports'])) instead?

having a test covering this combination would probably be good

stof · 2016-12-26T21:35:29Z

src/Symfony/Component/Workflow/Registry.php

    }

    /**
     * @param object      $subject
     * @param string|null $workflowName
     *
     * @return Workflow
+     *
+     * @throws InvalidArgumentException


we don't add this phpdoc when we cannot document in which case the exception is thrown

stof · 2016-12-26T21:36:58Z

src/Symfony/Component/Workflow/Registry.php

    {
-        if (!$subject instanceof $className) {
+        if (is_string($supportStrategy) && !$subject instanceof $supportStrategy) {


Rather than dealing with BC here, I would convert the string to a SupportStrategy when setting it instead.

…ed yet

xabbuh · 2017-01-17T21:42:12Z

src/Symfony/Component/Workflow/SupportStrategy/ClassInstanceSupportStrategy.php

+
+use Symfony\Component\Workflow\Workflow;
+
+class ClassInstanceSupportStrategy implements SupportStrategyInterface


you should add a docblock and add yourself as an author

same for the interface

and the class should become final

xabbuh · 2017-01-17T21:43:05Z

src/Symfony/Component/Workflow/SupportStrategy/ClassInstanceSupportStrategy.php

+{
+    private $className;
+
+    public function __construct($className)


you should add a docblock documenting the argument as a string

lyrixx · 2017-01-18T14:16:26Z

Closing in favor of #21334

…ce (andesk, lyrixx) This PR was merged into the 3.3-dev branch. Discussion ---------- [Workflow] Introduce concept of SupportStrategyInterface | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #20751 | License | MIT | Doc PR | - Commits ------- 134a58b [Workflow] Fixed code and tests 1843012 [Workflow] Introduce concept of SupprtStrategyInterface to allow other support checks than class instance

This PR was merged into the 3.3-dev branch. Discussion ---------- [Workflow] sync the changelog | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20751, #21334, #21933, #21935, #21950 | License | MIT | Doc PR | Commits ------- 98a18ee [Workflow] sync the changelog

[Workflow] Introduce concept of SupprtStrategyInterface to allow othe…

6421c10

…r support checks than class instance

carsonbot added Status: Needs Review Workflow Feature BC Break labels Dec 4, 2016

andesk commented Dec 4, 2016

View reviewed changes

[Workflow] Refactoring to resolve BC break and adding deprecated warning

bfd3611

andesk commented Dec 5, 2016

View reviewed changes

andesk changed the title ~~[PoC][Workflow] Introduce concept of SupportStrategyInterface~~ [Workflow] Introduce concept of SupportStrategyInterface Dec 5, 2016

[Workflow] Moving and tagging legacy unit tests

c0cbc85

nicolas-grekas removed the BC Break label Dec 6, 2016

nicolas-grekas added this to the 3.3 milestone Dec 6, 2016

xabbuh reviewed Dec 10, 2016

View reviewed changes

andesk added 3 commits December 10, 2016 19:15

[Workflow] Some CS fixes

fa448af

[Workflow] Removing some docblocks

19fb676

Merge remote-tracking branch 'upstream/3.2' into 3.2

faddc97

# Conflicts: # src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

Nyholm requested changes Dec 13, 2016

View reviewed changes

andesk added 2 commits December 14, 2016 23:51

Merge remote-tracking branch 'upstream/3.2' into 3.2

a115bdd

# Conflicts: # src/Symfony/Component/Workflow/Registry.php

[Workflow] fixing service definition of workflow registry in framewor…

d22fe01

…k bundle

Nyholm approved these changes Dec 14, 2016

View reviewed changes

fabpot reviewed Dec 15, 2016

View reviewed changes

andesk added 2 commits December 17, 2016 10:54

[Workflow] fixing code style mostly, removing exception

047a2e7

[Workflow] tiny cs fix in docblock...

a34e4af

andesk added 2 commits December 17, 2016 11:08

[Workflow] removing deprecated test case for removed InvalidArgumentE…

1e65b12

…xception

Merge remote-tracking branch 'upstream/3.2' into 3.2

f4e0d62

lyrixx requested changes Dec 19, 2016

View reviewed changes

stof requested changes Dec 26, 2016

View reviewed changes

[Workflow] Some more adaptions after code review comments, not finish…

2d11004

…ed yet

xabbuh reviewed Jan 17, 2017

View reviewed changes

lyrixx mentioned this pull request Jan 18, 2017

[Workflow] Introduce concept of SupportStrategyInterface #21334

Merged

lyrixx closed this Jan 18, 2017

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

xabbuh mentioned this pull request Apr 5, 2017

[Workflow] sync the changelog #22291

Merged

		@@ -23,19 +24,34 @@ class Registry

		/**
		* @param Workflow $workflow


		use Symfony\Component\Workflow\Workflow;

		class ClassInstanceSupportStrategy implements SupportStrategyInterface

[Workflow] Introduce concept of SupportStrategyInterface #20751

[Workflow] Introduce concept of SupportStrategyInterface #20751

Conversation

andesk commented Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Dec 5, 2016

andesk commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andesk commented Dec 5, 2016

andesk commented Dec 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Dec 13, 2016

andesk commented Dec 13, 2016

lyrixx commented Dec 13, 2016 • edited Loading

andesk commented Dec 13, 2016

lyrixx commented Dec 13, 2016

Nyholm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andesk commented Dec 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andesk commented Dec 4, 2016 •

edited

Loading

lyrixx commented Dec 13, 2016 •

edited

Loading