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

Update Phpredis stubs to return false on failure #8555

Merged
merged 4 commits into from Oct 10, 2022

Conversation

kkmuffme
Copy link
Contributor

@kkmuffme kkmuffme commented Oct 9, 2022

Started in phpredis/phpredis#2120 (comment) - this PR adds false in additional methods

Makes mGet return type more detailed

Force values to be string, to avoid implicit cast creating hard to track bugs phpredis/phpredis#2015 (comment)

address phpredis/phpredis#2120 (comment) - weedwacker method, as I don't have time to check it all one by one
technically all stringable types work phpredis/phpredis#1735 (comment) however they're all cast to string implicitly, which unevitably leads to unexpected results (see riskyCast,...)
@kkmuffme kkmuffme marked this pull request as ready for review October 9, 2022 15:07
@orklah
Copy link
Collaborator

orklah commented Oct 9, 2022

Didn't we had the official stub since last PR?

Is it incomplete still? Maybe it should be updated on their end first?

@kkmuffme
Copy link
Contributor Author

kkmuffme commented Oct 10, 2022

Yes, the official stubs from a PR - that PR is merged to phpredis:develop now.

However the issues addressed here aren't fixed and will not get fixed in a long time, since phpredis' whole documentation needs to be updated too - this is really time consuming, so it won't be happening any time soon.
This PR addresses this issues for stubs though - it's the original stubs with false added to most (all?) methods that can return false.

See phpredis/phpredis#2154, phpredis/phpredis#1921, phpredis/phpredis#1923, ... - these pop-up every couple weeks/months for another function, so I applied "false" liberally: phpredis/phpredis#1921 (comment) phpredis/phpredis#2154 (comment)

@orklah
Copy link
Collaborator

orklah commented Oct 10, 2022

ok fine but it's something we may have to revert if some user complain that some function can't return false and we can't find a case when it does

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Oct 10, 2022
@orklah orklah merged commit e440b34 into vimeo:4.x Oct 10, 2022
@kkmuffme kkmuffme deleted the phpredis-methods-return-false-on-failure branch January 15, 2023 09:16
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