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

Fix RedisCluster::getOption() and RedisCluster::setOption() argument types #7030

Merged
merged 1 commit into from Dec 2, 2021

Conversation

ostrolucky
Copy link
Contributor

@ostrolucky ostrolucky commented Nov 30, 2021

@orklah
Copy link
Collaborator

orklah commented Nov 30, 2021

I may be wrong here but the link you posted points to Redis, not RedisCluster

If the link is good though, the param name should change too (from name to option)

@ostrolucky
Copy link
Contributor Author

Thx. Fixed the link and renamed name too.

@orklah
Copy link
Collaborator

orklah commented Nov 30, 2021

link is still wrong :) here's the right one:
https://github.com/phpredis/phpredis/blob/aac42cd33510030bb62973f8886f4cd98b9b8003/redis_cluster.stub.php#L122

In the PR for setOption, the contributor decided to go for int|string because he didn't know if it has been a change from older versions or if it was a mistake.

Do you know the answer? If it change between versions of Redis, we should keep retrocompatibility and allow string too, but if it's a mistake, it would be cool to fix getOption in that way too to keep consistency

@ostrolucky
Copy link
Contributor Author

ostrolucky commented Nov 30, 2021

I am not 100% sure, but I think it was a mistake. We never used anything else than int in SncRedisBundle. I think contributor used string because phpstorm stubs are most likely wrong - those specify string. I've changed it now for getOption too...

@ostrolucky ostrolucky changed the title Fix RedisCluster::getOption() argument types Fix RedisCluster::getOption() and RedisCluster::setOption() argument types Nov 30, 2021
@ostrolucky
Copy link
Contributor Author

ostrolucky commented Nov 30, 2021

Interesting thing that should be also mentioned, is that phpredis is doing something with this type currently in their dev branch phpredis/phpredis#2038. I would assume they broke their types they provide in reflection. But again, it's their dev branch only and this must have been done recently only. To go forward with this PR, I guess we should get an answer there first.

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Nov 30, 2021
@orklah
Copy link
Collaborator

orklah commented Nov 30, 2021

(note: the CI failure is unrelated, you can rebase to fix it)

@ostrolucky
Copy link
Contributor Author

So, referenced issue was solved. So indeed it's int. I would say PR is ready.

ostrolucky added a commit to ostrolucky/phpstorm-stubs that referenced this pull request Dec 2, 2021
@orklah
Copy link
Collaborator

orklah commented Dec 2, 2021

Thanks!

@orklah orklah merged commit 90d4216 into vimeo:master Dec 2, 2021
ostrolucky added a commit to ostrolucky/phpstorm-stubs that referenced this pull request Dec 2, 2021
ostrolucky added a commit to ostrolucky/phpstorm-stubs that referenced this pull request Dec 2, 2021
DmitryTronin pushed a commit to JetBrains/phpstorm-stubs that referenced this pull request Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix The PR will be included in 'Fixes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants