[DI] Deprecate non-string default envs #27808

ro0NL · 2018-07-02T18:27:11Z

Q	A
Branch?	master
Bug fix?	yes-ish
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	no
Fixed tickets	#27680, #27470 (comment)
License	MIT
Doc PR	symfony/symfony-docs#...

This is a failing test to further clarify the issues raised in #27680

So given #27680 (comment)

We should be sure this solves a real-world issue.

I think it solves a real bug :)

src/Symfony/Component/DependencyInjection/Tests/Compiler/ValidateEnvPlaceholdersPassTest.php

nicolas-grekas · 2018-07-02T18:48:44Z

Didn't open the patch yet, still have a comment :)
We should make it possible to define
env(json:FOO): [...]
Basically we should use processors type to validate.

ro0NL · 2018-07-02T19:25:15Z

Isnt that over-complicated? IMHO there's only one real env, and thus only 1 default for it as well.

Following your logic we could do:

env(json:FOO): [1, 2, 3]
env(float:FOO): 1.5
env(FOO): "arbitrary"

And use ref: %env(my:truth:here:float:FOO)% on reference somewhere / anywhere.

Im not sure that makes sense... or at least is extremely hard to reason about.

Basically we should use processors type to validate.

Which we do already, the processors defined on the reference-side (e.g. for my:truth:here:float we receive types from my - a custom one in this case).

ro0NL · 2018-07-02T19:35:35Z

Ok. Deja vu: #23901 (comment)

Indeed, we kept this opportunity open.. but the current implem doesnt handle this at all

nicolas-grekas · 2018-07-03T15:39:52Z

Let's say today I rely on having e.g.:
env(json:SECRETS): { API_KEY: default-api-key, OTHER_KEY: other-token }
this PR will break my config but won't provide any alternative, isn't it?
It should be fine to me instead...

ro0NL · 2018-07-03T16:20:23Z

Let's say today I rely on having e.g.:
env(json:SECRETS): { API_KEY: default-api-key, OTHER_KEY: other-token }

You can't ... :)

The default value of an env() parameter must be scalar or null, but "array" given to "env(json:FOO)".

Ok, let's use env(float:FOO): 1.5 (conceptually the same thing - a prefixed default env), using it will throw:

Environment variable not found: "FOO".

I.e. the prefixed default is not used, which is the same behavior during config validation:

symfony/src/Symfony/Component/DependencyInjection/Compiler/ValidateEnvPlaceholdersPass.php

Lines 50 to 51 in f27c3a8

    
           if (false === $i = strpos($env, ':')) { 
        
               $default = $defaultBag->has("env($env)") ? $defaultBag->get("env($env)") : null;

this PR will break my config but won't provide any alternative

No. It will only trigger a deprecation declaring default env as non-string.

And given any prefixed default env is not used today, i think we should deprecate that as well :) and open-up whenever it's actually supported (of which i'm not sure we should do it).

nicolas-grekas · 2018-07-04T07:36:50Z

Environment variable not found: "FOO".

Oh, great then I forgot that, so all good, let's make tests pass :)

ro0NL · 2018-07-04T08:54:22Z

I need an extra hint :) do you suggest to make the test pass by preserving the assertion float(3.5) on $foo->default?

Or this PR; asserting string("3.5") along with a deprecation for declaring it as float(3.5)?

ro0NL · 2018-07-13T16:21:48Z

@nicolas-grekas any thoughts already?

fabpot · 2018-09-04T08:49:11Z

/cc @nicolas-grekas

nicolas-grekas · 2018-09-23T09:18:47Z

@ro0NL would you mind rebasing please? I'd like to see tests green :)

fabpot · 2018-10-10T10:41:12Z

I'm afraid tests are not green yet :(

ro0NL · 2018-10-11T06:36:35Z

Correct, it's still a failing test only :) to move forward the open questions are

