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

Add a few more predis integration tests with different connection types #794

Closed
wants to merge 1 commit into from

Conversation

ajessu
Copy link

@ajessu ajessu commented Mar 16, 2020

Description

Closes #315 (original issue) .
Closes #479 (attempted fix, which was already fixed by #574)

This was previously fixed by #574. As I explored #315 and the fix, noticed the current fix leaves out a few rarely used connection types without host and port tags information, as they do not use the AbstractConnection, while still being single node configurations.
Not the end of the world, but fix was simple.

Also added tests for many other connection types that do not need extra integrations (live connections into a Redis instance) to initialize.


More detail:

This moves from checking on extending the abstract class, into checking on implementing thise single node connection interface. All but one one core connection (Webdis) uses the interface without extending the abstract class. Any custom connection a user can define could also fall under this, though this is again, a rare case.

This change focused on the original intent of #574. Avoids trying to get a connection's host and port from a Redis cluster, since these cluster might have more than one connections configured. These connection types all implement the AggregateConnectionInterface.

On the other hand, single server connections implement the NodeConnectionInterface, which is also implemented by the AbstractConnection.

The NodeConnectionInterface guarantees the getParameter method, which returns a ParameterInterface implementing object, which in turn guarantees for any getter methods that:

  • at least an string empty value for the host and port value, if they don't exist.
  • the actual host and port configured for the Redis node.

See:
https://github.com/nrk/predis/blob/v1.1/src/Connection/ParametersInterface.php#L21-L32
and
https://github.com/nrk/predis/blob/v1.1/src/Connection/Parameters.php#L146-L151

Readiness checklist

  • (only for Members) Changelog has been added to the appropriate release draft. Create one if necessary.
  • Tests added for this feature/bug.

Reviewer checklist

  • Appropriate labels assigned.
  • Milestone is set.
  • Changelog has been added to the appropriate release draft. For community contributors the reviewer is in charge of this task.

@ajessu ajessu force-pushed the predis-integration-tests branch from 4638f11 to b245f13 Compare March 16, 2020 07:52
@krakjoe
Copy link
Contributor

krakjoe commented Dec 30, 2021

I'm going to close this as it would appear to be stale (as well as at least partly inapplicable).

If I'm wrong to do that, please open a new PR, and accept my apologies in advance.

@krakjoe krakjoe closed this Dec 30, 2021
@bwoebi
Copy link
Collaborator

bwoebi commented Jan 26, 2022

Reference: #1094 fixed this as well.

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.

Predis Integration exception
3 participants