Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix JSON mapping linting against subset of builtin types #11076

Merged
merged 1 commit into from
Dec 2, 2023

Conversation

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

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

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

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

Ah, perfect. 😎

@derrabus
Copy link
Member

derrabus commented Nov 27, 2023

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
Copy link
Contributor Author

Updated to always ignore mixed type

@norkunas
Copy link
Contributor Author

norkunas commented Dec 1, 2023

Can we get this merged? 🙂

@derrabus derrabus merged commit 44e943e into doctrine:2.17.x Dec 2, 2023
58 checks passed
@norkunas norkunas deleted the fix-json-mapping-validation branch December 2, 2023 10:37
@norkunas
Copy link
Contributor Author

norkunas commented Dec 2, 2023

Thank you

@gnutix
Copy link

gnutix commented Dec 4, 2023

@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