Skip to content

Fix JSON mapping linting against subset of builtin types#11076

Merged
derrabus merged 1 commit into
doctrine:2.17.xfrom
norkunas:fix-json-mapping-validation
Dec 2, 2023
Merged

Fix JSON mapping linting against subset of builtin types#11076
derrabus merged 1 commit into
doctrine:2.17.xfrom
norkunas:fix-json-mapping-validation

Conversation

@norkunas

Copy link
Copy Markdown
Contributor

Closes #11072

@norkunas
norkunas force-pushed the fix-json-mapping-validation branch 5 times, most recently from 33d1977 to 0d2bd95 Compare November 20, 2023 13:36
greg0ire
greg0ire previously approved these changes Nov 20, 2023
SenseException
SenseException previously approved these changes Nov 20, 2023
@greg0ire greg0ire added this to the 2.17.2 milestone Nov 21, 2023
@greg0ire greg0ire added the Bug label Nov 21, 2023
Comment thread lib/Doctrine/ORM/Tools/SchemaValidator.php
@norkunas
norkunas dismissed stale reviews from SenseException and greg0ire via 93d2057 November 27, 2023 09:30
@norkunas
norkunas force-pushed the fix-json-mapping-validation branch from 0d2bd95 to 93d2057 Compare November 27, 2023 09:30
@derrabus

Copy link
Copy Markdown
Member

How does this fix play with union types? For example:

#[Column(type: 'json')]
private array|bool|null $data;

@norkunas

Copy link
Copy Markdown
Contributor Author

How does this fix play with union types? For example:

#[Column(type: 'json')]
private array|bool|null $data;

It is not linted, because it's already ignored here:

// If the property type is not a named type, we cannot check it
if (! ($propertyType instanceof ReflectionNamedType)) {
return null;
}

@derrabus

Copy link
Copy Markdown
Member

Ah, perfect. 😎

Comment thread tests/Doctrine/Tests/ORM/Functional/Ticket/GH11072/GH11072EntityBasic.php Outdated
@norkunas
norkunas force-pushed the fix-json-mapping-validation branch from 93d2057 to 8bdc88e Compare November 27, 2023 09:45
@derrabus

derrabus commented Nov 27, 2023

Copy link
Copy Markdown
Member

It is not linted, because it's already ignored here:

// If the property type is not a named type, we cannot check it
if (! ($propertyType instanceof ReflectionNamedType)) {
return null;
}

Actually, I think we should extend this condition to also include mixed. Performing this validation on mixed properties is pretty pointless, given that mixed is a union of every value type known to PHP. We're about to build a special case for JSON on mixed, but I think this applies to more or less any DBAL type, built-in or custom.

@norkunas
norkunas force-pushed the fix-json-mapping-validation branch from 8bdc88e to bb54409 Compare November 27, 2023 09:54
@norkunas

Copy link
Copy Markdown
Contributor Author

Updated to always ignore mixed type

@norkunas

norkunas commented Dec 1, 2023

Copy link
Copy Markdown
Contributor Author

Can we get this merged? 🙂

@greg0ire
greg0ire requested a review from derrabus December 1, 2023 11:08
@derrabus
derrabus merged commit 44e943e into doctrine:2.17.x Dec 2, 2023
@norkunas
norkunas deleted the fix-json-mapping-validation branch December 2, 2023 10:37
@norkunas

norkunas commented Dec 2, 2023

Copy link
Copy Markdown
Contributor Author

Thank you

@gnutix

gnutix commented Dec 4, 2023

Copy link
Copy Markdown

@derrabus @greg0ire Would you mind releasing a 2.17.2 version with this fix ? 🙏

greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Dec 28, 2023
Spotted while trying to merge doctrine#11076
(among other things) up into 3.0.x. On that branch, it is no longer
possible for an entity to extend another entity without specifying an
inheritance mapping type.

I think the goal of that inheritance was just to reuse the identifier
anyway, so let's just duplicate the identifier declaration instead.
@greg0ire greg0ire mentioned this pull request Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSON field mapping validation

6 participants