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

[DI] Deprecate non-string default envs #27808

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
5 participants
@ro0NL
Copy link
Contributor

commented Jul 2, 2018

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 :)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

Ok. Deja vu: #23901 (comment)

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jul 3, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2018

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:

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

This comment has been minimized.

Copy link
Member

commented Jul 4, 2018

Environment variable not found: "FOO".

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

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2018

@nicolas-grekas any thoughts already?

@fabpot

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 23, 2018

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

@ro0NL ro0NL force-pushed the ro0NL:di/env-stages branch from 145dcc6 to cc387a7 Sep 23, 2018

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

I'm afraid tests are not green yet :(

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2018

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

This comment has been minimized.

Copy link
Member

commented Oct 11, 2018

do we want to deprecate non-string default envs?

👍 for me

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

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

@ro0NL ro0NL force-pushed the ro0NL:di/env-stages branch 2 times, most recently from d998f97 to 38243a0 Mar 26, 2019

@ro0NL ro0NL force-pushed the ro0NL:di/env-stages branch from 38243a0 to b5bd84a Mar 26, 2019

@ro0NL ro0NL force-pushed the ro0NL:di/env-stages branch from b5bd84a to 2311437 Mar 26, 2019

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

@fabpot @nicolas-grekas this should do

status: needs review

@fabpot

fabpot approved these changes Mar 27, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

Thank you @ro0NL.

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

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 27, 2019

feature #27808 [DI] Deprecate non-string default envs (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     <!-- see https://symfony.com/bc -->
| Deprecations? | yes
| Tests pass?   | no    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #27680, #27470 (comment)
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

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

@ro0NL ro0NL deleted the ro0NL:di/env-stages branch Apr 2, 2019

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

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.