Comments

gabesullice created an issue. See original summary.

gabesullice’s picture

Updating credits.

wim leers’s picture

#2937279: [META] Introduce concept of internal resource types says this is minor, but this issue is marked major. Which is it?

gabesullice’s picture

One small step for man, one giant leap for JSON API.

Or something like that :P

In the parent issue it's minor wrt to progress, i.e., "it shouldn't be much work". It's still a "major" priority. I should have a patch today.

wim leers’s picture

👍

gabesullice’s picture

Status: Active » Needs review
Issue tags: +API-First Initiative, +blocked
StatusFileSize
new2.33 KB
new4.17 KB
new6.8 KB

The last submitted patch, 6: 2939729-6.will_not_apply.patch, failed testing. View results

gabesullice’s picture

StatusFileSize
new2.33 KB
new4.18 KB
new6.81 KB
new627 bytes

derp

The last submitted patch, 8: 2939729-8.will_not_apply.patch, failed testing. View results

wim leers’s picture

Status: Needs review » Needs work
  1. +++ b/tests/src/Functional/InternalEntitiesTest.php
    @@ -154,4 +154,25 @@ class InternalEntitiesTest extends BrowserTestBase {
    +    if (!method_exists(EntityTypeInterface::class, 'isInternal')) {
    +      $this->markTestSkipped('The Drupal Core version must be >= 8.5');
    +      return;
    +    }
    +    $this->drupalLogin($this->testUser);
    +    $entity_type_id = $this->referencingEntity->getEntityTypeId();
    +    $bundle = $this->referencingEntity->bundle();
    +    $path = "/jsonapi/{$entity_type_id}/{$bundle}/{$this->referencingEntity->uuid()}";
    +    $response = $this->drupalGet($path, [], ['Accept' => 'application/vnd.api+json']);
    

    This is 90% duplicating \Drupal\Tests\jsonapi\Functional\InternalEntitiesTest::testIncludes().

    I think it makes sense to refactor to share code now — because now there's clearly multiple callers. (::testEntryPoint() is another one.)

  2. +++ b/tests/src/Unit/Normalizer/Value/RelationshipNormalizerValueTest.php
    @@ -27,10 +27,14 @@ class RelationshipNormalizerValueTest extends UnitTestCase {
    +    $resource_type = new ResourceType($this->randomMachineName(), $this->randomMachineName(), NULL);
    

    This is not testing an internal resource type? Why then are we changing this? I think I'm missing something obvious, sorry :(

  3. +++ b/src/Normalizer/Value/RelationshipNormalizerValue.php
    @@ -85,23 +85,42 @@ class RelationshipNormalizerValue extends FieldNormalizerValue {
    +    $resource_types = $this->resourceType->getRelatableResourceTypesByField($field_name);
    +    if (static::hasNonInternalResourceType($resource_types)) {
    +      $links['related'] = $this->linkManager->getEntityLink(
             $this->hostEntityId,
             $this->resourceType,
             $route_parameters,
             'related'
    -      ),
    -    ];
    +      );
    +    }
    

    AFAICT this means that we never set the related link? Even if there's 10 relatable resource types that are non-internal, and a single one that is internal?

gabesullice’s picture

StatusFileSize
new7.28 KB
new12.47 KB

1. Done.

2. Calling getRelatableResourceTypes requires that setRelatableResourceTypes is called first. Which is done automatically by the resource type repository, but we have to do manually in tests. This might be worth a follow-up, as it's a tedious thing to do everywhere.

3. static::hasNonInternalResourceType, so it will set related as long as just one resource type is not "internal". It's inverse would be "allInternalResourceTypes".

wim leers’s picture

Status: Needs work » Needs review
  1. ✔️
  2. Ok.
  3. ✔️ (oops!)

Marking NR to get this patch tested.

gabesullice’s picture

StatusFileSize
new12.47 KB

wim leers’s picture

Status: Needs review » Fixed
StatusFileSize
new1.42 KB

Weird, #12 should totally have triggered testing of #11.

  1. +++ b/src/Normalizer/Value/RelationshipNormalizerValue.php
    @@ -65,33 +65,63 @@ class RelationshipNormalizerValue extends FieldNormalizerValue {
    +    // @link http://jsonapi.org/format/#document-resource-object-linkage
    

    I totally forgot about @link! ❤️

  2. +++ b/tests/src/Functional/InternalEntitiesTest.php
    @@ -154,4 +132,73 @@ class InternalEntitiesTest extends BrowserTestBase {
    +    $decoded = $this->getIndividual($this->referencingEntity);
    

    If we use $document elsewhere, then why not also here?

    Fixing on commit.

e0ipso’s picture

💥

Thank you for taking care of this guys!

Status: Fixed » Closed (fixed)

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