Skip to content

Entity form type fails when submiting an empty value for non-string identifier type on Postgres #23808

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
maryo opened this issue Aug 6, 2017 · 5 comments
Closed

Entity form type fails when submiting an empty value for non-string identifier type on Postgres #23808

maryo opened this issue Aug 6, 2017 · 5 comments
Labels
Bug DoctrineBridge Status: Needs Review

Comments

@maryo
Copy link
Contributor

maryo commented Aug 6, 2017 •
edited
Loading

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no

This is almost the same issue as #14583 but I am afraid nobody would notice this old closed issue.

I am using entity type with required set to false and the entity's id is set to uuid type using ramsey/uuid-doctrine package with Postgres. It uses a different doctrine type than native guid - https://github.com/ramsey/uuid-doctrine/blob/master/src/UuidType.php named uuid. Doctrine maps it to guid Postgres type OFC but $metadata->getTypeOfField($identifier) returns uuid. So this condition is not satisfied
https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/Form/ChoiceList/ORMQueryBuilderLoader.php#L75

A solution would be to check native types instead.

$databasePlatform = $qb->getEntityManager()->getConnection()->getDatabasePlatform();
$identifierMapping = $metadata->getFieldMapping($identifier);

if (Type::getType($metadata->getTypeOfField($identifier))->getSQLDeclaration($identifierMapping, $databasePlatform) === $databasePlatform->getGuidTypeDeclarationSQL($identifierMapping)) {
    // all types mapped to GUID
}

I can send a PR... but it's actually not very nice as well as filtering out those non-integer values in the condition above.

Why can't we filter out empty strings in all cases? Or at least skip array('') which covers entity type when both multiple and required is set to false (the values is an empty array in case when multiple is set true)? Or can we? Is there a case where empty string is not an empty choice?
@Tobion
Copy link
Contributor

Tobion commented Aug 9, 2017 •
edited
Loading

The problem is that empty strings could be a valid choice in other columns. For example varchar primary keys allow empty strings (in mysql and postgres for sure). So there could a real choice behind an empty value. See http://sqlfiddle.com/#!17/14518/1
But trying to select an empty string for a GUID column will fail as the above example also shows. So it really depends on the column type what is allowed and what not.

Maybe the safer solution is to just also add an explicit case for uuid, i.e. 'guid' === $metadata->getTypeOfField($identifier) || 'uuid' === $metadata->getTypeOfField($identifier)

@Tobion
Copy link
Contributor

Tobion commented Aug 9, 2017

@maryo can you create a PR?

@maryo
Copy link
Contributor Author

maryo commented Aug 9, 2017 •
edited
Loading

I know it can be valid in SQL, I'm just not sure if empty string is always treated as an empty choice or not because the Form component is quite complex (there is FormUtil::isEmpty, empty_data option and so on).

Your proposal covers only this exact issue (custom DBAL type named uuid) but that's good enough for me. Will ask a colleague to make the PR :-D

@Tobion
Copy link
Contributor

Tobion commented Aug 9, 2017

I guess \Doctrine\ORM\Mapping\ClassMetadataInfo::$fieldMappings['columnDefinition'] is not set at this point which would reference the real column type. If that is not available, I don't see another solution.

@maryo
Copy link
Contributor Author

maryo commented Aug 10, 2017 •
edited
Loading

@Tobion I think columnDefinition is even optional and rarely used at all. That's why I posted that snippet in the initial comment of this issue

$databasePlatform->hasNativeGuidType() && (Type::getType($metadata->getTypeOfField($identifier))->getSQLDeclaration($identifierMapping, $databasePlatform) === $databasePlatform->getGuidTypeDeclarationSQL($identifierMapping)))

This one works. But... There are still soo many Postgres types that would fail
https://www.postgresql.org/docs/9.2/static/datatype.html

@mlazovla mlazovla mentioned this issue Aug 10, 2017
Merged
fabpot added a commit that referenced this issue Sep 11, 2017
@fabpot
bug #23853 Filtering empty uuids in ORMQueryBuilderLoader. (mlazovla)
47871c9 
This PR was merged into the 2.7 branch.

Discussion
----------

Filtering empty uuids in ORMQueryBuilderLoader.

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

Commits
-------

27e8cd2 Filtering empty uuids in ORMQueryBuilderLoader.
@fabpot fabpot closed this as completed Sep 11, 2017
@Devristo Devristo mentioned this issue Jan 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DoctrineBridge Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @Tobion @maryo @xabbuh @carsonbot