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

Conversation

Projects
None yet
5 participants
@lyrixx
Copy link
Member

lyrixx commented Mar 13, 2019

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

@lyrixx lyrixx force-pushed the lyrixx:w-deprecate-marking-store branch from 7b982c8 to a71a233 Mar 13, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 13, 2019

@lyrixx lyrixx force-pushed the lyrixx:w-deprecate-marking-store branch 7 times, most recently from cde1f17 to e95c3a4 Mar 13, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Mar 13, 2019

@nicolas-grekas I don't understand the failure.


1) Symfony\Bridge\Twig\Tests\Extension\WorkflowExtensionTest::testCanTransition
Symfony\Component\Workflow\Exception\LogicException: The method "stdClass::getMarking()" does not exists.
/home/travis/build/symfony/symfony/src/Symfony/Bridge/Twig/vendor/symfony/workflow/MarkingStore/MethodMarkingStore.php:54
/home/travis/build/symfony/symfony/src/Symfony/Bridge/Twig/vendor/symfony/workflow/Workflow.php:49
/home/travis/build/symfony/symfony/src/Symfony/Bridge/Twig/vendor/symfony/workflow/Workflow.php:90
/home/travis/build/symfony/symfony/src/Symfony/Bridge/Twig/Extension/WorkflowExtension.php:55
/home/travis/build/symfony/symfony/src/Symfony/Bridge/Twig/Tests/Extension/WorkflowExtensionTest.php:68

I do not use anymore stdClass anymore.

Do you have an idea ?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 13, 2019

The bridge 4.2 is loaded with workflow 4.3 and this fails.

@lyrixx lyrixx force-pushed the lyrixx:w-deprecate-marking-store branch 3 times, most recently from 8e34c44 to a2c4ffb Mar 13, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Mar 13, 2019

Okay. Got it. old components are tested against master (this PR) (see this )

@lyrixx lyrixx force-pushed the lyrixx:w-deprecate-marking-store branch 4 times, most recently from 81c0274 to 262b9eb Mar 13, 2019

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented Mar 17, 2019

This PR is now ready

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

+1 with minor comments, thanks.

@lyrixx lyrixx force-pushed the lyrixx:w-deprecate-marking-store branch from 262b9eb to bb776b7 Mar 20, 2019

@lyrixx lyrixx force-pushed the lyrixx:w-deprecate-marking-store branch from bb776b7 to 4d58beb Mar 22, 2019

@lyrixx lyrixx merged commit 4d58beb into symfony:master Mar 22, 2019

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 Mar 22, 2019

minor #30551 [Workflow] Deprecate MultipleStateMarkingStore and Singl…
…eStateMarkingStore in favor of MethodMarkingStore (lyrixx)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Workflow] Deprecate MultipleStateMarkingStore and SingleStateMarkingStore in favor of MethodMarkingStore

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

Commits
-------

4d58beb [Workflow] Deprecate MultipleStateMarkingStore and SingleStateMarkingStore in favor of MethodMarkingStore

@lyrixx lyrixx deleted the lyrixx:w-deprecate-marking-store branch Mar 22, 2019

marking_store:
type: method
arguments:
- true

This comment has been minimized.

@pbowyer

pbowyer Mar 23, 2019

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

This comment has been minimized.

@HeahDude

HeahDude Mar 23, 2019

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Mar 23, 2019

Member

Please don't discuss on closed issues/PR

This comment has been minimized.

@HeahDude

HeahDude Mar 23, 2019

Member

Yes I've just opened #30656.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.