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

[6.x] Normalize scheme in Redis connections #33892

Merged
merged 2 commits into from Aug 17, 2020

Conversation

sebdesign
Copy link
Contributor

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

@sebdesign sebdesign changed the title Redis scheme [6.x] Normalize scheme in Redis connections Aug 15, 2020
@taylorotwell taylorotwell merged commit 69cbfc0 into laravel:6.x Aug 17, 2020
@driesvints
Copy link
Member

@sebdesign this seems to have broken some things. See #34018 & #34017

Can you please take a look at this?

@sebdesign
Copy link
Contributor Author

@driesvints I'll look into it right now.

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.

None yet

3 participants