Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DI] Add a "default" EnvProcessor#28976

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

Merged
nicolas-grekas merged 1 commit intosymfony:masterfromjderusse:env-default
Dec 1, 2018

Conversation

@jderusse
Copy link
Member

@jderussejderusse commentedOct 25, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PRTODO

This PR add a new fallback env processor in order to return a default value when the primary processor is not able to fetch a value (env variable, file or key does not exists)

# default_host: localhosthost: '%env(default:default_host:OPTIONAL_ENV_VARIABLE)%"default_secret: this secret is not secretsecret: '%env(default:default_secret:file:THIS_FILE_ONLY_EXIST_IN_PRODUCTION)%"default_charset: utf8charset: '%env(default:default_charset:key:charset:json:DATABASE_CONFIG)%"

emr reacted with thumbs up emoji
@ro0NL
Copy link
Contributor

why not stick withenv(SOME): 'default' in code?

@jderusse
Copy link
MemberAuthor

@ro0NL because sometime the env variable exist, but the processing doesn't. See my examples.

secret: '%env(fallback:this secret is not secret:file:THIS_FILE_ONLY_EXIST_IN_PRODUCTION)%'charset: '%env(fallback:utf8:key:charset:json:DATABASE_CONFIG)%'

@ro0NL
Copy link
Contributor

understood. Im a bit skeptical about making values part of the prefix string, but i cant think of anything better either.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedOct 25, 2018•
edited
Loading

That won't work as the%env(...)% allows only a very limited set of characters between the brackets - on purpose. The fallback should be given as an environment variable. Or maybe a parameter name.
What about this:%env(catch:...:param)%?param would be the name of a parameter that would define the fallback value.

4.3.0
-----

* added`%env(catch:...)%` processor to catch exception and fallback to a default value
Copy link
Contributor

@ro0NLro0NLNov 5, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

it must be(catch:...:DEFAULT_KEY) right

Copy link
MemberAuthor

@jderussejderusseNov 5, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

to be consistent withadded %env(key:...)% processor

}

$next =substr($name,0,$i);
$default =substr($name,$i +1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

perhaps check to be non empty, im also worried it causes confusion as the last segment will always be used. If you forget it, another part will be implicitly used.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

refactored the logic todefault:fallback:origin in order to catch onlyNotFoundException and letRuntimeException bubble

@jderusse
Copy link
MemberAuthor

Thank your for the review@ro0NL , changed the logic in the PR to trap only theNotFound exceptions. Because it was a mistake to catch RuntimeException.
And move the fallback parameter next to the keyWord to be consistent with other processors

// previous%env(catch:origin:fallback)%// after%env(default:fallback:origin)%
ro0NL reacted with thumbs up emoji

return$getEnv($next);
}catch (EnvNotFoundException$e) {
if (!$this->container->hasParameter($default)) {
thrownewEnvNotFoundException(sprintf('Parameter "%s" not found. (fallback from not found "%s")',$default,$next),0,$e);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

sprintf('Invalid env fallback in "default:%s": parameter "%s" not found.', $name, $default)?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

should it be a runtime exception btw?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

fix message.
what's aboutdefault:my_fallback:default:optionnal_parameter_injected_by_third_party:foo?

@jderussejderusseforce-pushed theenv-default branch 2 times, most recently fromb7e1db3 to16461d6CompareNovember 5, 2018 10:43
@jderussejderusse changed the titleAdd a fallback EnvProcessor[DI] Add a "default" EnvProcessorNov 5, 2018
*
* @author Nicolas Grekas <p@tchwork.com>
*/
class EnvNotFoundExceptionextends InvalidArgumentException
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

while at it..extends RuntimeException?

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

If the variable is not found or a a key contain a typo, it make sens to be a logic exception.

@javiereguiluz
Copy link
Member

@jderusse your examples are great as always ... but would this feature require any changes in Symfony apps? (both when using it as a stand-alone component and when using it as part of a full-stack Symfony app). Thanks!

@jderusse
Copy link
MemberAuthor

@javiereguiluz Same implementation thant#28975 😉
So the response is yes, it works out of the box

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

please submit a doc PR or doc issue at least

@nicolas-grekas
Copy link
Member

Thank you@jderusse.

@nicolas-grekasnicolas-grekas merged commitaee4e33 intosymfony:masterDec 1, 2018
nicolas-grekas added a commit that referenced this pull requestDec 1, 2018
This PR was squashed before being merged into the 4.3-dev branch (closes#28976).Discussion----------[DI] Add a "default" EnvProcessor| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | TODOThis PR add a new fallback env processor in order to return a default value when the primary processor is not able to fetch a value (env variable, file or key does not exists)```#default_host: localhosthost: '%env(default:default_host:OPTIONAL_ENV_VARIABLE)%"default_secret: this secret is not secretsecret: '%env(default:default_secret:file:THIS_FILE_ONLY_EXIST_IN_PRODUCTION)%"default_charset: utf8charset: '%env(default:default_charset:key:charset:json:DATABASE_CONFIG)%"```Commits-------aee4e33 [DI] Add a \"default\" EnvProcessor
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestJan 9, 2019
This PR was merged into the master branch.Discussion----------Add env default processorThis PR documentssymfony/symfony#28976Commits-------8d3afcc Add env default processor
@itscaro
Copy link

itscaro commentedJan 9, 2019•
edited
Loading

Hey guys, awesome work! thanks for this change.

But IMO, the syntax is a little bit illegible, if it would be great to use the syntax in shell%env(origin:-fallback)%

I can do a PR for that, wdyt?

fabpot added a commit that referenced this pull requestMar 10, 2019
… "default" one (nicolas-grekas)This PR was merged into the 4.3-dev branch.Discussion----------[DI] replace "nullable" env processor by improving the "default" one| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Neither `nullable` nor `default` are released yet.I propose to replace the `nullable` processor (see#29767) with an improved `default` one (from#28976).`%env(default::FOO)%` now defaults to `null` when the env var doesn't exist or compares to false".ping@jderusse@bpolaszekCommits-------c50aad2 [DI] replace "nullable" env processor by improving the "default" one
@rubenrua
Copy link
Contributor

IMHO and my devops teamdefault is a bad name. Looks like it returns the passed default value, not the value of other parameter.fallback is more coherent. (Checkhttps://twig.symfony.com/doc/2.x/filters/default.html)

Let me know in a new pull request with this rename and creating a newdefault EnvProcessor.

@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
@jderussejderusse deleted the env-default branchAugust 2, 2019 12:14
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

8 participants

@jderusse@ro0NL@nicolas-grekas@javiereguiluz@itscaro@rubenrua@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp