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

Allow for Redis info arrays which use Clients and Server keys #60

Open
wants to merge 7 commits into
base: 1.25.x
Choose a base branch
from

Conversation

Sacrome
Copy link

@Sacrome Sacrome commented Nov 15, 2022

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

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 and uptime_in_seconds from the array returned by Predis\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.

@Sacrome Sacrome changed the base branch from 1.20.x to 1.24.x January 17, 2023 14:30
@Sacrome
Copy link
Author

Sacrome commented Jan 17, 2023

@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?

@Sacrome
Copy link
Author

Sacrome commented Mar 22, 2023

@Ocramius Could you have a look at this?

@Ocramius Ocramius changed the base branch from 1.24.x to 1.25.x March 22, 2023 11:55
@Ocramius
Copy link
Member

About testing, I wonder if we can perhaps really spin up a redis instance? 🤔

@mremi
Copy link

mremi commented Dec 5, 2023

Hi,
I have the same issue, could it be merged?
Thanks

@lonely-pilot
Copy link

Is it possible to finally fix this issue after more than a year, please? 🙏🏻

@Sacrome
Copy link
Author

Sacrome commented Feb 10, 2024

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 😅)

@mvhirsch
Copy link

I just ran into this. I can confirm that this fixes the bug throwing an error:

PHP WARNING: Undefined array key "connected_clients"

I'm running a Redis 6 setup using Redis extension locally for development and Predis on production. I copied these changes and it just works. Can we please merge this and provide an interface and/or tests in a follow up PR?

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.

5 participants