Problem/Motivation

#2343043: valid_email_address() should use egulias/EmailValidator and become deprecated deprecated valid_email_address().

Proposed resolution

Replace calls to valid_email_address() with \Drupal::service('email.validator')->isValid($mail).

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because Drupal will work without making this change.
Issue priority Normal because this removes usages of a deprecated function but there will remain the backwards-compatible function.
Disruption Not disruptive for core/contributed and custom modules/themes because the BC function will remain through 8.x.
CommentFileSizeAuthor
#16 remove_usages_of_the-2417327-16.patch7.63 KBcilefen
#16 interdiff-13-16.txt1.66 KBcilefen
#13 remove_usages_of_the-2417327-13.patch6.55 KBcilefen
#13 interdiff-11-13.txt1.84 KBcilefen
#11 remove_usages_of_the-2417327-11.patch7.83 KBcilefen
#11 interdiff-7-11.txt3.79 KBcilefen
#8 remove_usages_of_the-2417327-7.patch5.2 KBcilefen
#8 interdiff-4-7.txt810 bytescilefen
#5 remove_usages_of_the-2417327-4.patch5.19 KBcilefen
#5 interdiff-2-4.txt429 bytescilefen
#2 remove_usages_of_the-2417327-2.patch5.35 KBcilefen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Related issues: +#2343043: valid_email_address() should use egulias/EmailValidator and become deprecated
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Active » Needs review
FileSize
remove_usages_of_the-2417327-2.patch5.35 KB
cilefen’s picture

cilefen CreditAttribution: cilefen commented

I manually tested the affected forms:

  • The update settings form.
  • The contact form edit form.
  • The email action form.
  • The email field form.
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Needs review » Needs work 
+++ b/core/modules/action/src/Plugin/Action/EmailAction.php
@@ -58,13 +59,21 @@ class EmailAction extends ConfigurableActionBase implements ContainerFactoryPlug
+  /**
+   * The language manager.

unrelated change.

I know it is difficult to resist, but only change lines that are needed to stay in scope.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
interdiff-2-4.txt429 bytes
remove_usages_of_the-2417327-4.patch5.19 KB

Removed an unrelated change. Here is the grep command for finding usages:

$ grep -r valid_email_address core/
core//includes/common.inc:function valid_email_address($mail) {
cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue summary: View changes
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Needs review » Needs work 
+++ b/core/modules/action/src/Plugin/Action/EmailAction.php
@@ -83,8 +91,10 @@ class EmailAction extends ConfigurableActionBase implements ContainerFactoryPlug
+   * @param \Egulias\EmailValidator\EmailValidator
+   *   The email validator.

since this a new line, we can make it meet standards (but dont fix the lines above that are not changed by this patch).

https://www.drupal.org/node/1354#param

add the var name.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs work » Needs review
FileSize
interdiff-4-7.txt810 bytes
remove_usages_of_the-2417327-7.patch5.2 KB

Fixed the omitted the parameter name in the EmailAction constructor.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Status: Needs review » Reviewed & tested by the community

pre-emptive rtbc, as it *should* come back green.

Read the whole thing and it stays in scope.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/lib/Drupal/Core/Render/Element/Email.php
@@ -64,7 +64,7 @@ public static function validateEmail(&$element, FormStateInterface $form_state,
-    if ($value !== '' && !valid_email_address($value)) {
+    if ($value !== '' && !\Drupal::service('email.validator')->isValid($value)) {

    I think we can inject this too by implementing ContainerInjectionInterface. 

  2. +++ b/core/modules/contact/src/ContactFormEditForm.php
@@ -92,7 +92,7 @@ public function validate(array $form, FormStateInterface $form_state) {
+      if (!\Drupal::service('email.validator')->isValid($recipient)) {

+++ b/core/modules/update/src/UpdateSettingsForm.php
@@ -86,7 +86,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+          if (\Drupal::service('email.validator')->isValid($email)) {

    We should be injecting the service here rather that using \Drupal.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs work » Needs review
FileSize
interdiff-7-11.txt3.79 KB
remove_usages_of_the-2417327-11.patch7.83 KB

@alexpott Thank you for reviewing.

Status: Needs review » Needs work

The last submitted patch, 11: remove_usages_of_the-2417327-11.patch, failed testing.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs work » Needs review
FileSize
interdiff-11-13.txt1.84 KB
remove_usages_of_the-2417327-13.patch6.55 KB

ContactFormEditForm is injected. I don't know how to implement ContainerInjectionInterface in the context of static methods so \Drupal\Core\Render\Element\Email accesses the email.validator from \Drupal in this patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

@cilefen sorry I missed that it was a static method - we have to leave a \Drupal :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Needs work 
+++ b/core/modules/update/src/UpdateSettingsForm.php
@@ -86,7 +86,7 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
-          if (valid_email_address($email)) {
+          if (\Drupal::service('email.validator')->isValid($email)) {

This definitely can be injected :)

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Status: Needs work » Needs review
FileSize
interdiff-13-16.txt1.66 KB
remove_usages_of_the-2417327-16.patch7.63 KB

@alexpott I should have checked for that. I am sorry to waste your time.

abhi170893’s picture

abhi170893 CreditAttribution: abhi170893 commented
Status: Needs review » Reviewed & tested by the community

After applying the patch, the grep command for finding the usage of valid_email_address() gives only 1 result ( see issue summary):

grep -r valid_email_address core/ 
core/includes/common.inc:function valid_email_address($mail) {
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed bdb618d and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed bdb618d on 8.0.x 
    Issue #2417327 by cilefen: Remove usages of the deprecated...

Status: Fixed » Closed (fixed)

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