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

This extension represents an incorrect type mismatch for the simple_array type #436

Open
stof opened this issue Mar 17, 2023 · 7 comments
Open

Comments

@stof
Copy link
Contributor

stof commented Mar 17, 2023

The simple_array type converts NULL database values to an empty array: https://github.com/doctrine/dbal/blob/3.6.1/src/Types/SimpleArrayType.php#L53-L57
This means that a nullable column does not mean that the PHP property needs to be nullable.

This would probably require a way for type descriptors to tell whether they have a special handling of null or no.

@ondrejmirtes
Copy link
Member

What's your code and what error phpstan-doctrine reports?

@stof
Copy link
Contributor Author

stof commented Mar 17, 2023

the code looks like this:


    /**
     * @var list<string>
     *
     * @ORM\Column(type="simple_array", nullable=true)
     */
    private $subnetFilters = [];

phpstan-doctrine report this:

Property Incenteev\WebBundle\Entity\Organization::$subnetFilters type mapping mismatch: database can contain array|null but property expects list<string>.

The array vs list<string> part is easy to solve (the type description is not precise enough right now, but SimpleArrayType indeed returns a list<string>).
But the nullability part is handled at https://github.com/phpstan/phpstan-doctrine/blob/62bd362b432fe29e175168689510ddd927b698f8/src/Type/Doctrine/Query/QueryResultTypeWalker.php#L1342C8-L1365 (and maybe other places). The TypeDescriptor interface currently describes only the type for non-null database values and the code elsewhere assumes that a NULL value from the DB is converted to null in PHP all the type. But this assumption is wrong (most types work this way but not all).
I guess we should have TypeDescriptor::getDatabaseNullType(): Type returning the type used to represent the NULL value.

@ondrejmirtes
Copy link
Member

I get it, your suggestion makes sense, but I see Doctrine's behaviour as very surprising here. You can also get rid of this mess if you make the column non-nullable and represent empty arrays not in two ways (null and []), but only a single way ([]).

@stof
Copy link
Contributor Author

stof commented Mar 21, 2023

@ondrejmirtes in the DB, an empty array is always represented as NULL, not as an empty string: https://github.com/doctrine/dbal/blob/3.6.1/src/Types/SimpleArrayType.php#L37-L44

So making the DB field non-nullable actually forbids storing an empty array.

@ondrejmirtes
Copy link
Member

This leads to more constraints that should be reported here: Non-nullable column should only be combined with non-empty-array

@stof
Copy link
Contributor Author

stof commented Mar 21, 2023

If we want to solve that, we would need 2 new methods actually, one describing the PHP type corresponding to a DB NULL value as suggested in my previous comment, and one representing the PHP types that gets written as a NULL value.

And then, SimpleArrayType::getWritableToDatabaseType would be amended to report non-empty-array<string> instead of string[]

@micheh
Copy link

micheh commented May 5, 2023

This is a duplicate of #222

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants