Skip to content
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] Deprecate MultipleStateMarkingStore and SingleStateMarkingStore in favor of MethodMarkingStore #30551

Merged
merged 1 commit into from
Mar 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions UPGRADE-4.3.md
Original file line number Diff line number Diff line change
Expand Up @@ -177,3 +177,46 @@ Workflow
}
}
```

* `MultipleStateMarkingStore` is deprecated. Use `MethodMarkingStore` instead.

Before:
```yaml
framework:
workflows:
article:
marking_store:
type: multiple
```

After:
```yaml
framework:
workflows:
article:
marking_store:
type: method

```

* `SingleStateMarkingStore` is deprecated. Use `MethodMarkingStore` instead.

Before:
```yaml
framework:
workflows:
article:
marking_store:
type: single
```

After:
```yaml
framework:
workflows:
article:
marking_store:
type: method
arguments:
- true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am coming to this late because it was merged yesterday, but in my opinion the DX of this change is not good.

Reading the YAML config file, the meaning of this first argument is not clear (what does true mean?).

For specifying a custom property name (as I do) with a multiple state workflow, I now need to remember to do:

framework:
    workflows:
        article:
            marking_store:
                type: method
                arguments:
                    - false
                    - myColumnName

This being YAML, can we use named arguments here? The code would then become:

framework:
    workflows:
        article:
            marking_store:
                type: method
                arguments:
                    singleState: false
                    property: myColumnName

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fully agree with that I was about to open an issue for it. We should only rely on the type (workflow or state_machine) and don't care about that argument. so we could simplify by:

framework:
    workflows:
        article:
            type: state_machine
            marking_store:
                type: method
                property: myColumnName

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't discuss on closed issues/PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I've just opened #30656.

