[DoctrineBridge][Validator] Allow validating every class against unique entity constraint #38662

wkania · 2020-10-21T19:03:49Z

Q	A
Branch?	7.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #22592
License	MIT
Doc PR	symfony/symfony-docs#14458

Yet another try to allow validating every class against unique entity constraint.
Based on the knowledge from issue #22592 and pull request #24974.

This constraint doesn’t provide any protection against race conditions, which is enough in most cases. You can always try-catch flush method. Let's not talk about this problem.

Current state:

can be applied only to an entity,
support entity inheritance,
can validate unique combinations of multiple fields,
meaningful errors related to entities,
is valid during an update, when the only matched entity is the same as the entity being validated

New state:

preserve the current state,
all old tests pass (no changes in them),
no breaking changes,
can be applied to any class (like DTO),
can map object fields to entity fields (optional),
when the object update some entity, there is an option to provide the identifier field names to match that entity (optional)

Examples:

DTO adds a new entity.

namespace App\Message;

use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;

/**
 * @UniqueEntity(fields={"name"}, entityClass="App\Entity\User")
 */
class HireAnEmployee
{
    public $name;

    public function __construct($name)
    {
        $this->name = $name;
    }
}

DTO adds a new entity, but the name of the field in the entity is different.

namespace App\Message;

use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;

/**
 * @UniqueEntity(fields={"name": "username"}, entityClass="App\Entity\User")
 */
class HireAnEmployee
{
    public $name;

    public function __construct($name)
    {
        $this->name = $name;
    }
}

DTO updates an entity.

namespace App\Message;

use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity;

/**
 * @UniqueEntity(fields={"name"}, entityClass="App\Entity\User", identifierFieldNames={"uid": "id"})
 */
class UpdateEmployeeProfile
{
    public $uid;
    public $name;

    public function __construct($uid, $name)
    {
        $this->uid = $uid;
        $this->name = $name;
    }
}

CHANGELOG-5.2.md

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

wkania · 2020-11-01T20:40:40Z

The only not successful check is about the PHP attributes code syntax. I can't fix it.
@derrabus Hi, should I do anything else?

xabbuh · 2020-11-03T07:57:52Z

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

            }
        }

-        $class = $em->getClassMetadata(\get_class($entity));
+        try {
+            $repository = $em->getRepository($entityClass);


Shouldn't we use $constraint->entityClass here if possible?

I need to check if the object being validated is an entity.
The $constraint->entityClass always have an entity class.
Maybe I could change $entityClass to $objectClass? After all I changed @param object $entity to @param object $object.

$entityClass = $constraint->entityClass ?? \get_class($object);

What I mean is couldn't we simply update line 67 to something like this and then use $entityClass in the rest of the method?

Ok, I could do it like that, but I also need to know if the object being validated is an entity.
This knowledge is useful in line 104. The error is related to the entity inheritance case and the helpful message, like
The "Symfony\Bridge\Doctrine\Tests\Fixtures\SingleStringIdEntity" entity repository does not support the "Symfony\Bridge\Doctrine\Tests\Fixtures\Person" entity. The entity should be an instance of or extend "Symfony\Bridge\Doctrine\Tests\Fixtures\SingleStringIdEntity"..

So I would change line 88 to $repository = $em->getRepository(\get_class($object));. I hope it won't affect any older tests. Which I didn't want to change.

@xabbuh Sadly, in lines 94-107 I need both values and I can't move it before line 69. The code use $em which is set after line 69.
I changed $constraint->entityClass to $entityClass after line 107.
I also simplified this->registry->getManagerForClass call.

7008ba6

In this block, for simplicity, we should ONLY try to determine $isEntity. By also trying to set $repository, I think it confuses things. I would:

try { $em->getRepository($value::class); $isEntity = true; } catch (ORMMappingException|PersistenceMappingException) { $isEntity = false; }

Or would this be enough?

$isEntity = $em->contains($value::class)?

Also, calling thex variable $isObjectEntity or $isValueEntity might be even more clear :)

Then, get the $repository always below this.

I don't remember why I didn't use contains 3 years ago. During the weekend I will check it on real examples, not interfaces.

