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

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

Closed
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

@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 :).

</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

@vinise
Copy link

vinise commented Mar 26, 2019

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

Missing "method" value ;)

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

UPGRADE-4.3.md Show resolved Hide resolved
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)

@@ -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
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
…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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants