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

[Validator] Add new json Validator #28477

Merged
merged 1 commit into from Feb 13, 2019

Conversation

@zairigimad
Copy link
Contributor

commented Sep 15, 2018

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

Hi,

in so many cases we want to test if a value is a valid json or not, so I propose this new NotJson Json constraint for this need.

cordially,

@zairigimad zairigimad force-pushed the zairigimad:is-json-validator branch from 2d4352b to ddd393c Sep 15, 2018

@chalasr chalasr added the Validator label Sep 16, 2018

@chalasr chalasr added this to the next milestone Sep 16, 2018

@zairigimad zairigimad force-pushed the zairigimad:is-json-validator branch 3 times, most recently from c61fa99 to 24a15c2 Sep 16, 2018

@zairigimad

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

Hi @onEXHovia , @apfelbox ,
I've just resolved @onEXHovia discussion and i can't see your discussion @apfelbox ,
Sorry i did an --amed.

@apfelbox

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

Look here: #28477 (comment)

Can you see it with this link?

@apfelbox

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2018

For the early exit, see:

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/BicValidator.php#L29-L31
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/CountryValidator.php#L35-L37
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/CurrencyValidator.php#L36-L38

(and the other validators)
You need this check, as your validator should only check something, if the value is not empty. Because currently you can't implement an optional field, that has to be valid JSON if it is filled. Currently it has to be filled and be valid JSON.

But for the "it has to be filled part" you should use NotBlank or NotNull. That's why your validator should bail if the field is just empty.

@zairigimad zairigimad force-pushed the zairigimad:is-json-validator branch 2 times, most recently from 0c13707 to b061fcf Sep 17, 2018

@zairigimad zairigimad force-pushed the zairigimad:is-json-validator branch 3 times, most recently from 801f88e to 0ed8620 Sep 18, 2018

@zairigimad zairigimad force-pushed the zairigimad:is-json-validator branch from 0ed8620 to d59691b Sep 19, 2018

@ro0NL

ro0NL approved these changes Sep 20, 2018

Copy link
Contributor

left a comment

nice :) one last minor comment.

Show resolved Hide resolved src/Symfony/Component/Validator/Constraints/Json.php Outdated

@zairigimad zairigimad force-pushed the zairigimad:is-json-validator branch 3 times, most recently from 5887556 to b8e7c7d Oct 1, 2018

@ro0NL

ro0NL approved these changes Oct 2, 2018

@xabbuh

xabbuh approved these changes Oct 3, 2018

Copy link
Member

left a comment

Can you resolve the conflict? Besides that this looks good to me.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 3, 2018

Sorry to come late here, but what's the purpose of a JSON validator?
This forces a double decoding of the JSON payload, since I suppose there will be another place that will have to get its decoded value. This other place will also expect some format for the decoded JSON, so that this should be the place where the validation should happen really. Did I miss something?

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2018

I've at least added this once in a project, to validate (for the backend) arbitrary JSON sent by DraftJS. No decoding, just accepting json strings.

@zairigimad

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2018

Hi, we used to validate a lot of normal string inputs filled by contributors to see if valid JSON or not before saving them to the database. ( Application that provides configs as JSON used by other applications ). I'm not sure if it was the best way to provide configs but it was something like Cloudformation.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/images/designer-jsoneditor.png

@nicolas-grekas nicolas-grekas changed the title Add new json Validator [Validator] Add new json Validator Jan 27, 2019

@chalasr
Copy link
Member

left a comment

LGTM, needs rebase

@zairigimad zairigimad force-pushed the zairigimad:is-json-validator branch from ce33faa to cc65f3c Feb 2, 2019

@zairigimad

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2019

@chalasr rebase done :)

@javiereguiluz
Copy link
Member

left a comment

I left a minor comment, but I like this. Thanks @zairigimad.

Show resolved Hide resolved src/Symfony/Component/Validator/Constraints/Json.php Outdated

@zairigimad zairigimad force-pushed the zairigimad:is-json-validator branch from c4616c2 to 13ba8ad Feb 7, 2019

@fabpot

fabpot approved these changes Feb 13, 2019

@@ -330,6 +330,10 @@
<source>This Business Identifier Code (BIC) is not associated with IBAN {{ iban }}.</source>
<target>This Business Identifier Code (BIC) is not associated with IBAN {{ iban }}.</target>
</trans-unit>
<trans-unit id="85">

This comment has been minimized.

Copy link
@fabpot

fabpot Feb 13, 2019

Member

Should be 86.

This comment has been minimized.

Copy link
@fabpot

@fabpot fabpot force-pushed the zairigimad:is-json-validator branch from 13ba8ad to 131febc Feb 13, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

Thank you @zairigimad.

@fabpot fabpot merged commit 131febc into symfony:master Feb 13, 2019

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

fabpot added a commit that referenced this pull request Feb 13, 2019

feature #28477 [Validator] Add new json Validator (zairigimad)
This PR was squashed before being merged into the 4.3-dev branch (closes #28477).

Discussion
----------

[Validator] Add new json Validator

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| License       | MIT
| Doc PR        | symfony/symfony-docs#10351

Hi,

in so many cases we want to test if a value is a valid json or not, so I propose this new ~~NotJson~~  `Json` constraint for this need.

cordially,

Commits
-------

131febc [Validator] Add new json Validator

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 4, 2019

minor #10351 [Validator] Add new json Validator Documentation (zairig…
…imad)

This PR was squashed before being merged into the master branch (closes #10351).

Discussion
----------

[Validator] Add new json Validator Documentation

Hi,

introduce ~~IsJson~~ `Json` Constraint .
this is a documentation for the new feature in the pull request symfony/symfony#28477

cordially,

Commits
-------

01ea746 [Validator] Add new json Validator Documentation

@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

DavidPrevot added a commit to DavidPrevot/symfony that referenced this pull request May 13, 2019

Drop useless executable bit
The files were added as executable via symfony#28477.

fabpot added a commit that referenced this pull request May 13, 2019

minor #31488 Drop useless executable bit (DavidPrevot)
This PR was merged into the 4.3 branch.

Discussion
----------

Drop useless executable bit

The files were added as executable via #28477.

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Commits
-------

7905863 Drop useless executable bit
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.