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][Registry] Added a new 'all' method #26656

Merged
merged 2 commits into from Apr 2, 2018

Conversation

Projects
None yet
7 participants
@alexpozzi
Contributor

alexpozzi commented Mar 23, 2018

[Workflow][Registry] Added the 'all' method which returns all the workflows associated to a specific object #26618

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

As explained in 26628 I added the 'all' method which returns all the workflows associated to a specific object.

@lyrixx

Hello.

First thanks for your first contribution on Symfony 🎉

Then, It's almost perfect. I just let few minor comments

}
}
if (empty($matched)) {
throw new InvalidArgumentException(sprintf('Unable to find any workflow for class "%s".', get_class($subject)));

This comment has been minimized.

@lyrixx

lyrixx Mar 23, 2018

Member

I would not throw an exception here. Just return the empty array

*
* @return Workflow[]
*/
public function all($subject)

This comment has been minimized.

@lyrixx

lyrixx Mar 23, 2018

Member

Could you add the return type hint: : array

{
$workflows = $this->registry->all(new Subject1());
$this->assertInternalType('array', $workflows);
$this->assertEquals(1, count($workflows));

This comment has been minimized.

@lyrixx

lyrixx Mar 23, 2018

Member

you can use $this->assertCount(1, $workflows);

/**
* @expectedException \Symfony\Component\Workflow\Exception\InvalidArgumentException
* @expectedExceptionMessage Unable to find any workflow for class "stdClass".

This comment has been minimized.

@lyrixx

lyrixx Mar 23, 2018

Member

So you should remove theses expectation

@alexpozzi

This comment has been minimized.

Contributor

alexpozzi commented Mar 23, 2018

@lyrixx Ok, I committed all the changes.

@lyrixx

lyrixx approved these changes Mar 23, 2018

*
* @return Workflow[]
*/
public function all($subject): array

This comment has been minimized.

@lyrixx

lyrixx Mar 26, 2018

Member

Actually I'm not really sure about this name. You don't return all Workflows, but only workflows for a specific subject.
Maybe filterBySubject is a better name? What do you think?

This comment has been minimized.

@alexpozzi

alexpozzi Mar 26, 2018

Contributor

I chose all because the other function is called get (and not getBySubject).
BTW I think that filterBySubject is fine as well and it better explains what the function does.

Let me know if I have to change the name of the function, so I'll submit it together with the updated CHANGELOG.md file.

This comment has been minimized.

@lyrixx

lyrixx Mar 26, 2018

Member

You are right about the get. Let's keep all.

This comment has been minimized.

@xabbuh

xabbuh Apr 1, 2018

Member

What about allBySubject() then?

@lyrixx

👍 with the CHANGELOG

*
* @return Workflow[]
*/
public function all($subject): array

This comment has been minimized.

@lyrixx

lyrixx Mar 26, 2018

Member

You are right about the get. Let's keep all.

@lyrixx

lyrixx approved these changes Mar 26, 2018

@@ -11,6 +11,7 @@ CHANGELOG
* Added TransitionBlockers as a way to pass around reasons why exactly
transitions can't be made.
* Added a `MetadataStore`.
* Added the method `all` which returns all the workflows associated to the specified subject

This comment has been minimized.

@lyrixx

lyrixx Mar 26, 2018

Member

Added Registry::all to returns all [...] subject.

@@ -11,6 +11,7 @@ CHANGELOG
* Added TransitionBlockers as a way to pass around reasons why exactly
transitions can't be made.
* Added a `MetadataStore`.
* Added Registry::all to return all the workflows associated to the specified subject

This comment has been minimized.

@fabpot

fabpot Mar 27, 2018

Member

with the specific subject?

alexpozzi and others added some commits Mar 23, 2018

@lyrixx

This comment has been minimized.

Member

lyrixx commented Apr 2, 2018

Thank you @alexpozzi.

@lyrixx lyrixx merged commit ca1352d into symfony:master Apr 2, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

lyrixx added a commit that referenced this pull request Apr 2, 2018

feature #26656 [Workflow][Registry] Added a new 'all' method (alexpoz…
…zi, lyrixx)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Workflow][Registry] Added a new 'all' method

[Workflow][Registry] Added the 'all' method which returns all the workflows associated to a specific object #26618

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

As explained in [26628](#26618 (comment))  I added the 'all' method which returns all the workflows associated to a specific object.

Commits
-------

ca1352d Fixed CHANGELOG
baec431 [Workflow][Registry] Added the 'all' method which returns all the workflows associated to a specific object #26618

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment