Skip to content

[FrameworkBundle] Fix allow loose as an email validation mode #60705

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: 6.4
Choose a base branch
from

Conversation

rhel-eo
Copy link

@rhel-eo rhel-eo commented Jun 5, 2025

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

After upgrading to Symfony 7.2.7 we observe this error:

In EnumNode.php line 82:
                                                                               
  The value "loose" is not allowed for path "framework.validation.email_valid  
  ation_mode". Permissible values: "html5-allow-no-tld", "html5", "strict"     
                                                                               

Our configuration is:

framework:
    ...
    validation:
        ...
        email_validation_mode: loose

From bin/console config:dump-reference framework we observe:

framework:
...
    validation:
        ...
        email_validation_mode: html5 # One of "html5-allow-no-tld"; "html5"; "strict"

After this change, the above error no longer occurs and expected allowed values are observed:

$ php bin/console config:dump-reference framework
framework:
...
    validation:
        ...
        email_validation_mode: ~ # One of "html5-allow-no-tld"; "html5"; "strict"; "loose"

See #60373 and #60365 where the previous code was introduced.

@stof
Copy link
Member

stof commented Jun 5, 2025

Can you also add loose in the emailValidationModeProvider of the test added in #60365 to have a non-regression test ?

@rhel-eo rhel-eo changed the title fix allow "loose" as an email validation mode [FrameworkBundle] Fix allow "loose" as an email validation mode Jun 5, 2025
@stof
Copy link
Member

stof commented Jun 5, 2025

Actually, I think the array_merge should be dropped (putting loose in the fallback array when the class does not exist anymore):

  • Email::VALIDATION_MODES contains loose in 6.4
  • in 7.x where loose does not exist in Email::VALIDATION_MODES, using loose is not supported and should indeed be reported as invalid config (instead of failing when instantiating the service)

@@ -272,5 +272,6 @@ public static function emailValidationModeProvider()
foreach (Email::VALIDATION_MODES as $mode) {
yield [$mode];
}
yield ['loose'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this. Based on my follow-up commit, loose is already part of Email::VALIDATION_MODES in the 6.4 branch.

@@ -1067,7 +1067,7 @@ private function addValidationSection(ArrayNodeDefinition $rootNode, callable $e
->validate()->castToArray()->end()
->end()
->scalarNode('translation_domain')->defaultValue('validators')->end()
->enumNode('email_validation_mode')->values((class_exists(Email::class) ? Email::VALIDATION_MODES : ['html5-allow-no-tld', 'html5', 'strict']) + ['loose'])->end()
->enumNode('email_validation_mode')->values(array_merge(class_exists(Email::class) ? Email::VALIDATION_MODES : ['html5-allow-no-tld', 'html5', 'strict'], ['loose']))->end()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
->enumNode('email_validation_mode')->values(array_merge(class_exists(Email::class) ? Email::VALIDATION_MODES : ['html5-allow-no-tld', 'html5', 'strict'], ['loose']))->end()
->enumNode('email_validation_mode')->values(class_exists(Email::class) ? Email::VALIDATION_MODES : ['html5-allow-no-tld', 'html5', 'strict', 'loose'])->end()

@rhel-eo
Copy link
Author

rhel-eo commented Jun 5, 2025

Prior to the change in #60365, the validation of this config option explicitly included loose, so I understood the intention was to avoid a patch release breaking projects that were using this value in their config.

With the suggested change, projects that were using the previously allowed loose value would still receive a configuration validation error after a patch update.

If you prefer to make this explicitly invalid because it is not supported, I understand that decision but wouldn't be able to contribute that on this PR as I don't have the context around any other consequences.

@stof
Copy link
Member

stof commented Jun 5, 2025

ah, I missed that we forgot to change the FrameworkBundle config in 7.0, and that some projects not using the constraint might have an invalid config.

So yes, keep loose in the list all the time. And then, submit another PR to the 7.4 branch to deprecate the loose value

@OskarStark OskarStark changed the title [FrameworkBundle] Fix allow "loose" as an email validation mode [FrameworkBundle] Fix allow loose as an email validation mode Jun 5, 2025
@rhel-eo
Copy link
Author

rhel-eo commented Jun 5, 2025

Thanks, I've opened #60706 for that follow up change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants