[Symfony 6] Add "array|string|int|float|bool|\ArrayObject|null" to all Normalizer classes that implement the method ::normalize()

Most (all?) of these contrib usages are implementations of the interface, not subclasses of core implementations, so I think we should be OK here in 9.x.

Most (all?) of these contrib usages are implementations of the interface, not subclasses of core implementations, so I think we should be OK here in 9.x.

Union types are PHP 8 only, so I think this is D10 only

Bumping this to critical, that's a problem.

Ok, I thought i'd do a first round of research on this :)

Unfortunately it doesn't only wrap arrays, but also other types like array|string|int|float|bool This means we cant just implement ArrayObject and call it a day. Also when looking around it seem core/modules/jsonapi/src/Serializer/Serializer.php has not been changed regarding the return type for ::normalize and ::denormalize which I assume you should in this issue?

I've also had a look from where I remembered talk about the normalizer which was (comment in issue #3014283: Use ResourceTypeFields to statically determine the field normalizer by @gabesullice and subsequently @wimleers (he/him)). Where it seems the whole CacheableNormalization as a return value has been perhaps not the most sound option.

It seems the whole jsonapi normalizing is not really compatible with Symfony, the following doc in the Serializer class in the jsonapi module is a hint also.

/**
 * Overrides the Symfony serializer to cordon off our incompatible normalizers.
....
 */

Which also has an interesting link with the discussion how this came to be 2923779#comment-12407443 .

This is as far as my research got me right now. I will need more time to start to form some possible solutions. It's a bit late here now.

Just pasting a comment by @gabesullice from Slack since it is relevant.

gabesullice 11 hours ago
The CacheableNormalization object has been great IMHO. IIRC, it exposed a couple caching bugs that we were able to fix and it makes it easy to bubble cacheability metadata without weird context gymnastics.

However, it has definitely been difficult to get it to play nicely with Symfony. We are already violating the documented interface. It was only a matter of time before type hints at the language level would break things.

We have gone to extreme lengths to prevent third parties from using the Symfony serialization system to inject custom normalizers into the JSON:API serialization "tree"

With all three of those things in mind, I think we should stop using Symfony serialization stuff instead of trying to become compatible with it.

In that "tree" we could hand off serialization to the Symfony system once we hit the typed data level. Everything "above" that is described by the JSON:API spec and so there's little reason not to hardcode things.

We now have an API for renaming/disabling fields and resource types, which is the only reason I've seen for providing custom normalizers.

tl;dr: providing custom normalizers and different formats is the whole point of using Symfony and jsonapi doesn't need either. (edited)

Maybe an other solution is to ask Symfony if they can change the return type hint to add "object" to the list or change it to "mixed".

I would assume they would want some sort of interface and not a general object. There is actually login the the serializers handling the specific case of ArrayObject (only used to be able to end up with {}). This would mean more handling there.

Although it might be a bit of work, thinking about this a little more yesterday night and this morning in the shower just making sure we adhere to the interface and seeing how much effort it is to make the normalization to the JSON:API member properties not use the Symfony serializer. Not entirely sure of the scope of that path yet though.

Thanks for digging up the history, that's useful.

I think it's worth asking about #10 - although would not bank on them changing it.

From the original discussion, #2923779-19: Normalizers violate NormalizerInterface:

I was not able to remove NormalizerInterface from our normalizers or else I would have to rewrite a lot more of Symfony's code (which does type assertions on their interface)

Diverging further from Symfony's Normalizer in jsonapi would be a way to keep the same implementation, would mean at a minimum forking NormalizerInterface, Normalizer and Serializer, or better just stripping down to what we need to use the JSON:API normalizers.

Wrote this before seeing #12, but that's heading along the same lines.

I was thinking, since PHP 8.0 is a think we could work with https://www.php.net/manual/en/class.stringable.php. Easy to implement (i think), close to the truth and would at least allow us to stay close to the interface?

I also thing Stringable SHOULD be part of the interface of the normalizer @ symfony.

What do you guys think?

This crossed my mind briefly, but I forgot to check which PHP version it was introduced in. If it's viable for us, I think it's worth asking Symfony and a lot more likely to get accepted.

I've proposed it in the linked github issue. But the answer is no. Rationale being:

> shouldn't that interface support Stringable?

No, because casting a stringable object to a string should be the concern of the normalizer, not the encoder.

Pretty hard no it seems

I've been trying to get a hang on how to attack this problem. My end conclusion after swimming around in the code for an good hour is that the comment by @catch in #14, where he suggested we need to fork a few of the classes in jsonapi, is probably the most realistic way to go about it.

