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

[Doctrine] [Bridge] Add a "repository" option to the uniqueEntity validator #12573

Closed
ogizanagi opened this issue Nov 25, 2014 · 0 comments
Closed

Comments

@ogizanagi
Copy link
Member

Related to #4087

I encountered kind of the same issue today.

Here is the inheritance scheme of my use case:

FOSUser
 └─ User
     ├─ Customer
     └─ Society

The BaseUser, handled by FOSUserBundle, defines an UniqueEntity constraint on both emailCanonical and usernameCanonical fields. But this constraint use the entity repository depending of the real child type of the user I want to register (i.e CustomerRepository when trying to register a new Customer).

Therefore, trying to register a new Customer with the same email or username as a Society will not fail the validation, but fails at database level.

Given the fact that a new repositoryMethod option was added in #4979, an easy workaround is to create a dedicated method in both Customer and Society repositories, calling internally the UserRepository, as for example:

# CustomerRepository | SocietyRepository

    /**
     * @param array $criteria
     * @return array
     */
    public function findByInRootUser(array $criteria)
    {
        return $this->_em->getRepository('ACMEUserBundle:User')
            ->findBy($criteria);
    }

and use it in UniqueEntity constraint definition in both entities :

#ACME\UserBundle\Entity\Customer
/**
 * @ORM\Entity(repositoryClass="ACME\UserBundle\Entity\Repositories\CustomerRepository")
 *
 * @UniqueEntity(fields="usernameCanonical", repositoryMethod="findByInRootUser",
 * message="fos_user.username.already_used", errorPath="username", groups={"Registration", "Profile"})
 * @UniqueEntity(fields="emailCanonical", repositoryMethod="findByInRootUser",
 * message="fos_user.email.already_used", errorPath="email", groups={"Registration", "Profile"})
 *
 */
class Customer extends User
{
}

#ACME\UserBundle\Entity\Society
/**
 * @ORM\Entity(repositoryClass="ACME\UserBundle\Entity\Repositories\SocietyRepository")
 *
 * @UniqueEntity(fields="usernameCanonical", repositoryMethod="findByInRootUser",
 * message="fos_user.username.already_used", errorPath="username", groups={"Registration", "Profile"})
 * @UniqueEntity(fields="emailCanonical", repositoryMethod="findByInRootUser",
 * message="fos_user.email.already_used", errorPath="email", groups={"Registration", "Profile"})
 *
 */
class Society extends User
{
}

Although it works, this induces useless code duplication (beside the fact that the FOSUser defined constraints are useless, but triggered, because we cannot overload the validation/orm.xml. But that is another story :) )
I certainly could have define the UniqueEntity constraint into my own User class. But then, when adding a new user type, I must not forget to create the findByInRootUser method into the new repository (or create a repository class to extend).

Here comes my suggestion:
Add a repository option to the UniqueEntity validator.

This will ensure the proper repository will be used by the constraint, and I could define it only at the User entity level:

#ACME\UserBundle\Entity\User
/**
 * @ORM\Entity(repositoryClass="ACME\UserBundle\Entity\Repositories\UserRepository")
 *
 * @UniqueEntity(fields="usernameCanonical", repository="ACME\UserBundle\Entity\Repositories\UserRepository",
 * message="fos_user.username.already_used", errorPath="username", groups={"Registration", "Profile"})
 * @UniqueEntity(fields="emailCanonical", repository="ACME\UserBundle\Entity\Repositories\UserRepository",
 * message="fos_user.email.already_used", errorPath="email", groups={"Registration", "Profile"})
 *
 */
class User extends FOSUser
{
}

If a value for the repository option isn't provided, the behavior remains the same: The UniqueConstraintValidator will get the repository from the real class at runtime.
Do you see any issue with that approach ?

I'm not sure of the component ability to guess the class on which the constraint was defined, but if it is possible, it could not be considered, because of the BC promise. That's why a suggested another approach.
Maybe for the 3.0 ? But it might lead to more unwanted behaviors than benefits.

lemoinem added a commit to lemoinem/symfony that referenced this issue Dec 11, 2015
lemoinem added a commit to lemoinem/symfony that referenced this issue Dec 12, 2015
lemoinem added a commit to lemoinem/symfony that referenced this issue Dec 16, 2015
lemoinem added a commit to lemoinem/symfony that referenced this issue Feb 2, 2016
lemoinem added a commit to lemoinem/symfony that referenced this issue Feb 2, 2016
lemoinem added a commit to lemoinem/symfony that referenced this issue Feb 2, 2016
lemoinem added a commit to lemoinem/symfony that referenced this issue Feb 2, 2016
lemoinem added a commit to lemoinem/symfony that referenced this issue Feb 3, 2016
fabpot added a commit that referenced this issue Oct 14, 2016
…ed by the UniqueEntity validator (ogizanagi)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[DoctrineBridge] Add a way to select the repository used by the UniqueEntity validator

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12573, #4087, #12977
| License       | MIT
| Doc PR        | symfony/symfony-docs#7057

This is a cherry pick of #12977 on ~~2.8~~ 3.2 branch, as it is clearly a new feature, even if it was primary introduced in order to fix an inappropriate behavior that might be considered as a bug.

Commits
-------

00d5459 [Doctrine] [Bridge] Add a way to select the repository used by the UniqueEntity validator
@fabpot fabpot closed this as completed Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants