Upgrade EmailValidator to 2.x

Maybe here a really naive question: Do we have to move to version 2.x of the library? Is there a fundamental reason to do so, beside receiving bug fixes?

@cilefen
That is a strong point, let's add this to the issue summary.

Accommodated #16 to move this forward. Is it RTBC or N/W?

//Naveen

I'm wondering whether you could do something with autoloading stuff inside composer.json

We don't need the hook_update because this will go in a new version and the container is cached with a version key.

Here's a version where typehints of both \Drupal\Component\Utility\EmailValidatorInterface and \Egulias\EmailValidator\EmailValidator will both work. So existing code in contrib or custom will continue to work but we'll also get to typehint on the new EmailValidatorInterface.

I think we need a CR to tell people to update any typehints to use the new interface.

+++ b/core/core.services.yml
@@ -1630,7 +1630,7 @@ services:
   email.validator:
-    class: Egulias\EmailValidator\EmailValidator
+    class: Drupal\Component\Utility\EmailValidator

Here is a question: Should we introduce a new validation service, and mark the old one as deprecated? This way we could inform people that they need to change stuff (Like changing typehints)

Just marked #2930358: Update to Swift Mailer 6.x as postponed on this issue. Swiftmailer cannot upgrade to 6.x because of this dependency conflict.

Bumping priority as there doesn't seem to be a good workaround for this contrib blocker.