Time is a bit limited today so I probably don't have the time to explore the exact implications of that divergence.

Couldn't we get around this by making CacheableNormalization extend \ArrayObject - ala https://3v4l.org/b26UN ?

Or can we ask for a new interface that we can return ie. NormalizationInterface that has a getNormalization method?

In most cases, CacheableNormalization::$normalization is an array, so CacheableNormalization being an ArrayObject would make sense for those cases. In some cases, CacheableNormalization::$normalization is a scalar. That happens when FieldItemNormalizer runs for a single-property field. Not sure if it happens for any other code path. For that case, an ArrayObject implementation wouldn't have sensible behavior. However, if this is just to get around the type hints and if it doesn't get passed to any code outside of jsonapi that will actually invoke any ArrayObject methods, then that might be ok.

It could be a combination where the last one in the chain working on fielditems would not be the cacheable normalization. This would be in line with what gabesullice mentioned where on that level we just return a string.

Although I'm a little scared of triggering some specific code when implementing arrayobject, there was a reason I discarded that idea earlier. But that might've been triggered by a lack ovellf overview.

My hands are itching to try... but time is not very abundant ATM.

(Sent on mobile)

Curious if this causes any tests to fail.

I was just opening my editor, got myself an hour. Will try combined with the patch provided in the issue.

Well you test didn't fail. But it feels so wrong T_T

What does this mean in terms of:

I would assume they would want some sort of interface and not a general object. There is actually login the the serializers handling the specific case of ArrayObject (only used to be able to end up with {}). This would mean more handling there.

from #3232074-13: [Symfony 6] Add "array|string|int|float|bool|\ArrayObject|null" to all Normalizer classes that implement the method ::normalize() ?

If this 'just works' as a workaround, then I think it would be OK to commit and then open a follow-up to re-evaluate the serializer/normalizer stuff in JSON:API.

My English was a bit broken in that sentence. But what I was afraid of is that if the CachableNormalization at some point gets to the Symfony serializer and gets run though normalize there it could end up in a different branch of logic than it does right now (See Serializer:161). This might end up resulting in unexpected behavior.

But looking at the code right now, if that was happening, it would be throwing an exception since it fall through to the bottom of the method. So I guess it is fine for now.

And yeah, a follow-up is needed to clean this up at some point.

If we go this route, we also need to decide, either make it accessible as an array object, or add a comment of some sort to explain it is not to be used as such.

I think we should start by adding a trait that implements all of ArrayObject's methods and throws an exception when any are called.

We can add a followup, and an @todo in the code that links to that followup, to explore whether we then want CacheableNormalization to override any of those methods with an implementation that does something other than throw an exception.

Restoring original issue title and changing component to reflect this issue's scope per #28. The followup issue can be the jsonapi-specific one.

Awesome that #26 passed! NW for #31.

Bit like Drupal\Component\Utility\DeprecatedArray. There is a case to be made that deprecation is actually correct :x Even though it was never implemented.

I think we should start by adding a trait that implements all of ArrayObject's methods and throws an exception when any are called.

Agree

I'll build up the class tomorrow. Suggestions for the name and exception message are welcome.

This will be D10 only because of union types

Contemplating posting this to the Symfony issue. But i'm not yet completely confident in my ability to describe this correctly ;) What do you guys think?

I'd like to add how we use the `normalize()` function in Drupal and why it is as it is. I also have a possible solution that might be usefull.

Drupal uses de serializer component for a few things. One of these things is serializing our entity objects to something that can be returned as json. The entity objects contain fields which can have one or more values, so we start high up and normalize all the way to the bottom of the stack, the field values.

In order to get the combined cache tag information from every level of serialization we've chosen to track that in the normalizer. It knows what it did and we track this in a normalized value object (called CachableNormalization). This means we can consolidate the proper caching dependency tree information of the object without having to do some weird gymnastics to track this information somewhere else.

It would help us loads if we would have some sort of `NormalizationValueInterface` that can be returned by the normalize method. This would enable us to get things working as intented and keep a less complex cache-tracking system in place. An interface like this would work for us quite handily.

namespace Drupal\jsonapi\Normalizer\Value;

/**
 * Use to store normalized data and its cacheability.
 */
interface NormalizationValueInterface {

  /**
   * Gets the decorated normalization.
   *
   * @return array|string|int|float|bool|\ArrayObject|null
   *   The normalization.
   */
  public function getNormalization(): array|string|int|float|bool|\ArrayObject|null;

}

