Skip to content

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Aug 26, 2025

@staabm staabm marked this pull request as ready for review August 26, 2025 09:39
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

stubs/core.stub Outdated
Comment on lines 367 to 373
* @param string|array<string> $command
* @param list<list<string>|resource> $descriptor_spec
* @param array<resource> $pipes
* @param array<mixed> $env_vars
* @param array<string, bool> $options
*
* @param-out list<resource> $pipes
Copy link

@mvorisek mvorisek Aug 27, 2025

Choose a reason for hiding this comment

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

Suggested change
* @param string|array<string> $command
* @param list<list<string>|resource> $descriptor_spec
* @param array<resource> $pipes
* @param array<mixed> $env_vars
* @param array<string, bool> $options
*
* @param-out list<resource> $pipes
* @template P of int = int
*
* @param string|list<string> $command
* @param array<P, <list<string>|resource> $descriptor_spec
* @param array<string, mixed> $env_vars
* @param array<string, bool> $options
*
* @param-out array<P, resource> $pipes

reasoning:

@template / $descriptor_spec / $pipes

$descriptor_spec describes the pipes returned in $pipes and in my understanding the keys always match.

$pipes param-in type is not wanted as it can be anything, not only array, but also null/uninitialized variable. Like $matches in preg_match.

$env_vars

Only string keys should be allowed, as at least in bash keys cannot start with number.

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

You're closing an issue, please add a regression test for https://phpstan.org/r/dc1d4f91-b22d-4eaa-b796-34a353062477

@ondrejmirtes
Copy link
Member

(I don't think the issue is solved so that's why I'm asking for a test for the new behaviour)

@staabm
Copy link
Contributor Author

staabm commented Aug 30, 2025

@ondrejmirtes test added. I think it works.

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.

PHPStan does not understand proc_open
4 participants