szepeviktor / phpstan-wordpress

WordPress extensions for PHPStan ⛏️
https://packagist.org/packages/szepeviktor/phpstan-wordpress
MIT License
262 stars 26 forks source link

Remove deprecated `instanceof` #184

Closed IanDelMar closed 1 year ago

IanDelMar commented 1 year ago

What I was wondering when updating this extension. I can have a non constant array that has a key 'fields'.

// Non constant array with fields first
$array = [
    'fields' => 'ids',
    $_GET['foo'] => $_GET['bar'],
];
assertType('array<int, int|WP_Post>', get_posts($array));

// Non constant array with fields last
$array = [
    $_GET['foo'] => $_GET['bar'],
    'fields' => 'ids',
];
assertType('array<int, int>', get_posts($array));

get_post($array) with the fields first case returns array<int, int|WP_Post> because $_GET['foo'] may be 'fields' and overwrite 'ids'. get_post($array) with the fields last case returns array<int, int> as 'fields' is not affected by $_GET['foo'] and hence will be 'ids'. With limited PHPStan knowledge, I was not able to find a way to check the order of $_GET['foo'] and 'fields' or more specifically to check whether there is a non constant key after the fields key.

Is there a simple way to check the order of the keys with PHPStan?

Part of #149

szepeviktor commented 1 year ago

We could open a ticket at @phpstan and talk to @ondrejmirtes about a function changing its return type based on its parameter 'fields' => 'ids' which is 😢 an array. This is so anti-PHPStan, so PHP 4, I am so ashamed.

Let's support only usage where there is only 1 fields (string constant) element (anywhere) in the array.

IanDelMar commented 1 year ago

I'd like to add more tests. Please wait with merging.

IanDelMar commented 1 year ago

We could open a ticket at @phpstan and talk to @ondrejmirtes about a function changing its return type based on its parameter 'fields' => 'ids' which is 😢 an array.

I also thought we need something like that.

IanDelMar commented 1 year ago

I can't think of any other test cases to add. Do you?

szepeviktor commented 1 year ago

We already have much more tests than possible problems.

Thank you.

herndlm commented 1 year ago

How exactly is the type looking in your more dynamic example? It's still a constant array though, right? (= array shape and not just e.g. array<.., ..>) Is the key just string then? Maybe it helps if you do checks for the key of the whole array? string|'fields' would be generalised to string and then you can handle this case differently maybe? Something like if the key is a supertype of string -> handle different. Or I misunderstood the problem :)

szepeviktor commented 1 year ago

We are digging too deep. Let's fix future problems in the next PR!