-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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] Added Definition builder #20451
Conversation
*/ | ||
public function __construct(array $places = array(), array $transitions = array()) | ||
public function __construct(array $places = array(), array $transitions = array(), $initialPlace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that the default values for $places
and $transitions
can be removed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct
*/ | ||
class Definition | ||
final class Definition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we okey with making this final?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please next time don't ever do this without providing abstraction that final class implements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because then there is neither a way how to switch nor extend implementation.
https://ocramius.github.io/blog/when-to-declare-classes-final/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not an answer. Sometimes you don't need to "switch" or extend implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is no such need, it means there is no need to make it final.
Edit: To not seem snarky: Marking the class as final is used to force user of the class to use composition over inheritance. If you don't provide abstraction (interface), he can't do that. If it's obvious there is never a need to switch/extend the implementation, keyword final doesn't change the thing. But if it turns out you are wrong and there is single case in world where it makes sense, poor sob who needs it can't do it and needs to wait for upstream to change this, which can take a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A: Hey, guys, this class is self-sufficient and we don't see sense in other implementations. Also, we want to encourage to use composition over inheritance. Let's mark it final
.
B: Don't use final
.
A: Why?
B: Because it makes impossible to extend this class nor replace implementation.
A: ... That's the point. You shouldn't do it.
Our dialog looks like above as long as you don't provide real use case when you need to inherit this class. It's perfectly fine to have the only implementation sometimes, especially for VOs.
Because then there is neither a way how to switch nor extend implementation.
It's like saying that poison shouldn't be marked with "don't eat it" label, because you want to eat it. Well, that's not the purpose of the object. Don't eat extend it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's obvious there is never a need to switch/extend the implementation, keyword final doesn't change the thing
If someone needs to replace/extend implementation, it doesn't mean he is right and extending is correct. He should find better ways to solve his issue. You are as a vendor know how to use your tool and you are allowed to introduce some impediments to avoid incorrect usage of your product.
Im done with this implementation now. |
LGTM |
Actually, I have one question: I don't really like the idea to define the Definition as a service. What about adding |
The definition is not a service... or not a public service. (https://github.com/symfony/symfony/pull/20451/files#diff-0e793081ceb720201745c982a568903fR426) I agree with your arguments before saying that the definition is an value object.
I agree. I did not add it here because I thought that would be a separate PR. But I add it now. |
*/ | ||
public function build() | ||
{ | ||
return new Definition($this->places, $this->transitions, $this->initialPlace); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about reseting $this->places, $this->transitions, $this->initialPlace
in order to be able to re-use the Builder to build another Definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure about that. One might want to have definition A and B where A is a subset of B.
If we automatically reset the builder one have to add places and transaction twice.
How do you feel about adding DefinitionBuilder::reset()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok for reset ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. I've updated the PR.
Should I change my doc PR to use the builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if you put it in a new PR. The current doc PR is already pretty big and I would like to merge it as quick as possible.
throw new InvalidArgumentException(sprintf('The place "%s" contains invalid characters.', $place)); | ||
} | ||
|
||
if (!count($this->places)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is always array this can be just:
if (!$this->places) {}
@@ -59,30 +59,30 @@ public function provideWorkflowDefinitionWithoutMarking() | |||
|
|||
public function createprovideComplexWorkflowDefinition() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
provideComplexWorkflowDefinition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I've updated the PR to comply with your suggestions
@@ -29,18 +30,28 @@ class Definition | |||
* | |||
* @param string[] $places | |||
* @param Transition[] $transitions | |||
* @param string $initialPlace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string|null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
From design point of view, the idea of DI-service If you really want to have a builder, why not just instantiate new builder every time you want to build $builder = new DefinitionBuilder();
$definition = $builder->...->build(); This logic is static in fact (that's why it perfectly can be moved to the I call this idea dangerous, because it makes mutable service. $this->builder->reset();
$this->builder->addState('foo')->addState($this->stateProvider->getBar())->build(); What if After all, what's the benefit of this complexity? |
Hello @unkind I'm not sure to understand your comment, and what you propose.
Which complexity are you talking about? |
The builder is not a public service so you can not use it like in your examples. |
Ah, you mean it is inlined? My bad.
I'm talking about introducing new entity rather than moving this logic to the $definition = Definition::withInitialState(..., 'bar');
$definition = $definition->withNewState('foo'); |
@unkind using methods returning a new mutated instance rather than mutating the existing one makes things harder for the DIC registration, as we still need to have a way to build the whole object without having to replace it by a different object in the meantime (unless we want to build a chain of private services being the factory for the next one, which becomes a nightmare) |
You still can have a factory just for DIC: public function createDefinition()
{
$definition = ...;
foreach ($this->transitions as ...) {
$definition = ...;
}
return $definition;
} |
b0f1152
to
096a85f
Compare
I missed that PR. Good! I've rebased my PR now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one Little things to remove, another one to try to remove and then It will be good.
foreach ($input->getArgument('marking') as $place) { | ||
$marking->mark($place); | ||
} | ||
|
||
$output->writeln($dumper->dump($definition, $marking)); | ||
$output->writeln($dumper->dump($workflow->getDefinition(), $marking)); | ||
} | ||
|
||
private function getProperty($object, $property) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot to remove this method.
$container->setDefinition($workflowId, $workflowDefinition); | ||
$container->setDefinition(sprintf('%s.definition', $workflowId), $definitionDefinition); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to store it with an id, right? (ie, juste remove just line, everything should be OK)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this line or else we can grab it from the container in the compiler pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. I Though the service was in-lined later ;)
👍 |
👍 |
Thanks Tobias for working on this feature, this is much appreciated. |
Every time I submit a PR to Symfony I get great response no matter if my PR is good or not. Thank you! |
$reflectionProperty->setAccessible(true); | ||
|
||
return $reflectionProperty->getValue($object); | ||
$output->writeln($dumper->dump($workflow->getDefinition(), $marking)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice :)
$definitionDefinition = new Definition(Workflow\Definition::class); | ||
$definitionDefinition->addMethodCall('addPlaces', array($workflow['places'])); | ||
// Create a DefinitionBuilder | ||
$definitionBuilderDefinition = new Definition(Workflow\DefinitionBuilder::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we make use of the DefinitionBuilder
in the container extension? While I agree that the builder is useful in userland code, I do not see the added value here. On the downside, it just adds another level of indirection that is not really needed, is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no real reason. I can try to remove it.
This PR was squashed before being merged into the 3.2-dev branch (closes #20478). Discussion ---------- [Workflow] Removed definition builder | Q | A | ------------- | --- | Branch? | "master" | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a As of @xabbuh comment in #20451 (comment) We do not really need the definition builder here. Commits ------- caa3d6f [Workflow] Removed definition builder
Make the Definition immutable and add a DefinitionBuilder.
I basically broke up the Definition class into an immutable class and one builder.