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

Allow to call JsonSerializationVisitorFactory::setOptions() with value 0 #749

Merged
merged 1 commit into from Jun 4, 2019

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Jun 3, 2019

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

Without these changes, configuring the serializer with the following definition is not possible.

jms_serializer:
    visitors:
        json_serialization:
            options: 0 # json_encode options bitmask, suggested JSON_PRETTY_PRINT in development

@goetas
Copy link
Collaborator

goetas commented Jun 3, 2019

Thanks for the PR.

Isn't 0 the default in both of the cases?

@phansys
Copy link
Contributor Author

phansys commented Jun 3, 2019

Yes, I must improve the tests. The fact is that !empty() with value 0 returns false, so JsonSerializationVisitorFactory::setOptions() is never called with this value. And the default value on that class is 1024.

@phansys phansys force-pushed the json_visitors_config branch 2 times, most recently from 3151505 to 8750e1c Compare June 3, 2019 21:31
@phansys phansys changed the title Allow to use 0 as value for some "visitors" children nodes Allow to call JsonSerializationVisitorFactory::setOptions() with value 0 Jun 3, 2019
@phansys
Copy link
Contributor Author

phansys commented Jun 3, 2019

Tests and PR title updated.

@phansys phansys force-pushed the json_visitors_config branch 2 times, most recently from c57b293 to d3f3e1a Compare June 3, 2019 22:04
@goetas goetas merged commit 91a8688 into schmittjoh:master Jun 4, 2019
@goetas
Copy link
Collaborator

goetas commented Jun 4, 2019

Thanks for contributing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants