Remove JSON API's ScalarNormalizer: it's no longer necessary thanks to #2751325's PrimitiveDataNormalizer

There's no unit test for this class, but apparently there's some entities that do use ScalarNormalizer. I couldn't get things to fail with article nodes or taxonomy terms though.

It looks like at least all config entity normalizations are using this, this fatal error shows up multiple times in the failed test output:

PHPUnit_Framework_Exception: PHP Fatal error:  Call to a member function setPropertyType() on a non-object in /var/www/html/modules/contrib/jsonapi/src/Normalizer/ConfigEntityNormalizer.php on line 58

Thanks for investigating that!

I do wonder if ConfigEntityNormalizer will end up being the only place affected by this. Because for content entities, everything automatically gets wrapped in a FieldNormalizerValue (see \Drupal\jsonapi\Normalizer\FieldNormalizer::normalize() + \Drupal\jsonapi\Normalizer\FieldNormalizer::normalizeFieldItems()).

The root cause of this bug seems to be that \Drupal\jsonapi\Normalizer\ConfigEntityNormalizer::serializeField() blindly assumes that either it'll get back an array (in which case it does this wrapping too, just like for content entities above), or it already is a FieldNormalizerValue. That should have an assertion at the very least.

Interestingly, ConfigEntityNormalizer::serializeField() does do something like this for array values, since #2795109: [BUGFIX] Support configuration entities with array values.

More interestingly, ConfigEntityNormalizer::serializeField() is calling $this->serializer->normalize() on every field. Despite it being a fact that it always contains only scalar data, thanks to \Drupal\jsonapi\Normalizer\ConfigEntityNormalizer::getFields(). That's how #2733097: [FEATURE] Add GET support for configuration entities introduced this normalizer in the first place.

Which means that AFAICT we can fix this by hugely simplifying ConfigEntityNormalizer. It needs to do only two things:

1. check if the result from $this->serializer->normalize() is not an array: if it is not, turn it into an array, because FieldItemNormalizerValue() requires an array to be passed
2. unconditionally wrap the now-guaranteed-to-be-an-array $output in FieldItemNormalizerValue() and FieldNormalizerValue()

This passes JsonApiFunctionalTest.

Interestingly, this also affects the set of cache tags that are asserted in JsonApiDocumentTopLevelNormalizerTest. I've not yet been able to determine the root cause for that. That'll need further analysis.

And d.o ate my files in #8. 😩😵

And actually, we should be able to remove that $this->serializer->normalize() call altogether!

Oh, interesting! #9 is failing because of the changes that I mentioned at the bottom of #8 that I did not yet understand:

+++ b/tests/src/Kernel/Normalizer/JsonApiDocumentTopLevelNormalizerTest.php
@@ -264,7 +264,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
-      ['file:1', 'node:1', 'taxonomy_term:1', 'taxonomy_term:2'],
+      ['file:1', 'node:1', 'taxonomy_term:1', 'taxonomy_term:2', 'user:1'],

@@ -362,7 +362,7 @@ class JsonApiDocumentTopLevelNormalizerTest extends JsonapiKernelTestBase {
-      ['node:1', 'taxonomy_term:1', 'taxonomy_term:2'],
+      ['node:1', 'taxonomy_term:1', 'taxonomy_term:2', 'user:1'],

… so when the tests run on d.o, these changes are not actually necessary. This is … strange. Reverting those changes for now.

I think we can remove this altogether now.

👌 Done!

@Wim Leers, I'm not clear on the caching issue, is that something that still needs to be fixed?

If you apply the patch locally and run JsonApiDocumentTopLevelNormalizerTest, does it pass?

Oh, and it was #2751325: All serialized values are strings, should be integers/booleans when appropriate that added \Drupal\serialization\Normalizer\PrimitiveDataNormalizer/serializer.normalizer.primitive_data, which was in Drupal 8.3.0.

So this change means that the JSON API module now requires Drupal 8.3.0 at minimum. Which is fine, because 8.4.x is the only supported version anyway, and 8.2.x is absolutely unsupported.

Yep.

Then it's a problem on my machine. Interesting. Then the cacheability portion is done too.

I am not sure that unsupported versions grant for breaking BC, is there any reason to remove this instead of just marking it deprecated as core does?

First: There is zero functional change. Normalizers are an implementation detail, not an API.

Second: Drupal 8.2 is definitely unsupported. In fact, 8.3 is also unsupported, because 8.4 is out. See https://www.drupal.org/core/release-cycle-overview.

Third: we're not removing an API, we're removing an implementation detail that never was an API. Core does this too.

Drupal 8, specially in the API area is very much moving target with still big changes in the works such as #2543726: Make $term->parent behave like any other entity reference field, to fix REST and Migrate support and de-customize its Views integration that might force some sites to lag behind.

This is why issues like #2929932: Work around core's ill-designed @FieldType-level TimestampItemNormalizer normalization until #2926508 lands exist: to port fixes of the next Drupal 8 minor to the current JSON API release, so that sites using JSON API can benefit from it now rather than in the future.

Moving back to Needs review, I want @e0ipso to RTBC this :)

