[Notifier] Change notifier recipient handling #35773

jschaedl · 2020-02-18T12:57:00Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #35558
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

wouterj

Looks great!

I think we should mention the changed things (the added methods + interfaces and the removed AdminRecipient class) in the UPGRADE guide. Experimental means we are allowed to break stuff, but we the breaks have to be documented afaik.

OskarStark

Did you check the current bridge implementations if there are some changes needed?

src/Symfony/Component/Notifier/Recipient/Recipient.php

jschaedl · 2020-02-18T20:13:56Z

@OskarStark

Did you check the current bridge implementations if there are some changes needed?

Yes I checked the bridges too. All should be good there.

xabbuh

Having hasEmail() and hasPhone() methods does not look right to me. I'd prefer dedicated classes that only implement EmailRecipientInterface and/or SmsRecipientInterface when the required data is actually present.

src/Symfony/Component/Notifier/CHANGELOG.md

jschaedl · 2020-02-28T16:34:57Z

According to @xabbuh comment

Having hasEmail() and hasPhone() methods does not look right to me. I'd prefer dedicated classes that only implement EmailRecipientInterface and/or SmsRecipientInterface when the required data is actually present.

I gave it a try and introduced EmailRecipient and SmsRecipient in addition to the Recipient class: 7df6936

I also made sure that the different Recipient objects are always in a valid state to be able to remove the check for a non empty email and phone in the EmailChannel and SmsChannel.

Some tests were added as well: f45f246

I think this solution could be the way to go. WDYT?

P.S.: No idea why the AppVeyor build is failing.

src/Symfony/Component/Notifier/CHANGELOG.md

src/Symfony/Component/Notifier/Recipient/Assert.php

src/Symfony/Component/Notifier/Recipient/EmailValidationTrait.php

src/Symfony/Component/Notifier/Recipient/PhoneValidationTrait.php

src/Symfony/Component/Notifier/Tests/Recipient/RecipientTest.php

src/Symfony/Component/Notifier/CHANGELOG.md

This PR was merged into the 5.0 branch. Discussion ---------- [Notifier] Add unit tests | Q | A | ------------- | --- | Branch? | 5.0 | Bug fix? | no | New feature? | no  | Deprecations? | no  | Tickets | -  | License | MIT | Doc PR | -  - [x] `AbstractChannel` - [x] `ChannelPolicy` - [x] `RecipientTest` (done in PR #35773) - [x] `EmailRecipientTest` (done in PR #35773) - [x] `SmsRecipientTest` (done in PR #35773) - [x] `Transports` (see PR #35834) Commits ------- 022c170 [Notifier] Add tests for AbstractChannel and ChannelPolicy

src/Symfony/Component/Notifier/Recipient/SmsRecipientTrait.php

src/Symfony/Component/Notifier/Recipient/EmailRecipientTrait.php

jschaedl · 2020-04-17T13:25:07Z

@fabpot Thanks for your review. I hope I addressed all of your comments :-) See: 9368554

fabpot · 2020-04-17T14:27:08Z

src/Symfony/Component/Notifier/Recipient/Recipient.php

+    use SmsRecipientTrait;

-    public function __construct(string $email = '')
+    public function __construct(string $email = '', string $phone = '')


When setting the phone to an empty string, it will be passed anyway to SMS transports as it implements the interface. Before, we checked for a non-empty phone, which I think must be kept.

I think keeping the non-empty phone check would mean if I forgot to set a phone number even though I would like to send a SmsMessage, the SmsChannel would be skipped and I don't get any information why my notification was not sent as sms.

I would suggest a different approach: We could make the creation of SmsMessage more strict and throw an InvalidArgumentException with a proper exception message in case the $phone argument is empty. We could apply the same to EmailMessage

@fabpot WDYT?

Works for me.

I think you forgot to enforce a non-empty phone on SmsMessage (and the same for the other messages), right?

Oh no 🙈 thats true. On it.

Fixed: d11567f

fabpot

Sorry for the delay.

fabpot · 2020-08-06T04:07:42Z

