Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Serializer][FrameworkBundle] Add a YAML encoder#19326
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| private $parser; | ||
| private $defaultContext; | ||
| public function __construct(Dumper $dumper = null, Parser $parser = null, $defaultContext = array('yaml_inline' => 0, 'yaml_indent' => 0, 'yaml_flags' => 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.
Add typehintarray $defaultContext
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.
rather than putting a default value here, you should merge the default with the argument being received.
Otherwise, you have no guarantee about the available keys (and so your access to context options will be unsafe)
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.
Thanks! Fixed.
dunglas commentedJul 17, 2016
ping @symfony/deciders |
| $definition->addTag('serializer.normalizer',array('priority' => -900)); | ||
| } | ||
| if (class_exists(YamlEncoder::class)) { |
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.
You must also ensure thatsymfony/yaml is loaded as it isa dev dependency of theframework-bundle.
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.
It is not necessary because of this particular check. If the class doesn't exist (composant not installed), the encoder will not be registered.
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.
@dunglas yes, but here, YamlEncoder refer to the serializer component.
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.
In fact no. The class constant has a special behavior.
useSomething\That\Not\Exist;echo Exist::class;
This snippet will display the FQCN even if the class not exists.
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.
Dueuse Symfony\Component\Serializer\Encoder\YamlEncoder; this line will not check whether or not a class exists in the Yaml namespace
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.
Your check ensures that theSerializer component is loaded but you don't know if theYaml component is loaded too.
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.
Ok got it.
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.
The existing check is useful though, to keep compatibility with serializer 3.1 (other checks above also ensure compat with different versions of the component)
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.
You also need to ensure that the Yaml component is present in version 3.1 or higher (e.g. by checking for existence of one of theYaml::* constants).
dunglas commentedAug 16, 2016
Comments fixed. OK to merge? |
fabpot commentedSep 14, 2016
Thank you@dunglas. |
…las)This PR was squashed before being merged into the 3.2-dev branch (closes#19326).Discussion----------[Serializer][FrameworkBundle] Add a YAML encoder| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | todoAdd YAML support to the Serializer.Commits-------9366a7d [Serializer][FrameworkBundle] Add a YAML encoder
| { | ||
| $context = array_merge($this->defaultContext, $context); | ||
| return Yaml::parse($data, $context['yaml_flags']); |
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.
Shouldn't you use the injected parser instead of performing a static call?
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 agree (see#20006).
This PR was merged into the 3.2-dev branch.Discussion----------[Serializer] use the injected YAML parser| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#19326 (comment)| License | MIT| Doc PR |Commits-------a40c0e4 [Serializer] use the injected YAML parser
This PR was submitted for the master branch but it was merged into the 3.2 branch instead (closes#7040).Discussion----------[Serializer] Docs for CSV and YAML encodersDocs forsymfony/symfony#19197 andsymfony/symfony#19326.Commits-------5687972 [Serializer] Docs for CSV and YAML encoders.
…r (GuilhemN)This PR was submitted for the 3.2 branch but it was merged into the 3.3 branch instead (closes#7654).Discussion----------[Serializer] Document the CsvEncoder and the YamlEncoderDocument the CsvEncoder and the YamlEncoder introduced insymfony/symfony#19197 andsymfony/symfony#19326.Commits-------6944ea1 [Serializer] Document the CsvEncoder and the YamlEncoder
Add YAML support to the Serializer.