8000 [RFC][DX][Workflow] Configuration of initial places and marking store · Issue #30656 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC][DX][Workflow] Configuration of initial places and marking store #30656

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
HeahDude opened this issue Mar 23, 2019 · 4 comments
Closed

[RFC][DX][Workflow] Configuration of initial places and marking store #30656

HeahDude opened this issue Mar 23, 2019 · 4 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed) Workflow

Comments

@HeahDude
Copy link
Contributor

Description

  1. In [Workflow] Deprecate MultipleStateMarkingStore and SingleStateMarkingStore in favor of MethodMarkingStore #30551, the marking store has been "simplified" internally, but the configuration is now more confusing (ref [Workflow] Deprecate MultipleStateMarkingStore and SingleStateMarkingStore in favor of MethodMarkingStore #30551 (review)).

  2. In [Workflow] Added support for many inital places #30468, initial_place has been renamed initial_places but still supports passing one place that will be normalized to an array despite this single example in the upgrade file. Also, this naming is now a bit confusing too, whether we can use it with one place is not obvious anymore.

Proposition

  1. Simplify the method type configuration which is the only one built-in we want to support now, and infer the single state from the type of the workflow, this leaves one argument to configure, the property name:

    framework:
 workflows:
     article:
         type: state_machine # or workflow for multiple states
         marking_store:
             type: method # this line can be removed in Symfony 5.0
             property: state

    Instead of inferring we can use another key: state (single or multiple) along with property,
    or use a boolean as suggested by @pbowyer in the review linked above.

  2. We could rename initial_places to something invariant like initial_marking or initial_state.

Any other suggestions? What do you think? ping @javiereguiluz @lyrixx
@HeahDude HeahDude mentioned this issue Mar 23, 2019
Merged
@pbowyer
Copy link
Contributor
pbowyer commented Mar 23, 2019

Re the marking store, this is how the 4.3 behaviour looks when added to the documentation: symfony/symfony-docs@6f51164.

I like the idea of inferring the single state from the workflow type state_machine. If there are complications, would it be bad though to keep single_state and multiple_state as wrapper classes/shims for easier configuration by developers? This doesn't change the implementation, but makes it obvious what developers need to use.

@HeahDude
Copy link
Contributor Author

Currently I see a DX problem that should be handled at configuration level, because having state_machine by default (https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php#L269) is not compatible with default false in the marking store (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Workflow/MarkingStore/MethodMarkingStore.php#L40).

@pbowyer pbowyer mentioned this issue Mar 23, 2019
Closed
3 tasks
@lyrixx
Copy link
Member
lyrixx commented Mar 23, 2019

@HeahDude Very nice idea. I love it. Would you mind submitting a PR?

type: method # this line can be removed in Symfony 5.0
This is not possible, since it could be a service (but could be the default value though)

@HeahDude
Copy link
Contributor Author

Ok I can do it, thank you for the feedback.

@HeahDude HeahDude mentioned this issue Mar 23, 2019
Closed
@javiereguiluz javiereguiluz added RFC RFC = Request For Comments (proposals about features that you want to be discussed) DX DX = Developer eXperience (anything that improves the experience of using Symfony) labels Mar 23, 2019
@lyrixx lyrixx mentioned this issue Apr 6, 2019
Merged
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this issue Apr 6, 2019
@fabpot
feature #30890 [Workflow] Changed initial_places to initial_marking, …
9754e87 
…added property (HeahDude, lyrixx)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Workflow] Changed initial_places to initial_marking, added property

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

EUFOSSA

---

* [Workflow] Changed initial_places to initial_marking, added property instead of type
* [Workflow] Finished integration of initial_marking + deprecated support for workflow + single state markin store
 [Workflow] Deprecate worflow and single state marking

---

Here is an exemple of deprecation:
```

  3x: Passing something else than "method" has been deprecated in Symfony 4.3.
    1x in PhpFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in XmlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in YamlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection

  3x: The "framework.workflows.workflows.legacy.marking_store.arguments" configuration key has been deprecated in Symfony 4.3. Use "property" instead.
    1x in PhpFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in XmlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in YamlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection

  3x: The "framework.workflows.workflows.legacy.initial_place" configuration key has been deprecated in Symfony 4.3, use the "initial_marking" configuration key instead.
    1x in PhpFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in XmlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in YamlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
  ```

Commits
-------

039353546f [Workflow] Deprecate worflow and single state marking
87839cfaf9 [Workflow] Finished integration of initial_marking + deprecated support for workflow + single state markin store
73708a61b6 [Workflow] Changed initial_places to initial_marking, added property instead of type
fabpot added a commit that referenced this issue Apr 6, 2019
@fabpot
feature #30890 [Workflow] Changed initial_places to initial_marking, …
ede6660 
…added property (HeahDude, lyrixx)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Workflow] Changed initial_places to initial_marking, added property

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

EUFOSSA

---

* [Workflow] Changed initial_places to initial_marking, added property instead of type
* [Workflow] Finished integration of initial_marking + deprecated support for workflow + single state markin store
 [Workflow] Deprecate worflow and single state marking

---

Here is an exemple of deprecation:
```

  3x: Passing something else than "method" has been deprecated in Symfony 4.3.
    1x in PhpFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in XmlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in YamlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection

  3x: The "framework.workflows.workflows.legacy.marking_store.arguments" configuration key has been deprecated in Symfony 4.3. Use "property" instead.
    1x in PhpFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in XmlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in YamlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection

  3x: The "framework.workflows.workflows.legacy.initial_place" configuration key has been deprecated in Symfony 4.3, use the "initial_marking" configuration key instead.
    1x in PhpFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in XmlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
    1x in YamlFrameworkExtensionTest::testWorkflowLegacy from Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection
  ```

Commits
-------

0393535 [Workflow] Deprecate worflow and single state marking
87839cf [Workflow] Finished integration of initial_marking + deprecated support for workflow + single state markin store
73708a6 [Workflow] Changed initial_places to initial_marking, added property instead of type
@fabpot fabpot closed this as completed Apr 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) RFC RFC = Request For Comments (proposals about features that you want to be discussed) Workflow
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @javiereguiluz @pbowyer @lyrixx @xabbuh @HeahDude
0