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.

Sign up for GitHub

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

Jump to bottom

RedisEnvDsn will break with Symfony 3.4/4.1 #356

Closed
nicolas-grekas opened this issue Aug 28, 2017 · 7 comments
Closed

RedisEnvDsn will break with Symfony 3.4/4.1 #356

nicolas-grekas opened this issue Aug 28, 2017 · 7 comments
Labels
bug Not working as intended

Comments

@nicolas-grekas
Copy link
Contributor

In https://github.com/snc/SncRedisBundle/blob/master/DependencyInjection/Configuration/RedisEnvDsn.php#L25

you hardcode the env placeholder regexp. This is an internal detail that is not covered by the BC policy and will change in 3.4 - thus the logic will break then.

The correct logic should use $container->resolveEnvPlaceholders() instead.
@ksaveras
Copy link

ksaveras commented Sep 4, 2017

+1

1 similar comment
@birkof
Copy link

birkof commented Sep 13, 2017

+1

@nicolas-grekas nicolas-grekas mentioned this issue Oct 8, 2017
Merged
@nicolas-grekas
Copy link
Contributor Author

The break will be removed by symfony/symfony#24486
(but using resolveEnvPlaceholder would still be a good idea.)

fabpot added a commit to symfony/symfony that referenced this issue Oct 8, 2017
@fabpot
minor #24486 [DI] Remove colon from env placeholders (nicolas-grekas)
e7c3456 
This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Remove colon from env placeholders

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | snc/SncRedisBundle#356
| License       | MIT
| Doc PR        | -

These colons in placeholders make them less neutral. Let's remove them.

Commits
-------

dca930f [DI] Remove colon from env placeholders
@nicolas-grekas
Copy link
Contributor Author

Reopening: version 4.1 did change the pattern of env placeholders. This bundle will break unless this is fixed.

@nicolas-grekas nicolas-grekas changed the title RedisEnvDsn will break with Symfony 3.4 RedisEnvDsn will break with Symfony 3.4/4.1 Apr 26, 2018
@curry684
Copy link
Collaborator

Hrm thanks for spotting it, we really should reconsider the entire mechanism for 3.x of this bundle.

@curry684 curry684 added the bug Not working as intended label Apr 26, 2018
@curry684 curry684 mentioned this issue Apr 26, 2018
Merged
@B-Galati
Copy link
Collaborator

I will try to fix this in coming weeks for 2.1.

B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Apr 28, 2018
@B-Galati
Remove env placeholder regexp
7b80c40 
fix snc#356

This is an internal detail that is not covered by the BC policy of Symfony and can thus break on any version
We now detect if env variable is used by checking the 3rd parameter of method ContainerBuilder::resolveEnvPlaceholders
@B-Galati B-Galati mentioned this issue Apr 28, 2018
Merged
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Apr 28, 2018
@B-Galati
Remove env placeholder regexp
18fa371 
fix snc#356

This is an internal detail that is not covered by the BC policy of Symfony and can thus break on any version
We now detect if env variable is used by checking the 3rd parameter of method ContainerBuilder::resolveEnvPlaceholders
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Apr 28, 2018
@B-Galati
Remove env placeholder regexp
f4d50c2 
fix snc#356

This is an internal detail that is not covered by the BC policy of Symfony and can thus break on any version
We now detect if env variable is used by checking the 3rd parameter of method ContainerBuilder::resolveEnvPlaceholders
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Apr 28, 2018
@B-Galati
Remove env placeholder regexp
fdec2b2 
fix snc#356

This is an internal detail that is not covered by the BC policy of Symfony and can thus break on any version
We now detect if env variable is used by checking the 3rd parameter of method ContainerBuilder::resolveEnvPlaceholders
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Apr 28, 2018
@B-Galati
Remove env placeholder regexp
a4853a6 
fix snc#356

This is an internal detail that is not covered by the BC policy of Symfony and can thus break on any version
We now detect if env variable is used by checking the 3rd parameter of method ContainerBuilder::resolveEnvPlaceholders
@curry684
Copy link
Collaborator

Still an issue in 2.1.3

@curry684 curry684 reopened this May 22, 2018
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Jun 3, 2018
@B-Galati
Fix support for DSN env variable with phpredis
73d2be4 
fix snc#428 snc#356
@B-Galati B-Galati mentioned this issue Jun 3, 2018
Merged
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Jun 16, 2018
@B-Galati
Fix support for DSN env variable with phpredis
e5b56a5 
fix snc#428 snc#356
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Jun 16, 2018
@B-Galati
Fix support for DSN env variable with phpredis
93f5c76 
fix snc#428 snc#356
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Jun 16, 2018
@B-Galati
Fix support for DSN env variable with phpredis
f1f720a 
fix snc#428 snc#356
B-Galati added a commit to B-Galati/SncRedisBundle that referenced this issue Jun 25, 2018
@B-Galati
Fix support for DSN env variable with phpredis
3bb59d9 
fix snc#428 snc#356
curry684 pushed a commit that referenced this issue Jun 25, 2018
@B-Galati @curry684
Fix support for DSN env variable with phpredis (#432)
9bb9dd6 
fix #428 #356
curry684 pushed a commit that referenced this issue Jul 19, 2018
@B-Galati @curry684
Fix support for DSN env variable with phpredis (#432)
4a57d3c 
fix #428 #356

(cherry picked from commit 9bb9dd6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Not working as intended
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@birkof @nicolas-grekas @ksaveras @B-Galati @curry684