```
2 changes: 2 additions & 0 deletions UPGRADE-5.0.md
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,8 @@ Workflow
* `ClassInstanceSupportStrategy` has been removed, use `InstanceOfSupportStrategy` instead.
* `MarkingStoreInterface::setMarking()` has a third argument: `array $context = []`.
* Removed support of `initial_place`. Use `initial_places` instead.
* `MultipleStateMarkingStore` has been removed.
* `SingleStateMarkingStore` has been removed.

Yaml
----
Expand Down
46 changes: 30 additions & 16 deletions src/Symfony/Bridge/Twig/Tests/Extension/WorkflowExtensionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use PHPUnit\Framework\TestCase;
use Symfony\Bridge\Twig\Extension\WorkflowExtension;
use Symfony\Component\Workflow\Definition;
use Symfony\Component\Workflow\MarkingStore\MethodMarkingStore;
use Symfony\Component\Workflow\Metadata\InMemoryMetadataStore;
use Symfony\Component\Workflow\Registry;
use Symfony\Component\Workflow\SupportStrategy\ClassInstanceSupportStrategy;
Expand Down Expand Up @@ -49,30 +50,28 @@ protected function setUp()
);
}
$definition = new Definition($places, $transitions, null, $metadataStore);
$workflow = new Workflow($definition);
$workflow = new Workflow($definition, new MethodMarkingStore());

$registry = new Registry();
$addWorkflow = method_exists($registry, 'addWorkflow') ? 'addWorkflow' : 'add';
$supportStrategy = class_exists(InstanceOfSupportStrategy::class)
? new InstanceOfSupportStrategy(\stdClass::class)
: new ClassInstanceSupportStrategy(\stdClass::class);
? new InstanceOfSupportStrategy(Subject::class)
: new ClassInstanceSupportStrategy(Subject::class);
$registry->$addWorkflow($workflow, $supportStrategy);
$this->extension = new WorkflowExtension($registry);
}

public function testCanTransition()
{
$subject = new \stdClass();
$subject->marking = [];
$subject = new Subject();

$this->assertTrue($this->extension->canTransition($subject, 't1'));
$this->assertFalse($this->extension->canTransition($subject, 't2'));
}

public function testGetEnabledTransitions()
{
$subject = new \stdClass();
$subject->marking = [];
$subject = new Subject();

$transitions = $this->extension->getEnabledTransitions($subject);

Expand All @@ -83,9 +82,7 @@ public function testGetEnabledTransitions()

public function testHasMarkedPlace()
{
$subject = new \stdClass();
$subject->marking = [];
$subject->marking = ['ordered' => 1, 'waiting_for_payment' => 1];
$subject = new Subject(['ordered' => 1, 'waiting_for_payment' => 1]);

$this->assertTrue($this->extension->hasMarkedPlace($subject, 'ordered'));
$this->assertTrue($this->extension->hasMarkedPlace($subject, 'waiting_for_payment'));
Expand All @@ -94,21 +91,18 @@ public function testHasMarkedPlace()

public function testGetMarkedPlaces()
{
$subject = new \stdClass();
$subject->marking = [];
$subject->marking = ['ordered' => 1, 'waiting_for_payment' => 1];
$subject = new Subject(['ordered' => 1, 'waiting_for_payment' => 1]);

$this->assertSame(['ordered', 'waiting_for_payment'], $this->extension->getMarkedPlaces($subject));
$this->assertSame($subject->marking, $this->extension->getMarkedPlaces($subject, false));
$this->assertSame($subject->getMarking(), $this->extension->getMarkedPlaces($subject, false));
}

public function testGetMetadata()
{
if (!class_exists(InMemoryMetadataStore::class)) {
$this->markTestSkipped('This test requires symfony/workflow:4.1.');
}
$subject = new \stdClass();
$subject->marking = [];
$subject = new Subject();

$this->assertSame('workflow title', $this->extension->getMetadata($subject, 'title'));
$this->assertSame('ordered title', $this->extension->getMetadata($subject, 'title', 'orderer'));
Expand All @@ -117,3 +111,23 @@ public function testGetMetadata()
$this->assertNull($this->extension->getMetadata($subject, 'not found', $this->t1));
}
}

final class Subject
{
private $marking;

public function __construct($marking = null)
{
$this->marking = $marking;
}

public function getMarking()
{
return $this->marking;
}

public function setMarking($marking)
{
$this->marking = $marking;
}
}
5 changes: 3 additions & 2 deletions src/Symfony/Bridge/Twig/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,13 @@
"symfony/var-dumper": "~3.4|~4.0",
"symfony/expression-language": "~3.4|~4.0",
"symfony/web-link": "~3.4|~4.0",
"symfony/workflow": "~3.4|~4.0"
"symfony/workflow": "~4.3"
},
"conflict": {
"symfony/console": "<3.4",
"symfony/form": "<4.3",
"symfony/translation": "<4.2"
"symfony/translation": "<4.2",
"symfony/workflow": "<4.3"
},
"suggest": {
"symfony/finder": "",
Expand Down
4 changes: 3 additions & 1 deletion src/Symfony/Component/Workflow/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ CHANGELOG
4.3.0
-----

* Trigger `entered` event for subject entering in the Workflow for the first time
* Trigger `entered` event for subject entering in the Workflow for the first time.
* Added a context to `Workflow::apply()`. The `MethodMarkingStore` could be used to leverage this feature.
* Add style to transitions by declaring metadata:

Expand All @@ -27,6 +27,8 @@ CHANGELOG
* Dispatch `CompletedEvent` on `workflow.completed`
* Dispatch `AnnounceEvent` on `workflow.announce`
* Added support for many `initialPlaces`
* Deprecated the `MultipleStateMarkingStore` class, use the `MethodMarkingStore` instead.
* Deprecated the `SingleStateMarkingStore` class, use the `MethodMarkingStore` instead.

4.1.0
-----
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@
*
* This store deals with a "single state" or "multiple state" Marking.
*
* "single state" Marking means a subject can be in one and only one state at
* the same time. Use it with state machine or specific workflow.
*
* "multiple state" Marking means a subject can be in many states at the same
* time. Use it with workflow.
*
* @author Grégoire Pineau <lyrixx@lyrixx.info>
*/
class MethodMarkingStore implements MarkingStoreInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\Workflow\MarkingStore;

@trigger_error(sprintf('"%s" is deprecated since Symfony 4.3, use "%s" instead.', MultipleStateMarkingStore::class, MethodMarkingStore::class), E_USER_DEPRECATED);

use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\Workflow\Marking;
Expand All @@ -22,6 +24,8 @@
* This store deals with a "multiple state" Marking. It means a subject can be
* in many states at the same time.
*
* @deprecated since Symfony 4.3, use MethodMarkingStore instead.
*
* @author Grégoire Pineau <lyrixx@lyrixx.info>
*/
class MultipleStateMarkingStore implements MarkingStoreInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

namespace Symfony\Component\Workflow\MarkingStore;

@trigger_error(sprintf('"%s" is deprecated since Symfony 4.3, use "%s" instead.', SingleStateMarkingStore::class, MethodMarkingStore::class), E_USER_DEPRECATED);

use Symfony\Component\PropertyAccess\PropertyAccess;
use Symfony\Component\PropertyAccess\PropertyAccessorInterface;
use Symfony\Component\Workflow\Marking;
Expand All @@ -21,6 +23,8 @@
* This store deals with a "single state" Marking. It means a subject can be in
* one and only one state at the same time.
*
* @deprecated since Symfony 4.3, use MethodMarkingStore instead.
*
* @author Grégoire Pineau <lyrixx@lyrixx.info>
*/
class SingleStateMarkingStore implements MarkingStoreInterface
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
use Psr\Log\AbstractLogger;
use Symfony\Component\EventDispatcher\EventDispatcher;
use Symfony\Component\Workflow\EventListener\AuditTrailListener;
use Symfony\Component\Workflow\MarkingStore\MultipleStateMarkingStore;
use Symfony\Component\Workflow\MarkingStore\MethodMarkingStore;
use Symfony\Component\Workflow\Tests\Subject;
use Symfony\Component\Workflow\Tests\WorkflowBuilderTrait;
use Symfony\Component\Workflow\Workflow;

Expand All @@ -18,22 +19,21 @@ public function testItWorks()
{
$definition = $this->createSimpleWorkflowDefinition();

$object = new \stdClass();
$object->marking = null;
$object = new Subject();

$logger = new Logger();

$ed = new EventDispatcher();
$ed->addSubscriber(new AuditTrailListener($logger));

$workflow = new Workflow($definition, new MultipleStateMarkingStore(), $ed);
$workflow = new Workflow($definition, new MethodMarkingStore(), $ed);

$workflow->apply($object, 't1');

$expected = [
'Leaving "a" for subject of class "stdClass" in workflow "unnamed".',
'Transition "t1" for subject of class "stdClass" in workflow "unnamed".',
'Entering "b" for subject of class "stdClass" in workflow "unnamed".',
'Leaving "a" for subject of class "Symfony\Component\Workflow\Tests\Subject" in workflow "unnamed".',
'Transition "t1" for subject of class "Symfony\Component\Workflow\Tests\Subject" in workflow "unnamed".',
'Entering "b" for subject of class "Symfony\Component\Workflow\Tests\Subject" in workflow "unnamed".',
];

$this->assertSame($expected, $logger->logs);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Symfony\Component\Workflow\EventListener\GuardExpression;
use Symfony\Component\Workflow\EventListener\GuardListener;
use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\Tests\Subject;
use Symfony\Component\Workflow\Transition;
use Symfony\Component\Workflow\WorkflowInterface;

Expand Down Expand Up @@ -130,13 +131,12 @@ public function testGuardExpressionBlocks()

private function createEvent(Transition $transition = null)
{
$subject = new \stdClass();
$subject->marking = new Marking();
$subject = new Subject();
$transition = $transition ?: new Transition('name', 'from', 'to');

$workflow = $this->getMockBuilder(WorkflowInterface::class)->getMock();

return new GuardEvent($subject, $subject->marking, $transition, $workflow);
return new GuardEvent($subject, new Marking($subject->getMarking() ?? []), $transition, $workflow);
}

private function configureAuthenticationChecker($isUsed, $granted = true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use PHPUnit\Framework\TestCase;
use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\MarkingStore\MethodMarkingStore;
use Symfony\Component\Workflow\Tests\Subject;

class MethodMarkingStoreTest extends TestCase
{
Expand Down Expand Up @@ -52,23 +53,3 @@ public function testGetSetMarkingWithSingleState()
$this->assertEquals($marking, $marking2);
}
}

final class Subject
{
private $marking;

public function __construct($marking = null)
{
$this->marking = $marking;
}

public function getMarking()
{
return $this->marking;
}

public function setMarking($marking)
{
$this->marking = $marking;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\MarkingStore\MultipleStateMarkingStore;

/** @group legacy*/
class MultipleStateMarkingStoreTest extends TestCase
{
public function testGetSetMarking()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Symfony\Component\Workflow\Marking;
use Symfony\Component\Workflow\MarkingStore\SingleStateMarkingStore;

/** @group legacy*/
class SingleStateMarkingStoreTest extends TestCase
{
public function testGetSetMarking()
Expand Down
Loading