-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
Allow for Redis info arrays which use Clients and Server keys #60
base: 1.25.x
Are you sure you want to change the base?
Conversation
026cc90
to
99d1229
Compare
Signed-off-by: Tim Visser <[email protected]>
Signed-off-by: Tim Visser <[email protected]>
99d1229
to
2d51d4c
Compare
Signed-off-by: Tim Visser <[email protected]>
ea008dd
to
e66b6b1
Compare
Signed-off-by: Tim Visser <[email protected]>
0879aae
to
4be10bd
Compare
Signed-off-by: Tim Visser <[email protected]>
4be10bd
to
fddceab
Compare
… information Signed-off-by: Tim Visser <[email protected]>
Signed-off-by: Tim Visser <[email protected]>
@Ocramius I've reworked the changes a bit - they now include some missing checks/guards as Psalm helpfully commented on and the change now targets the 1.24.x branch which I've merged into my branch. Wrt the 'needs unit test' label - the only way I could test this is by mocking Redis/ClientInterface which are obviously both not part of laminas-diagnostics. So that only provides false sense of security as they could change their API at any moment and the test would be none the wiser. Wdyt? |
@Ocramius Could you have a look at this? |
About testing, I wonder if we can perhaps really spin up a redis instance? 🤔 |
Hi, |
Is it possible to finally fix this issue after more than a year, please? 🙏🏻 |
Setting up a testing scenario for specific Redis versions is not something I’m able to do unfortunately 🤷♂️ Also I’m currently not actively working on PHP projects anymore so I’m affraid I won’t be actively following this PR (especially after it has been up here for more than a year 😅) |
I just ran into this. I can confirm that this fixes the bug throwing an error:
I'm running a Redis 6 setup using |
Description
The Redis Check has been extended by including response time, number of connected clients and uptime.
(https://github.com/laminas/laminas-diagnostics/pull/57/files#diff-ae274e5d76cba73799bb44059a715045e3721dbe71f125ad128df5d2152c2a52)
However, the number of connected clients and uptime may need to be retrieved in slightly different ways, as the array structure returned by
Predis\ClientInterface::info()
can have different data structures.The previous behaviour only allowed directly retrieving
connected_clients
anduptime_in_seconds
from the array returned byPredis\ClientInterface::info()
. The suggested changes allow for those keys residing in$array['Clients']
and$array['Server']
respectively, as was the case in my setup.In my testing I used Redis V6.0.6 and Predis V1.1.10.
NB: The suggested change allows for both data structures of the info array.