Problem/Motivation

Email FAPI element was added without support of multiple attribute. See https://www.drupal.org/project/drupal/issues/1174620#comment-4564144.

Currently even if the attribute set directly through #attributes property Drupal does not accept comma-separated emails.

Steps to reproduce

  • Add to any form an email element 
$form['email'] = [
  '#type' => 'email',
  '#title' => $this>t('Email'),
];
  • Try to add a multiple attribute via the #attributes property 
    •   '#attributes' => ['multiple' => '']
  • Fill in the email input element by several comma-separated emails. E.g. foo@example.com,bar@example.com
  • Submit the form.
  • Expected: the form must be submitted without errors
  • Actual: HTML5 doesn't return any errors but Drupal element validation does.
    The email address <em>foo@example.com,bar@example.com</em> is not valid. Use the format user@example.com.

    • Proposed resolution

    Support "multiple" attribute same way as for select lists.

    $form['emails'] = [
  '#type' => 'email',
  '#title' => $this->t('Emails'),
  '#multiple' => TRUE,
  '#description' => $this->t('Comma-separated email addresses.')
];
    • Allow to add multiple attribute via #multiple element property
    • Update element validation to support #multiple property

    Later, it will be possible to add this option to Email fields as well, but first, an element itself needs to comply with the HTML documentation.

    Remaining tasks

    Review
    Commit

    User interface changes

    NA

    API changes

    The API of the Email Render element must be updated.
    https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

    Data model changes

    NA

    Release notes snippet

    Provide multiple attribute support for Email render element.

    CommentFileSizeAuthor
    #35 3296190-nr-bot.txt90 bytesneeds-review-queue-bot
    #33 3296190-nr-bot.txt90 bytesneeds-review-queue-bot
    #27 3296190-nr-bot.txt90 bytesneeds-review-queue-bot
    #14 3296190-support-email-multiple-10.1.5.patch8.48 KBrowrowrowrow
    #10 3296190-support-email-multiple-10.patch8.78 KBHitchShock
    #8 3296190-support-email-multiple-8.patch7.26 KBHitchShock

    Issue fork drupal-3296190

    Command icon Show commands

    Start within a Git clone of the project using the version control instructions.

    Or, if you do not have SSH keys set up on git.drupalcode.org:

    About issue forks
    Support from Acquia helps fund testing for Drupal Acquia logo

    Comments

    Chi created an issue. See original summary.

    immaculatexavier made their first commit to this issue’s fork.

    Chi’s picture

    Chi CreditAttribution: Chi commented
    Issue summary: View changes

    Version: 9.5.x-dev » 10.1.x-dev

    Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 10.1.x-dev » 11.x-dev

    Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    HitchShock made their first commit to this issue’s fork.

    HitchShock opened merge request !5063

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Active » Needs review
    FileSize
    3296190-support-email-multiple-8.patch7.26 KB

    Status: Needs review » Needs work

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

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review
    FileSize
    3296190-support-email-multiple-10.patch8.78 KB
    smustgrave’s picture

    smustgrave CreditAttribution: smustgrave at Mobomo commented
    Issue summary: View changes
    Status: Needs review » Needs work
    Issue tags: +Needs issue summary update, +Needs screenshots

    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.

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Issue summary: View changes
    Status: Needs work » Needs review
    smustgrave’s picture

    smustgrave CreditAttribution: smustgrave at Mobomo commented
    Issue summary: View changes
    Status: Needs review » Reviewed & tested by the community
    Issue tags: -Needs issue summary update, -Needs screenshots +Needs Review Queue Initiative

    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

    rowrowrowrow’s picture

    rowrowrowrow CreditAttribution: rowrowrowrow as a volunteer commented
    FileSize
    3296190-support-email-multiple-10.1.5.patch8.48 KB

    This is a version of the patch provided in #10 but for core version 10.1.5

    xjm’s picture

    xjm
    she/her
    Primary language English
    CreditAttribution: xjm at HeroDevs commented
    Title: Support multiple attribute for email element » Support the '#multiple' attribute for email element
    Status: Reviewed & tested by the community » Needs work

    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!

    xjm’s picture

    xjm
    she/her
    Primary language English
    CreditAttribution: xjm at HeroDevs commented
    Title: Support the '#multiple' attribute for email element » Support the '#multiple' attribute for the email element
    andypost’s picture

    andypost
    he/him
    Primary language Russian
    CreditAttribution: andypost as a volunteer and at Skilld commented
    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review
    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs review » Reviewed & tested by the community

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

    longwave’s picture

    longwave
    he/him
    Primary language English
    Location UK
    CreditAttribution: longwave at Full Fat Things commented
    Status: Reviewed & tested by the community » Needs work

    Added some questions/feedback to the MR.

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review

    Fixed two issues from the comments.

    smustgrave’s picture

    smustgrave CreditAttribution: smustgrave at Mobomo commented
    Status: Needs review » Reviewed & tested by the community

    Seems additional feedback has been addressed.

    longwave’s picture

    longwave
    he/him
    Primary language English
    Location UK
    CreditAttribution: longwave at Full Fat Things commented
    Status: Reviewed & tested by the community » Needs work

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

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Reviewed & tested by the community

    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 - #3401988: Spell-checking job fails with "Argument list too long" when too many files are changed.
    Seems like the last merging of the 11.x caused the error.
    Other tests are still passing successfully.

    longwave’s picture

    longwave
    he/him
    Primary language English
    Location UK
    CreditAttribution: longwave at Full Fat Things commented
    Status: Reviewed & tested by the community » Needs work

    Added a few more nits and questions to the MR.

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review

    @longwave I went over your last comments.

    needs-review-queue-bot’s picture

    needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
    Status: Needs review » Needs work
    FileSize
    3296190-nr-bot.txt90 bytes

    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.

    longwave’s picture

    longwave
    he/him
    Primary language English
    Location UK
    CreditAttribution: longwave at Full Fat Things commented

    Responded to the MR comments.

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review

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

    smustgrave’s picture

    smustgrave CreditAttribution: smustgrave at Mobomo commented
    Status: Needs review » Reviewed & tested by the community

    Appears feedback has been addressed.

    quietone’s picture

    quietone CreditAttribution: quietone at PreviousNext commented
    Status: Reviewed & tested by the community » Needs work
    Issue tags: +Needs usability review, +Needs change record updates

    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.

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review

    @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.

    needs-review-queue-bot’s picture

    needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
    Status: Needs review » Needs work
    FileSize
    3296190-nr-bot.txt90 bytes

    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.

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review
    needs-review-queue-bot’s picture

    needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
    Status: Needs review » Needs work
    FileSize
    3296190-nr-bot.txt90 bytes

    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.

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review
    benjifisher’s picture

    benjifisher
    he/him or they/them
    Primary language English
    Location Boston area
    CreditAttribution: benjifisher as a volunteer commented
    Status: Needs review » Needs work
    Issue tags: -Needs usability review

    Usability review

    We discussed this issue at #3440370: Drupal Usability Meeting 2024-04-19. 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.

    HitchShock’s picture

    HitchShock
    Location Ukraine
    CreditAttribution: HitchShock at DevBranch, Drupal Ukraine Community, Dropsolid commented
    Status: Needs work » Needs review

    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 of making 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.