Skip to content

Fix #9413 - allow Redis::connect() to have 7 parameters - #2442

Merged
ondrejmirtes merged 3 commits into
phpstan:1.10.xfrom
thomas-0816:1.10.x
Jun 8, 2023
Merged

Fix #9413 - allow Redis::connect() to have 7 parameters#2442
ondrejmirtes merged 3 commits into
phpstan:1.10.xfrom
thomas-0816:1.10.x

Conversation

@thomas-0816

@thomas-0816 thomas-0816 commented Jun 8, 2023

Copy link
Copy Markdown
Contributor

Comment thread resources/functionMap.php Outdated
'Redis::close' => ['bool'],
'Redis::config' => ['string', 'operation'=>'string', 'key'=>'string', 'value='=>'string'],
'Redis::connect' => ['bool', 'host'=>'string', 'port='=>'int', 'timeout='=>'float', 'persistent_id='=>'?string', 'retry_interval='=>'?int', 'read_timeout='=>'float'],
'Redis::connect' => ['bool', 'host'=>'string', 'port='=>'int', 'timeout='=>'float', 'persistent_id='=>'?string', 'retry_interval='=>'?int', 'read_timeout='=>'float', 'context'=>'?array{auth?:array,stream?:array}'],

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be optional - 'context='

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And what kind of arrays are those? Could be described precisely

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

seems there are multiple structures possible for auth:
https://github.com/phpredis/phpredis/blob/develop/library.c#L4358
[NULL, 'password']
[false, 'password']
['password']

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for "stream" I guess we can assume array<string, mixed> ?
https://github.com/phpredis/phpredis/blob/develop/library.c#L3257

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah sure :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added a more precise type description for the context elements.

@ondrejmirtes
ondrejmirtes merged commit 5530021 into phpstan:1.10.x Jun 8, 2023
@ondrejmirtes

Copy link
Copy Markdown
Member

Thank you.

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.

2 participants