Skip to content

UniqueEntityValidator error ? #16791

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.

Sign up for GitHub

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

Jump to bottom
Closed
poolerMF opened this issue Dec 1, 2015 · 7 comments
Closed

UniqueEntityValidator error ? #16791

poolerMF opened this issue Dec 1, 2015 · 7 comments
Labels
Bug Status: Needs Review Validator

Comments

@poolerMF
Copy link

poolerMF commented Dec 1, 2015

hello ... I'm using SYMFONY 2.7.3, my situation:

validation.yml

XYZ/SubjectData:
    constraints:
        - Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity:
            fields: [client, name, regid, taxid, vatid, registerNumber]
            message: client.not_unique
            errorPath: regid
            groups: [create, update]

creating form:

$form = $this->createForm('subject_data_type', $object, ['addClient' => false, 'addValidFrom' => true, 'submitButton' => true, 'validation_groups' => ['create']]);
  1. Iadd bullshit to validation fields (not mapped field) .. for example: "awegresg"
  2. I set form to existed data
  3. validation passed and SQL error occured (not unique entity)

I also see, that in UniqueEntityValidator, lines 89->91:

if ($constraint->ignoreNull && null === $criteria[$fieldName]) {
    return;
}

why return ? why not continue ??
@dmaicher
Copy link
Contributor

Why would you not return in that case? I mean it iterates over all fields that are specified to be unique ;)

So your unmapped "awegresg" field will not be one of them.

Where is your validation.yml file located? is it loaded?

@poolerMF
Copy link
Author

In my described case ignoreNull is true and fields contains unmapped "awegresg". Description of ignoreNull in documentation:

ignoreNull
type: boolean default: true
If this option is set to true, then the constraint will allow multiple entities to have a null value for a field without failing validation. If set to false, only one null value is allowed - if a second entity also has a null value, validation would fail.

I think this description do not fit the code I see in UniqueEntityValidator.
When return is called in foreach, then cycle ends, main function returns and VALIDATION PASSED (I think it's bad)

Maybe I do not understand to this parameter "ignoreNull" from description, but I think it all should work in these way:

  • when I add whatever unmapped field, error should by thrown no matter of ignoreNull (in case I described above, It did not happen)
  • when ignoreNull is true, repository call method only with fields, that are not null
  • when ignoreNull is false, repository call method with all fields, containing null values

So .. when I just change return to continue, all this works and I'm getting Exception "The field "awegresg" is not mapped by Doctrine, so it cannot be validated for uniqueness"

There is also no problem with file validation.yml (file is correctly loaded)

@xabbuh xabbuh added the Bug label Jan 12, 2016
@xabbuh
Copy link
Member

xabbuh commented Jan 12, 2016

I agree with @poolerMF. The exception should be thrown.

@dmaicher
Copy link
Contributor

@poolerMF sorry now I understand what you mean.

I agree and it should indeed always throw the exception in case an unmapped field is in the list.

Lets say we have a class A with two properties a and b.

This record is already in the database: a=1, b=NULL.

If we now validate the record a=1, b=NULL with ignoreNull=true and a constraint across columns a and b it would be valid, right?
Maybe it should behave more like a unique constraint in MySQL and like @poolerMF suggested in this case only ignore the column b when checking for an existing row?

@xabbuh xabbuh mentioned this issue Jan 27, 2017
Merged
@xabbuh
Copy link
Member

xabbuh commented Jan 27, 2017

@poolerMF Can you take a look at #21431?

@poolerMF
Copy link
Author

@xabbuh I looked at it ... now it should be OK

@xabbuh
Copy link
Member

xabbuh commented Jan 27, 2017

@poolerMF I just reopen here as that PR isn't merged yet (we will close it when the PR is merged). :)

@xabbuh xabbuh reopened this Jan 27, 2017
fabpot added a commit that referenced this issue Jan 27, 2017
@fabpot
bug #21431 [DoctrineBridge] always check for all fields to be mapped …
bc391c1 
…(xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[DoctrineBridge] always check for all fields to be mapped

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #16791
| License       | MIT
| Doc PR        |

Commits
-------

1e3421d always check for all fields to be mapped
@fabpot fabpot closed this as completed Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review Validator
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @dmaicher @xabbuh @poolerMF @carsonbot