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] Deprecated using env vars with cannotBeEmpty() #28858

Merged
merged 1 commit into from Dec 1, 2018

Conversation

Projects
None yet
5 participants
@ro0NL
Copy link
Contributor

commented Oct 14, 2018

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

Continuation of #28838 for 4.2

Using environment variables for nodes marked cannotBeEmpty() is semantically not possible, we'll never know the value is empty yes/no during compile time. Neither we should assume one or another.

@nicolas-grekas
Copy link
Member

left a comment

Nice. Using an env as part of a string would work isn't it? (foo: 'https://%env(BAR)%')
Worth a test case?

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2018

Correct. Test added.

@@ -16,6 +16,7 @@ Config

* Deprecated constructing a `TreeBuilder` without passing root node information.
* Deprecated `FileLoaderLoadException`, use `LoaderLoadException` instead.
* Deprecated using placeholder values (e.g. environment variables) with `cannotBeEmpty()`

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 14, 2018

Member

I think we should remove the word placeholder and use env var in all messages in this PR. It's interal details.

@ro0NL ro0NL force-pushed the ro0NL:env-empty branch 2 times, most recently from 30c915e to dad2470 Oct 14, 2018

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 14, 2018

@@ -81,6 +81,17 @@ protected function validateType($value)
*/
protected function finalizeValue($value)
{
if (!$this->allowEmptyValue && $this->isHandlingPlaceholder()) {
@trigger_error(sprintf('Using a placeholder value at path "%s" when empty values are not allowed by definition is deprecated since Symfony 4.2.', $this->getPath()), E_USER_DEPRECATED);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Oct 14, 2018

Member

Maybe: Setting path "%s" to an environment variable is deprecated since Symfony 4.2.?
Also when the expected value is a string, should we suggest env vars can still be part of the option?
That could help make the notice a bit more actionable, which is important I think.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Oct 15, 2018

Author Contributor

What about

Setting path "%s" to an environment variable is deprecated since Symfony 4.2.
Remove "cannotBeEmpty()" or include a non-empty string instead.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Oct 15, 2018

Author Contributor

Updated

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Thinking a bit more about this, I think it will prevent legitimate configs. Can we throw only when a custome config validator is also used?

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

You mean validate() right? I dont see how that's related?

From the issue, even without the validation rule, our conclusion remains right? We cant test an env placeholder to be empty yes/no during compile, so we abstain.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

Abstaining would mean not throwing unless we're sure this gonna be empty.
Here we can trivially break existing apps that eg use DATABASE_URL without providing any alternative, while it currently works.
It's related since the linked issue is exactly about that: combining cannot be empty with a validator.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

Abstaining would mean not throwing

only as of 5.0 and when not removing cannotBeEmpty() :)

I see your point though, we're having the same discussion as in #26747 which lead to this change in the first place.

Basically we changed the solution, now with an upgrade path via deprecation. That's at least valid on the technical side to me. We can debate about empty envs in practice, but we cant deny its possibility.

I thought about allowing e.g. bool: and int: (see isValueEmpty() in BooleanNode and NumericNode), but from Symfony perspective all envs are nullable :(

I think to solve the linked issue, #28838 alone is sufficient (it's about getting null not an empty string).

Tend to prefer the current behavior and assume envs not being empty compared to a conditional throw =/ So we could close here, the goal for 5.0 was noble though :)

without providing any alternative, while it currently works.

ideally we'd infer empty yes/no from the prefix (e.g. %(not-empty:SOME)%)

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2018

I got it 😂 checking for custom validators would (at least) avoid ever passing an empty string (or null for that matter) to closures, which would always be unexpected. And affects less apps, sure :)

Status: needs work

@stof

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

I think we need to have a forbidRuntimeValues or something like that, which would allow authors to tell the Config component that this value must be known at compile time, to allow providing a better error message. It is quite common to have some settings for which supporting an env placeholder will never be possible (settings supporting a service id for instance).

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

that could work yes, and basically let the config author control

protected function allowPlaceholders(): bool

how does it relate to cannotBeEmpty()? Should we continue with this PR alogn with the suggestion to only trigger in case of custom validators?

Or would a new forbidRuntimeValues() supersede it?

@stof

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@ro0NL I would say that in 5.0, cannotBeEmpty would automatically imply forbidRuntimeValues (as it would be performing a compile-time validation)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

