Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Notifier] Remove superfluous parameters in *Message::fromNotification()#35167
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
10b01dd to24a8f38Compare| @@ -35,7 +35,7 @@ public function __construct(RawMessage $message, Envelope $envelope = null) | |||
| $this->envelope = $envelope; | |||
| } | |||
| public static function fromNotification(Notification $notification, Recipient $recipient, string $transport = null): self | |||
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.
Alternative: for the $transport, we could set the transport directly from this method then? But the current code uses the setter everytime.
| @@ -32,7 +31,7 @@ public function __construct(string $subject, MessageOptionsInterface $options = | |||
| $this->options = $options; | |||
| } | |||
| public static function fromNotification(Notification $notification, Recipient $recipient, string $transport = null): self | |||
| public static function fromNotification(Notification $notification): self | |||
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.
Maybe we need $recipient here in the future? But being forced to pass an useless argument is worse in practice for me.
There is a failure on deps=low |
24a8f38 to98d24d9CompareUh oh!
There was an error while loading.Please reload this page.
98d24d9 toe4dec22Comparee4dec22 tod0dacf5CompareThere 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 did that on purpose. I'm 👎 for this change.
@fabpot In this case, shouldn't the methods at least set the passed $transport? I don't understand how keeping any unused argument can be better for usability. Especially the mandatory $recipient on ChatMessage. Could you explain your intention please? Thank you :) |
Thank you@fancyweb. |
…fromNotification() (fancyweb)This PR was merged into the 5.1-dev branch.Discussion----------[Notifier] Remove superfluous parameters in *Message::fromNotification()| Q | A| ------------- | ---| Branch? | 5.0| Bug fix? | no| New feature? | no| Deprecations? | no| Tickets | -| License | MIT| Doc PR | -Those classes are final so I think we don't need those extra arguments.Commits-------d0dacf5 [Notifier] Remove superfluous parameters in *Message::fromNotification()
Those classes are final so I think we don't need those extra arguments.