Mark serializer encoders @internal

I guess we could do the same for every normalizer as well?

This seems like sensible hardening. The question @dawehner asked is also the first thing I thought, but this seems like a sensible first step in any case?

Yes, I think if we're going to do this, we should do it for all classes. Encoders, and normalizers. They are all in the same boat really, they are just components making up a working serializer service.

But normalizers are kind of an API — there are contrib modules providing normalizers that extend core's normalizers. So marking them @internal would be a BC break?

Hmm, true. I guess we would then be allow ourselves to change them at any time, leaving anyone extending them high and dry. So I guess it's a BC break waiting to happen, rather than a BC break right now.

Another option would be to make them private services in the container?

Another option would be to make them private services in the container?

I like this!

Scenario: Drupal\custom\Normalizer\EntityReferenceItemNormalizer extends from Drupal\hal\Normalizer\EntityReferenceItemNormalizer which extends from Drupal\serialization\Normalizer\FieldItemNormalizer.

In the custom Normalizer, I find a reason to suppress the Hal normalizer for a specific field. Since there is no structure such as grandparent::normalize(), I directly call the relevant normalizer as a service. My alternatives were:

• Call the normalizer directly
• Duplicate the code of the grandparent normalizer
• Somehow cast the field as a more basic field type and call the normalizer on it

I realize this is a pretty advanced use case, but I found challenges similar to this (but generally avoidable) when operating in the space of creating my own normalization layer on top of an existing format so I could own the final output (customized to purpose, versioned by media type, etc)

@Grayside: thanks for that input. It's certainly helpful. IMHO you're still relying on implementation details, including any potential bugs. So it's then up to you to have the necessary test coverage to ensure that you are notified of any upstream changes/regressions. Furthermore, you could've seen that there is no unit test coverage for either of the two classes whose behaviors you're extending, which is a strong signal that you probably shouldn't do that.

HOWEVER that being said, I agree with you, which is why I wrote #10:

But normalizers are kind of an API — there are contrib modules providing normalizers that extend core's normalizers. So marking them @internal would be a BC break?

In other words: we need to be very prudent. This is why I proposed to only make this about encoders, not about normalizers. Your example would be wholly unaffected then.

On top of that, the proposal that @damiankloip posted in #11 would also not affect you, since that's solely about ability to access services via the container, not about ability to extend classes!

Thoughts?

So what if we automatically mark every normalizer and every encoder as a private service?

@Wim Leers #14: I didn't just extend the classes, in order to access the normalization routine I called \Drupal::service({normalizer-identifier})->normalize($data);. The need for this is largely a matter of code reuse to ensure consistency in an unusual situation.

I'm raising this not necessarily in support of what I did, but rather to illustrate these kinds of customization requirements. If I kept with a similarly expedient approach to that problem without direct access to abuse the normalizer service, my answer would have been to copy and paste code from core.

What's done here has implications for changes in support of individual site customization.

For a second I was wondering whether we could flag normalizer services as deprecated, but I'm not sure whether this might actually cause failures when they are used valid in the serializer service, see https://symfony.com/doc/current/service_container/alias_private.html#dep...

#16

I didn't just extend the classes […]

Ohhh… I didn't realize that when I read your previous comment. Thanks for clarifying :)

I'm raising this not necessarily in support of what I did, but rather to illustrate these kinds of customization requirements. If I kept with a similarly expedient approach to that problem without direct access to abuse the normalizer service, my answer would have been to copy and paste code from core.

Why were you unable to just extend the existing classes? Because you were overriding those too? If that's the case, then why did you not inject those existing services? That'd still allow you to use private services just fine. And that's always been the recommended approach.

#17: interesting! But in this case, the services are not actually deprecated — what if we really deprecate them in the future? Then we won't have a way to signal that. Using normalizer service injection like I described above is a valid use. Thoughts?

If that's the case, then why did you not inject those existing services? That'd still allow you to use private services just fine.

It never occurred to me to inject one normalizer service into another, perhaps because of "parts of a whole" that normalizers represent. It would definitely have worked to inject the other normalizer and invoke it.

#17: interesting! But in this case, the services are not actually deprecated — what if we really deprecate them in the future? Then we won't have a way to signal that. Using normalizer service injection like I described above is a valid use. Thoughts?

Well, it woul be nice if the direct accessing could be marked as deprecated but the actual service would still be deprecated.

Firstly, I do like the approach we are taking here, making them private. As they really are components of a serializer. I definitely hear the issues of Grayside though, I have seen this kind of usage before. I would always say that specific normalizers should not be called from normalize() methods though, just a recursive normalize() call on the serializer. If we need better ways to reuse some pieces of code, we should add helpers or traits for that. A custom normalizer could then always trump the default one.

It's a very blurry line as to what we can/should change regarding what we call an API here.... :/

Added some changes to the test coverage for Wim's patch.

Maybe its better to move the private part into its own issue, given it probably requires some interaction from one of the release managers.

#21: Good test coverage hardening, thank you! 👏 I should've thought of that :)

#22: So I think you're proposing to keep only the "mark encoders @internal" part of the patch, and move all other stuff (i.e. marking services private) to a follow-up?

I agree with #22, just the @internal updates are a lot easier to understand and getting those in should be trivial to get in compared to marking the services private.

#22: So I think you're proposing to keep only the "mark encoders @internal" part of the patch, and move all other stuff (i.e. marking services private) to a follow-up?

Yeah exactly. Ideally we would first throw them to be deprecated and then make them private

WFM

Thank you @Wim Leers!

Awesome, let's open a followup for the rest of the work.

Follow-up created: #2920536: Force all serializer encoders + normalizers services to be private. Now working on migrating relevant information + patch.

Given the debate so far on the issue, can we have @Grayside confirm that he does not use JsonEncoder and XmlEncoder already? Because if he does, or if that seems the way to solve certain problems in contrib/custom code, then even marking them @internal is a BC break.

It also means this probably needs a change record.

Thanks!

These never were APIs. They implement Symfony's "Serializer component" API.

The whole reason this exists is because some people ended up using these non-API services as if they were APIs (see #2910549: XML serialization module internally broken), this issue communicates the non-API-ness more explicitly by marking them @internal.

@Grayside is only referring to normalizers, not encoders. Normalizers aren't touched here. We'll want Grayside to weigh in over at #2920536: Force all serializer encoders + normalizers services to be private.

Thanks for the feedback. Let's settle down with the em tags, though.

Even if these not-APIs were not intended to be used as APIs, the fact that they were used as if they were such means that we should at least add a change record indicating that they are internal and that instead of using them directly the developer should [insert what to do here].

I do have a custom JSON Encoder written against Drupal 8.1 that extends Symfony\Component\Serializer\Encoder\JsonEncoder to the same purpose as the HAL module: to associate the encoder with a custom format. This is in private site code, I do not have any encoders defined in Schemata project, though now that I'm looking for it, I don't know why I don't. As long as encoders can be extended to support new serialization formats, I see no reason for them to be callable as APIs.

I have seen a developer use the encoder from csv_serialization module as a way to decode a CSV file because working with the rest of serialization for the custom business logic seemed like added time overhead. I'd say that's likely more a documentation/DX issue with normalization than a reason to block this issue.

I will follow-up on private normalizers in the other issue.

Thanks @Grayside!

Still need that CR though.

Thanks, @Grayside!

CR created: https://www.drupal.org/node/2921232.

Committed to 8.5.x and published the change record. Thanks!