FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add false as possible return value in stub for some get-type connection metadata methods by crocodele · Pull Request #2527 · phpredis/phpredis · GitHub

Add false as possible return value in stub for some get-type connection metadata methods - #2527

Open
crocodele wants to merge 1 commit into
phpredis:developfrom
crocodele:chore/StubFalseNotConnected
Open

Add false as possible return value in stub for some get-type connection metadata methods#2527
crocodele wants to merge 1 commit into
phpredis:developfrom
crocodele:chore/StubFalseNotConnected

Conversation

Copy link
Copy Markdown
Contributor

These methods return false if we're not connected. This should be reflected in the stub and arginfo.

I'm sure that the same applies to many other methods as well, but here I've focused on the get*() methods that return metadata about the connection.

Copy link
Copy Markdown
Contributor Author

@michael-grunder, @yatsukhnenko, any thoughts on this?

michael-grunder commented Sep 26, 2024
edited
Loading

Copy link
Copy Markdown
Member

This should work for most of the methods but the ones that throw an exception don't technically return anything.

Ideally, instead of returning false in the C code, we should use RETURN_THROWS. We could maybe update the logic to do that in 6.1.1 though.

I am able to get a false return from these methods:

$r->getDBNum() => false
$r->getHost() => false
$r->getPersistentID() => false
$r->getPort() => false
$r->getReadTimeout() => false

These throw an exception:

$r->getLastError() => exception
$r->getMode() => exception

Copy link
Copy Markdown
Contributor Author

I'll wait for the release of 6.1.0, then I can change the methods to use RETURN_THROWS.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL