Add YAML support to serialization module

This is really looking great so far.

PS: Do we still need the needs tests tag?

+++ b/core/lib/Drupal/Core/Serialization/SerializerFormatSubscriber.phpundefined
@@ -0,0 +1,37 @@
+   * @param Symfony\Component\HttpKernel\Event\GetResponseEvent $event

+++ b/core/lib/Drupal/Core/Serialization/YamlEncoder.phpundefined
@@ -0,0 +1,100 @@
+   * @var Symfony\Component\Yaml\Dumper
...
+   * @var Symfony\Component\Yaml\Parser

Missing starting "\"

+++ b/core/lib/Drupal/Core/Serialization/SerializerFormatSubscriber.phpundefined
@@ -0,0 +1,37 @@
+    $event->getRequest()->setFormat('yaml', array('application/x-yaml', 'text/yaml'));

Maybe we should explain why application/x-yaml or text/yaml is used, I guess because of http://stackoverflow.com/questions/332129/yaml-mime-type

+++ b/core/lib/Drupal/Core/Serialization/YamlEncoder.phpundefined
@@ -0,0 +1,100 @@
+      // Set Yaml\Dumper's default indentation for nested nodes/collections to
+      // 2 spaces for consistency with Drupal coding standards.

Hey that comment is stolen :)

