Allow removal of to/cc/bcc/reply-to addresses (after they are added to an email)

We need this functionality for the proper work of https://www.drupal.org/project/reroute_email →

As cc/bcc/reply-to addresses can be erased for "the rerouting emails".
So, a new merge request with appropriate changes has been created:
https://git.drupalcode.org/project/symfony_mailer/-/merge_requests/66
Looking forward to this change being reviewed and merged/committed to the module.

Thanks!

Thanks. I have some small comments

Round #2 of Needs work » Needs Review 😃

@AdamPS, thanks for your prompt reaction and all your contributions to the community!

Great thanks, just one last change please

Plus, please can we have a test? I am accepting bug fixes without a test, and asking that new features include a test.

In SymfonyMailerKernelTest can add a new function testAddress(). Call a few of the BaseEmailInterface address functions with set and get. If possible use a single address, and multiple. Use the different options in Address::create.

Here is a short summary of the updated Merge request:

1) We need a way to set/get all headers (cc/bcc/reply-to) and it will be a bad way to duplicate the logic in every setter/getter.
So an addition method getAddress has been added to the interface and its implementation.
The input params for setAddress and getAddress have been aligned.

2) The same with tests. Added MailerTestTrait::assertAddress method.

3) Added LegacyTestEmailForm::ADDRESS_TO / LegacyTestEmailForm::ADDRESS_CC / LegacyTestEmailForm::ADDRESS_BCC constants.
To avoid hard-coding of the same addresses across the test module and the tests itself.

4) Added testEmailAddresses test to actually test all possible combinations of:
- different headers (cc, bcc, reply-to);
- different addresses for those headers (string, '', Address object without display name, Address object with display name, an array of all those options);
- it also tests both sets and erases (NULL value) for all of these headers.

It becomes to be a quite big Merge request, but this is the only way to put it all together.

ps. The first commit failed due to PHP 7.4 limitation (mixed typehint has been added in PHP 8). This was fixed and all tests are green now.

Looking forward!

Great many thanks for your enthusiasm to contribute to this module. The new changes are basically good, but please avoid unnecessary changes.

1) Here is a comment about mb_strtolower:
https://git.drupalcode.org/project/symfony_mailer/-/merge_requests/66/di...

2) Here is a comment about moving re-usable variables into constants:
https://git.drupalcode.org/project/symfony_mailer/-/merge_requests/66/di...

3) Param $header renamed to $name in setAddress/getAddress methods and everywhere for consistency with other code.
assertCc / assertTo methods are back as requested. Added corresponding assertBcc / assertReplyTo methods. Commit:
https://git.drupalcode.org/project/symfony_mailer/-/merge_requests/66/di...

All tests are passed after the changes.

Hopefully, we're finally ready to merge this request and proceed with other issues on the queue.

Thanks!

Thank you for your continued work on this issue.

1) I can't find your comment about mb_strtolower().

2)

Those addresses are used in different places (code, tests, etc).
We should have constants for them to avoid changing the address in some places and possibly forgetting (don't find) some others.
My bet is this class is good to store them. If some other better class/interface in the module to do so, please let me know.

Yes I agree that constants are good. The previous developer didn't use them, and I didn't insist because it wasn't that important. Why would we ever change the address? In fact you are the only one trying to change the address😃, which means that if any other developer had used LegacyTestEmailForm in their own tests then you would break their code.

This is an issue about removal of addresses. It doesn't really work well if each person who makes a patch tries to rewrite the code that the previous person wrote because they don't like the way it was done before. There have been a number of complaints for sites using this module about their sites breaking with new versions. My job as maintainer is to avoid unnecessary changes. Therefore please can you remove all the changes that are not related to this fix? For example TestEmailTest.php and testInlineCss().

Hi Adam,
Thanks for your passion while maintaining this module!
Hopefully, my comments are reading in a good manner. Cause I have a plan to propose some more MR in some other tasks here on drupal-org issue queue :) I maintain a few modules related to mail delivery ( reroute_email → , mailgun → , mailjet → , unione → ), and there is a plan on my plate to release D10 versions for all of them (compatible with Drupal Symfony Mailer).

