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

[7.x] Normalize scheme in Redis connections #33871

Closed
wants to merge 2 commits into from

Conversation

sebdesign
Copy link
Contributor

@sebdesign sebdesign commented Aug 13, 2020

Description

This PR attempts to improve the consistency between Predis and PhpRedis drivers regarding the configuration of the scheme.

Current behavior

By default both drivers are connecting to the Redis server using tcp. It is possible to configure the scheme but each driver requires a different setup:

  • The Predis client accepts a scheme key in the $parameters array of its constructor. E.g. 'scheme' => 'tls'.
  • The PhpRedis client needs to have the scheme prefixed to the $host string of its constructor. E.g. tls://127.0.0.1.

In Laravel that means we have to use different configurations in database.php:

// predis
'default' => [
    'scheme' => 'tls',
    'host' => '127.0.0.1',
    'port' => '6379',
]

// phpredis
'default' => [
    'host' => 'tls://127.0.0.1',
    'port' => '6379',
]

Since #33800 was merged, it is possible to define the scheme using the url key for phpredis. The url being parsed transforms the host in the correct format as tls://127.0.0.1:

// phpredis
'default' => [
    'url' => 'tls://127.0.0.1:6379',
]

But using the url key for the predis driver results in a connection error, because the host is again formatted as tls://127.0.0.1 but the client has its scheme as tcp internally, so it tries to connect to tcp://tls://127.0.0.1:6379 which is invalid.

Proposed changes

With this PR, it is possible to use the scheme key as well as the url key for both drivers, thus improving the consistency in the configuration and allowing to switch drivers more easily:

// scheme using url (predis / phpredis)
'default' => [
    'url' => 'tls://127.0.0.1:6379',
]

// scheme using key (predis / phpredis)
'default' => [
    'scheme' => 'tls',
    'host' => '127.0.0.1',
    'port' => '6379',
]

Instead of formatting the host in RedisManager like the aforementioned PR does, it assigns the scheme key in the configuration, which will be handled in the connectors: The PredisConnector will pass it to the client's constructor, and the PhpRedisConnector will format the host using the scheme if it was defined in the configuration or parsed from the url.

This change has no breaking changes, as it follows the functionality of the previous PR, allowing redis:// protocols as well as tcp:// and tls:// in the url key. It's also compatible with the existing functionality regarding the scheme key for the predis driver.

Summary

  • Add support for defining the scheme in the url key for predis
  • Add support for defining the scheme key for phpredis
  • Functionality for defining the scheme in the url key for phpredis remains the same
  • Functionality for defining the scheme key for predis remains the same
  • All functionality also apply to clusters
  • Add tests asserting both drivers are connecting correctly with scheme or url
  • Add test asserting the ConfigurationUrlParser returns the correct driver for Redis URLs with scheme

@GrahamCampbell GrahamCampbell changed the title Normalize scheme in Redis connections [7.x] Normalize scheme in Redis connections Aug 13, 2020
@driesvints
Copy link
Member

Ping @tillkruss

@tillkruss
Copy link
Contributor

@sebdesign: Should we also support rediss:// and redis:// out of the box?

@sebdesign
Copy link
Contributor Author

@tillkruss I believe URLs that begin with redis:// are using the default tcp scheme. I'll take a look at the docs of both drivers to see how we can handle redis and rediss protocols.

@tillkruss
Copy link
Contributor

@sebdesign: I believe Predis supports both out of the box, but we can just universally map them to tcp:// and tls://.

@tillkruss
Copy link
Contributor

Beauty! That will make DO and Heroku integration easier 🙌🏻

@GrahamCampbell
Copy link
Member

I'm tempted to say this should go to 6.x, tbh.

@driesvints
Copy link
Member

I agree. @sebdesign can you send this to 6.x? Thanks

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.

4 participants