Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Workflow] Introduce a Workflow interface#24751

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@Simperfit
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#23910
LicenseMIT
Doc PRtodo

@chalasr I think all the points you made in 23910 has been done. Needs to update the docs too.

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch 2 times, most recently fromdf6cd73 to4b25e0dCompareOctober 30, 2017 10:04
@nicolas-grekasnicolas-grekas added this to the4.1 milestoneOct 30, 2017
@nicolas-grekas
Copy link
Member

Note that changing a public interface is a BC break, so this cannot be merged as is, isn't it?

@chalasr
Copy link
Member

@nicolas-grekas the upgrade note is misleading, this replaces the whole interface.

@chalasrchalasr self-requested a reviewOctober 30, 2017 20:51
@Simperfit
Copy link
ContributorAuthor

Simperfit commentedOct 30, 2017 via email•
edited by lyrixx
Loading

I guess i have to update the upgrade node

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch 4 times, most recently from347c55e toa673253CompareNovember 1, 2017 18:43
Copy link
Member

@lyrixxlyrixx left a comment

Choose a reason for hiding this comment

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

Some minor comments, but it's good.
Thanks.


* Deprecated the `add` method in favor of the `addWorkflow` method in `Workflow\Registry`
* Deprecated the interface `SupportStrategyInterface` in favor of the interface `SupportStrategryWorkflowInterface`

Copy link
Member

Choose a reason for hiding this comment

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

Looks like there is a space here

-----

