NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

Allow "json:" env var processor to accept null value#26498

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

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfrommcfedr:json_env_any

Mar 27, 2018

Merged

Allow "json:" env var processor to accept null value#26498

fabpot merged 1 commit intosymfony:masterfrommcfedr:json_env_any

Mar 27, 2018

Conversation

Copy link

Contributor

mcfedr commentedMar 12, 2018

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	Currently no docs for this feature

Edits the EnvVarProcessor so that it allows any type of variable in JSON encoded vars.

Previously it was only possible to use%env(json:ENV)% for array types, but this seems to be an arbitrary restriction, when I could have any other data type in that JSON.

Valid JSON that was previously rejected:

1
null
"string"

carsonbot added Status: Needs Review Feature labels

Mar 12, 2018

Copy link

Member

nicolas-grekas commentedMar 12, 2018

There are reasons for the current restriction:

having a single type (array) makes it easier to validate types, eg for chained processors or config validation
the other JSON types can easily be achieved using other processors (ie int, bool, string, null)

nicolas-grekas added this to the4.1 milestone

Mar 12, 2018

Copy link

ContributorAuthor

mcfedr commentedMar 12, 2018

I wondered if this might be what's happening. I have a case where in passing either an array or null.
What if I changed this to allownull? I feel like this might be a useful case.

Copy link

Member

nicolas-grekas commentedMar 12, 2018

if you want to setnull, I think you can already achieve it with the const processor:const:null

Copy link

Member

javiereguiluz commentedMar 12, 2018

@nicolas-grekas but I think he's saying that an external piece of software generates this config that usually is a JSON-encoded array but it can benull when the config is empty (Symfony does the same and we usearray|null everywhere instead ofarray|empty array). But I guess you'll need to change that external software to encode an empty JSON array when the config is empty.

Copy link

ContributorAuthor

mcfedr commentedMar 12, 2018

Exactly, I need to accept either array or null.

mcfedr force-pushed thejson_env_any branch frome3ce146 todb50d62Compare

March 12, 2018 17:45

mcfedr changed the title~~Allow "json:" env var processor to parse any json value~~Allow "json:" env var processor to accept null value

Mar 12, 2018

Copy link

Member

nicolas-grekas commentedMar 14, 2018

would you mind adding a test case please?

mcfedr force-pushed thejson_env_any branch 2 times, most recently from05ec676 tofdee150Compare

March 15, 2018 11:29

Copy link

ContributorAuthor

mcfedr commentedMar 15, 2018

Sure, also added some more general testing around EnvVarProcessor as there don't seem to exist any at the moment.

mcfedr force-pushed thejson_env_any branch fromfdee150 toebcf075Compare

March 15, 2018 12:02

nicolas-grekas reviewed

Mar 15, 2018

View reviewed changes

src/Symfony/Component/DependencyInjection/Tests/EnvVarProcessorTest.php

		useSymfony\Component\DependencyInjection\EnvVarProcessor;
		useSymfony\Component\DependencyInjection\Exception\LogicException;

		class EnvVarProcessorTestextends TestCase

Copy link

Member

nicolas-grekasMar 15, 2018

Choose a reason for hiding this comment

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

would you mind opening a separate PR against branch 3.4 for this test class?

Copy link

ContributorAuthor

mcfedrMar 15, 2018•
edited
Loading

Choose a reason for hiding this comment

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

Ok, I moved to#26542, but had to remove the case forjson:null because it will fail in 3.4, and i cannot add it here until that PR lands in master. I can probably make another PR later with more cases.

mcfedr mentioned this pull request

Mar 15, 2018

[DI] Add tests for EnvVarProcessor#26542

Closed

mcfedr force-pushed thejson_env_any branch fromebcf075 toc5c23c2Compare

March 15, 2018 15:19

javiereguiluz reviewed

Mar 16, 2018

View reviewed changes

src/Symfony/Component/DependencyInjection/EnvVarProcessor.php Outdated


		if (!is_array($env)) {
		if (null !==$env &&!is_array($env)) {
		thrownewRuntimeException(sprintf('Invalid JSON env var "%s": array expected, %s given.',$name,gettype($env)));

Copy link

Member

javiereguiluzMar 16, 2018

Choose a reason for hiding this comment

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

The error message should be updated:array expected ... ->array or null expected ...

mcfedr force-pushed thejson_env_any branch 2 times, most recently fromd523a2c to98b1ae5Compare

March 19, 2018 10:31

Copy link

ContributorAuthor

mcfedr commentedMar 19, 2018

Rebased on master to see if it would fix the tests, but now seems to fail on some other doctrine tests

nicolas-grekas added a commit that referenced this pull request

Mar 19, 2018

minor#26542[DI] Add tests for EnvVarProcessor (mcfedr)

d818636

This PR was squashed before being merged into the 3.4 branch (closes#26542).Discussion----------[DI] Add tests for EnvVarProcessor| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        | n/aAdd tests for the `EnvVarProcessor` as it doesn't have any at the moment.Originally from this PR against master,#26498Commits-------2992bb3 [DI] Add tests for EnvVarProcessor

Copy link

Member

nicolas-grekas commentedMar 19, 2018

@mcfedr you said had more tests in26542 before moving it to 3.4. Did you add them back here after the rebase?

Copy link

ContributorAuthor

mcfedr commentedMar 19, 2018

I was thinking it would make sense for the tests in#26542 in to be expanded to include this specific case of paring"null" as valid json, as this commit is based on master I cannot add that here until that commitd818636 is merged into master.

Copy link

Member

nicolas-grekas commentedMar 19, 2018

Indeed. Merge done, rebase unlocked on your side.

mcfedr force-pushed thejson_env_any branch from98b1ae5 toe7e5cfdCompare

March 19, 2018 17:51

Copy link

ContributorAuthor

mcfedr commentedMar 19, 2018

Ok, rebased with extra test cases

Allow "json:" env var processor to parse null values

abc7480

Amend EnvVarProcessorTest to include all possible json values

mcfedr force-pushed thejson_env_any branch frome7e5cfd toabc7480Compare

March 19, 2018 17:53

nicolas-grekas approved these changes

Mar 22, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Mar 22, 2018

fabpot approved these changes

Mar 27, 2018

View reviewed changes

Copy link

Member

fabpot commentedMar 27, 2018

Thank you@mcfedr.

fabpot merged commitabc7480 intosymfony:master

Mar 27, 2018

fabpot added a commit that referenced this pull request

Mar 27, 2018

feature#26498Allow "json:" env var processor to accept null value (…

2e47edc

…mcfedr)This PR was merged into the 4.1-dev branch.Discussion----------Allow "json:" env var processor to accept null value| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        | Currently no docs for this featureEdits the EnvVarProcessor so that it allows any type of variable in JSON encoded vars.Previously it was only possible to use `%env(json:ENV)%` for array types, but this seems to be an arbitrary restriction, when I could have any other data type in that JSON.Valid JSON that was previously rejected:- `1`- `null`- `"string"`Commits-------abc7480 Allow "json:" env var processor to parse null values

Copy link

Contributor

ro0NL commentedMar 27, 2018

@mcfedr anything to do for this test?https://github.com/symfony/symfony/pull/23888/files#diff-d81d0f25c32e251a89ce524b0495113dR74

Copy link

ContributorAuthor

mcfedr commentedMar 27, 2018

@ro0NL I don't see anything specific to this change, this PR makes it possible for thejson: processor to return either an array ornull, so it can still never be anint.