Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
wkania wants to merge1 commit intosymfony:7.1fromwkania-forks:ticket_22592

Conversation

wkania
Copy link
Contributor

@wkaniawkania commentedOct 21, 2020
edited
Loading

QA
Branch?7.x
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#22592
LicenseMIT
Doc PRsymfony/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;    }}
  1. 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;    }}
  1. 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;    }}

stollr, bigfoot90, and peter-si reacted with thumbs up emojiMatTheCat reacted with hooray emojihenry2778, BoShurik, ihmels, spackmat, Heyden7611, norkunas, pluseg, althaus, HypeMC, YaFou, and 10 more reacted with rocket emoji
@wkaniawkania changed the title[DoctrineBridge] Allow validating every class against unique entity constraint[DoctrineBridge][Validator] Allow validating every class against unique entity constraintOct 21, 2020
@wkania
Copy link
ContributorAuthor

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

}
}

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

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

@wkaniawkaniaNov 3, 2020
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

$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?

bernard-ng reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

@wkaniawkaniaNov 5, 2020
edited
Loading

Choose a reason for hiding this comment

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

@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 simplifiedthis->registry->getManagerForClass call.

7008ba6

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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.

weaverryan reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

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 👍

@carsonbotcarsonbot changed the title[DoctrineBridge][Validator] Allow validating every class against unique entity constraint[DoctrineBridge] [Validator] Allow validating every class against unique entity constraintJan 20, 2021
@wkaniawkaniaforce-pushed theticket_22592 branch 3 times, most recently fromd98262a to1053a55CompareJanuary 20, 2021 19:13
@wkania
Copy link
ContributorAuthor

wkania commentedMar 21, 2021
edited
Loading

Hi@derrabus, after reading theconversation, 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
Copy link
Member

derrabus commentedMar 21, 2021
edited
Loading

Hi@derrabus, after reading theconversation, 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 byConstraintValidatorInterface.

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
Copy link
ContributorAuthor

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
Copy link
Member

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.

@carsonbotcarsonbot changed the title[DoctrineBridge] Allow validating every class against unique entity constraint[DoctrineBridge][Validator] Allow validating every class against unique entity constraintDec 27, 2023
@wkania
Copy link
ContributorAuthor

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

Copy link
Member

@kbondkbond left a comment

Choose a reason for hiding this comment

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

Let's get this into 7.1!

flohw reacted with hooray emojiflohw reacted with heart emojiflohw, webda2l, wkania, and zavarock reacted with rocket emoji
@wkania
Copy link
ContributorAuthor

Fixed merge conflict.Commit was the reason

@nicolas-grekas
Copy link
Member

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

@wkania
Copy link
ContributorAuthor

@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
Copy link
Member

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

wkania reacted with thumbs up emoji

@wkania
Copy link
ContributorAuthor

ok, I'll do it later today

@wkania
Copy link
ContributorAuthor

Done

kbond reacted with heart emoji

@flohw
Copy link
Contributor

@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. 😬

webda2l and geoffroyp reacted with thumbs up emojiwkania reacted with laugh emoji

@kbondkbond added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelApr 15, 2024
@fabpot
Copy link
Member

Thank you@wkania.

flohw and wkania reacted with hooray emojiflohw and althaus reacted with heart emojiflohw and norkunas reacted with rocket emoji

fabpot added a commit that referenced this pull requestMay 2, 2024
…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 <!-- see below -->| Bug fix?      | no| New feature?  | yes <!-- pleasedate src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets       |Fix#22592| License       | MIT| Doc PR        |symfony/symfony-docs#14458 <!-- required for new features -->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 validatedNew 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
Copy link
Member

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

wkania reacted with thumbs up emoji

@fabpotfabpot closed thisMay 2, 2024
@fabpotfabpot mentioned this pull requestMay 2, 2024
xabbuh added a commit that referenced this pull requestMay 31, 2024
…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       | MITBefore#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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot left review comments

@derrabusderrabusderrabus requested changes

@xabbuhxabbuhxabbuh left review comments

@alexandre-dauboisalexandre-dauboisalexandre-daubois left review comments

@ihmelsihmelsihmels left review comments

@SmachnijStrelokSmachnijStrelokSmachnijStrelok left review comments

@kbondkbondkbond approved these changes

@jderussejderusseAwaiting requested review from jderusse

@srozesrozeAwaiting requested review from sroze

@ycerutoycerutoAwaiting requested review from yceruto

@weaverryanweaverryanAwaiting requested review from weaverryan

Assignees
No one assigned
Labels
DoctrineBridgeFeature❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze"Status: ReviewedValidator
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

[DoctrineBridge] UniqueEntity validator problem with entityClass
19 participants
@wkania@derrabus@YaFou@MikePolyakovsky@1ed@javiereguiluz@webda2l@Amunak@norkunas@weaverryan@nicolas-grekas@flohw@fabpot@kbond@xabbuh@alexandre-daubois@ihmels@SmachnijStrelok@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp