[Workflow] Fix Event constructor requirements #44642
Conversation
This has already been deprecied in 4.x But while cleaning the 5.x branch, I forgot to apply this patch See https://github.com/symfony/symfony/pull/31824/files#diff-5f386ffb0109cc731bd98e63eea021b32faadd98791bd6ba65926d09c5e2ec40L37
|
Thank you @lyrixx. |
| @@ -29,7 +29,7 @@ class Event extends BaseEvent | |||
| private $transition; | |||
| private $workflow; | |||
|
|
|||
| public function __construct(object $subject, Marking $marking, Transition $transition = null, WorkflowInterface $workflow = null, array $context = []) | |||
| public function __construct(object $subject, Marking $marking, Transition $transition = null, WorkflowInterface $workflow, array $context = []) | |||
There was a problem hiding this comment.
I understand that it was quite unfortunate that we forgot to remove the nullability of the argument after having deprecated passing null. However, this change is problematic:
- Right now, it is possible to create the object without passing a workflow or setting it to
null. You would break that functionality. Then again, actually setting the workflow tonullwould breakgetWorkflow(),getWorkflowName()andgetMetadata(). - This change will trigger a warning on PHP 8 because
$workflowis mandatory now although it follows an optional parameter$transition. - The parameter
$transitionis effectively mandatory as well now which is a BC break.
There was a problem hiding this comment.
Oops, I merged too fast, now reverted. Please submit again @lyrixx
There was a problem hiding this comment.
Oups, good point !
So we need another round of deprecation ? we need to re-swap all arguments ? 😨
There was a problem hiding this comment.
Or maybe just turning Transition into a mandatory nullable argument rather than swapping.
anyway, I don't expect this constructor to be used a lot in third-party code. This code is expected to listen to events, not to trigger them.
There was a problem hiding this comment.
Or maybe just turning Transition into a mandatory nullable argument rather than swapping.
Not possible, some events are not directly related to transition
There was a problem hiding this comment.
Not possible, some events are not directly related to transition
Those events would eplicity pass null as transition then.
anyway, I don't expect this constructor to be used a lot in third-party code. This code is expected to listen to events, not to trigger them.
Me neither. This would mostly affect third party code that extends the workflow component with own logic. But since neither the Event class nor its constructor is flagged as @internal, calling the constructor in third-party code is valid.
There was a problem hiding this comment.
but for that code, marking the argument as required or swapping arguments is a BC break in both cases
* 5.3: Revert "minor #44642 [Workflow] Fix Event constructor requirements (lyrixx)" Skip transient tests on macos [Workflow] Fix Event constructor requirements [Translation] Handle the blank-translation in Loco Adapter [Serializer] Fix symfony/uid requirement
* 5.4: Revert "minor #44642 [Workflow] Fix Event constructor requirements (lyrixx)" Skip transient tests on macos [Workflow] Fix Event constructor requirements [Translation] Handle the blank-translation in Loco Adapter [Serializer] Fix symfony/uid requirement
* 6.0: Revert "minor #44642 [Workflow] Fix Event constructor requirements (lyrixx)" [Workflow] Remove redundant type check Skip transient tests on macos [Workflow] Fix Event constructor requirements [Translation] Handle the blank-translation in Loco Adapter [Serializer] Fix symfony/uid requirement
This has already been deprecied in 4.x
But while cleaning the 5.x branch, I forgot to apply this patch
See
https://github.com/symfony/symfony/pull/31824/files#diff-5f386ffb0109cc731bd98e63eea021b32faadd98791bd6ba65926d09c5e2ec40L37
More over,
Event::getWorkflow()has the correct return type.