As explained above, I don't think this would be appropriate. It's very common to use an env var in a cannotbeempty option and have things just work. Breaking these without providing alternatives is a bad plan we should avoid IMHO.
I'm still thinking we should only conflict when a custom validator is defined, and let the rest in peace.
This feature is complex enough to not add more complexity on top...

@stof

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@nicolas-grekas there is no way to guarantee that the env variable will be non-empty at runtime. So if your code is fine with that, it means it does not actually care about forbidding the empty value.

but anyway, I'm fine with having forbidRuntimeValues separate from cannotBeEmpty. I still think it makes sense to provide forbidRuntimeValues (there are use cases for that, and I already got bug reports in some bundles I maintain because of people trying to use env variables in a place where we were not supporting them and so ending up with %env(...)% as part of the service name for the alias)

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

if your code is fine with that, it means it does not actually care about forbidding the empty value

If the env var is always set in practice, the code never has to deal with empty value, that's why it works.

makes sense to provide forbidRuntimeValues

could be, but let's not hurry for it, as again complexity is high enough. Let's find a real use case first.

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

I'm glad we're on the same level here, in terms of understanding and settling. I agree with @nicolas-grekas to not rush the complexity with new features.

We should also investigate the other way around: assume envs will not be empty during runtime (check for that). If that's 9/10 cases, a opt-in thru env-prefix to indicate "could be empty" could also do it.

@stof

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@nicolas-grekas the first real-world use case is in DoctrineBundle. the default entity manager cannot be changed using an env variable

@stof

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

And we also have many similar cases in Symfony itself, where some settings are necessary at compile time (look at SecurityBundle for instance)

@ro0NL ro0NL force-pushed the ro0NL:env-empty branch 2 times, most recently from 036a586 to 65e2ccf Oct 16, 2018

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Ready. I've added comments to clarify the intend as much as possible :) hope it's clear like this.

See #28896 for the other discussion.

status: needs review

@ro0NL ro0NL force-pushed the ro0NL:env-empty branch 2 times, most recently from e6ad6ce to 812660d Oct 16, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

assume envs will not be empty during runtime (check for that).

I like this idea, we could even deprecate empty env vars and throw when one is defined in 5.0!

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@ro0NL could you please create a Symfony Docs issue for this? If possible, please review the existing docs to tell us if this new feature requires to change/remove anything in the current docs. And also, please tell us if we need to add something new. Thanks a lot!

@ro0NL

This comment has been minimized.

Copy link
Contributor Author

commented Nov 8, 2018

im hesitating we should tell ppl :)

@nicolas-grekas nicolas-grekas removed the Bug label Dec 1, 2018

@nicolas-grekas nicolas-grekas force-pushed the ro0NL:env-empty branch from c1f6f9b to 397c19e Dec 1, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Thank you @ro0NL.

@nicolas-grekas nicolas-grekas merged commit 397c19e into symfony:master Dec 1, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 1, 2018

feature #28858 [DI] Deprecated using env vars with cannotBeEmpty() (r…
…o0NL)

This PR was squashed before being merged into the 4.3-dev branch (closes #28858).

Discussion
----------

[DI] Deprecated using env vars with cannotBeEmpty()

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes-ish
| New feature?  | yes
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | yes
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #28827
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Continuation of #28838 for 4.2

Using environment variables for nodes marked `cannotBeEmpty()` is semantically not possible, we'll never know the value is empty yes/no during compile time. Neither we should assume one or another.

Commits
-------

397c19e [DI] Deprecated using env vars with cannotBeEmpty()

@ro0NL ro0NL deleted the ro0NL:env-empty branch Dec 1, 2018

@ro0NL ro0NL referenced this pull request Dec 1, 2018

Merged

Add upgrade from 4.2 to 4.3 #29400

nicolas-grekas added a commit that referenced this pull request Dec 1, 2018

minor #29400 Add upgrade from 4.2 to 4.3 (ro0NL)
This PR was merged into the 4.3-dev branch.

Discussion
----------

Add upgrade from 4.2 to 4.3

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Forgotten in #28858 i guess. cc @nicolas-grekas

Commits
-------

ce6ecaf Add upgrade from 4.2 to 4.3

@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.