8000 [WIP][Workflow] Changed initial_places to initial_marking, added property instead of type by HeahDude · Pull Request #30662 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WIP][Workflow] Changed initial_places to initial_marking, added property instead of type #30662

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
wants to merge 1 commit into from
Closed

[WIP][Workflow] Changed initial_places to initial_marking, added property instead of type #30662

wants to merge 1 commit into from

Conversation

HeahDude
Copy link
Contributor
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #30656
License MIT
Doc PR symfony/symfony-docs#11211

ping @lyrixx

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 25, 2019
@nicolas-grekas nicolas-grekas requested a review from lyrixx March 25, 2019 07:56
@lyrixx
Copy link
Member
lyrixx commented Mar 25, 2019

@HeahDude Hi. Thanks for the PR. What is missing? Do you think you will be able to finish it before the feature freeze?

@HeahDude
Copy link
Contributor Author

Hey!

What is missing?

I need to fix the CI and get your approval :). Is the naming ok? I'm not sure about the deprecation messages, I surely need to update the changelog and upgrade files about them and the initial_marking.

Do you think you will be able to finish it before the feature freeze?

Sure, if you ask me, I would love to have it merged for tomorrow :).

HeahDude
HeahDude commented Mar 25, 2019
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
</xsd:complexType>

<xsd:simpleType name="marking_store_type">
<xsd:restriction base="xsd:string">
<xsd:enumeration value="multiple_state" />
<xsd:enumeration value="single_state" />
<xsd:enumeration value="method" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a kind of bug fix

@HeahDude HeahDude force-pushed the workflow/default-config branch from 0737d3e to 95bc4ef Compare March 26, 2019 06:21
@vinise
Copy link
vinise commented Mar 26, 2019

->enumNode('type') ->values(['workflow', 'state_machine']) ->defaultValue('state_machine')

Missing "method" value ;)

lyrixx
lyrixx reviewed Mar 26, 2019
View reviewed changes
Copy link
Member
@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Thanks, I left few comments

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
if ('method' === $workflow['marking_store']['type']) {
$markingStoreDefinition->setArguments([
'state_machine' === $type, //single state
$workflow['marking_store']['property'] ?? 'marking',
Copy link
Member

Choose a reason for hiding this comment

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

This should be the default value in the Configuration (not the extension)

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd
@@ -270,7 +270,7 @@

<xsd:complexType name="workflow">
<xsd:sequence>
<xsd:element name="initial-place" type="xsd:string" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="initial-marking" type="xsd:string" minOccurs="0" maxOccurs="unbounded" />
Copy link
Member

Choose a reason for hiding this comment

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

both element should be here to support the BC, no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@lyrixx
Copy link
Member
lyrixx commented Apr 6, 2019

@HeahDude I could finish this PR. Are you OK with that?

@HeahDude
Copy link
Contributor Author
HeahDude commented Apr 6, 2019

Yes please do!

@lyrixx lyrixx mentioned this pull request Apr 6, 2019
Merged
@lyrixx
8000 Copy link
Member
lyrixx commented Apr 6, 2019

Thank you very much for you initial work on this PR.
I finished it in #30890

@lyrixx lyrixx closed this Apr 6, 2019
fabpot added a commit that referenced this pull request 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
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyrixx lyrixx lyrixx left review comments

Assignees
No one assigned
Labels
Deprecation Feature Status: Needs Review Workflow
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
5 participants
@HeahDude @lyrixx @vinise @nicolas-grekas @carsonbot
0