Cool - thanks :). And more important for me is to ONLY set the $isEntity variable in this block. Then move figuring out the $repository until later... ideally moving all of the logic for getting the repository back the bottom - https://github.com/symfony/symfony/pull/38662/files#diff-fd1785451fbb1d6abe41ae644ddbe7b39b6f03f3ba31f7ce07a18ef376e61293L130 - as that would greatly reduce the diff. I'm a little obsessed with reducing the diff on this PR just because with the current set of changes, it's really hard to see & understand what's changed. It may turn out that the diff DOES need to be large like this, but I'm hoping we can shrink it down by avoiding unnecessary code movement. That'd make the PR much easier to merge 👍

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php

wkania · 2021-03-21T12:22:10Z

Hi @derrabus, after reading the conversation, I'm not sure should I fix those Psalm errors.
UniqueEntity is not the only constraint affected by this.

If it should be fixed, should it be part of this PR or should be fixed in all affected constraints? Also, should it be applied to branch 5.2, if we want branch 4.4 to remain stable?

P.S.
Waiting for the fabbot to use PHP-CS-Fixer 2.18.4, so it wouldn't report coding standards problems.

derrabus · 2021-03-21T12:45:49Z

Hi @derrabus, after reading the conversation, I'm not sure should I fix those Psalm errors.

Psalm's complaint is valid: we should not change the name of a parameter declared by an interface when implementing that interface. Yet, we do this already and this of course is not your fault. However I don't think we should now pick another name that differs from the upstream declared one.

Option A) We do not rename the parameter and live with the fact that $entity is not a 100% accurate variable name in your context.
Option B) We rename the parameter to $value as declared by ConstraintValidatorInterface.

Waiting for the fabbot to use PHP-CS-Fixer 2.18.4, so it wouldn't report coding standards problems.

Fabbot currently reports two problems. One of them is valid. 😉

wkania · 2021-03-21T13:08:39Z

We do not rename the parameter and live with the fact that $entity is not a 100% accurate variable name in your context.

This PR changed it from the $entity to the $object. I can go with option B, but should it be part of another PR that would affect also branch 4.4 and not only 5.x?

Fabbot currently reports two problems. One of them is valid. wink

Ye, I only added the blank line to trigger Travis and see if Fabbot already uses the new version of PHP-CS-Fixer. I don't see any button (in Github UI) to run the tests again or info what version of PHP-CS-Fixer, Fabbot use.

derrabus · 2021-03-21T15:30:10Z

I can go with option B, but should it be part of another PR that would affect also branch 4.4 and not only 5.x?

No, I wouldn't backport the parameter name change.

I only added the blank line to trigger Travis and see if Fabbot already uses the new version of PHP-CS-Fixer.

Just remove that extra line again and don't mind the \Attribute issue. We are aware of the false positive and can ignore Fabbot when merging.

wkania · 2023-12-18T22:13:00Z

@weaverryan I have rebased the code with branch 7.1, answered all questions and the diff is now smaller.
Please review.

wkania · 2023-12-27T17:20:11Z

In 2021 carsonbot removed [Validator] from the title. Now we are back to the original title :).

kbond

Let's get this into 7.1!

wkania · 2024-02-11T13:05:17Z

Fixed merge conflict. Commit was the reason

nicolas-grekas · 2024-02-11T13:09:07Z

Can you please rebase (and squash while doing so)? We don't merge PRs with merge commits in them.

wkania · 2024-02-12T08:27:06Z

@nicolas-grekas So that there are no misunderstandings.
So I shouldn't have used github UI to resolve merge conflict?
I can squash all 38 commits into one?

nicolas-grekas · 2024-02-12T08:42:09Z

I guess the UI doesn't help, I never use it myself.
Yes, please squash all commits into one.

wkania · 2024-02-12T08:46:05Z

ok, I'll do it later today

…ue entity constraint

wkania · 2024-02-12T20:38:57Z

Done

flohw · 2024-04-15T07:06:20Z

@nicolas-grekas @kbond
Any update for this PR? End of may can be shortly in sight! 🙂
I doubt that @wkania will keep the pr up to date indefinitely. 😬

src/Symfony/Bridge/Doctrine/CHANGELOG.md

fabpot · 2024-05-02T09:00:38Z

Thank you @wkania.