Would that be a functional change for 8.2?

Yes.

Like I wrote in #16:

So this change means that the JSON API module now requires Drupal 8.3.0 at minimum. Which is fine, because 8.4.x is the only supported version anyway, and 8.2.x is absolutely unsupported.

I think that's because we acknowledge that people can either get stuck in older versions of Drupal and/or their custom code may be.

I think you're confusing functionality backwards compatibility (in this case: the HTTP responses generated by the module are the functionality) with API backwards compatibility (PHP APIs).

Drupal 8 will keep API backwards compatibility until 9.0.0. It does refactor existing code away to use new APIs, as long as it keeps functionality backwards compatibility.

This issue is keeping backwards compatibility (both API and functionality). It only requires you to be on the up-to-date on Drupal core minor to keep functionality BC. (Necessary to be secure.) This never even was an API, so that BC can't be broken.

Finally: if you're not going to update Drupal core, then why would you update JSON API?

I'm RTBCing this patch, but I will create jsonapi-8.x-2.x because of it. I think that's the best course of action. I will not tag a release just yet so we can identify other breaking changes that we want to introduce and craft some upgrade notes.

Before you do that: are you going to do this for every next minor too? Because this will occur again in the future.

Just discussed this with @e0ipso. He tended to agree with #19 in the end, but I proposed something different still: keep ScalarNormalizer, but only add it when PrimitiveDataNormalizer is not present. That would mean 8.3, 8.4, 8.5 etc would use core's PrimitiveDataNormalizer, and 8.2 would use JSON API's ScalarNormalizer.

Patch soon.

This should work on 8.2, 8.3, 8.4 and 8.5. But \Drupal\Tests\jsonapi\Unit\Normalizer\ConfigEntityNormalizerTest::testNormalize() should fail on 8.2.

Ehm … so I wanted to queue an 8.2 test for #26, but I can't, because d.o only allows testing against the current minor (8.4), the previous minor (8.3) and the next minor (8.5).

This should make \Drupal\Tests\jsonapi\Unit\Normalizer\ConfigEntityNormalizerTest::testNormalize() pass on 8.2. (I didn't test it, because I don't want to change my entire development environment.)

Finally, these are the deprecation indications that are necessary.

Correct patch for #29.

So now it's up to @e0ipso to decide.

• Either we keep 8.2 support and we commit the patch in #30 now, which includes conditional BC layers, which the attached patch will eventually need to remove.
• Or we remove 8.2 support and we commit the patch in #16.

So … the problem I pointed out in #13… I figured out why I was the only one able to reproduce this locally.

Because that only fails against 8.5, and both DrupalCI and @gabesullice have been testing with 8.4. And JSON API's tests haven't been running against 8.5: #2931831: Tests not running against Drupal 8.5 anymore.

Created #2931844: Make JsonApiDocumentTopLevelNormalizerTest pass on Drupal 8.5 to fix that.

Thanks for humoring me and providing options 💆🏽‍♂️

Of course! I do want you to feel comfortable. And the pace of development and feedback is 10 times faster here in the JSON API contrib module than in core, so then I definitely don't mind creating alternative solution patches so you can pick the one you feel most comfortable with :)

It's so great to collaborate with you, and to make decisions based based on carefully evaluated options! ❤️👍

#2931844: Make JsonApiDocumentTopLevelNormalizerTest pass on Drupal 8.5 landed, so this can move forward now.

@e0ipso made a decision in #34, but I still want to leave it up to him to commit this, in case he changes his mind.

Reuploaded the patch from #16, testing against both 8.4 and 8.5. Should come back green now.

Yay!