Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (seehttps://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
• Features and deprecations must be submitted against the 5.x branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

Tested the code, and it's working.

If we add a new parametertls in query string, what is the purpose ofrediss://? /cc@nicolas-grekas

There really is no difference betweenredis andrediss at the moment, they're acting in exactly the same way. So you're right, usingrediss:// would be a better option.

From thepredis Readme:

Same set of parameters, but using an URI string:
$client = new Predis\Client('tls://127.0.0.1?ssl[cafile]=private.pem&ssl[verify_peer]=1');
The connection schemes redis (alias of tcp) and rediss (alias of tls) are also supported, with the difference that URI strings containing these schemes are parsed following the rules described on their respective IANA provisional registration documents.

IMHO the fix should be about keeping the scheme defined here:

And use that scheme in the relevant places:

• symfony/src/Symfony/Component/Cache/Traits/RedisTrait.php

  Line 135 in6dc5fea

  $hosts[$host] = ['scheme' =>'tcp','host' =>$host,'port' =>6379] +$parameters;

• symfony/src/Symfony/Component/Cache/Traits/RedisTrait.php

  Line 153 in6dc5fea

  array_unshift($hosts, ['scheme' =>'tcp','host' =>$params['host'],'port' =>$params['port'] ??6379]);

• symfony/src/Symfony/Component/Cache/Traits/RedisTrait.php

  Line 204 in6dc5fea

  @$redis->{$connect}($host,$port,$params['timeout'], (string)$params['persistent_id'],$params['retry_interval'],$params['read_timeout']);

Forphpredis to work, it requires the use oftls://127.0.0.1. However, forpredis, you can usetls:// orrediss:// (which is an alias fortls:// as it's said in the Predis README you mentioned). So we can't actually use the scheme for connecting withphpredis, but it can be used forpredis though.

Forphpredis to work, it requires the use oftls://127.0.0.1. However, forpredis, you can usetls:// orrediss:// (which is an alias fortls:// as it's said in the Predis README you mentioned). So we can't actually use the scheme for connecting withphpredis, but it can be used forpredis though.

And it doesn't work with the same DSN on Symfony Messenger because ofhttps://github.com/symfony/messenger/blob/5.x/Transport/TransportFactory.php#L46:

  [Symfony\Component\Messenger\Exception\InvalidArgumentException]                                                                                                     No transport supports the given Messenger DSN "rediss://<...>"

I don't have strong opinion aboutrediss:// vs?tls= but IMHO this should be consistent across all components:
I suggest to:

• deprecates either?tls in messenger orrediss:// in cache, lock, session
• in all cases fix RedisTrait

Given The RedisTrait didn't work, maybe it's easier to deprecate therediss:// scheme? @symfony/mergers

I've created a PR to addrediss:// support to the Messenger (which is really going to be an alias totls=1), without deprecating thetls option (yet). Either way, it should definitely be standardized one way or another (or to support both).

For reference, a codebase I took over recently uses this bundle to configure their redis connections:https://github.com/snc/SncRedisBundle

The developers told me that they in favor of that bundle mainly because it allowed them to configure TLS connections, which is a requirement when using the managed Redis services of our current hoster Digital Ocean. Our DSNs are all configured with therediss scheme,but if I understood the bundle corectly, it mainly passes the DSN down to Predis. I got that wrong, thanks@njutn95 for the correction.

SncRedisBundle is indeed reading the TLS configuration from therediss:// scheme

I've created a PR to addrediss:// support to the Messenger (which is really going to be an alias totls=1), without deprecating thetls option (yet). Either way, it should definitely be standardized one way or another (or to support both).

For now, I changed the code to support both.rediss DSN sheme changes Redis scheme bytls (for Predis particularly) and adds the prefixtls:// in host for Redis extension.

Eithertls option should not be added here,rediss scheme should be deprecated.

We should not support both.

To be consistent with#39607, I keep therediss scheme for TLS and removed the option.

@nicolas-grekas I would vote for merging that in 4.4 as a bugfix, as symfony/cache claims to supportrediss in 4.4 already. Supportingrediss without actually enabling TLS qualifies as a bug to me.