do we want to deprecate non-string default envs?
```
parameters:
  env(SOME): 1.5 # before
  env(SOME): '1.5' # after
```
currently it's implicitly cast to string during runtime, but not during compile time (config validation) as it broke BC, but this inconsistency is weird.
somewhat related, but different PR: do we want to deprecate default prefixed envs?
```
parameters:
  env(foo:SOME): 'val' # before
  env(SOME): 'val' # after
```
a prefixed default env is not detected as such (i.e. above foo:SOME wouldnt override SOME.

xabbuh · 2018-10-11T08:07:05Z

do we want to deprecate non-string default envs?

👍 for me

fabpot · 2019-03-24T19:45:11Z

"do we want to deprecate non-string default envs?" 👍

ro0NL · 2019-03-26T17:22:34Z

@fabpot @nicolas-grekas this should do

status: needs review

fabpot · 2019-03-27T06:32:22Z

Thank you @ro0NL.

This PR was merged into the 4.3-dev branch. Discussion ---------- [DI] Deprecate non-string default envs | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes-ish | New feature? | yes | BC breaks? | no  | Deprecations? | yes | Tests pass? | no  | Fixed tickets | #27680, #27470 (comment) | License | MIT | Doc PR | symfony/symfony-docs#...  This is a failing test to further clarify the issues raised in #27680 So given #27680 (comment) > We should be sure this solves a real-world issue. I think it solves a real bug :) Commits ------- 2311437 [DI] Deprecate non-string default envs

…ers (ro0NL) This PR was merged into the 5.0-dev branch. Discussion ---------- [DI] remove support for non-string default env() parameters | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no  | Deprecations? | no | Tests pass? | yes  | Fixed tickets | #...  | License | MIT | Doc PR | symfony/symfony-docs#...  See #27808 Commits ------- 614f5da [DI] remove support for non-string default env() parameters

Since Symfony 4.3 it is deprecated to use any non-string value for default env variable value symfony/symfony#27808

carsonbot added Status: Needs Review DependencyInjection Bug Feature Deprecation labels Jul 2, 2018

ro0NL commented Jul 2, 2018

View reviewed changes

src/Symfony/Component/DependencyInjection/Tests/Compiler/ValidateEnvPlaceholdersPassTest.php Outdated Show resolved Hide resolved

ro0NL commented Jul 2, 2018

View reviewed changes

src/Symfony/Component/DependencyInjection/Tests/Compiler/ValidateEnvPlaceholdersPassTest.php Outdated Show resolved Hide resolved

nicolas-grekas added Status: Needs Work and removed Bug Deprecation Status: Needs Review labels Jul 3, 2018

nicolas-grekas added this to the next milestone Jul 3, 2018

ro0NL mentioned this pull request Jan 8, 2019

Nullable environment variable processor #29767

Merged

[DI] Deprecate non-string default envs

2311437

carsonbot removed the Status: Needs Work label Mar 26, 2019

carsonbot added Status: Needs Review and removed Status: Needs Review labels Mar 26, 2019

fabpot approved these changes Mar 27, 2019

View reviewed changes

carsonbot added the Status: Reviewed label Mar 27, 2019

fabpot merged commit 2311437 into symfony:master Mar 27, 2019

ro0NL deleted the di/env-stages branch April 2, 2019 14:00

ro0NL mentioned this pull request Apr 2, 2019

Deprecate non-string default envs symfony/symfony-docs#11279

Closed

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

fabpot mentioned this pull request May 9, 2019

Release v4.3.0-BETA1 #31435

Merged

ro0NL mentioned this pull request May 30, 2019

[DI] remove support for non-string default env() parameters #31731

Merged

andrew-demb added a commit to andrew-demb/jaeger-client-symfony-bridge that referenced this pull request Jan 28, 2022

Default env variable value should be a string

f6a8995

Since Symfony 4.3 it is deprecated to use any non-string value for default env variable value symfony/symfony#27808

andrew-demb mentioned this pull request Jan 28, 2022

Default env variable value should be a string code-tool/jaeger-client-symfony-bridge#23

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Deprecate non-string default envs #27808

[DI] Deprecate non-string default envs #27808

ro0NL commented Jul 2, 2018 •

edited

Loading

nicolas-grekas commented Jul 2, 2018 •

edited

Loading

ro0NL commented Jul 2, 2018 •

edited

Loading

ro0NL commented Jul 2, 2018 •

edited

Loading

nicolas-grekas commented Jul 3, 2018

ro0NL commented Jul 3, 2018

nicolas-grekas commented Jul 4, 2018

ro0NL commented Jul 4, 2018 •

edited

Loading

ro0NL commented Jul 13, 2018

fabpot commented Sep 4, 2018

nicolas-grekas commented Sep 23, 2018

fabpot commented Oct 10, 2018

ro0NL commented Oct 11, 2018 •

edited

Loading

xabbuh commented Oct 11, 2018

fabpot commented Mar 24, 2019

ro0NL commented Mar 26, 2019 •

edited

Loading

fabpot commented Mar 27, 2019

[DI] Deprecate non-string default envs #27808

[DI] Deprecate non-string default envs #27808

Conversation

ro0NL commented Jul 2, 2018 • edited Loading

nicolas-grekas commented Jul 2, 2018 • edited Loading

ro0NL commented Jul 2, 2018 • edited Loading

ro0NL commented Jul 2, 2018 • edited Loading

nicolas-grekas commented Jul 3, 2018

ro0NL commented Jul 3, 2018

nicolas-grekas commented Jul 4, 2018

ro0NL commented Jul 4, 2018 • edited Loading

ro0NL commented Jul 13, 2018

fabpot commented Sep 4, 2018

nicolas-grekas commented Sep 23, 2018

fabpot commented Oct 10, 2018

ro0NL commented Oct 11, 2018 • edited Loading

xabbuh commented Oct 11, 2018

fabpot commented Mar 24, 2019

ro0NL commented Mar 26, 2019 • edited Loading

fabpot commented Mar 27, 2019

ro0NL commented Jul 2, 2018 •

edited

Loading

nicolas-grekas commented Jul 2, 2018 •

edited

Loading

ro0NL commented Jul 2, 2018 •

edited

Loading

ro0NL commented Jul 2, 2018 •

edited

Loading

ro0NL commented Jul 4, 2018 •

edited

Loading

ro0NL commented Oct 11, 2018 •

edited

Loading

ro0NL commented Mar 26, 2019 •

edited

Loading