[Serializer][FrameworkBundle] Add a YAML encoder #19326
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.
Add typehint array $defaultContext
There was a problem hiding this comment.
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)
|
ping @symfony/deciders |
| @@ -977,6 +978,12 @@ private function registerSerializerConfiguration(array $config, ContainerBuilder | |||
| $definition->addTag('serializer.normalizer', array('priority' => -900)); | |||
| } | |||
|
|
|||
| if (class_exists(YamlEncoder::class)) { | |||
There was a problem hiding this comment.
You must also ensure that symfony/yaml is loaded as it is a dev dependency of the framework-bundle.
There was a problem hiding this comment.
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.
@dunglas yes, but here, YamlEncoder refer to the serializer component.
There was a problem hiding this comment.
In fact no. The class constant has a special behavior.
use Something\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.
Due use 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.
Your check ensures that the Serializer component is loaded but you don't know if the Yaml component is loaded too.
There was a problem hiding this comment.
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.
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 the Yaml::* constants).
|
Comments fixed. OK to merge? |
|
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 | todo Add 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.
Shouldn't you use the injected parser instead of performing a static call?
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 encoders Docs for symfony/symfony#19197 and symfony/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 YamlEncoder Document the CsvEncoder and the YamlEncoder introduced in symfony/symfony#19197 and symfony/symfony#19326. Commits ------- 6944ea1 [Serializer] Document the CsvEncoder and the YamlEncoder
Add YAML support to the Serializer.