Skip to content

[Notifier] Rename/Change AdminRecipient class #35558

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

Closed
wouterj opened this issue Feb 2, 2020 · 1 comment · Fixed by #35773
Closed

[Notifier] Rename/Change AdminRecipient class #35558

wouterj opened this issue Feb 2, 2020 · 1 comment · Fixed by #35773
Labels

Comments

@wouterj
Copy link
Member

wouterj commented Feb 2, 2020

While testing out the new Notifier component to write some docs about it (ref symfony/symfony-docs#13025), I've found the AdminRecipient classname a bit confusing. It makes sense when purely looking at it from an error notification perspective.

However, I think the Notifier component shines in being able to sent any kind of notification to any kind of transport.

I would propose to merge this with Recipient, so the base class can have both email or phone:

$recipient = (new Recipient())->phone(...);
$recipient = (new Recipient())->email(...);
$recipient = (new Recipient())
    ->phone(...)
    ->email(...)
;
@wouterj wouterj changed the title [Notifier] Rename AdminRecipient [Notifier] Rename/Change AdminRecipient class Feb 2, 2020
@xabbuh xabbuh added the Notifier label Feb 3, 2020
@fabpot
Copy link
Member

fabpot commented Apr 12, 2020

Let's close the issue and continue the discussion on the PR instead.

@fabpot fabpot closed this as completed Apr 12, 2020
fabpot added a commit that referenced this issue Aug 7, 2020
This PR was squashed before being merged into the 5.2-dev branch.

Discussion
----------

[Notifier] Change notifier recipient handling

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #35558 <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | tbd.

According to @wouterj's reasoning in #35558 I did the following to merge the `AdminRecipient` with the `Recipient` class:

- introduced a `EmailRecipientInterface` with `getEmail(): string` methods
- introduced a `RecipientInterface` that is extended by `EmailRecipientInterface` and `SmsRecipientInterface`
- changed `Recipient` class which now holds an email and a phone number property and implements the `EmailRecipientInterface` and the `SmsRecipientInterface`
- changed `NoRecipient` class which now implements the `RecipientInterface`
- removed the `AdminRecipient` and fixed all related type-hints
- introduced an `EmailRecipient` and `SmsRecipient`
- changed the type-hint of the `$recipient` argument in `NotifierInterface::send()`, `Notifier::getChannels()`, `ChannelInterface::notifiy()` and `ChannelInterface::supports()` to `RecipientInterface`
- changed the type hint of the `$recipient` argument in the `as*Message()` of the  `EmailNotificationInterface` and `SmsNotificationInterface` which now uses the introduced interfaces
- changed the type hint of the `$recipient` argument in the static `fromNotification()` factory methods in `EmailMessage` and `SmsMessage` which now uses the introduced interfaces
- changed `EmailChannel` to only support recipients which implement the `EmailRecipientInterface`
- changed `SmsChannel` only supports recipients which implement the `SmsRecipientInterface`

Commits
-------

588607b [Notifier] Change notifier recipient handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants