Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Config] Removed env var support with cannotBeEmpty()#31678
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
nicolas-grekas commentedMay 29, 2019
A test case is failing. |
| "require-dev": { | ||
| "symfony/yaml":"^4.4|^5.0", | ||
| "symfony/config":"^4.4|^5.0", | ||
| "symfony/config":"^5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is there a way not doing this? We'd better resist these when possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not sure how to detect Config 5.0 :/
then again .. it's nice to enforce the same behavior for everyone on DI 5.0 no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would try to try harder here: shouldn't the test case be moved to the config component?
This doesn't look legit to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
updated
shouldn't the test case be moved to the config component?
i favored a di+config integration test, instead of a config unit test. Integration test have to deal with low/high deps either way. It now covers the deps=high case, assuming the legacy test is covered on lower branches still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
agree 4.4|5.0 is the better constraint, which is conceptually the same behavior already but allows users to opt for deprecations vs. exceptions basically.
nicolas-grekas commentedJun 8, 2019
Thank you@ro0NL. |
…o0NL)This PR was merged into the 5.0-dev branch.Discussion----------[Config] Removed env var support with cannotBeEmpty()| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no <!-- seehttps://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 -->See#28858Commits-------ec27d74 [Config] Removed env var support with cannotBeEmpty()
See#28858