Support multiple attribute for email element

The last submitted patch, 8: 3296190-support-email-multiple-8.patch, failed testing. View results →

Issue summary seems incomplete, I got it started but if TBD sections can be reviewed and if they don't apply can put NA. But seems like a UI or API change that should be noted.

May need a change record but not 100% there.

Rebased to run test-only feature

1) Drupal\Tests\system\Functional\Form\EmailTest::testFormEmail
Behat\Mink\Exception\ElementNotFoundException: Form field with id|name|label|value "email_multiple" not found.
/builds/issue/drupal-3296190/vendor/behat/mink/src/WebAssert.php:662
/builds/issue/drupal-3296190/core/tests/Drupal/Tests/UiHelperTrait.php:84
/builds/issue/drupal-3296190/core/modules/system/tests/src/Functional/Form/EmailTest.php:36
/builds/issue/drupal-3296190/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!

Which is good.

Updated the change record some. Think it would be good to get in and then work on conversions in core but lets see.

Rest looks good

The issue summary includes both patches and multiple merge requests. There should be only one canonical patch or merge request listed.

Please close any non-canonical merge request(s) and hide non-canonical patches. If you don't have permission to close merge requests, please hide any non-canonical patches and then document which merge request(s) should be closed in an issue comment and under a separate header in the issue summary. This will allow a committer to close them for you. Thanks!

Seems like we already have hidden non-canonical patches.
Updated PR from the 11.x branch and returned the previous status.

Added some questions/feedback to the MR.

Fixed two issues from the comments.

Seems additional feedback has been addressed.

Sorry, one more little bit of feedback: we should add docs so users know this is possible.

Updated the element doc and info.

Since the last changes don't have any effect on the element behavior I switch back the RTBC status.

P.S. Ignore the spell-checking error. "Argument list too long" is a Drupal core issue - 🐛 Spell-checking job fails with "Argument list too long" when too many files are changed Active .
Seems like the last merging of the 11.x caused the error.

Added a few more nits and questions to the MR.

@longwave I went over your last comments.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Responded to the MR comments.

Handled the last issues.
One of them was a really important one, @longwave nice catch!

Appears feedback has been addressed.

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions.

I read the MR and left some suggestions. I also think input is needed from Usability on the new strings. Adding tag for a usability review.

I read the MR and I do suggest a change. When I read "Provide #multiple property for email render element with correct validation." I ask myself why we would provide an element with incorrect validation. I think it could be reworded. I know it is minor.

Setting to NW.

@quietone I went through your proposals in MR.
Some of them I've fixed and for some of them I wrote an answer. You can recheck them.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-04-19 Needs work . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @shaal, @simohell, @skaught, and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

There was some discussion about whether we should handle multiple email addresses in separate textfields instead of allowing a comma-separated list. We decided that the current approach is the right choice. It is consistent with the email specification, and the Field API (not the Form API) is the right place to handle multiple textfields.

We tested various invalid entries, and we used the Disable HTML5 validation → module so that we could see the error messages provided by this issue. The current approach works well, but we would like to make two changes:

1. Do not treat empty emails differently from other invalid emails.
2. Insert a comma in the second sentence: "Use the format user@example.com, and separate the addresses with a comma."

An error message should do two things: describe the error state and give information on how to fix it. The current message, "All email addresses must be non-empty.", tries to do both at once. We think it would be clearer to use the same format as the other error messages:

The email address "" is not valid. Use the format user@example.com, and separate the addresses with a comma.

If possible, use the un-trimmed value in the error message so that there is a distinction between ,, and , ,.

Other argument in favor of (1):

• The error messages are more consistent.
• If a user enters an empt address and an invalid one, then currently they get the message about an empty address. They fix it and try again, and get the message about the invalid address. Let's list all the errors at once.

The second point is purely grammatical: when combining independent clauses with "and", there should be a comma after the first one.

Hi @benjifisher
1. Fixed a small grammatic issue
2. About the empty email error message:

If a user enters an empt address and an invalid one, then currently they get the message about an empty address. They fix it and try again, and get the message about the invalid address. Let's list all the errors at once.

I really liked this idea and implemented it.
But I don't like the idea to make an "empty" message like this

The email address "" is not valid. Use the format user@example.com, and separate the addresses with a comma.

For me, this message is unclear/unreadable/not user-friendly. It is very difficult to correctly display an "empty" value. Using quotation marks for this purpose can only worsen the situation, as the user may start looking for something that is not there. A message like that really can confuse a user.
While the direct text that an empty email address value has been entered is clear and not ambiguous.

Made the small change to the CR mentioned.

Appears to be 1 open thread but overall looking good! Believe this one to be very close.

Wrote an answer to the last comment in the MR

Not sure if it saved but don't see the comment?

Sorry @smustgrave, forgot to submit my comment there :)

@HitchShock no worries figured that was probably the case. All threads have been addressed/answered so believe this one to be good.

