[Workflow] Added DefinitionBuilder::setMetadataStore(). #27190
Conversation
04d528b to
675e6fd
Compare
| * @param string[] $places | ||
| * @param Transition[] $transitions | ||
| * @param string|null $initialPlace | ||
| * @param MetadataStoreInterface|null $metadataStore |
There was a problem hiding this comment.
We prefer doing partial docblocks, keeping them only when they provide something over the signature:
instead of adding this, I'd suggest to remove the one for initialPlace
| @@ -113,6 +117,8 @@ public function addTransitions(array $transitions) | |||
| } | |||
|
|
|||
| /** | |||
| * @param Transition $transition | |||
There was a problem hiding this comment.
should be removed (see previous comment)
| @@ -122,6 +128,18 @@ public function addTransition(Transition $transition) | |||
| return $this; | |||
| } | |||
|
|
|||
| /** | |||
| * @param MetadataStoreInterface $metadataStore | |||
|
@nicolas-grekas , thank you for explanations, done. |
|
@lyrixx , can't see it... |
| @@ -122,6 +126,16 @@ public function addTransition(Transition $transition) | |||
| return $this; | |||
| } | |||
|
|
|||
| /** | |||
| * @return $this | |||
There was a problem hiding this comment.
I would use a type hint instead of a php doc
There was a problem hiding this comment.
What type hint? : self?
There was a problem hiding this comment.
Suppose NewDefinitionBuilder extends our non-final DefinitionBuilder. Then return type of $newDefinitionBuilder->setMetadataStore() will be DefinitionBuilder, not NewDefinitionBuilder.
There was a problem hiding this comment.
Yes indeed. And it's not an issue ;)
There was a problem hiding this comment.
@lyrixx But method autocompletion in IDE for methods from the inherited class will stop after a call to a method with self as return type.
There was a problem hiding this comment.
Typehinting fluent pattern with : self is okay in a fluent interface because you are not supposed to add any new public methods in it's implementations. And probably in a final class for the same reason.
There was a problem hiding this comment.
found some relevant occurrences in symfony code:
but I still don't agree it is correct...
There was a problem hiding this comment.
With : self it is actually correct to do this:
<?php
class A
{
public function foo(): self
{
return $this;
}
}
class B extends A
{
public function foo(): A
{
return new A();
}
}It does not contradict the contract.
While @return $this indicates that the returned value should be the same instance.
There was a problem hiding this comment.
Makes sense to keep it as this provides more than just the return type.
In the code base, we use both return static and $this, dunno if there is a difference thought here.
| @@ -122,6 +126,16 @@ public function addTransition(Transition $transition) | |||
| return $this; | |||
| } | |||
|
|
|||
| /** | |||
| * @return $this | |||
There was a problem hiding this comment.
Makes sense to keep it as this provides more than just the return type.
In the code base, we use both return static and $this, dunno if there is a difference thought here.
62b352c to
2882f8d
Compare
|
Thank you @vudaltsov. |
…(vudaltsov) This PR was squashed before being merged into the 4.1 branch (closes #27190). Discussion ---------- [Workflow] Added DefinitionBuilder::setMetadataStore(). | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This PR complements #26092. Commits ------- 2882f8d [Workflow] Added DefinitionBuilder::setMetadataStore().
This PR complements #26092.