After some slack discussion with @alexpott about this issue i've created a PR for Symfony so we can see if we can fix this upstream by introducing a NormalizedValueInterface.

See symfony/symfony/pull/43498.

That would open up making this abide to the interface and perhaps in the future see if this way of caching makes sense in the other places we use normalization.

I'll build up the class tomorrow. Suggestions for the name and exception message are welcome.

Well, life said nope to this as I needed to go to the doctor with my son. I'm on vacation coming week so small chance i'll find the time to sit behind an editor.

Thank you daffie. Few things, shouldn't we just do @inheritdoc?

Also, the message could be a variable, they are all pretty much the same right. :)

Personally I would probably extend a class instead of using a trait but I think that is mostly personal preference. I don't feel strongly about that.

Ah unfortunate, thanks:)

Looking at the errors, which seem to be errors in how the array object methods are implemented. Most of that is argument type hints it seems.

I think extending a class instead of a trait are a good idea. This would expose all the wrong methods right away instead of when the tests are run.

I've made a patch with this pattern to see how that goes.

Added interdiff

Perhaps we could put it at the same place as DeprecatedArray.

<?php
namespace Drupal\Component\Utility;
/**
 * An array that triggers a deprecation warning when accessed.
 */
class DeprecatedArray extends \ArrayObject {

But we would like the input of a committer (talked to Daffie on Slack)

I think I'd prefer to add this to jsonapi for the moment - since we might come up with a way to do this differently and remove the class altogether again. Once it's in Drupal\Core it's part of the API - we can always move it when we have another use case, but that's easier than having to go through deprecation/removal.

The Drupal PHP 8.1 failures are introduced by this patch so marking needs work for that.

Sure, I'll place it somewhere sane in jsonapi. Also your comment on Drupal 8.1 confused me, but you mean php 8.1 :)

Renamed class to Normalizer/Value/TemporaryArrayObjectThrowingExceptions and fixed a few of the arguments probably. Hopefully it will pass now, i'll wait for that.

Thanks :) Requeueing anyways ;)

Committed 9aa1d53 and pushed to 9.4.x. Thanks!

I don't think this has anything we particularly need in 9.3.x, so leaving against 9.4.x

This has broken HEAD testing on PHP 7.3 for the 9.4.x branch, for dbs other than MySql, I am afraid. https://www.drupal.org/node/3060/qa

Reverted. We should try to resolve the PHP 7.3 issues and re-commit if we can, otherwise this'll end up 10.x-only.

Hmm, that is a bit weird. I'll have a look.

Serialization changed in 7.3 to 7.4 and we need to allow serialization as this is how \Drupal\jsonapi\EventSubscriber\ResourceObjectNormalizationCacher::set() works. ON PHP 7.4 and up it is calling the __serialize() method...

Test are green. I've also checked (just to be sure) if serializing/unserializing this array object gives the proper output on the different php versions. Might not have been needed, but felt like due diligence.

PHP 8

string(99) "O:38:"TemporaryArrayObjectThrowingExceptions":4:{i:0;i:0;i:1;a:0:{}i:2;a:1:{s:4:"test";i:1;}i:3;N;}"
object(TemporaryArrayObjectThrowingExceptions)#2 (2) {
  ["test"]=>
  int(1)
  ["storage":"ArrayObject":private]=>
  array(0) {
  }
}


PHP 7.4

string(99) "O:38:"TemporaryArrayObjectThrowingExceptions":4:{i:0;i:0;i:1;a:0:{}i:2;a:1:{s:4:"test";i:1;}i:3;N;}"
object(TemporaryArrayObjectThrowingExceptions)#2 (2) {
  ["test"]=>
  int(1)
  ["storage":"ArrayObject":private]=>
  array(0) {
  }
}

PHP 7.3.25

string(87) "C:38:"TemporaryArrayObjectThrowingExceptions":36:{x:i:0;a:0:{};m:a:1:{s:4:"test";i:1;}}"
object(TemporaryArrayObjectThrowingExceptions)#2 (2) {
  ["test"]=>
  int(1)
  ["storage":"ArrayObject":private]=>
  array(0) {
  }
}

I used the class from the patch and just added a public property.

Anyways, looks fine.

That's very straightforward to avoid the regression, and don't think we should worry about adding back those exceptions in 10.0.x - better to see what else we can come up with that doesn't involved ArrayObject at all.

Committed 5c78c18 and pushed to 9.4.x. Thanks!