What is the error message now when there are two or more commas in a row? That scenario was the reason behind the suggestion @skaught raised. By removing extra commas the message wouldn't been necessary anymore. Without the stripping it is necessary.

one other detail, at first i thought it is unrelated to this issue but it seems it is. in the ux meeting as well as testing for myself, after applying the patch, i've added '#multiple' => TRUE, line to the email field in SiteinformationForm.php. Problem is as soon as the multiple line is added the email field is shown empty on page reload and or save (in Safari 17.4.1 in Sonoma 14.4.1). At the same time I am able to see the content of the field when i open the page in for example Firefox. As soon as i remove the multiple equals true line the emails become visible again in Safari as well.

Thanks for working on this one, left some comments on the MR

Solved the last PR issues

smustgrave → changed the visibility of the branch 11.x to hidden.

smustgrave → changed the visibility of the branch 3296190-support-multiple-attribute to hidden.

Re-ran test-only feature

1) Drupal\Tests\system\Functional\Form\FormTest::testDisabledElements
TypeError: Drupal\Tests\system\Functional\Form\FormTest::assertFormValuesDefault(): Argument #1 ($values) must be of type array, null given, called in /builds/issue/drupal-3296190/core/modules/system/tests/src/Functional/Form/FormTest.php on line 790
/builds/issue/drupal-3296190/core/modules/system/tests/src/Functional/Form/FormTest.php:799
/builds/issue/drupal-3296190/core/modules/system/tests/src/Functional/Form/FormTest.php:790
ERRORS!
Tests: 16, Assertions: 622, Errors: 1.

So change has coverage

Believe all feedback has been addressed and code change seem fine to me.
I followed the steps in the summary and manually can confirm this works as advertised.

Thanks for the continued work on this issue.

I reviewed the MR an the comments in the MR. I have left several suggestions for comment and code changes. Also, #44 and #45 need to be addressed. This is making changes to the UI and there should be a screenshot to show the changes. In particular, it would help to see the error messages in regard to the testing done in #44 and #45.

I now see that I have erred by missing the usability review in #37. I will take a wee break and then read that and make any adjustments needed to my comments. I will leave at RTBC until I do that.

There is still a question about the error message change suggested in #37 and disagreed with in #38. It is the same text I have questioned as well. I suggest the solution is to ask in #usability for more guidance.

I see that the simpler the work, the more it is scrutinized, and everyone sees something different and changes the same thing several times. And they pick on such trifles that are absolutely not critical and will not affect the overall functionality.
Instead of quickly implementing a simple change, everyone is engaged in bureaucratic ping-pong for no real reason.

In a month, someone else will come and say that they don't like some error text (which is generally displayed only when UI validation is disabled, which is almost never) or comment text.

I'm tired of fulfilling the whims of everyone who comes to check the task. So I'm stopping working on the issue.

Believe a few threads are still open or pending response from those that opened it. Going to leave in review for additional eyes.

@HitchShock, I read your comment that you were stopping work on this issue. I want to thank you for the work you have done to complete this issue, it is much appreciated!

You may not be aware of the reason for the reviews and I apologize in advance if you already know this. All changes to Drupal core should pass what we call the Core Gates → . These exist to assist maintaining our standards, such as stability, code quality, documentation etc. And like anything, it takes time to learn about them and to start using them when writing or reviewing code. For this issue, the reviewers have mostly been core committers who have an added responsibility to ensure the work passes those gates. And yes, I too have experienced many rounds of feedback on an issue and it can be frustrating! But, on the good side, it has always resulted in a better change.

I added an remaining item to confirm there are no implications for things like \Drupal\Core\Field\Plugin\Field\FieldFormatter\MailToFormatter,
I have not reviewed the MR.

This one has sat ideal for a bit should it go to NW for the research about what's mentioned in #57?

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

False positive

@smustgrave @quietone @hitchshock @rkoller I am sympathetic to @hitchshock's viewpoint expressed in #53. However I am also appreciative of the very clear and helpful analysis of @rkoller at #44 and #45.

In response to #58 I suggest not sending this issue into the 'long grass'. Let's harvest the fruit while it is ripe! #44 and #45 reveal that there are at most 3 things that can go wrong with user input:

1. The user puts a single comma in the wrong place eg at end of the emails
2. The user puts two commas in succession
3. The user enters one or more malformed email addresses

I suggest therefore that instead of using several different messages that either one or two different messages are used. The message(s) will urge the user to check for instances of the 3x possible errors listed above. One message could be enough and it would surely simplify the fix, for example, "Please check the comma-separated email addresses. There must be only one comma separating each address and all email addresses must be valid."

I suppose it may be necessary to have 3 messages. One for only comma-related errors, one for malformed email addresses and one for a combination of both. But if we can reduce it to 2 or even one that would be much better.

Can we agree a one message solution. If that turns out impossible let's seek a 2 messages one.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.