Skip to content

Fix JSON mapping linting against subset of builtin types#11076

Merged
derrabus merged 1 commit intodoctrine: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 intodoctrine:2.17.xfrom
norkunas:fix-json-mapping-validation

Conversation

@norkunas
Copy link
Copy Markdown
Contributor

@norkunas norkunas commented Nov 20, 2023

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
View reviewed changes
SenseException
SenseException previously approved these changes Nov 20, 2023
View reviewed changes
@greg0ire greg0ire added this to the 2.17.2 milestone Nov 21, 2023
@greg0ire greg0ire added the Bug label Nov 21, 2023
janatjak
janatjak reviewed Nov 27, 2023
View reviewed changes
@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

derrabus commented Nov 27, 2023

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

norkunas commented Nov 27, 2023

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:

orm/lib/Doctrine/ORM/Tools/SchemaValidator.php

Lines 373 to 376 in 93d2057

// 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

derrabus commented Nov 27, 2023

Ah, perfect. 😎

@norkunas norkunas force-pushed the fix-json-mapping-validation branch from 93d2057 to 8bdc88e Compare November 27, 2023 09:45
@derrabus
Copy link
Copy Markdown
Member

derrabus commented Nov 27, 2023 •
edited
Loading

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

orm/lib/Doctrine/ORM/Tools/SchemaValidator.php

Lines 373 to 376 in 93d2057

// 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

norkunas commented Nov 27, 2023

Updated to always ignore mixed type

@norkunas
Copy link
Copy Markdown
Contributor Author

norkunas commented Dec 1, 2023

Can we get this merged? 🙂

@norkunas norkunas mentioned this pull request Dec 1, 2023
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
Copy link
Copy Markdown
Contributor Author

norkunas commented Dec 2, 2023

Thank you

@gnutix
Copy link
Copy Markdown

gnutix commented Dec 4, 2023 •
edited
Loading

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

@greg0ire greg0ire mentioned this pull request Dec 6, 2023
Closed
@github-actions github-actions bot mentioned this pull request Dec 20, 2023
Merged
greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Dec 28, 2023
@greg0ire
Remove inheritance
ebb1010 
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
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@greg0ire greg0ire greg0ire approved these changes

@derrabus derrabus derrabus approved these changes

@SenseException SenseException Awaiting requested review from SenseException

+1 more reviewer

@janatjak janatjak janatjak left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

2.17.2

Development

Successfully merging this pull request may close these issues.

JSON field mapping validation

6 participants

@norkunas @derrabus @gnutix @greg0ire @SenseException @janatjak