Skip to content
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

fix: add UniqueEntity constraint to email field. #1562

Merged

Conversation

m-ar-c
Copy link
Contributor

@m-ar-c m-ar-c commented Aug 11, 2022

Subject

This PR add UniqueEntity constraint to email field in validation.xml.

Why :
The email property is defined as unique in the DB, so we need to
constraint the field to unique in the form in order to prevent a
UniqueConstraintViolationException exception.

I am targeting this branch, because it is BC.

Changelog

### Fixed
- Add UniqueEntity constraint to email field in order to prevent UniqueConstraintViolationException exception.

The email property is defined as unique in the DB, so we need to
constraint the field to unique in the form in order to prevent a
UniqueConstraintViolationException exception.
@Hanmac
Copy link
Contributor

Hanmac commented Aug 12, 2022

i see this a bit critical, for one of my projects i need to have users with different ids but with same email

it is easy to overwrite the doctrine setting for uniq column, but it isn't possible to remove unique entry validation.
that only works if you can somehow set a different valdiation group (which i don't know if it is possible with sonata user bundle)

@VincentLanglet
Copy link
Member

i see this a bit critical, for one of my projects i need to have users with different ids but with same email

it is easy to overwrite the doctrine setting for uniq column, but it isn't possible to remove unique entry validation. that only works if you can somehow set a different valdiation group (which i don't know if it is possible with sonata user bundle)

As long as we define the property as unique in database, we should be consistent and add an annotation IMHO.

How do you override the unique definition ? It could be interesting to see if you cannot override the annotation too.
Because it's kinda the job of the one overriding the unique=true to remove the annotation and not the bundle to not provide an annotation in order to allow people to override. (cc @jordisala1991 if you know how)

@Hanmac
Copy link
Contributor

Hanmac commented Aug 12, 2022

For ORM, @AttributeOverride

for Validator:

To overcome this, the 3rd party bundle needs to have configuration for validation groups. For instance, the FOSUserBundle has this configuration. To create your own validation, add the constraints to a new validation group

the other contraints have groups like "Registration", the new one has not.

Also it should probably be better configured where the Validation Groups are used.

@VincentLanglet
Copy link
Member

Indeed since there is validation group everywhere we should use it too.

Does adding

                <option name="groups">
                    <value>Registration</value>
                    <value>Profile</value>
                </option>

would solve your issue @Hanmac ?

@Hanmac
Copy link
Contributor

Hanmac commented Aug 12, 2022

Indeed since there is validation group everywhere we should use it too.

Does adding

                <option name="groups">
                    <value>Registration</value>
                    <value>Profile</value>
                </option>

would solve your issue @Hanmac ?

i would need to debug it, but i could work with that i think

@VincentLanglet
Copy link
Member

If you have time to try in order to confirm it, it would be great. This would avoid us to block you ^^

@m-ar-c
Copy link
Contributor Author

m-ar-c commented Aug 12, 2022

So, to add validation group, I just need to remove the line (24) <constraint name="Email"/> I added, since there already was a

            <constraint name="Email">
                <option name="groups">
                    <value>Registration</value>
                    <value>Profile</value>
                </option>
            </constraint>

block in the code. Am I right ? (I tested of course, unique validation on email still works with the line removed).

@@ -18,6 +21,7 @@
</constraint>
</property>
<property name="email">
<constraint name="Email"/>
Copy link
Member

Choose a reason for hiding this comment

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

This line could be removed.

@@ -1,6 +1,13 @@
<?xml version="1.0" encoding="UTF-8"?>
<constraint-mapping xmlns="http://symfony.com/schema/dic/constraint-mapping" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/constraint-mapping http://symfony.com/schema/dic/constraint-mapping/constraint-mapping-1.0.xsd">
<class name="Sonata\UserBundle\Model\User">
<constraint name="Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity">
<option name="fields">email</option>
<option name="groups">
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
<option name="groups">
<option name="groups">

I'm not sure but the indent seems weird

@jordisala1991 jordisala1991 merged commit 3c401bb into sonata-project:5.x Aug 12, 2022
@jordisala1991
Copy link
Member

Thanks @m-ar-c

@Hanmac You can override the validation groups for the admin, take a look at the model user admin.

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

Successfully merging this pull request may close these issues.

4 participants