…ss against unique entity constraint (wkania) This PR was merged into the 7.1 branch. Discussion ---------- [DoctrineBridge][Validator] Allow validating every class against unique entity constraint | Q | A | ------------- | --- | Branch? | 7.x  | Bug fix? | no | New feature? | yes  | Deprecations? | no  | Tickets | Fix #22592 | License | MIT | Doc PR | symfony/symfony-docs#14458  Yet another try to allow validating every class against unique entity constraint. Based on the knowledge from issue #22592 and pull request #24974. This constraint doesn’t provide any protection against race conditions, which is enough in most cases. You can always try-catch flush method. Let's not talk about this problem. Current state: - can be applied only to an entity, - support entity inheritance, - can validate unique combinations of multiple fields, - meaningful errors related to entities, - is valid during an update, when the only matched entity is the same as the entity being validated New state: - preserve the current state, - all old tests pass (no changes in them), - no breaking changes, - can be applied to any class (like DTO), - can map object fields to entity fields (optional), - when the object update some entity, there is an option to provide the identifier field names to match that entity (optional) Examples: 1. DTO adds a new entity. ``` namespace App\Message; use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity; /** * `@UniqueEntity`(fields={"name"}, entityClass="App\Entity\User") */ class HireAnEmployee { public $name; public function __construct($name) { $this->name = $name; } } ``` 2. DTO adds a new entity, but the name of the field in the entity is different. ``` namespace App\Message; use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity; /** * `@UniqueEntity`(fields={"name": "username"}, entityClass="App\Entity\User") */ class HireAnEmployee { public $name; public function __construct($name) { $this->name = $name; } } ``` 3. DTO updates an entity. ``` namespace App\Message; use Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity; /** * `@UniqueEntity`(fields={"name"}, entityClass="App\Entity\User", identifierFieldNames={"uid": "id"}) */ class UpdateEmployeeProfile { public $uid; public $name; public function __construct($uid, $name) { $this->uid = $uid; $this->name = $name; } } ``` Commits ------- adb9afa [DoctrineBridge][Validator] Allow validating every class against unique entity constraint

fabpot · 2024-05-02T09:05:18Z

Closing (it has been merged but due to a race condition, it has not been marked as merged)

…ject (HypeMC) This PR was merged into the 7.1 branch. Discussion ---------- [DoctrineBridge] Fix `UniqueEntityValidator` with proxy object | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #57075 | License | MIT Before #38662, `$fieldValue = $class->reflFields[$fieldName]->getValue($entity);` was used to get the value of a property, so it makes sense to keep using it when the object is an entity. Commits ------- 99f279b [DoctrineBridge] Fix `UniqueEntityValidator` with proxy object

carsonbot added Status: Needs Review DoctrineBridge Feature labels Oct 21, 2020

wkania changed the title ~~[DoctrineBridge] Allow validating every class against unique entity constraint~~ [DoctrineBridge][Validator] Allow validating every class against unique entity constraint Oct 21, 2020

derrabus requested changes Oct 21, 2020

View reviewed changes

CHANGELOG-5.2.md Outdated Show resolved Hide resolved

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Oct 21, 2020

derrabus added this to the 5.x milestone Oct 21, 2020

wkania force-pushed the ticket_22592 branch from 9e01afc to 014285d Compare October 22, 2020 17:39

xabbuh reviewed Nov 3, 2020

View reviewed changes

xabbuh reviewed Nov 6, 2020

View reviewed changes

src/Symfony/Bridge/Doctrine/Validator/Constraints/UniqueEntityValidator.php Outdated Show resolved Hide resolved

wkania force-pushed the ticket_22592 branch from d98262a to d7332be Compare January 20, 2021 18:43

wkania requested review from jderusse, sroze and yceruto as code owners January 20, 2021 18:43

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 20, 2021

carsonbot changed the title ~~[DoctrineBridge][Validator] Allow validating every class against unique entity constraint~~ [DoctrineBridge] [Validator] Allow validating every class against unique entity constraint Jan 20, 2021

wkania force-pushed the ticket_22592 branch 3 times, most recently from d98262a to 1053a55 Compare January 20, 2021 19:13

xabbuh mentioned this pull request Jan 27, 2021

[DoctrineBridge] Use $entityClass intead of the the entity class in UniqueEntity #39983

Closed

wkania force-pushed the ticket_22592 branch from 82763fc to 2015ad9 Compare February 10, 2021 21:32

wkania mentioned this pull request Feb 10, 2021

fabbot.io tries to apply a patch for #[\Attribute #40149

Closed

wkania force-pushed the ticket_22592 branch from aeafb79 to 89df38d Compare March 21, 2021 11:27

xabbuh added the Validator label Dec 27, 2023

