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

Add return types for query strings to echo key extension #146

Merged
merged 6 commits into from
Feb 10, 2023

Conversation

IanDelMar
Copy link
Contributor

This PR addresses EchoKeyDynamicFunctionReturnTypeExtension. It

  • adds return types for query string arguments,
  • adds tests for $args['echo']=0, unknown values and function calls with query string,
  • improves the logic of the helper methods.

@johnbillion
Copy link
Contributor

I don't think echo=false works as you would expect. I'm pretty sure it results in string 'false' which evaluates to true. Needs testing.

@IanDelMar
Copy link
Contributor Author

@johnbillion True!

@IanDelMar IanDelMar marked this pull request as draft February 9, 2023 13:51
@IanDelMar
Copy link
Contributor Author

3e1c9d1

  1. fixes the return types for query strings (Add return types for query strings to echo key extension #146 (comment)),
  2. removes instanceof (see Add dynamic return type extension for wp_tag_cloud() #147 (comment)),
  3. reduces complexity by not checking whether a function accepts an integer as value for echo as WP uses loose comparison to determine whether to echo or not (if ($args['echo]) {)

@szepeviktor 3. requires PHPStan 1.9.4. I can either revert using the new methods or bump the minimum supported version in composer.json.

Co-authored-by: Viktor Szépe <[email protected]>
@szepeviktor
Copy link
Owner

bump the minimum supported version in composer.json.

OK!

"phpstan/phpstan": "^1.8.7",

@IanDelMar IanDelMar marked this pull request as ready for review February 10, 2023 16:42
@szepeviktor
Copy link
Owner

@johnbillion Please give the start signal.

@herndlm
Copy link
Contributor

herndlm commented Feb 10, 2023

I only skimmed through it but saw first element access of array and string arrays. Jfyi in most cases the generic approach is to handle all of them and union the results then 😊

@IanDelMar
Copy link
Contributor Author

I only skimmed through it but saw first element access of array and string arrays. Jfyi in most cases the generic approach is to handle all of them and union the results then 😊

I thought about going that route. But don't we know that there is only one array element? Generically looping through non existent elements (all but one) and then generically building a union of all the non existent elements seems pretty verbose to me. I think that in most WP cases there is no need to handle more than one element. However, if there is a clear advantage to using this general approach, I am happy to do so.

@herndlm
Copy link
Contributor

herndlm commented Feb 10, 2023

Don't worry, I haven't thought it through. I'm just used to that approach from phpstan internals. Here it might not add much benefits. And it could still be improved any time anyways

@IanDelMar
Copy link
Contributor Author

IanDelMar commented Feb 10, 2023

Would you please point me to some use cases of the generic approach? Maybe there is some benefit to it I can't imagine if I don't see it in action?! Of course, doesn't have to be right now.

@herndlm
Copy link
Contributor

herndlm commented Feb 10, 2023

you can then support things like https://github.com/phpstan/phpstan-src/blob/3103141be78f902ff7caa207d41cc7e80d528e19/tests/PHPStan/Analyser/data/array-slice.php#L44 but this a very artificial example I guess. not sure about real world use cases, e.g. everything involving if conditions along the way like

$in = $someExpression ? ['foo'] : ['bar'];
$out = array_pop($in);
// $out is 'foo'|'bar' now

but I guess this is also quite artificial :)

@IanDelMar
Copy link
Contributor Author

It is clearer to me now. Thank you for taking the time!

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

Successfully merging this pull request may close these issues.

4 participants