src/Symfony/Component/Notifier/CHANGELOG.md

 CHANGELOG
 =========

 5.1.0


Moved: 1dab3f5

fabpot · 2020-08-06T04:07:54Z

src/Symfony/Component/Notifier/CHANGELOG.md

+   `RecipientInterface`.
+ * Changed `EmailChannel` to only support recipients which implement the `EmailRecipientInterface`.
+ * Changed `SmsChannel` to only support recipients which implement the `SmsRecipientInterface`.
+ * Added the Mattermost notifier bridge.


not related

Will revert it.

fabpot · 2020-08-06T04:12:16Z

src/Symfony/Component/Notifier/Recipient/Recipient.php

-    public function __construct(string $email = '')
+    public function __construct(string $email = '', string $phone = '')
    {
+        if (empty(trim($email)) && empty(trim($phone))) {


We try to avoid empty in Symfony. Also, I would not trim the value.

Adjusted: 1dab3f5

fabpot · 2020-08-06T04:13:48Z

src/Symfony/Component/Notifier/Recipient/Recipient.php


-    public function getEmail(): string
+    /**
+     * @return $this


I would keep the comment that was part of the interface: Sets the phone number (no spaces, international code like in +3312345678).

Comment added: 1dab3f5

fabpot · 2020-08-06T05:40:43Z

src/Symfony/Component/Notifier/Recipient/Recipient.php

+    use SmsRecipientTrait;

-    public function __construct(string $email = '')
+    public function __construct(string $email = '', string $phone = '')


Works for me.

jschaedl · 2020-08-07T05:59:09Z

Hi @fabpot thanks for your review :-)

I made the requested changes and rebased the branch on master. fabbot.io is complaining though, but it looks like a false positive again.

jschaedl · 2020-08-07T08:16:45Z

src/Symfony/Component/Notifier/composer.json

        "php": ">=7.2.5",
-        "symfony/polyfill-php80": "^1.15"
+        "symfony/polyfill-php80": "^1.15",
+        "psr/log": "~1.0"


I recognized this was missing, because the EmailMessageTest failed locally while using the Notification class that depends on psr/log.

fabpot · 2020-08-07T08:41:05Z

src/Symfony/Component/Notifier/Tests/Message/EmailMessageTest.php

@@ -0,0 +1,24 @@
+<?php
+
+declare(strict_types=1);


Should be removed everywhere, we're using that in Symfony.

fabpot · 2020-08-07T08:44:08Z

Thank you @jschaedl.

…ony/symfony#35773

nicolas-grekas · 2020-10-22T17:50:23Z

This PR is missing corresponding entries in UPGRADE-5.2.md
@jschaedl would you mind sending a PR to fix this, please?

jschaedl · 2020-10-23T13:35:28Z

@nicolas-grekas here you go: #38691

…og (jschaedl) This PR was merged into the 5.x branch. Discussion ---------- [Notifier] Add missing upgrade entries and fixed changelog | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | no | New feature? | no  | Deprecations? | no  | Tickets |  | License | MIT | Doc PR |   Follow-up of #35773 Commits ------- c6b32cd add missing upgrade entries and fixed changelog

carsonbot added Status: Needs Review Feature labels Feb 18, 2020

wouterj approved these changes Feb 18, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 18, 2020

OskarStark reviewed Feb 18, 2020

View reviewed changes

src/Symfony/Component/Notifier/Recipient/Recipient.php Show resolved Hide resolved

jschaedl force-pushed the notifier-recipient branch from a768a56 to 76d5a45 Compare February 18, 2020 20:58

OskarStark approved these changes Feb 18, 2020

View reviewed changes

nicolas-grekas added this to the next milestone Feb 20, 2020

jschaedl requested a review from fabpot February 23, 2020 15:14

carsonbot added Status: Needs Review and removed Status: Reviewed labels Feb 23, 2020

jschaedl mentioned this pull request Feb 24, 2020

[Notifier] Add unit tests #35847

Merged

6 tasks

xabbuh added the Notifier label Feb 26, 2020

xabbuh reviewed Feb 26, 2020

View reviewed changes

src/Symfony/Component/Notifier/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/CHANGELOG.md Outdated Show resolved Hide resolved

jschaedl force-pushed the notifier-recipient branch 2 times, most recently from 21bbb8d to 72cb155 Compare February 28, 2020 12:20

xabbuh reviewed Feb 29, 2020

View reviewed changes

src/Symfony/Component/Notifier/CHANGELOG.md Outdated Show resolved Hide resolved

jschaedl requested a review from xabbuh March 1, 2020 21:59

xabbuh reviewed Mar 3, 2020

View reviewed changes

src/Symfony/Component/Notifier/Recipient/Assert.php Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Recipient/Assert.php Outdated Show resolved Hide resolved

src/Symfony/Component/Notifier/Recipient/Assert.php Outdated Show resolved Hide resolved

jschaedl force-pushed the notifier-recipient branch 2 times, most recently from 5dfc57b to fd0b2d5 Compare March 6, 2020 17:01

xabbuh approved these changes Mar 7, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Mar 7, 2020

jschaedl force-pushed the notifier-recipient branch 2 times, most recently from f718a5f to d66a235 Compare March 8, 2020 09:51

carsonbot added Status: Needs Work and removed Status: Reviewed labels Apr 12, 2020

jschaedl force-pushed the notifier-recipient branch from 77585c7 to 9368554 Compare April 17, 2020 13:18

jschaedl commented Apr 17, 2020

View reviewed changes

src/Symfony/Component/Notifier/Recipient/SmsRecipientTrait.php Outdated Show resolved Hide resolved

jschaedl commented Apr 17, 2020

View reviewed changes

src/Symfony/Component/Notifier/Recipient/EmailRecipientTrait.php Outdated Show resolved Hide resolved

fabpot reviewed Apr 17, 2020

View reviewed changes

jschaedl force-pushed the notifier-recipient branch from 7c45d7b to ca644f9 Compare June 12, 2020 13:20

fabpot requested changes Aug 6, 2020

View reviewed changes

jschaedl force-pushed the notifier-recipient branch 2 times, most recently from a31b649 to 1dab3f5 Compare August 7, 2020 05:54

jschaedl commented Aug 7, 2020

View reviewed changes

fabpot approved these changes Aug 7, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Aug 7, 2020

[Notifier] Change notifier recipient handling

588607b

fabpot force-pushed the notifier-recipient branch from d11567f to 588607b Compare August 7, 2020 08:44

fabpot mentioned this pull request Aug 7, 2020

[Notifier] Change notifier recipient handling symfony/symfony-docs#14033

Closed

fabpot merged commit 7520884 into symfony:master Aug 7, 2020

jschaedl added a commit to jschaedl/symfony-docs that referenced this pull request Aug 7, 2020

[Notifier] Adjust notifier documentation according to changes in symf…

7b46948

…ony/symfony#35773

nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020

fabpot mentioned this pull request Oct 5, 2020

Release v5.2.0-BETA1 #38417

Merged

jschaedl mentioned this pull request Oct 23, 2020

[Notifier] Add missing upgrade entries and fixed changelog #38691

Merged

jschaedl deleted the notifier-recipient branch June 2, 2021 20:02

		@@ -0,0 +1,24 @@
		<?php

		declare(strict_types=1);

Uh oh!

[Notifier] Change notifier recipient handling #35773

[Notifier] Change notifier recipient handling #35773

Uh oh!

Conversation

jschaedl commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wouterj left a comment

Choose a reason for hiding this comment

Uh oh!

OskarStark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jschaedl commented Feb 18, 2020

Uh oh!

xabbuh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jschaedl commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jschaedl commented Apr 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jschaedl Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jschaedl commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jschaedl commented Feb 18, 2020 •

edited

Loading

jschaedl commented Feb 28, 2020 •

edited

Loading

jschaedl Aug 7, 2020 •

edited

Loading

jschaedl commented Aug 7, 2020 •

edited

Loading