+++ b/core/lib/Drupal/Component/Utility/EmailValidator.php
@@ -0,0 +1,22 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function isValid($email, EmailValidation $email_validation = NULL) {

+++ b/core/lib/Drupal/Component/Utility/EmailValidatorInterface.php
@@ -0,0 +1,21 @@
+  /**
+   * Validates an email address.
+   *
+   * @param string $mail
+   *   A string containing an email address.
+   *
+   * @return bool
+   *   TRUE if the address is valid.
+   */
+  public function isValid($mail);

Hmmm these don't match. Need to document the $email_validation param.

re #22 that gets us into the fun of service versions - unless we can think of a better name than email.validator.v2 - I guess we could take the opportunity to deprecate and namespace the service name ie. core.email.validator. But I'm not sure that's a great solution.

This patch is a re-roll, upgrades egulias/email-validator from 2.1.2 to 2.1.5, addresses the feedback from #28, and renames the EmailValidator::isvalid() $mail parameter to $email for consistency with the implemented interface.

I published a change record.

@Xano thanks for the change record.

Reading it makes me ponder if we're doing this right. I think the question is on what level should the validators be set. Is it the code level or the application level? I think for the email.validator service it probably should be on the application level - ie. via the container. So I'm not sure that email.validator should have a method that allows you to change the validators. If you want to do that in code just create an Egulias\EmailValidator\EmailValidator object and away you go.

So I'm thinking that the service itself should not really allow the validator to be passed in. It feels odd to do that. If you want to customise the email validation for the entire application you should replace the service. What I'm thinking is that we should throw an error if isValid() is called with the validator argument. And in Drupal 9 we should decouple Drupal\Component\Utility\EmailValidator from Egulias\EmailValidator\EmailValidator. In D8 it is useful that they are coupled because anything typehinting on Egulias\EmailValidator\EmailValidator will still work but in D9 everything you typehint to the interface and the interface should reflect the concerns of the service i.e. not passing in a validator.

+++ b/core/lib/Drupal/Component/Utility/EmailValidator.php
@@ -0,0 +1,31 @@
+  /**
+   * Validates an email address.
+   *
+   * @param string $email
+   *   A string containing an email address.
+   * @param \Egulias\EmailValidator\Validation\EmailValidation|null $email_validation
+   *   The validation method to use. If NULL, the method will default to
+   *   \Egulias\EmailValidator\Validation\RFCValidation.
+   *
+   * @return bool
+   *   TRUE if the address is valid.
+   */
+  public function isValid($email, EmailValidation $email_validation = NULL) {
+    $email_validation = $email_validation ?: new RFCValidation();

+++ b/core/lib/Drupal/Component/Utility/EmailValidatorInterface.php
@@ -0,0 +1,21 @@
+interface EmailValidatorInterface {
+
+  /**
+   * Validates an email address.
+   *
+   * @param string $email
+   *   A string containing an email address.
+   *
+   * @return bool
+   *   TRUE if the address is valid.
+   */
+  public function isValid($email);

The current patch is not documenting $email_validation on the interface. I agree with this. It shouldn't be part of the service interface.

Here's a patch that enforces the fact that we ignore custom validation on our service.

I've swapped around the change record to detail what we really want people to do. Which is to update any typehints from \Egulias\EmailValidator\EmailValidator to \Drupal\Component\Utility\EmailValidatorInterface. The fact that we're also upping the version is an implementation detail.

Crediting all the people who worked on #2749873: "email.validator" core service cannot be easily replaced since these issues have turned out to be duplicates.

I just stumbled upon #2965887: Require egulias/email-validator >= 1.2.14 and was wondering if we want to credit that person too when we close that issue after this one has been fixed.

@Xano I think we close but not credit on this issue. That's because the other did the same as this to make the service swappable but that issue just bumps a dependency in core/composer.json.

#33 still applies and LGTM.

Anything blocking landing #33 at this point?

Spoke too soon. Re-roll of #33.

+++ b/core/lib/Drupal/Component/Utility/EmailValidator.php
@@ -0,0 +1,33 @@
+  public function isValid($email, EmailValidation $email_validation = NULL) {
+    if ($email_validation) {
+      throw new \BadMethodCallException('Calling \Drupal\Component\Utility\EmailValidator::isValid() with the second argument is not supported. See https://www.drupal.org/node/2997196');
+    }

Do we want to add the same kind of check for the third argument ($strict) that 1.x supported as well? Maybe emit a deprecation warning for that instead of a bad method call exception?

@effulgentsia I'm not sure that that is worth it. Here's why. The old signature is public function isValid($email, $checkDNS = false, $strict = false); so if you've called it like so ->isValid('blha', FALSE, TRUE) you're going to get a hard error due to the second argument not being an instance of \Egulias\EmailValidator\Validation\EmailValidation.

This makes this issue interesting because it makes it impossible to provide a new email.validator service that satisfies the two BC concerns... ie. still implements \Egulias\EmailValidator\EmailValidatorInterface and accepts either format of isValid().

I think the current solution is the best one because

• code that assumes the service implements \Egulias\EmailValidator\EmailValidatorInterface or extends from \Egulias\EmailValidator\EmailValidator still works
• if you have added additional params to isValid() the break is going to be hard and the error message will point to exactly what's wrong.

However we could argue that we should only be making this change in Drupal 9.

Huge plus one on this going back to RTBC from me.

hmm I think at least the information in #49 should be in the change record - however that limitation doesn't make this issue a problem for a minor IMO.

I've added the following caveat to the change record.

If you have overridden the email.validator service there is a small chance that the overridden service will not implement EmailValidatorInterface::isValid() correctly. In such cases, PHP will error immediately and the solution is to fix your overridden implementation to comply with the interface.

Committed 9e7b020 and pushed to 8.7.x. Thanks!

Would a backport of to 8.6.x ever be considered for this issue? From what I can gather, it is pretty much impossible to implement a similar core patch in a local project that uses a composer to include Drupal with drupal/core.

Echoing Bonus here: is it possible to backport this? Not asking for someone else to do the work necessarily, but I would love the opinion of someone who understands the subsystems here better, to know if a backport is even possible or reasonable.

I think bumping a major version dependency can be done in a minor release. But not within a hotfix in 8.6.x. Gotta keep our BC promises.

That's fair. 8.7 is not that far off. Thanks!

Published the change record.

Just tried to check the Drupal 8.7 compatibility of the module that I maintain and all classes where I injected the email validator service is broken :face_with_rolling_eyes:

1) Drupal\Tests\apigee_edge\Functional\ConfigurationPermissionTest::testAccess
TypeError: Argument 3 passed to Drupal\apigee_edge\Entity\Controller\Cache\DeveloperAppCacheFactory::__construct() must be an instance of Egulias\EmailValidator\EmailValidatorInterface, instance of Drupal\Component\Utility\EmailValidator given, called in /var/www/html/build/core/lib/Drupal/Component/DependencyInjection/Container.php on line 277

The reason is quite obvious before Drupal 8.7 there was no `Drupal\Component\Utility\EmailValidatorInterface` interface, I could only use the interface provided by the vendor package. Isn't it some kind of BC break?

https://github.com/drupal/core/blob/8.6.x/core.services.yml#L1691-L1692
vs.
https://github.com/drupal/core/blob/8.7.x/core.services.yml#L1692-L1693

https://github.com/drupal/core/tree/8.6.x/lib/Drupal/Component/Utility
vs.
https://github.com/drupal/core/tree/8.7.x/lib/Drupal/Component/Utility

As I can see I can only support both 8.6. and 8.7 at the same time if I implement an adapter or a proxy class that I'll use in type-hints and which will translate between email-validator >=1.2 and >=2.0.

Apologies if this is OT. Not sure where I should post this to follow the issue, but upgrading to Drupal 8.7.1 breaks webform, which gives HTTP ERROR 500, upon submission:

PHP Fatal error: Declaration of Drupal\Component\Utility\EmailValidator::isValid($email, ?Egulias\EmailValidator\Validation\EmailValidation $email_validation = NULL) must be compatible with Egulias\EmailValidator\EmailValidator::isValid($email, $checkDNS = false, $strict = false) in /var/www/iweek/site/core/lib/Drupal/Component/Utility/EmailValidator.php on line 12

Where might be the correct place to follow this or submit an issue? As it seems to be a core issue.

Any advice gratefully appreciated.

I am also having this problem upgrading to 8.7.1 when trying to see the admin/config/system/site-information page and also when trying to update the Basic Site Email.
How can I add this to the issue? I have not found it reported yet.

I guess you forgot to update the email validator package. You should see this output for this command:

$ composer show | grep email
egulias/email-validator              2.1.7              A library for validating emails against several RFCs

If you see 1.x.x as a version you should update the email validator package to 2.x.

Thanks for the swift response.
I seem to have the latest version
egulias/email-validator 2.1.7 A library for validating emails against several RFCs

> This will not break any usages in contrib. Existing injections of the service will still work because it extends from the same class.

It does, if contrib injections of the service typehint on Egulias\EmailValidator\EmailValidatorInterface, which is gone in version 2.0 of the package.

#68 +1

Another way it breaks contrib is that if you contrib wanted to use the old service like so:

$validator->isValid('my@mail.com', TRUE, TRUE)

Currently this is not the function signature of the same service and its method. Also, it is not possible to check dns (second parameter) in the new core service. So if I had a module that would check dns on registration, for example, it would all of a sudden not work even one tiny bit after I upgraded to 8.7.

The wording in the change record is therefore not completely true :)

Basic email validation is unchanged:

One can easily get around this by using the EmailValidator class directly, but it is for sure a BC break in my opinion.

Can we add this documentation to the change record somehow? Or somewhere else? Should be documented at least? What can I do to help others that needs these other parameters?