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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cache] Fix return type of Redis6Proxy::mget() #52700

Closed

Conversation

alexandre-daubois
Copy link
Contributor

@alexandre-daubois alexandre-daubois commented Nov 23, 2023

Q A
Branch? 6.3
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52668
License MIT

After @dkarlovi's comment, here is another try that will not break returns types. Thank you for taking the time to explain where the exact problem is, Dalibor 馃憤

Todo: having a look at failing tests

@nicolas-grekas
Copy link
Member

Did yo confirm the extension can return false?
Its signature is not allowing false so it shouldn't be possible.
To me this should be fixed upstream. Not here.
If upstream is not production ready, don't use it.

@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Nov 23, 2023

Yes, I can confirm the extension can return false:

image

Tested on PHP 8.2, Redis Extension 6.0.2

Shouldn't we provide a fix of this kind to comply to what the extension actually does? Or are you 馃憥 on this one because of the potential maintenance cost maybe?

(this case makes me think of this comment)

@nicolas-grekas
Copy link
Member

I'd like to see the pressure on this issue go from this repo to the source one yes. Sending fixes, etc ;)

@dkarlovi
Copy link
Contributor

Its signature is not allowing false so it shouldn't be possible.

PHP extensions don't need to follow their own signatures, confirmed in Slack with @alcaeus

@alcaeus
Copy link
Contributor

alcaeus commented Nov 23, 2023

Its signature is not allowing false so it shouldn't be possible.

PHP extensions don't need to follow their own signatures, confirmed in Slack with @alcaeus

I'll add a clarification to that: the type of a value returned in an extension method is only checked when PHP has been compiled with debug symbols. When not running in debug mode, extension developers are trusted to "not do stupid things" and return only values that conform to the method signature.

I absolutely agree that this is an upstream issue that needs fixing.

@alexandre-daubois
Copy link
Contributor Author

alexandre-daubois commented Nov 23, 2023

I am currently trying to work on a fix on the extension. Will link this in the issue, closing for now 馃憤 Thank you for all your answers!

@alexandre-daubois
Copy link
Contributor Author

For the record: phpredis/phpredis#2422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants