egulias/EmailValidator prior to 2.1.22 allows addresses with a space in the domain part

This seems similar to https://github.com/egulias/EmailValidator/issues/15.

The good news is that you can override or decorate the service while waiting for a solution. You could do the following, for example:

$validator = new EmailValidator();
$multipleValidations = new MultipleValidationWithAnd([
    new RFCValidation(),
    new DNSCheckValidation()
]);
$validator->isValid("example@example .com", $multipleValidations); // FALSE

FWIW here is a patch that changes how Drupal core uses the validator. I haven't tested it in the Drupal context.

This is probably preferable so as to never have a random fail in UserEditTest.

AFAIK via egulias/EmailValidator issues, the email address you cited in the summary may be technically legal in the email RFC but not actually a routable domain. There are probably other cases of oddities like this.

The patch in #11 is still valid. And RED (#3)/GREEN(#11) as expected

+++ b/core/modules/user/tests/src/Functional/UserEditTest.php
@@ -54,7 +54,7 @@ public function testUserEdit() {
-    $edit['mail'] = $this->randomMachineName() . '@new.example.com';
+    $edit['mail'] = $this->randomMachineName() . '@example.org';

Do not understand why this change is necessary. It's out of scope from my understanding. Or am I missed something? Otherwise it's RTBC to me.

Thanks!

Do not understand why this change is necessary. It's out of scope from my understanding. Or am I missed something? Otherwise it's RTBC to me.

Found in #9

This is probably preferable so as to never have a random fail in UserEditTest.

Then tagging "Bug Smash Initiative" and setting to RTBC.

Setting the target version to the current dev branch.

There might be an issue here, in so far as existing records that were previously valid, may now be invalid.

I'm going to ask other committers to see if we need to worry about that.

I think we should do manual testing and decide from there how much of a problem this is, here's some rough steps of things to try:

1. Register an account with an invalid e-mail - foo@example .com
2. Try to register a second account with the 'correct' version of that same e-mail: foo@example.com
3. Apply the patch
4. Log in to the site, go to your user profile, try to save - do you get a validation error?
5. Try to change foo@example .com to foo@example.com and save again.

(also the same steps as above, but missing out step #2)

Will definitely need a change record too.

Back to needs review for now.

So I dug into the source package and found some tests that cover this case and more:

https://github.com/egulias/EmailValidator/blob/cfa3d44/tests/EmailValida...

Upstream considers this case as "pass with warnings". Should we expand this issue to fail validation if upstream returns any warnings, rather than add the hardcoded check for spaces? I suspect we should be rejecting cases such as example@invalid.example(examplecomment).com and example@[127.0.0.1] as invalid email addresses as well, even if they are technically valid per the RFC.

Yeah that seems reasonable

Is there any reason not to use both RFCValidation and DNSCheckValidation as in the example in the README of the <a href="https://github.com/egulias/EmailValidator#user-content-available-validations">egulias/EmailValidator</a> repository? This would fix some other issues aswell.

@DieterHolvoet I think that would have to be opt-in, as we can't guarantee all Drupal sites have outbound network access to resolve DNS records. There are likely also performance considerations as DNS lookups take time.

Between 9.0.x and 9.1.x we upgraded egulias/email-validator and this seems to have resolved this issue.

Reuploading the test-only patch from #3.

If 9.0.x fails but 9.1.x passes I think we can just close this as won't fix.

Upstream this looks to be fixed by https://github.com/egulias/EmailValidator/commit/fda08d3d1b37d20e0a168e0... in 2.1.22.

However even in 9.2.x we only have a minimum version of egulias/email-validator:^2.0 and we have no specific test for this case or min-max dependency testing that would cover this.

So what do we do now? Should we bump the minimum version of egulias/email-validator? Should we add a specific test for this?

Bumping this one, as I've run into a production bug around this issue.

I have a site on 8.9.x using Drupal Commerce. A customer accidentally put a space in the domain part of their email address at checkout. I have a custom third party integration at the end of the checkout process that requires a valid email address. This integration failed due to a validation error, and so the order was stuck in Placed state and never completed.

This site is using drupal/core-recommended so egulias/email-validator is pinned to 2.1.17 and I can't just bump the version to fix the root cause of the bug.

Are we allowed to bump minor versions of dependencies in bug fix releases of core to solve issues like this?

https://www.drupal.org/about/core/policies/core-dependencies-policies/co... says

Patch-level updates may be done in patch releases to resolve bugs or security issues, but we do not provide patch updates in every patch release since there is a risk of disruption due to upstream regressions.

In this case I think the risk of regression is small and the bug is worth fixing, therefore we should bump the minimum version of egulias/email-validator in all supported branches.

2.1.17 to 2.1.22 is a patch level change and can be done in any Drupal release to fix bugs.

See #27 for test-only patch.

Forgot to run composer update --lock

8.9.x patch is much bigger because all the "support" sections are added to the lock file.

We should get this done, not only because of the spacing issue also versions prior to 2.1.16 throw an error "Trying to access array offset on value of type null" on PHP 7.4

See https://github.com/egulias/EmailValidator/commit/5065fafc8c29d229ff207f2...

composer.lock also needs updating.

LGTM

Bumping this to critical since PHP 7.3 is about to be unsupported and our minimum version constraints will not be compatible with the minimum supported version of PHP - which seems really odd.

In an ideal world composer would fix this for us and somehow know which version of egulias/EmailValidator to choose but there's no way to update old releases and say that it's not compatible with a PHP version that was not around when it was released.

I think we still need the manual testing from #3061074-20: egulias/EmailValidator prior to 2.1.22 allows addresses with a space in the domain part for what behaviour we get with existing data, and whether it's allowed duplicate e-mail addresses to be used. The tag was removed, but that seems to have been unrelated to the upgrade path.

Note that 9.1.x shipped with 2.1.22 and 9.2.x/9.3.x/9.4.x already ship with 2.1.25, and these versions will be used by core-recommended. This issue is now just changing the minimum version that is present in composer.json to prevent older versions from being used, and adding a test for the previously broken behaviour.

See also #3238763: Upgrade egulias/email-validator to v3 as I think 2.1.25 is likely the last release of the 2.x series.

OK that's fair enough, if we've already updated it doesn't make sense to hold up changing the minimum version to the one we've already updated to.

Committed/pushed to 9.4.x and cherry-picked to 9.3.x, thanks!