Error serializing field added by contrib module

I've just seen this happen on my site as well, but on taxonomy entities.

Debugging into it it looks like it's the Metatags field added by the metatag module.

$field was an instance of MetatagEntityFieldItemList, when that gets normalized it runs through MetatagNormalizer which simply returns an array rather than a FieldNormalizerValueInterface object that's expected.

This could happen with other contrib modules that add their own fields and normalizers too.

I'm not sure if @attheshow's issue is also caused by the metatag module.

This may be an issue in metatags. See: #2903457: Normalizer problem when Metatag is enabled.

That issue doesn't seem related at all?

I think at the very least, jsonapi should be a bit defensive in order to avoid other contrib modules breaking its functionality. Adding a check of the instance of the $output variable would stop this kind of thing breaking jsonapi entirely.

That issue doesn't seem related at all?

Not sure what are you basing that on. It is a known issue that Metatags normalizer is too greedy and tries to do its thing when it should not.

Adding a check of the instance of the $output variable would stop this kind of thing breaking jsonapi entirely.

That's a good approach. I like it! Would you care providing a patch for review?

Thanks!

This fixes the issue for me, I have looked into the tests and not too sure where this sort of thing would fit in. Happy to give the tests a go if you can point me at the right place :)

Sounds like this is another problem caused by #2852860: REST: top priorities for Drupal 8.4.x.

That was supposed to be #2860350: Document why JSON API only supports @DataType-level normalizers.

#3 linked to an issue without adding it as a related issue. Fixing.

Thanks @acbramley. I committed the improvement!

This still needs test coverage?

@Wim Leers yeah it does, afaict we'd need to implement a test module that implemented a field normalizer similar to what metatag does?

@acbramley, I had same error in EntityNormalizer.php. I applied your patch to Drupal Thunder distribution. Now I get different error - Call to undefined method Drupal\Core\StringTranslation\TranslatableMarkup::getInclude() in C:\DrupalThunder\modules\jsonapi\src\Normalizer\Value\FieldNormalizerValue.php on line 53

@spoorthy01 which EntityNormalizer is that happening on? It looks like jsonapi needs to be defensive there as well since it is expecting the array $values to be an array of FieldItemNormalizerValue objects.

@acbramley Initially the error was in modules\jsonapi\src\Normalizer\EntityNormalizer.php. The error was "Call to a member function setPropertyType() on a non-object". After updating JSON API module to 8.x-1.2, a new error in modules\jsonapi\src\Normalizer\Value\FieldNormalizerValue.php on line 53 has occured. The error says "Call to undefined method Drupal\Core\StringTranslation\TranslatableMarkup::getInclude()".

Hi, I have the same error as @spoorthy01. with the same scenario:
- version 1.1 -> "setPropertyType() on array" error
- version 1.2 -> "undefined method TranslatableMarkup::getInclude()" error

@spoorthy01 @acbramley I found that this problem was when using together with Metatag module and this patch helped me as temporary fix - https://www.drupal.org/node/2636852#comment-12247973. I'm using jsonapi 1.3 and current metatag dev.

I'm running into the same issue after adding a Normalizer to the contrib Serial module. I get:

Error: Call to a member function getInclude() on array in Drupal\jsonapi\Normalizer\Value\FieldNormalizerValue->Drupal\jsonapi\Normalizer\Value\{closure}() (line 53 of /web/modules/drupal/jsonapi/src/Normalizer/Value/FieldNormalizerValue.php) #0

This patch appears to resolve the issue I documented in #21.

I made some minor modifications on commit.

Are we okay with this going in without test coverage?

Are we okay with this going in without test coverage?

I'm leaning towards, yes. Even though it's obviously preferable to have test coverage there are several abandoned patches waiting for test coverage. I don't want the patches that are simpler to follow that fate.

I'm OK leaving this as needs work to add more tests, but I'd rather not halt all patches on this premise (some time that is ok though).

#26: I understand that, and it's fine for a contrib module. But unfortunately, this does mean it continues to get harder and harder to get this module into core. Comments like #26 make core committers very reluctant to add this to core…

@Wim Leers agreed. Whenever we pull this into core, we'll need to harden test coverage. I was trying to say that I'm OK having some issues merged with a test follow-up. In my experience, asking for tests tends to halt progress on the issue making it stale (although not always).

So, I think we are on the same page?

I was trying to say that I'm OK having some issues merged with a test follow-up

That's works in an ideal world.

In my experience, asking for tests tends to halt progress on the issue making it stale (although not always).

That's true.

There are consequences to this pragmatism: the necessary regression tests are never written, allowing the same regressions (or related ones) to be introduced later.

We're kind of on the same page, but not quite:

1. I think it was okay to apply this pragmatism while JSON API was a contrib module trying to be a contrib module.
2. I think it's no longer okay if JSON API is a contrib module trying to get into core.

I think it's no longer okay if JSON API is a contrib module trying to get into core.

Won't this be covered by the comprehensive test coverage epic?

That's true.

There are consequences to this pragmatism

Leaving it broken/uncommitted because there are no tests is not ideal. Having no tests to prevent regressions is not ideal. What can be a good balance here?

Won't this be covered by the comprehensive test coverage epic?

#2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only will add integration tests for every entity type, to verify you can do CRUD on all entity types, and will help ensure no BC breaks in the normalization. But it's not going to test every minute detail. Maybe it'll catch this, maybe it won't. I think it's unlikely.

Leaving it broken/uncommitted because there are no tests is not ideal. Having no tests to prevent regressions is not ideal. What can be a good balance here?

Indeed! It's a hard line to walk.
I think that a simple way to deal with this can be: if we want to commit a bugfix without test coverage, then a follow-up issue to add test coverage must be created before committing/marking "fixed", otherwise it's too easy to forget

@e0ipso I finally got to test this, and it looks like you made a change to my patch which breaks my patch. Instead of returning the value, it's returning NULL. When I reinstate my change, I start seeing the value again.

public function rasterizeValue() {
    if (empty($this->values)) {
      return NULL;
    }

    if ($this->cardinality == 1) {
      // Should not return NULL.
      return $this->values[0] instanceof FieldItemNormalizerValue
        ? $this->values[0]->rasterizeValue() : $this->values[0];
    }

    return array_map(function ($value) {
      return $value instanceof FieldItemNormalizerValue ? $value->rasterizeValue() : NULL;
    }, $this->values);
  }

@Wim Leers I'm happy to provide test coverage for this, but I could use some guidance as to how to provide a situation where there's a new field type with a custom normalizer. Any suggestions?

Kicking off tests.

@ebeyrent it seems that the patch you provided cannot be applied anymore. Can you do a re-roll?

Writing tests for this is pretty much impossible at this point, because several issues have been committed that fix bugs in this area. For example #2939800: FieldNormalizerValue::rasterizeValue() respects cardinality === 1, but doesn't verify that this is true and #2940342: Cacheability metadata on an entity fields' properties is lost.

Plus, we have the #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only integration test coverage plus follow-ups.

Closing as a duplicate.

This issue is still not fixed.

I tested the patch, and it indeed does not fix the issue for JSONAPI.

The issue occurs when;
- fetching a node (the serial filed value fetched is showed as an empty array, and when;
- Creating a node (the serial field value is not inserted)

@Houmanvg: please create a new issue with clear steps to reproduce. Thank you 🙏

See https://www.drupal.org/project/serial/issues/2921349