Avoid using the Serialization component for JSON API specific tasks

Turn normalizers from tagged services to a private factory to remove the ability for 3rd parties to inject code this way.

That will be a BC break for any modules that do have custom normalizers. Do we know of anybody who does that?

Design the new normalizers so they can support schema tracking in the future (definitely out of scope).

Agreed it feels out of scope, but OTOH adding it later means another BC break? Hard to balance :(

Let me begin by saying that I'm open to being convinced of anything :). And also, please excuse my judicious use of <hr />!

In #2923779: Normalizers violate NormalizerInterface, we bought ourselves time and freedom by making the entire serialization process internal. With that freedom in mind, I think this issue doesn't really identify what the problem is before stating a solution. Let's take a step back for a moment...

The original issue was "Normalizers violate NormalizerInterface". What was wrong there? For one, we were afraid that we would face pushback moving that into core. We also worried that we would have less freedom to refactor and/or pay down technical debt once we moved into core. Both those concerns have been addressed by the previous issue.

So, I think the issue title we need now is simply: "Restabilize the JSON API serialization system"

The requirements for that issue would be:

• Don't violate NormalizerInterface
• Don't create a higher support burden
• Don't introduce APIs that we're worried about breaking

To a lesser extent, we also want:

• To make it easy to identify regressions
• To make it easy to understand
• Minimize the amount of effort required

With those things out of the way, there are probably a number of approaches we could take here. And I think we should explore them all.

The currently proposed solution is, essentially, to invent and formalize our own serialization system with inspiration from Symfony.

Naturally, this sets off all kinds of 🚨🚨🚨 in my dev brain :P

My preference will almost always be to avoid going off onto our own island. We should find a way to play nicely with the serialization system again. In the famous words of Apocalypse Now, "Never get outta the boat, man!"

Why are we on our own island already? The difficulty is that:

[It's hard] to track both cacheability metadata associated with each normalized item in the tree (entity -> fields -> properties) and JSON API-specific metadata to ensure the right data ends up in the right places in the right place of the document structure

(see #2923779-15: Normalizers violate NormalizerInterface)

It's exactly that in-normalizer state tracking that makes the tracking especially difficult and error-prone. I've thought that for a while: #2917147: chore(Normalizer): Simplification and refactoring

We're doing doing two things in parallel, we're trying to find and arrange all the data we need all while trying to mutate that data into a serializable array. There's little separation of concerns, in fancy terms.

So, rather than formalize and make that system nearly permanent, I'd like to experiment with building the document in our own data structures. From that, we can serialize that data structure in the standard way. Build the document first, then serialize it.

Part of what drives a lot of confusion is that our normalizers are too closely coupled to the idea of serializing entities. We're trying to forcefully mutate and entities into a document. When the spirit of the spec is that a document is a wholly separate thing form the entities (read resources) it contains.

I'd love to see our system resemble something like this:

$document = JsonApiDocument::createFromEntity($entity, ['include' => [/* paths */]]);
return $normalizer->normalize($document, 'api_json', [
  'fields' => [/* fieldsets */],
]);

This is grossly simplified, but I have a proof-of-concept already :)

By building the document before serialization, all the state tracking is much more simple. The document can implement RefinableCacheableDependency to track cacheable metadata as it's built. By marking the document @internal, we can implement regular Symfony normalizers for our own internal class, which means no one can reasonably expect BC support for their own normalizers when the class doesn't have a public API.

Now, of course, there's a lot to hash out there. And I know that there are many fine details that have been worked out during and since the original implementation.

But before "doubling-down" on our own hand-rolled solution, I hope I've convinced you that there is still some exploration to be done and that this issue should be a little more open ended :)

Avoid using the Serialization component for JSON API specific tasks

Rereading this: I actually think this issue title applies to both how @e0ipso was thinking about this and how I'm thinking about this. So, already I'm seeing we're closer than I originally thought :)

1. Introduce a new interface that enforces the new normalizers to return a NormalizerValue based object (like they already do).
2. Turn normalizers from tagged services to a private factory to remove the ability for 3rd parties to inject code this way.
3. Design the new normalizers so they can support schema tracking in the future (definitely out of scope).

1. This is really the thing that I struggle most with.
2. I can see a reflection of my JsonApiDocument::createFromEntity() in this. I just think that it should have nothing to do with serialization.
3. +1!

@e0ipso: I suspect that you have a more detailed picture in your head of the architecture you're proposing than what is described in the IS. I think adding more detail would help make this issue much more actionable, would lead to consensus much faster!

We knew that. In fact there will not be a BC break, since the break happened already in #2923779: Normalizers violate NormalizerInterface.

In absolute terms, you are correct. In relative ones, I agree. Yes, #2923779 broke BC. But any existing module can easily make things work again as before by changing its service tag. In other words: we optimized for minimal disruption, for those who chose to use this API that was never documented to be off-limits until very recently.

But this issue is proposing to introduce new interfaces, with potentially completely different signatures/requirements. No longer minimal disruption. Unless you meant that you want to keep essentially the same interfaces, and just want to change the return value for ::normalize() to be a value object? If so, that's not what the IS suggests :)
I say this despite wishing very much that we wouldn't have to retain BC!

I only know one person that does that.

Great, this is very encouraging!

The use case is an edge case and the majority of the uses of that edge case are covered by computed fields. The remaining will require a custom normalizer.

Wonderful! You've taken my concerns in #4 away :)

It's a feature addition, we should be able to implement it without impacting BC.

It depends how you implement it: is it going to be a required method on the interface that this issue would add? Or is it going to be a new, optional interface? If it's an optional interface, it indeed would not break BC, but it also wouldn't guarantee that we'd be able to generate schemas.
That being said, I do think that blocking this on figuring out the details of schema generation is not a good idea. So what I'd propose is this: mark the interface @internal, meaning that we allow ourselves to refine it further. This signals to implementors (of which there apparently is only one so far!) that it's not yet finalized, and that they should expect to keep watching out for changes until we remove that @internal on the interface.

IOW: I think my comment at #4 sounded like massive disagreement, but that's not actually the case! The picture simply isn't clear enough y et.

#6

• I too am a bit surprised that @e0ipso has jumped on this, I was under the impression that he felt this was more of a low-priority thing for now, something for the fairly distant future. Maybe I misunderstood.
• RE: "resemble something like this" — this alternative picture isn't entirely clear to me yet either, just like @e0ipso's picture isn't clear yet :)

#7: That sounds encouraging!

For one, we were afraid that we would face pushback moving that into core. We also worried that we would have less freedom to refactor and/or pay down technical debt once we moved into core.

Both those concerns have been addressed by the previous issue.

I don't think that is incorrect. This issue exists because we are still doing final class Serializer extends SymfonySerializer

That's right, but we've "cordoned off" the code area that was in violation. It's something that is now an explicit implementation detail. And it'll continue to work fine as long as the Symfony Serialization component doesn't change significantly. What matters is that we've explicitly turned the "areas of violation" into a big black box, one big implementation detail. Which means we're now free to change it. That is what the previous issue was about. Sounds like we all had slightly differing opinions on the intent/goal/scope of that issue :) But still all very close, fortunately!

I think your suggestion is very close to do a full rewrite of the module, which I don't want.

This is a fair point, and a very important one. This goes back to what I said before: that @e0ipso felt fixing this In The Best Way Possible is a low-priority, a nice-to-have, something we shouldn't do right now. I agree with that.

But at the same time, @e0ipso argues in the issue summary for new normalizers, which implies (for me) a new API.

Now I'm starting to suspect that @e0ipso is not seeing this issue as "let's provide an API", I think he sees it more as "we've cordoned it off in #2923779: Normalizers violate NormalizerInterface, only one person is doing this sort of thing (#5), so … let's just make this impossible altogether now ("remove the ability for 3rd parties to…" — from the IS). As in: this is just a next small step. (EDIT: oh, and "All are internal and no BC will be broken."!)

Is that what you actually meant, @e0ipso? For me, the IS reads as if you want to add new APIs (hence increasing the API surface, and hence complicating future supportability because committing to those APIs will mean we have to support those pretty much forever, hence ALARM BELLS). I suspect it does for @gabesullice too. But perhaps that's not what you meant?

TL;DR: I suspect the disagreement here is not real; that there is disagreement only due to ambiguously interpretable communication?

Design the new normalizers so they can support schema tracking in the future. I think we did a pretty good job JSON RPC that could serve as inspiration.

I fear that if we want to achieve this as one of the goals for 2.x, that it'll take months. It'll be designing a new PHP API. And all the existing normalizers (including the @DataType-level normalizers that JSON API can reuse from core/contrib right now) will not be implementing this API.

For that I don't think much needs to happen:

Okay… let's see!

Instead of drawing the candidates from the service container, we have a hardcoded list (or any other similar alternative). No more injection of normalizer candidates.

WFM!

New normalizers can implement a WithSchemaInterface (or similar) so they provide the schema of the generated data, and the schema of the accepted input data. FieldEnhancers already do this and it's working really well in JSON API Extras.

can implement, hence optional? If so, then I'd propose to leave the WithSchemaInterface portion of this proposal for a future patch, and only do the first part.

I think we agree to:

1. Stop collecting normalizer candidates from tagged services.
2. Create a new issue to design schema tracking of normalizers.

👍

You just had me scared there for a moment :D

To make sure they implement we should be able to generate schemas from the data model, which we can't without schemata at the moment. We could entertain the idea of having different schema formats. Maybe TypedData could be one, I'm not sure yet. In any case I agree that we can break this part to a separate issue to avoid scope creep.

This still has me scared. This is basically #2822768: [PP-1] Add information about the schema in the json:api resource?

Okay, so #2822768: [PP-1] Add information about the schema in the json:api resource is specifically for exposing schemas in some way (and in the future hopefully the JSON API spec will define a way — but we of course should not wait for that).

But I thought the scope of this issue was meant to be solely Stop collecting normalizer candidates from tagged services. — which is just refactoring our existing logic. Adding support for schema generation seems like a pretty big undertaking, and I'm not sure that needs to be part of this issue's scope? I don't see why we can't stop using the Symfony Serialization component now (at which point all normalization logic will be more explicitly an @internal implementation detail and impossible to mistake for an API), and then in the future change it to add schema generation support?

I think stopping to use the Symfony Serialization component could be done in a matter of hours.

What am I missing? Perhaps you think that just stopping to use the Symfony serialization component is kinda pointless?

That'd be great! And I think each of those steps (1. stop using Symfony Serialization component, 2. start supporting schema generation) delivers a value step forward. It's easier to understand each of them if they're executed separately.

P.S.: you probably have a much better understanding of what it takes to generate schemas. For me, that's still mostly a mysterious black box that seems pretty damn scary because of the high potential complexity. So, feel free to view #17/#18 as let Wim think that he's staying away from scary things that actually aren't that scary 🎃 😅

Interpreting #12 and later, I think this is a first step along what @e0ipso wanted to see happen. The 3 "JSON API query param value object normalizers" are still injected directly, but are renamed to clarify their purpose (needs more work for sure), and are no longer registered.

After #20, I started working on hardcoding the list of normalizers in \Drupal\jsonapi\Serializer\Serializer. But then I saw a much simpler approach, which is also achieving your goal, I think. Please review!

So, how do we feel about committing #22, which is the super-minimal-but-it-gets-the-job-done solution?

We already have an issue for schema support: #2822768: [PP-1] Add information about the schema in the json:api resource — dealing with the normalizer aspect seems logical to be tackling over there too, I think?

This is just immanentizing the eschaton. We can't say we didn't warn anyone: jsonapi_normalizer_do_not_use_removal_imminent

I have no doubt we'll see bug reports about this, but I agree it's the right step to take. Better to break those sites completely on a 2.x than on a 2.1 when we actually start enforcing a different interface and opening normalization back up (with schema).

I brought this up in Monday's API-First call and @e0ipso said he thought #22 seemed to get the job done.

I just re-skimmed the issue and noticed that in #23 I'm basically saying "all things schema related -> #2822768", but @e0ipso indicated in #16 that he considers that issue to be solely concerned with figuring out how to expose it via JSON API.
So we still need a separate issue for "schema support in JSON API's normalizers", which @e0ipso said he was fine with in #18. So, did that: #2994473: [META] JSON API's normalizers support schema tracking, to guarantee comprehensive schema.

🎉

What about jsonapi_extras? There are two services:

  serializer.normalizer.entity.jsonapi_extras:
    class: Drupal\jsonapi_extras\Normalizer\ContentEntityNormalizer
    arguments: ['@jsonapi.link_manager', '@jsonapi.resource_type.repository', '@entity_type.manager']
    tags:
      - { name: jsonapi_normalizer_do_not_use_removal_imminent, priority: 22 }
  serializer.normalizer.config_entity.jsonapi_extras:
    class: Drupal\jsonapi_extras\Normalizer\ConfigEntityNormalizer
    arguments: ['@jsonapi.link_manager', '@jsonapi.resource_type.repository', '@entity_type.manager']
    tags:
      - { name: jsonapi_normalizer_do_not_use_removal_imminent, priority: 22 }

The patch from the issue breaks everything.

Allow using jsonapi_extras normalizers marked by the jsonapi_normalizer_do_not_use_removal_imminent tag.

@Wim Leers, @e0ipso I have 4 custom normalizers. Since they will no longer work, what I have to do with the logic inside of them?

The problem also is that JSON API catches the data before I can handle them by core's normalizer.

@BR0kEN: thank you so much for responding with your concerns only 3 days after we tagged the first 2.x release! Of course the jsonapi_normalizer_do_not_use_removal_imminent tag already warned about the upcoming break. In the 2.x branch, we're making that break.

You say you have 4 custom normalizers. In virtually all cases, you can convert them from a @FieldType-level normalizer to a @DataType-level normalizer. So instead of for example operating on \Drupal\Core\Field\Plugin\Field\FieldType\TimestampItem, you'd operate on \Drupal\Core\TypedData\Plugin\DataType\Timestamp. This has been explicitly documented in JSON API since December 2017: #2860350: Document why JSON API only supports @DataType-level normalizers. It is also the direction that core is going. See https://www.drupal.org/node/2934779.

So, those 4 custom normalizers, can't those be changed to work at the @DataType level? If you can't share the code publicly, I'd love to see it in a private call so that I can help you.

I was using the tag under my own responsibility as well as many-many functionality that marked as @internal in jsonapi. Unfortunately, I had to modify/override. I was expecting jsonapi will change this someday, so that's my problem :)

How to be with the data/field normalizers if they already defined but behave not in a way you want (i.e. I want to change the \Drupal\Core\TypedData\Plugin\DataType\Timestamp)? Override them via the hook? Can I have multiple normalizers per field/data type?

How to be with the data/field normalizers if they already defined but behave not in a way you want

Aha! @DataType-level normalizers you can override! Two possible ways:

1. Decorate. See html_response.attachments_processor.big_pipe in big_pipe.services.yml for an example of Symfony service decoration.
2. Override. Specify a similar normalizer-tagged service with a higher priority — then yours will win!

- But @FieldType and @DataType are both plugin-layer implementations, am I right? I don't know the way to decorate plugins as they not services, is there any?
- Yeah, I can define prioritized normalizer, but problem is that \Drupal\jsonapi\Serializer\Serializer has its own prioritization of internal (jsonapi-based) normalizers that are always win indenpendntly on a priority I set to my own, non jsonapi normalizer.

Example:

    if ($this->selfSupportsNormalization($data, $format)) {
      return parent::normalize($data, $format, $context);
    }
    if ($this->fallbackNormalizer->supportsNormalization($data, $format)) {
      return $this->fallbackNormalizer->normalize($data, $format, $context);
    }
    return parent::normalize($data, $format, $context);

I'm confused.

In #34 you say:

How to be with the data/field normalizers if they already defined but behave not in a way you want (i.e. I want to change the \Drupal\Core\TypedData\Plugin\DataType\Timestamp)? Override them via the hook? Can I have multiple normalizers per field/data type?

But in #36 you say:

- But @FieldType and @DataType are both plugin-layer implementations, am I right? I don't know the way to decorate plugins as they not services, is there any?

So, are you modifying the @FieldType or @DataType classes themselves? Or are you providing alternative normalizer services for existing normalizers? I thought it was the latter.

- \Drupal\Core\TypedData\Plugin\DataType\Timestamp is @DataType plugin, not a service. I cannot decorate it. All I can is to override the implementing class via hook_data_type_info_alter(). Same thing for @FieldType, but hook is hook_field_info_alter(). I do not like these hooks at all, but it looks like this is the only way to change the behavior.

- I can define normalizer-tagged services but they simply won't be used because of \Drupal\jsonapi\Serializer\Serializer. My old jsonapi_normalizer_do_not_use_removal_imminent-normalizers are:

  PROJECT.serializer.normalizer.entity_reference_field.jsonapi:
    class: Drupal\PROJECT\Normalizer\EntityReferenceFieldNormalizer
    parent: serializer.normalizer.entity_reference_field.jsonapi
    decorates: serializer.normalizer.entity_reference_field.jsonapi

  PROJECT.serializer.normalizer.field.jsonapi:
    class: Drupal\PROJECT\Normalizer\FieldNormalizer
    parent: serializer.normalizer.field.jsonapi
    decorates: serializer.normalizer.field.jsonapi

  PROJECT.serializer.normalizer.entity.jsonapi_extras:
    class: Drupal\PROJECT\Normalizer\ContentEntityNormalizer
    parent: serializer.normalizer.entity.jsonapi_extras
    decorates: serializer.normalizer.entity.jsonapi_extras

As you see, there are only overrides of jsonapi-based normalizers. Now overrides are forbidden and I can no longer use those. I also cannot move logic from them to the normalizer-tagged service because

  /**
   * {@inheritdoc}
   */
  public function normalize($data, $format = NULL, array $context = []) {
    if ($this->selfSupportsNormalization($data, $format)) {
      return parent::normalize($data, $format, $context);
    }
    if ($this->fallbackNormalizer->supportsNormalization($data, $format)) {
      return $this->fallbackNormalizer->normalize($data, $format, $context);
    }
    return parent::normalize($data, $format, $context);
  }

of \Drupal\jsonapi\Serializer\Serializer simply prioritizes jsonapi-based normalizers.

Hope that brings some clarification.

It's seems like there's been some miscommunication here. That's understandable. This is a hairy topic.

@Wim Leers was not suggesting you decorate the data type class. He was suggesting you decorate the normalizer for that data type. It was a bit confusing, because his example of how to decorate used a service that was not a normalizer. I think this made you think he was talking about decorating the plugin, not the normalizer.

\Drupal\jsonapi\Serializer\Serializer simply prioritizes jsonapi-based normalizers.

You are 85% correct here. The key distinction is that the JSON API serializer is not used at the @DataType level. Or, put another way, it is not used for field item properties. Therefore, normalizers for that data will be respected.

So what is "@DataType level"?

Drupal's data hierarchy goes like this: Entity -> Composed of FieldItemLists -> Composed of FieldItems -> Composed of properties, which are TypedData types.

Your normalizers are implemented on the FieldItemList level. You need to rewrite them so that they apply to the properties of the fields you're dealing with.

This conversation has effectively become a support/feature request. If the above is not enough to set you on the right track, would you mind opening a new "Support request" issue. An example of the default normalization and the normalizations that you would prefer would be helpful in helping us come up with a solution to your specific needs :)

#39++ Thanks Gabe, for explaining it more clearly than I did!

#38: are you really using hook_data_type_info_alter() to override @DataType plugins' classes?! 😮 Why do you need this? You only need to answer this if #39 didn't help you get back on track of course.

@Wim Leers, I don't. I thought that's you proposing me them :)

Needs to try the suggestion from #39. Thanks for the info!

@BR0kEN: oh, damn. I'm so sorry for not explaining it more clearly! 😞 If you get stuck on #39, use my d.o contact form to contact me, and we'll set up a quick call.