1) Regarding mb_strtolower, here is another link to the comment (which will not be outdated again):
https://www.drupal.org/project/symfony_mailer/issues/3285188#mr66-note21... ✨ Allow removal of to/cc/bcc/reply-to addresses (after they are added to an email) Needs work

2) Regarding hard-coded test addresses,
a) While maintaining other modules related to mail delivery, I have already faced a few issues when different modules use the same simple test addresses (like to@example.com, cc@example.com, bcc@example.com, etc). It's always better to use either module-specific addresses or $this->randomMachineName() to generate unique addresses on every test run).
b) We should not hard-coded simple addresses like 'test@example.com' in different places (test module form class, main module test classes, etc). As we can misunderstand which address is related to which functionality and tests (if we have exactly the same hard-coded addresses). Either variables or constants will greatly help understand the code for all further contributors (which address comes from and where they are used).
c) Introducing a new constant will not cause any incompatibility issues.

3) All changes from `tests/src/Functional/TestEmailTest.php` and `tests/src/Functional/OverrideTest.php` have been removed according to these comments:
✔ https://www.drupal.org/project/symfony_mailer/issues/3285188#mr66-note21... ✨ Allow removal of to/cc/bcc/reply-to addresses (after they are added to an email) Needs work
✔ https://www.drupal.org/project/symfony_mailer/issues/3285188#mr66-note21... ✨ Allow removal of to/cc/bcc/reply-to addresses (after they are added to an email) Needs work

4) `empty($addresses)` is replaced with `is_null($addresses)` to align with the interface's method documentation according to this comment:
✔ https://www.drupal.org/project/symfony_mailer/issues/3285188#mr66-note21... ✨ Allow removal of to/cc/bcc/reply-to addresses (after they are added to an email) Needs work
This one was a really good catch.

Thanks!

OK, thanks again, your new code is a good balance of stability and improving quality.

1 )I still don't see the comment about strtolower😃. If it's wrong then of course I'm willing to fix it. But in that case we should fix it in all places in this module, and perhaps it would be better on a different issue.

2) $this->assertContains($name, ['to', 'cc', 'bcc', 'reply-to']);
Please can we remove this line? (I believe I made a comment before but it got lost)

And instead put the assert in getAddress() same as setAddress()

    $name = strtolower($name);
    assert(isset($this->addresses[$name]));

Here is a copy-paste of the comment about `strtolower`:

All Drupal core and Drupal contrib modules are updating to use mb_strtolower instead of strtolower. Since Drupal 8.6.0 it's recommended to use mb_* functions (instead of drupal_strtolower, or Unicode::strtolower, or strtolower).
Change record: https://www.drupal.org/node/2850048 →
Core recommendation: Use mb_strtolower() instead.

_____

MR updated according to all requested changes in the last comment:

1) Replaced all usages of mb_strtolower to `strtolower` (as it is currently on 1.x branch).
We currently have four usages of `strtolower` function across the module.
I'll create another drupal-org issue to proceed on this.

2) Removed this line.

Hopefully, we're ready to merge this!
As it's required for the proper work of reroute_email module.

Great thanks. Sorry I know you want to get this in, and we're really close now.

2) had two parts: remove a line (in the tests), then add two lines (in the live code). Any chance you could make this one final change please then I will commit.

I'm afraid I can't understand this. Could you please clarify on:

remove a line (in the tests), then add two lines (in the live code).

Do you mean to create some new tests that remove lines from the email or does some code need to be changed?
But I can't find any lines in the code to represent this need.

Thanks!

Please could you add these two lines in BaseEmailTrait::getAddress() the same as setAddress()

    $name = strtolower($name);
    assert(isset($this->addresses[$name]));

This will catch getAddress with a wrong name

MR !66 updated with the requested changes (commit).

Great thanks

Automatically closed - issue fixed for 2 weeks with no activity.