Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Needs review » Needs work

The last submitted patch, 2: 3032451-2.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.97 KB
Wim Leers’s picture

Title: JSON:API 2.2 compatibility: JSON:API Extras 3.4 » Compatibility with upcoming JSON:API 2.2 release: JSON:API Extras 3.4

Status: Needs review » Needs work

The last submitted patch, 4: 3032451-2.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
8.9 KB
9.61 KB

Status: Needs review » Needs work

The last submitted patch, 7: 3032451-7.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
1) Drupal\Tests\jsonapi_defaults\Functional\JsonApiDefaultsFunctionalTest::testRead
mkdir(): File exists

Random fail … 🥺

Wim Leers’s picture

Wim Leers’s picture

gabesullice’s picture

#3015438: Wrap entity objects in a ResourceObject which carries a ResourceType is not yet committed, but will be soon. This is the interdiff to make this patch compatible with the upstream once it lands.

I was bummed to see that I wasn't able to get this done before #3028493: EntityToJsonApi includes do not honor overridden fields landed. It would have been a nice concrete benefit since this would have fixed it, now it's just a code simplification. 🤷🏼‍♂️

gabesullice’s picture

A quick summarization of some things in #12:

  1. +++ b/jsonapi_extras.services.yml
    @@ -16,16 +16,22 @@ services:
    -  serializer.normalizer.entity.jsonapi_extras:
    -    class: Drupal\jsonapi_extras\Normalizer\ContentEntityNormalizer
    -    parent: serializer.normalizer.entity.jsonapi
    -    calls:
    -      - [setSerializer, ['@jsonapi.serializer']]
    +  serializer.normalizer.resource_object.jsonapi_extras:
    +    class: Drupal\jsonapi_extras\Normalizer\ResourceObjectNormalizer
    +    decorates: serializer.normalizer.resource_object.jsonapi
    +    arguments: ['@serializer.normalizer.resource_object.jsonapi_extras.inner']
    +    tags:
    +      - { name: normalizer, 1022, format: api_json }
    +  serializer.normalizer.content_entity.jsonapi_extras:
    +    class: Drupal\jsonapi_extras\Normalizer\ContentEntityDenormalizer
    +    decorates: serializer.normalizer.content_entity.jsonapi
    +    arguments: ['@serializer.normalizer.content_entity.jsonapi_extras.inner']
    ...
    -    class: Drupal\jsonapi_extras\Normalizer\ConfigEntityNormalizer
    -    parent: serializer.normalizer.config_entity.jsonapi
    +    class: Drupal\jsonapi_extras\Normalizer\ConfigEntityDenormalizer
    +    decorates: serializer.normalizer.config_entity.jsonapi
    +    arguments: ['@serializer.normalizer.config_entity.jsonapi_extras.inner']
    

    After this interdiff is applied, all the entity level normalizers in this patch will use decoration rather than extension to alter JSON:API entity (de)normalization.

    That should make them less prone to breakage and easier to maintain!

  2. +++ b/src/EntityToJsonApi.php
    @@ -126,82 +115,35 @@ class EntityToJsonApi {
    -    $entity_type_id = $entity->getEntityTypeId();
    -    $resource_type = $this->resourceTypeRepository->get(
    -      $entity_type_id,
    -      $entity->bundle()
    -    );
    -    // The overridden resource type implementation of "jsonapi_extras" may
    -    // return a value containing a leading slash. Since this was initial
    -    // behavior we won't going to break the things and ready to tackle both
    -    // cases: with or without a leading slash.
    -    $resource_path = ltrim($resource_type->getPath(), '/');
    -    $path = sprintf(
    -      '%s/%s/%s',
    -      rtrim($this->jsonApiBasePath, '/'),
    -      rtrim($resource_path, '/'),
    -      $entity->uuid()
    -    );
    -    $request = Request::create($this->masterRequest->getUriForPath($path));
    -
    -    // We don't have to filter the "$include" since this will be done later.
    -    // @see JsonApiDocumentTopLevelNormalizer::expandContext()
    -    $request->query->set('include', implode(',', $includes));
    -    $request->attributes->set($entity_type_id, $entity);
    -    $request->attributes->set(Routes::RESOURCE_TYPE_KEY, $resource_type);
    -    $request->attributes->set(Routes::JSON_API_ROUTE_FLAG_KEY, TRUE);
    ...
    -      'resource_type' => $resource_type,
    -      'request' => $request,
    

    All of this is just unnecessary since #2997600: Resolve included resources prior to normalization and #2984911: Remove access to the Request object in the normalization process

  3. +++ b/src/EntityToJsonApi.php
    @@ -126,82 +115,35 @@ class EntityToJsonApi {
    -  private function prepareDocument(EntityInterface $entity, array $includes, array $context) {
    -    /** @var \Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType $resource_type */
    -    $resource_type = $context['resource_type'];
    -    $referenced_entities = [];
    -    foreach ($includes as $public_name) {
    -      // Ensure we are getting the proper field names of includes regardless of
    -      // whether the public or internal name was passed in.
    -      $internal_name = $resource_type->getInternalName($public_name);
    -      $referenced_entities = array_merge(
    -        $referenced_entities,
    -        $entity->get($internal_name)->referencedEntities()
    -      );
    -    }
    -    $entity_collection = new EntityCollection($referenced_entities);
    -    return new JsonApiDocumentTopLevel(
    -      $entity,
    -      $entity_collection,
    -      new LinkCollection([])
    -    );
    +  private function prepareDocument(ResourceObject $resource_object, array $includes) {
    +    $includes = !empty($includes)
    +      ? $this->includeResolver->resolve($resource_object, implode(',', $includes))
    +      : new NullEntityCollection();
    +    return new JsonApiDocumentTopLevel($resource_object, $includes, new LinkCollection([]));
    

    This is what would have fixed #3028493: EntityToJsonApi includes do not honor overridden fields

e0ipso’s picture

Status: Needs review » Needs work

@gabesullice it looks like the interdiff attached does not apply on top of 3032451-11-do-not-test.patch in #11.

The filename makes reference to the patch in #73 for the JSON:API issue. However the committed patch was #94. I suspect we need another iteration in the patch in #11 that contain these changes.

Wim Leers’s picture

Hm, I was being very careful, only committing after it was passing tests here, but apparently I made a mistake. Apologies. Will make sure you’ve got a passing patch later today.

e0ipso’s picture

No need to apologize! It is also very likely that I did something wrong (I was still sleepy).

Wim Leers’s picture

Turns out JSON:API Extras was depending on implementation details that trivial refactors removed, for example #3032818: Clean-up: \Drupal\jsonapi\Encoder\JsonEncoder::encode() should not have to deal with normalization logic. That'd explain why #14. Currently working on updated patch!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.18 KB
40.95 KB
41.67 KB

This patch is relative to #11+#12: the interdiff here is on top of those two.

Wim Leers’s picture

FileSize
6.03 KB

I uploaded #3033584: Follow-up for #3022531: Completely remove JsonApiParamEnhancer's interdiff :| Here is the right one.

Due to recent changes in JSON:API, I noticed that a few kernel tests in JSON:API Extras have been wrong for a very long time. It'd be great to convert them to functional tests to avoid being coupled to implementation details. But doing that is definitely out of scope here.

Status: Needs review » Needs work

The last submitted patch, 18: 3032451-18.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
954 bytes
41.72 KB
42.44 KB

#3026085: Unable to POST new entities with client-generated UUID landed while I worked on this and broke my patch. Here's a reroll.

Status: Needs review » Needs work

The last submitted patch, 21: 3032451-21.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
41.52 KB
42.24 KB

I rolled #21 wrong. (In #18 I got the patch right but the interdiff wrong, in #21 I got the interdiff right but the patch wrong 🙃)

  • Wim Leers authored 008b5ca on 8.x-3.x
    Issue #3032451 by Wim Leers, gabesullice, e0ipso: Compatibility with...
e0ipso’s picture

Status: Needs review » Fixed

🙏🏽 you are fabulous!

  • gabesullice authored 40e76aa on 8.x-3.x
    Issue #3032451 by Wim Leers, gabesullice, e0ipso: Compatibility with...
  • Wim Leers authored 68a5afb on 8.x-3.x
    Issue #3032451 by Wim Leers, gabesullice, e0ipso: Compatibility with...
e0ipso’s picture

Some additional commit credit for the fantastic lateral refactor.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture