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] Only prepend scheme to PhpRedis host when necessary #34017

Merged
merged 1 commit into from Aug 26, 2020
Merged

[6.x] Only prepend scheme to PhpRedis host when necessary #34017

merged 1 commit into from Aug 26, 2020

Conversation

ryzr
Copy link
Contributor

@ryzr ryzr commented Aug 26, 2020

Please see #34016

In cases where $options['scheme'] is set, but $options['host'] already has the scheme prepended, the resulting host will be scheme://scheme://host
@ryzr ryzr changed the title [6.x] Only prepend scheme to PhpRedis host when necessary #34016 [6.x] Only prepend scheme to PhpRedis host when necessary Aug 26, 2020
@GrahamCampbell
Copy link
Member

GrahamCampbell commented Aug 26, 2020

So I suppose the question is whether we want to support this. Do we take the position that you including the scheme in the host file is invalid syntax?

@ryzr
Copy link
Contributor Author

ryzr commented Aug 26, 2020

So I suppose the question is whether we want to support this. Do we take the position that you including the scheme in the host file is invalid syntax?

Good point. I'm actually prepending the tls:// scheme to env vars in Terraform config to get this working in the first place. It makes sense to me to specify host and scheme distinctively.

For me, upgrading from v7.25.0 to v7.26.0 was a breaking change (hence the PR), but curious how common this configuration was, if it wasn't what was intended anyway.

@sebdesign
Copy link
Contributor

sebdesign commented Aug 26, 2020

@ryzr I understand that in your config you have set the scheme to tls and also the host prefixed with tls://. In previous versions the scheme key did not do anything with the phpredis connector, but now it does prepend the tls:// in front of the host.

@driesvints I'm fine with this PR, as it addresses cases like this, which users have defined the scheme AND prepended the scheme in the host for the phpredis driver.

But configuring redis like this is not really correct, and it would certainly not work with the predis driver: you need to configure either the scheme or prefix the scheme in front of the host, but not both.

If folks need to use different connectors between environments, they could use the REDIS_URL or the REDIS_HOST, or even introduce a REDIS_SCHEME variable.

@driesvints
Copy link
Member

driesvints commented Aug 26, 2020

Maybe it's best that we fix this for 6.x & 7.x and then revert it again for 8.x?

@taylorotwell what are your thoughts?

@sebdesign
Copy link
Contributor

sebdesign commented Aug 26, 2020

@driesvints I agree, let's merge this. But we should be more clear about the correct configuration in the docs. I'll submit a PR to the laravel/docs.

@taylorotwell taylorotwell merged commit 0eb542d into laravel:6.x Aug 26, 2020
10 checks passed
@taylorotwell
Copy link
Member

taylorotwell commented Aug 26, 2020

I see no huge need to revert this after merging as long as our docs is correct.

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

5 participants