+++ b/core/lib/Drupal/Core/Serialization/YamlEncoder.phpundefined
@@ -75,8 +75,6 @@ public function supportsDecoding($format) {
-      // Set Yaml\Dumper's default indentation for nested nodes/collections to
-      // 2 spaces for consistency with Drupal coding standards.

Oh to be honest I didn't wanted it to be dropped, as that's helpful for everyone reading the code.

Is there a use case for this in core?

The REST and CMI teams already concluded that since CMI files are best deployed via Git, not via REST, so we're not supporting ConfigEntities. That means this wouldn't be useful for doing configuration deployments.

If there's a core-friendly use case here we can move forward. If not, I think this would be better off in contrib where we can sort out the mime type issue and other questions at our leisure.

well, maybe the question to consider is what are the overheads of having many serialization formats in core. YAML has an unstable MIME type, ok. JSON and XML are much more mature. Leaving aside the MIME type discussion, what is the code overhead everytime we add/allow a format? How much code do we need to add/maintain everytime we add a format? I'm assuming we're relying on the upstream serializers like PHP or Symfony, which are available no matter what. We just have to decide whether we allow them or not. What about performance overheads in adding more formats?

Edit: I don't particular care for YAML or XML one way or another, just trying to get the bigger picture here.

To be clear, I am not against being able to serialize to YAML. I'm just looking at it and going "uh, why?", especially since, as Lin notes, core moves more slowly than contrib. Aside from "ticking a box", does YAML serialization being in core buy us anything? If so, then I'm fine with including it.

We should have at minimum one non-JSON format in core to validate that serialization is in fact pluggable; XML already fills that role, though, plus gives a foundation for doing other XML formats. (Atom, SVG, etc. can probably reuse at least some code.)

not speaking on behalf of the team, but just my personal feeling as a member: it seems unlikely the security team would have the time or resources to do a full review on this, especially since the benefits of adding YAML in this instance (serializer) are still unclear/unknown. contrib seems like a good place for such serializer to live for now.

Symfony actually has this capability since mid-January. So this is just postponed on us updating the Yaml component.

http://symfony.com/blog/security-release-symfony-2-0-22-and-2-1-7-released

Reminder: The "Serialization API" handbook page at https://drupal.org/node/1900312 still has an example about "adding YAML serializer", with a "TODO Insert example once YAML patch [this issue here] is finalized" :-)

There is pretty solid use case for having Yaml serializer - deploying content.
Default content module uses Json format which is not as readable as Yaml. Demo content and Yaml content modules have own implementation of Yaml import but not export. I suppose Yaml serializer would help a lot on this.

FYI I've made a module from the patch (many thanks @damiankloip!). It's just a sandbox at the mo. I'll promote it in a few weeks if there's no further discussion - just in case this or Chi's message sparks more dialogue and people end up wanting it in core again :p

https://www.drupal.org/sandbox/andy/2924284

Thanks

To make sure Drupal\Core\Serialization onKernelRequest() event callabck is called just before Symfony\Component\HttpKernel\EventListener\RouterListene onKernelRequest(). Increased the priority for onKernelRequest() event callback from 0 to 40.

Unless yaml mime type is not taken for REST endpoints.

@manojapare thank you for reroll, please add #40 as code comment to keep knowledge of why 40

It needs some codestyle changes as well

1. +++ b/core/lib/Drupal/Core/Serialization/SerializerSubscriber.php
   @@ -0,0 +1,37 @@
   + * @file
   + * Contains \Drupal\Core\Serialization\SerializerSubscriber.
   
   +++ b/core/lib/Drupal/Core/Serialization/YamlEncoder.php
   @@ -0,0 +1,99 @@
   + * @file
   + * Contains \Drupal\Core\Serialization\YamlEncoder.
   

   should be removed

2. +++ b/core/lib/Drupal/Core/Serialization/SerializerSubscriber.php
   @@ -0,0 +1,37 @@
   +  static function getSubscribedEvents() {
   +    $events[KernelEvents::REQUEST][] = ['onKernelRequest', 40];
   

   Needs a code comment about why

3. +++ b/core/lib/Drupal/Core/Serialization/YamlEncoder.php
   @@ -0,0 +1,99 @@
   +  static protected $format = array('yaml');
   

   should be short array syntax

It looks like perhaps we should be taking a different approach since #2445723: Use the $request format instead of the ContentNegotation. landed of altering the DIC at build time to register the format with the middleware, similar to how \Drupal\hal\HalServiceProvider does it?

Rerolled the patch to 9.1.x-dev and taken care suggestions by @andypost.

Sorry my bad, fixed codesniffer list.

1. +++ b/core/modules/serialization/src/Encoder/YamlEncoder.php
   @@ -0,0 +1,89 @@
   +class YamlEncoder extends BaseYamlEncoder implements EncoderInterface, DecoderInterface {
   ...
   +  protected $baseEncoder;
   ...
   +  public function getBaseEncoder() {
   
   +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   +   * @param \Symfony\Component\HttpKernel\Event\GetResponseEvent $event
   

   Why do we need to extend the same encoder we are wrapping?

2. +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   + * Event subscriber for adding additional content types to the request.
   

   It does more than just register formats currently.

3. +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   +   *   The Event to process.
   

   s/Event/event

4. +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   +   * Register content type formats on the request object.
   

   Copy error.

5. +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   +   *   The Event to process.
   

   s/Event/event

6. +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   +      $event->setResponse(new Response('Yaml Parse Error', 400));
   

   This message is not really that helpful. Also missing test coverage.

7. +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   +   * Implements \Symfony\Component\EventDispatcher\EventSubscriberInterface::getSubscribedEvents().
   

   Here we can use {@inheritdoc}

8. +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   +    // To process REST POST endpoints with yaml mime type
   

   s/yaml/YAML
   s/callabck/callback

9. +++ b/core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php
   @@ -0,0 +1,51 @@
   +    // Symfony\Component\HttpKernel\EventListener\RouterListene::onKernelRequest().
   

   This line exceeds 80 characters.

Hi here is the updated testcase for patch 45 uploaded in #46

Hi @johnwebdev

I'd not get much about #1 and #6.

The attached patch has the suggested improvements and the interdiff of #49 and #50.

This patch contains the fixes for the 2 failures in #49 and #50

Unassigning this.

Adjusting title since this is a bit confusing when searching for other issues relating to yaml serialization, e.g. for parsing config. This is more for request/response serialization akin to JSON and XML.

Re-rolled #53

Tried to fix the custom error. Please have a look.

Resolved Custom Commands errors & also added an interdiff.

Fixed failed tests of patch #64.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Added patch against #66 in 10.1 version

Setting as need review for tests to run.

Seems changes made to core/modules/serialization/src/EventSubscriber/SerializerSubscriber.php in #66 are missing in #71.

Raised MR based on #66 against branch 10.1.x

@Akhil The previous patch is 8.41 KB. How can a re-roll be 3.97 KB?

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This could use an issue summary update. Example What is the proposed solution exactly?