carsonbot changed the title ~~[DoctrineBridge] Allow validating every class against unique entity constraint~~ [DoctrineBridge][Validator] Allow validating every class against unique entity constraint Dec 27, 2023

wkania requested a review from weaverryan January 1, 2024 19:25

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 1, 2024

kbond approved these changes Feb 7, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 7, 2024

[DoctrineBridge][Validator] Allow validating every class against uniq…

5072a75

…ue entity constraint

wkania force-pushed the ticket_22592 branch from da2336b to 5072a75 Compare February 12, 2024 20:28

kbond added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Apr 15, 2024

alexandre-daubois reviewed Apr 25, 2024

View reviewed changes

src/Symfony/Bridge/Doctrine/CHANGELOG.md Show resolved Hide resolved

fabpot closed this May 2, 2024

fabpot mentioned this pull request May 2, 2024

Release v7.1.0-BETA1 #54820

Merged

dmaicher mentioned this pull request May 23, 2024

[DoctrineBridge] issues with UniqueEntityValidator on 7.1-RC1 #57075

Closed

HypeMC mentioned this pull request May 27, 2024

[DoctrineBridge] Fix UniqueEntityValidator with proxy object #57188

Merged

Seb33300 mentioned this pull request Jun 1, 2024

[DoctrineBridge] UniqueEntity does no longer access nested property #57279

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoctrineBridge][Validator] Allow validating every class against unique entity constraint #38662

[DoctrineBridge][Validator] Allow validating every class against unique entity constraint #38662

wkania commented Oct 21, 2020 •

edited

Loading

wkania commented Nov 1, 2020

xabbuh Nov 3, 2020

wkania Nov 3, 2020 •

edited

Loading

xabbuh Nov 4, 2020

wkania Nov 4, 2020

wkania Nov 5, 2020 •

edited

Loading

weaverryan Oct 17, 2023

wkania Oct 19, 2023

weaverryan Oct 21, 2023

wkania commented Mar 21, 2021 •

edited

Loading

derrabus commented Mar 21, 2021 •

edited

Loading

wkania commented Mar 21, 2021

derrabus commented Mar 21, 2021

wkania commented Dec 18, 2023

wkania commented Dec 27, 2023

kbond left a comment

wkania commented Feb 11, 2024

nicolas-grekas commented Feb 11, 2024

wkania commented Feb 12, 2024

nicolas-grekas commented Feb 12, 2024

wkania commented Feb 12, 2024

wkania commented Feb 12, 2024

flohw commented Apr 15, 2024

fabpot commented May 2, 2024

fabpot commented May 2, 2024

[DoctrineBridge][Validator] Allow validating every class against unique entity constraint #38662

[DoctrineBridge][Validator] Allow validating every class against unique entity constraint #38662

Conversation

wkania commented Oct 21, 2020 • edited Loading

wkania commented Nov 1, 2020

xabbuh Nov 3, 2020

Choose a reason for hiding this comment

wkania Nov 3, 2020 • edited Loading

Choose a reason for hiding this comment

xabbuh Nov 4, 2020

Choose a reason for hiding this comment

wkania Nov 4, 2020

Choose a reason for hiding this comment

wkania Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

weaverryan Oct 17, 2023

Choose a reason for hiding this comment

wkania Oct 19, 2023

Choose a reason for hiding this comment

weaverryan Oct 21, 2023

Choose a reason for hiding this comment

wkania commented Mar 21, 2021 • edited Loading

derrabus commented Mar 21, 2021 • edited Loading

wkania commented Mar 21, 2021

derrabus commented Mar 21, 2021

wkania commented Dec 18, 2023

wkania commented Dec 27, 2023

kbond left a comment

Choose a reason for hiding this comment

wkania commented Feb 11, 2024

nicolas-grekas commented Feb 11, 2024

wkania commented Feb 12, 2024

nicolas-grekas commented Feb 12, 2024

wkania commented Feb 12, 2024

wkania commented Feb 12, 2024

flohw commented Apr 15, 2024

fabpot commented May 2, 2024

fabpot commented May 2, 2024

wkania commented Oct 21, 2020 •

edited

Loading

wkania Nov 3, 2020 •

edited

Loading

wkania Nov 5, 2020 •

edited

Loading

wkania commented Mar 21, 2021 •

edited

Loading

derrabus commented Mar 21, 2021 •

edited

Loading