We need a way to retrieve all serialization formats that are currently available in the system. Example: REST module wants to expose configuration options which formats are allowed on which route.

How can we achieve this in a clean way? JsonldBundle registers the encoders with a tag "encoder". First attempt:

$container = drupal_container();
$encoders = $container->findTaggedServiceIds('encoder');

That fails:

Fatal error: Call to undefined method service_container_prod_::findTaggedServiceIds()

So the findTaggedServiceIds() method is not available at runtime. Meh.

Do we want a CompilerPass that can execute findTaggedServiceIds() and stores the encoder somewhere?

This looks like another typical Drupal use case that is not supported by Symfony at all to me. Serialization formats are not statically defined and we need a list of what is available.

Comments

klausi’s picture

Issue tags: +WSCCI

forgot the WSCCI tag.

Crell’s picture

Issue tags: -WSCCI

Idea we discussed today on the REST Team call:

1) In the compiler pass, build up a list of available formats and stick it in a container parameter. Then we can just grab that list from the container (and can do it via injection, too, rather than needing the actual container passed to us).

2) Make each format its own serializer. So we register a serializer.jsonld, a serializer.atom, a serializer.svg, and whatever other formats. Then you can enable or disable different serializers for different paths very easily.

Crell’s picture

DAMNIT FIREFOX!

1) In the compiler pass, build a list of formats and stick it into a container parameter. Then we can access that list via the container easily. We can even have it injected.

2) Register each format as a separate service. So we get serializer.jsonld, serializer.atom, serializer.svg, etc. That makes it easier to wire up different serialization formats for different paths based on user configuration.

Crell’s picture

Issue tags: +WSCCI

DAMNIT DRUPAL!

kylebrowning’s picture

Is this as easy as grabbing the encoders from this line of code and storing them in the container, then building a form off that for configuration options?

<?php
    foreach ($container->findTaggedServiceIds('encoder') as $id => $attributes) {
      $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0;
      $encoders[$priority][] = new Reference($id);
    }
?>

(code taken from http://drupalcode.org/project/drupal.git/blob/9e28980b6f5e1ca6273cd93a7f...)

Crell’s picture

Almost. You wouldn't build an array of Reference objects but of either the service IDs or the name of the format, or maybe an associative array of that, or whatever it is klausi says he needs. But either way it would be a PHP primitive structure, no objects. Then you'd assign that to the container with $container->setProperty().

damiankloip’s picture

Just referencing #1819760: Add a REST export display plugin and serializer integration. here, I think this could be useful for that.

klausi’s picture

Status: Active » Needs review
StatusFileSize
new1.76 KB

First attempt: Added a format tag and container parameter.

You can now read all available formats with

dpm(drupal_container()->getParameter('serializer.formats'));

The tag is currently used on the encoder stuff of jsonld module, which is probably wrong.

damiankloip’s picture

Should this patch also include the regular JSON/AJAX stuff that got in too? #1846000: Add serializer support for JSON and AJAX

klausi’s picture

Issue tags: +Stalking Crell

Yes, I think so. I'm still not sure if jsonld module should really provide its own two serializer services, or if just tagging the encoders is good enough.

Anonymous’s picture

It would be nice if the Encoder could be aware of this tag itself. For example, if we could add supported formats to Symfony's JsonEncoder by tagging, then we wouldn't need to have the JsonldEncoder. However, I don't think that's likely.

This is a simple solution. While I was initially concerned that there's a slight conceptual mismatch, I'm having a hard time figuring out the corner cases to demonstrate a mismatch. At least for now, I think this is a reasonable solution.

Once it includes the JSON/AJAX stuff, I think we can RTBC this.

Anonymous’s picture

I'm noticing that in your code, you use 'format' as the tag. We should probably make this more descriptive, since format is a widely used word.

damiankloip’s picture

StatusFileSize
new2.55 KB
new2.83 KB

Changed the tag to format_info and added one for the json encoder.

Crell’s picture

format_info is still too generic. That could mean any number of things. The tag should probably be namespaced. (Are dots legal in tags?)

That said, why do we need a separate tag at all? Why can't it just be attributes on the encoder tag?

damiankloip’s picture

StatusFileSize
new2.54 KB
new2.84 KB

Yeah, that makes alot of sense, and encoder is exactly what it is.

Status: Needs review » Needs work
Issue tags: -WSCCI, -json-ld, -Stalking Crell, -serialization

The last submitted patch, 1850704-15.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

#15: 1850704-15.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +json-ld, +Stalking Crell, +serialization

The last submitted patch, 1850704-15.patch, failed testing.

klausi’s picture

+++ b/core/modules/jsonld/lib/Drupal/jsonld/JsonldBundle.php
@@ -63,7 +63,13 @@ public function build(ContainerBuilder $container) {
     // Add Encoders to service container.
     foreach ($encoders as $format => $encoder_class) {
       $container->register("serializer.encoder.{$format}", $encoder_class)
-        ->addTag('encoder', array('priority' => $priority));
+        ->addTag('encoder', array(
+          'priority' => $priority,
+          'format' => array(
+            'jsonld' => 'JSON-LD',
+            'drupal_jsonld' => 'Drupal JSON-LD',
+          ),

I think that is wrong, jsonld should only be added to serailizer.encoder.jsonld and drupal_jsonld to serializer.encoder.drupal_jsonld.

klausi’s picture

Status: Needs work » Needs review
StatusFileSize
new806 bytes
new2.84 KB

Forget my comment, we only have one encoder anyway in jsonld module. Should be fine.

I found the installation problem: instead of accessing $attributes directly we need to access the first item, because that is a nested array.

Status: Needs review » Needs work

The last submitted patch, serializer-formats-1850704-20.patch, failed testing.

klausi’s picture

Status: Needs work » Needs review
StatusFileSize
new1.12 KB
new3.96 KB

Ok, the serialization test format also needs a correctly formatted tag. Not sure we should add an isset() check if the format attribute actually exists on the encoder tag, but on the other hand every encoder should have it.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

Looks simple enough

Crell’s picture

#22 looks good to me, too.

klausi’s picture

Patch does not apply anymore, rerolled.

The interdiff contains the merge conflict resolution.

Git, did I tell you already today that I love you? I do, deeply.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, serializer-formats-1850704-25.patch, failed testing.

damiankloip’s picture

This fails again because #1854874: Add serializer support for XML got reverted. Could wait for that?

klausi’s picture

Status: Needs work » Postponed

Sure!

klausi’s picture

Status: Postponed » Needs work

Back to needs work.

klausi’s picture

Status: Needs work » Needs review
StatusFileSize
new4.3 KB

Rerolled.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Stalking Crell

Back to RTBC

damiankloip’s picture

Yep, this looks good now. Let's get this in. This will be useful for views related issues.

dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.