Do not include internal resource types under `include`

Updating title per #3.

1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -108,7 +116,12 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +      // Only add "public" resources.
   

   What do you think about s/"public"/non-internal/? "public" is a term we don't use/is undefined in this context.

   Or, better yet, just … no comment. A detailed explanation can be found in the related methods and interfaces.

2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -206,27 +222,42 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +  protected function isInternalResourceType(EntityInterface $entity) {
   

   Why is this helper method necessary?

3. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -206,27 +222,42 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +   * @param string $field
   

   s/$field/$field_name/

4. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -206,27 +222,42 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +   *   The internal field name.
   ...
   +   *   The resource type names of the non-internal relatable resource types for
   

   "internal field name" vs "non-internal relatable resource types"

   These are two VERY different kinds of "internal".

5. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -206,27 +222,42 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +  protected static function getAllowedResourceTypes(ResourceType $resource_type, $field) {
   

   Should be called getAllowedResourceTypeNames().

   That, or the body should remove the array_map().

6. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -206,27 +222,42 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +    $relationship_field = $resource_type->getInternalName($field);
   

   If $field is already supposed to be an internal field name, then why do we need to call ::getInternalName()?

#8:

1. ✔️ + I like "alias" for field names much better.
2. ✔️ (I think this is the first time ever somebody is quoting Martin Fowler back at me :D Love it :))
3. ✔️
4. ✔️
5. ✔️ (Merci!)
6. ✔️

1. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -108,7 +116,11 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
          // Get the referenced entity.
          $entity = $item->get('entity')->getValue();
          // And get the translation in the requested language.
   -      $entity_list[] = $this->entityRepository->getTranslationFromContext($entity);
   +      $translated = $this->entityRepository->getTranslationFromContext($entity);
   +
   +      if (!$this->isInternalResourceType($translated)) {
   +        $entity_list[] = $translated;
   +      }
   

   Nit, but shouldn't we do the "is internal" check before getting the translation and use continue instead? Avoids unnecessary work/is slightly clearer.

2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -207,26 +222,40 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +   * Gets a list of allowed resource type names for a specific resource field.
   

   s/resource field/reference field/?

3. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -207,26 +222,40 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +   *   The resource type names of the non-internal relatable resource types for
   +   *   the given field on the parent resource type.
   

   Nit: Perhaps mention that if the given field is NOT a reference field, that this will just return the empty array?

Once those nits are addressed, this is RTBC.

+++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
@@ -207,26 +222,40 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
+  protected static function getAllowedResourceTypeNames(ResourceType $resource_type, $field_name) {

Martin Fowler always has something to say.

I've grown to dislike static methods because they are namespaced global functions. Hard to stub and test.

https://www.martinfowler.com/bliki/StaticSubstitution.html

Not suggesting a change, I only wanted to join the party 😝 in #11. This can be committed once @Wim Leers' comments have been addressed.

Notice: Undefined variable: translated

Should be an easy fix. Then this is RTBC.

1. +++ b/tests/src/Unit/Normalizer/EntityReferenceFieldNormalizerTest.php
   @@ -32,6 +32,13 @@ class EntityReferenceFieldNormalizerTest extends UnitTestCase {
   +  protected $fakeType;
   
   @@ -69,15 +76,23 @@ class EntityReferenceFieldNormalizerTest extends UnitTestCase {
   +    $this->fakeType = new ResourceType('fake_entity_type', 'dummy_bundle', NULL);
   

   I was first wondering why we had to make this a property on the test class.

2. +++ b/tests/src/Unit/Normalizer/EntityReferenceFieldNormalizerTest.php
   @@ -69,15 +76,23 @@ class EntityReferenceFieldNormalizerTest extends UnitTestCase {
   +    $this->fakeType->setRelatableResourceTypes([
   +      'field_dummy' => [$this->fakeType, $internal_type],
   +      'field_dummy_single' => [$this->fakeType, $internal_type],
   +    ]);
   
   @@ -96,7 +111,7 @@ class EntityReferenceFieldNormalizerTest extends UnitTestCase {
   -      'resource_type' => new ResourceType('fake_entity_type', 'dummy_bundle', NULL),
   +      'resource_type' => $this->fakeType,
   

   This is the reason.

One last question:

+++ b/tests/src/Unit/Normalizer/EntityReferenceFieldNormalizerTest.php
@@ -69,15 +76,23 @@ class EntityReferenceFieldNormalizerTest extends UnitTestCase {
+      'field_dummy_single' => [$this->fakeType, $internal_type],

Why do we set this if we're not using it anywhere?

I extracted a test-only patch from #19. It passes tests, but it should not.

I don't think it make sense to postpone this on #2943209: Mark at least one entity_test entity type as 'internal'. I think this issue can continue by depending on the content_moderation module and use that module's internal entity type for now. We can then have a follow-up for this issue that is blocked on #2943209: Mark at least one entity_test entity type as 'internal' which would use that internal test entity type instead.

I agree with @Wim Leers. I don't think we should postpone one of our criticals based on a core issue that may (or may not) take months to resolve.

I don't think we should use content_moderation but I will not oppose it. I'd rather have a jsonapi_test module that builds upon entity_test to add that. Core can backport that at their own pace. Then we can deprecate jsonapi_test when #2943209: Mark at least one entity_test entity type as 'internal' is available in all supported core versions.

We can then have a follow-up for this issue that is blocked on #2943209: Mark at least one entity_test entity type as 'internal'

This is what I meant to say the other day in Slack @gabesullice.

1. +++ b/tests/modules/jsonapi_entity_test/jsonapi_entity_test.info.yml
   @@ -0,0 +1,9 @@
   +#dependencies:
   +#  - field
   +#  - text
   

   Nit: Let's remove these lines.

2. +++ b/tests/modules/jsonapi_entity_test/src/Entity/EntityTestNoLabel.php
   @@ -0,0 +1,35 @@
   +class EntityTestNoLabel extends EntityTest {
   

   Let's add a @todo to remove this in the follow-up issue that #23 and #24 refer to.

3. +++ b/tests/src/Functional/InternalEntitiesTest.php
   @@ -0,0 +1,95 @@
   +  protected static $modules = [
   

   Nit: Missing inheritdoc.

4. +++ b/tests/src/Functional/InternalEntitiesTest.php
   @@ -0,0 +1,95 @@
   +    $this->testUser = $this->drupalCreateUser([
   

   Nit: Needs to be documented on the test class.

5. +++ b/tests/src/Functional/InternalEntitiesTest.php
   @@ -0,0 +1,95 @@
   +    $this->testBundle = EntityTestBundle::create([
   

   Nit: Does not need to be an object property (i.e. can become $test_bundle instead of $this->testBundle.)

6. +++ b/tests/src/Functional/InternalEntitiesTest.php
   @@ -0,0 +1,95 @@
   +    $this->internalEntity = EntityTestNoLabel::create([]);
   

   Nit: Same here.

7. +++ b/tests/src/Functional/InternalEntitiesTest.php
   @@ -0,0 +1,95 @@
   +    $this->referencerEntity = EntityTestWithBundle::create([
   

   First: this needs to be documented (see two points higher).
   Second: "referencing entity" instead of "referencer entity", I think?

8. +++ b/tests/src/Functional/InternalEntitiesTest.php
   @@ -0,0 +1,95 @@
   +    $response = $this->jsonapiGetIndividual($this->referencerEntity, ['include' => ['field_internal']]);
   …
   +++ b/tests/src/Functional/InternalEntitiesTest.php
   @@ -0,0 +1,95 @@
   +  protected function jsonapiGetIndividual(EntityInterface $entity, $options = []) {
   

   This seems unnecessarily complex, and the helper function seems to make things even more complex:

   $url = Url::fromRoute(
     'jsonapi.jsonapi_entity_test_nl_label--internal_referencer.individual,
     ['jsonapi_entity_test_nl_label' =>  $this->referencingEntity->uuid(),
     ['query' => ['include' => ['field_internal']]]
   );
   $response = $this->drupalGet($url);
   

   And once #2878463: [PP-1] Define a JSON API link relation for entities lands:

   $url = $this->referencingEntity->toUrl('jsonapi')->setOption('query', ['include' => ['field_internal']]);
   $response = $this->drupalGet($url);878463] lands, it could just become:
   

+++ b/tests/modules/jsonapi_entity_test/src/Entity/EntityTestNoLabel.php
@@ -22,6 +22,9 @@ use Drupal\entity_test\Entity\EntityTest;
+ * @todo remove this class when this lands: https://www.drupal.org/project/drupal/issues/2943209

… and when JSON API requires a core version that contains it. That can be fixed on commit.

This patch does come with proper test coverage (that's in fact the majority of this patch!), and does it in the way that @e0ipso indicated he prefers in #24. RTBC :)

This broke JSON API on Drupal 8.6: https://www.drupal.org/pift-ci-job/884832.

1) Drupal\Tests\jsonapi\Functional\InternalEntitiesTest::testIncludes
PHPUnit_Framework_Exception: Argument #2 (No Value) of PHPUnit_Framework_Assert::assertArrayNotHasKey() must be a array or ArrayAccess

/var/www/html/modules/contrib/jsonapi/tests/src/Functional/InternalEntitiesTest.php:96

Looks like we're getting back a HTML 404 response.

Reverted.

Discovered that regression at #2930028-68: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only.

#40: Now that's a nice silver lining! 🎉

#40 was tested against both D8.4 and D8.6 … but before #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only was committed. Let's retest to make sure it still passes against both core branches.

If that comes back green, I'll commit this.

Both came back green! Committed!