-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Notifier] make Message classes extensible #45402
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
[Notifier] make Message classes extensible #45402
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
PushNotificationInterface
flexible to extend
Hey! I think @norkunas has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
@@ -16,7 +16,7 @@ | |||
/** | |||
* @author Tomas Norkūnas <norkunas.tom@gmail.com> | |||
*/ | |||
final class PushMessage implements MessageInterface | |||
final class PushMessage implements PushMessageInterface |
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'd rather make PushMessage non final and remove the interface; value objects should not be 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.
Should I change 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.
that's what I think
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 willing to, but notice that said implementing an interface first introduce a contract and secondly allow implementation vs extension which its always good
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.
My experience is that interfaces are needed to abstract services, but here we are representing data. A value object, thus a class, is SOLID enough to me.
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 have not found on the doc or on the code anyway to pass options to the transport from the notifier service, I can only do it using a new notification and that blocks me because of the message as explained above
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.
PushMessage
constructor accepts options as a third argument, so your transport could define options on it's own and then in the asPushMessage
method you could do something like:
if ($transport === 'pusher') {
$options = (new PusherOptions())->channels(..);
return new PushMessage(.., $options);
}
And in the transport you could check if that option is defined then use it to make api call
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.
got it. I can close this now but I advise to look at the options to include interfaces or drop the final on value objects as Nicolas propose
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.
Putting a final on a class that is referenced by an interface is an API-design bad practice to me, because it kills any extensibility. I'm still for removing the keyword.
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.
agreed and done
PushNotificationInterface
flexible to extend
Thank you @bitgandtter. |
Uh oh!
There was an error while loading. Please reload this page.