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

Added more precise return types on Zend_Controller_Request_Http #347

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

staabm
Copy link

@staabm staabm commented Apr 26, 2023

when invoked without parameterts like $post = $this->getRequest()->getPost(); with this PR we can assume we get an array.

this simplifies handling on the caller-side as we no longer get mixed.

the precise return type is inspired by phpstan-src

@@ -313,6 +315,8 @@ public function setPost($spec, $value = null)
* @param string $key
* @param mixed $default Default value to use if key not found
* @return mixed Returns null if key does not exist
*
* @phpstan-return ($key is null ? array<string, mixed> : mixed)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even if these annotations are prefixed with phpstan- they also work for psalm

@boenrobot
Copy link

boenrobot commented May 10, 2023

Just an opinion as a user...

If you're going to go all in on more accurate typing, maybe further expand the mixed into what it is... And on a related note, does PHP and ZF do casting to a number, or is it always a string? I'm pretty sure there's no way to get a boolean from a $_GET, $_POST or $_COOKIE, but I'm not so sure for the other scalar types. Oh, and maybe to define the return type in terms of the default value?

So something like this perhaps

@template T
@phpstan-param T $default
@phpstan-return ($key is null ? array<string, string|int|double|array> : string|int|double|array|T)

@develart-projects develart-projects merged commit b5c3d04 into Shardj:master Aug 9, 2023
@staabm staabm deleted the patch-1 branch August 9, 2023 10:24
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.

3 participants