* Removed class name support in`WorkflowRegistry::add()` as second parameter.
* Deprecate the usage of`add(Workflow $workflow, $supportStrategy) in`Workflow/Registry` use`addWorkflow(WorkflowInterface, $supportStrategy)` instead
Copy link
Member

Choose a reason for hiding this comment

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

missing backquote at the end ofadd(Workflow $workflow, $supportStrategy)

{
if (!$supportStrategyinstanceof SupportStrategyInterface) {
thrownew \InvalidArgumentException('The "supportStrategy" is not an instance of SupportStrategyInterface.');
@trigger_error('add is deprecated since Symfony 4.0. Use addWorkflow instead',E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

'add' method is ... use 'addWorkflow' insteand.
(don't miss the final dot)

publicfunctionaddWorkflow(WorkflowInterface$workflow,$supportStrategy)
{
if ($supportStrategyinstanceof SupportStrategyInterface) {
@trigger_error('Passing a SupportStrategyInterface in Registry::addWorkflow is deprecated since 4.0. Use SupportStrategyWorkflowInterface instead',E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

use quote to surround class name and add a final dot.

if ($supportStrategyinstanceof SupportStrategyInterface) {
@trigger_error('Passing a SupportStrategyInterface in Registry::addWorkflow is deprecated since 4.0. Use SupportStrategyWorkflowInterface instead',E_USER_DEPRECATED);
}
if (!($supportStrategyinstanceof SupportStrategyWorkflowInterface ||$supportStrategyinstanceof SupportStrategyInterface)) {
Copy link
Member

Choose a reason for hiding this comment

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

thisif could be transformed in anelseif (!$supportStrategy instanceof SupportStrategyWorkflowInterface)


namespaceSymfony\Component\Workflow\SupportStrategy;

@trigger_error('SupportStrategyInterface is deprecated since Symfony 4.0. Use SupportStrategyWorkflowInterface instead',E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

please add a final dot

*
* @return bool true if the transition is enabled
*/
publicfunctioncan($subject,$transitionName);
Copy link
Member

Choose a reason for hiding this comment

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

Hm, if getMarking can throw an exception, then can and apply may throw an exception too

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch 4 times, most recently froma597789 tof59403dCompareNovember 2, 2017 19:38
@Simperfit
Copy link
ContributorAuthor

@lyrixx Thanks for the review :)

* Removed class name support in `WorkflowRegistry::add()` as second parameter.

* Deprecated the `add` method in favor of the `addWorkflow` method in `Workflow\Registry`
* Deprecated the interface `SupportStrategyInterface` in favor of the interface `SupportStrategryWorkflowInterface`
Copy link
Member

Choose a reason for hiding this comment

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

typo here (strategry)

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch fromf59403d to6902c37CompareNovember 3, 2017 09:14

* Deprecated the `add` method in favor of the `addWorkflow` method in `Workflow\Registry`
* Deprecated the interface `SupportStrategyInterface` in favor of the interface `SupportStrategyWorkflowInterface`

Copy link
Member

Choose a reason for hiding this comment

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

And now there are two spaces here. You need to configure your editor to remove automatically all trailing spaces :D

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, you are right, I didn't configure it to do so !

{
if ($supportStrategyinstanceof SupportStrategyInterface) {
@trigger_error('Passing a "SupportStrategyInterface" in "Registry::addWorkflow" is deprecated since 4.0. Use "SupportStrategyWorkflowInterface" instead.',E_USER_DEPRECATED);
}elseif (!($supportStrategyinstanceof SupportStrategyWorkflowInterface ||$supportStrategyinstanceof SupportStrategyInterface)) {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you did not simplified the condition.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, sorry, that's modified.

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch from6902c37 to116d678CompareNovember 3, 2017 10:03
publicfunctionaddWorkflow(WorkflowInterface$workflow,$supportStrategy)
{
if ($supportStrategyinstanceof SupportStrategyInterface) {
@trigger_error('Passing a "SupportStrategyInterface" in "Registry::addWorkflow" is deprecated since 4.0. Use "SupportStrategyWorkflowInterface" instead.',E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

strings should be replacedSupportsStrategyInterface::class,__METHOD andSupportStrategyWorkflowInterface::class in this order, we always use FQCNs

Copy link
Member

Choose a reason for hiding this comment

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

same for the exception below

* @author Andreas Kleemann <akleemann@inviqa.com>
*/
finalclass ClassInstanceSupportStrategyimplementsSupportStrategyInterface
finalclass ClassInstanceSupportStrategyimplementsSupportStrategyWorkflowInterface
Copy link
Member

Choose a reason for hiding this comment

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

Not implementing the old interface is a BC break ($strategy instanceof SupportsStrategyInterface returning false). Personally I wouldn't care, this could be internal

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I think this is internal too.

Choose a reason for hiding this comment

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

Internal should be hinted by an annotation, we can't have a policy of telling "that's internal" and break BC.
I suggest to open a PR against 3.4 to really mark them internal so that we're free to change in 4.1.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Ill open that PR.

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch from116d678 to6503755CompareNovember 3, 2017 10:12
}

/**
* @return string
Copy link
Contributor

Choose a reason for hiding this comment

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

aren't we trying to get rid of these kind docblocks?

chalasr reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

👍 useless

useSymfony\Component\Workflow\Workflow;

/**
* @group legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

this file basically only needs this change.. what aboutSubjectA +SubjectB for the new test?

chalasr reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

done

Copy link
Contributor

Choose a reason for hiding this comment

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

still.. no real reason to optimize/tweak a legacy test (::class notation, class removals). Doesnt hurt; just extra diff :)

Copy link
Member

Choose a reason for hiding this comment

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

indeed, adding the::class notation was ok because the line was actually changed. Now it just gives merge conflicts, should be reverted

* @param Workflow $workflow
* @param SupportStrategyInterface $supportStrategy
* @param Workflow $workflow
* @param WorkflowSupportStrategyInterface $supportStrategy
Copy link
Contributor

Choose a reason for hiding this comment

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

the BC layer actually expects aSupportStrategyInterface? Using the new strategy interface should only go viaaddWorkflow IMHO.

Copy link
Member

Choose a reason for hiding this comment

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

Agree

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch 2 times, most recently fromcab29ee to860a174CompareNovember 12, 2017 06:55
Copy link
Contributor

@ro0NLro0NL left a comment•
edited
Loading

Choose a reason for hiding this comment

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

nice work :) i also asked@nicolas-grekas if this is really the desired upgrade path.

Basically i see one other option; to makeRegistry final since 3.4, so we can keep favoringadd(); meaning less upgrade hassle for users as the common usecase would keep working, givenWorkflow implements WorkflowInterface tomorrow.

Also means we close extensibility onRegistry, which we can open by introducing aRegistryInterface, when needed.

* @param Workflow $workflow
* @param SupportStrategyInterface $supportStrategy
*
* @deprecated since Symfony 4.1. Use addWorkflow() instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

@deprecated since version 4.1, to be removed in 5.0. Use addWorkflow() instead.

*/
publicfunctionadd(Workflow$workflow,$supportStrategy)
{
if (!$supportStrategyinstanceof SupportStrategyInterface) {
Copy link
Contributor

Choose a reason for hiding this comment

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

doesnt hurt to keep right? or why shouldnt it?

useSymfony\Component\Workflow\Workflow;

/**
* @group legacy
Copy link
Contributor

Choose a reason for hiding this comment

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

still.. no real reason to optimize/tweak a legacy test (::class notation, class removals). Doesnt hurt; just extra diff :)

*/
publicfunctiongetEnabledTransitions($subject);

publicfunctiongetName();
Copy link
Contributor

Choose a reason for hiding this comment

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

missing type info + {@inheritdoc} in sub class (id prefer a@return string here).

Copy link
ContributorAuthor

@SimperfitSimperfitDec 1, 2017•
edited
Loading

Choose a reason for hiding this comment

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

maybe we could typehint it instead of phpdoc ?

@Simperfit
Copy link
ContributorAuthor

Simperfit commentedNov 14, 2017•
edited
Loading

WDYT of@ro0NL's path ?

It's not too late to say that Registry is final since 3.4@nicolas-grekas ?

I guess it could be a thing too to be able to re-implement the registry, and users won't need to change alot of thing. I think it's a good idea@ro0NL. (i'll take care of the comments, I just want to wait a little for others' thoughts).

@chalasr
Copy link
Member

It's not too late to say that Registry if final since 3.4

It is to me, 3.4 should not receive new deprecations anymore. I think this is fine.

@Simperfit
Copy link
ContributorAuthor

Ill take care of the 2 comments.

@ro0NL
Copy link
Contributor

👍 lets go withaddWorkflow. It's not like that's super bad or so.. but im not sure which method comes after that ;-) in case we need to patch this signature one day.

@dunglas
Copy link
Member

Needs a rebase.

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch 2 times, most recently from1f0be16 to41d3793CompareDecember 1, 2017 12:19
@Simperfit
Copy link
ContributorAuthor

The PR has been rebased

*/
publicfunctiongetEnabledTransitions($subject);

publicfunctiongetName():string;
Copy link
Member

Choose a reason for hiding this comment

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

return type should be removed, otherwise it requires to add it toWorkflow::getName() which would break BC

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

removed

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch from41d3793 toa533a00CompareDecember 1, 2017 16:08
@chalasr
Copy link
Member

Looks good to me.

Copy link
Member

@lyrixxlyrixx left a comment

Choose a reason for hiding this comment

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

Sorry, I left one (two) extra comment.
I'm 👍 after the changes.

* @param WorkflowInterface $workflow
* @param object $subject
*
* @return bool
Copy link
Member

Choose a reason for hiding this comment

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

Sorry about asking again some change, but we don't need this PHP doc. Could you remove it?
Thanks

chalasr and Simperfit reacted with thumbs up emoji
Copy link
ContributorAuthor

@SimperfitSimperfitDec 4, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Don't be sorry, that's why the PR are made for ;)

Workflow
--------

* Deprecated the`add` method in favor of the`addWorkflow` method in`Workflow\Registry`
Copy link
Member

Choose a reason for hiding this comment

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

As I asked a mandatory change, Can I also ask you to add a final dot on each line of each .md updated?

@SimperfitSimperfitforce-pushed thefeature/introduce-an-interface-for-the-workflow-component branch froma533a00 toe8351d8CompareDecember 4, 2017 15:23
@Simperfit
Copy link
ContributorAuthor

done@lyrixx

@Simperfit
Copy link
ContributorAuthor

This PR is ready.

@lyrixx
Copy link
Member

It took time, but here we go, this is in now. Thank you very much Hamza.

@lyrixxlyrixx merged commite8351d8 intosymfony:masterDec 7, 2017
lyrixx added a commit that referenced this pull requestDec 7, 2017
This PR was merged into the 4.1-dev branch.Discussion----------[Workflow] Introduce a Workflow interface| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#23910| License       | MIT| Doc PR        | todo@chalasr I think all the points you made in 23910 has been done. Needs to update the docs too.Commits-------e8351d8 [Workflow] Introduce a Workflow interface
@Simperfit
Copy link
ContributorAuthor

🎉 Thanks for your time@lyrixx@ro0NL@xabbuh@chalasr@nicolas-grekas

@fabpotfabpot mentioned this pull requestMay 7, 2018
fabpot added a commit that referenced this pull requestSep 10, 2023
…(derrabus)This PR was merged into the 6.4 branch.Discussion----------[TwigBridge] Remove obsolete Workflow feature detection| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | N/A| License       | MIT| Doc PR        | N/AThe classes and methods that are detected here have been introduced with#24751 and#26092 (both Symfony 4.1)Commits-------7e9d5bb [TwigBridge] Remove obsolete Workflow feature detection
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@chalasrchalasrchalasr approved these changes

@lyrixxlyrixxlyrixx approved these changes

@xabbuhxabbuhxabbuh approved these changes

+1 more reviewer

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

8 participants

@Simperfit@nicolas-grekas@chalasr@xabbuh@ro0NL@dunglas@lyrixx@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp