Refactor FieldResolver and tests

I'm good with this in spirit. However I'm starting to worry about churn generated by personal preferences and code style idiosyncrasies.

To make myself clear: I think this patch is an improvement and we should commit it.

On a separate note, we should try to not increase patch size only to please our personal taste (array_reduce vs foreach, 0 cyclomatic complexity vs low cyclomatic complexity, type hinting vs comment based typed hinting, different wording in comments, …) unless it's justified (the previous comment wording misleading, we need side-effects in the loop, …).

I'll do a review of the patch soon.

However I'm starting to worry about churn generated by personal preferences and code style idiosyncrasies.

Making the code easier to understand for more people is worthwhile, right? I agree if it's pure coding style changes (except if it's making the code conform to core's coding standards, that's always okay). But in this case

• the original testing code is being abbreviated massively
• loops with huge bodies of code that get shortened by moving that logic into helper methods that make things much easier to understand
• adds missing docs (sometimes short comments, sometimes entire docblocks)

Patch review:

1. +++ b/src/Context/FieldResolver.php
   @@ -88,40 +120,33 @@ class FieldResolver {
   +
   ...
   +
   ...
   +
          // Update the entity type with the referenced type.
   ...
   +
          // $field_name may not be a reference field. In that case we should treat
   

   These \n additions are pure personal preference. Let's revert these.

2. +++ b/src/Context/FieldResolver.php
   @@ -161,32 +223,46 @@ class FieldResolver {
   +    $has_target_bundles = isset($handler_settings['target_bundles']) && !empty($handler_settings['target_bundles']);
   +    $target_bundles = $has_target_bundles ?
   +      $handler_settings['target_bundles']
   +      : $this->getAllBundlesForEntityType($entity_type_id);
   ...
   +  /**
   +   * Gets all bundle IDs for a given entity type.
   +   *
   +   * @param string $entity_type_id
   +   *   The entity type for which to get bundles.
   +   *
   +   * @return string[]
   +   *   The bundle IDs.
   +   */
   +  protected function getAllBundlesForEntityType($entity_type_id) {
   +    return array_keys($this->entityTypeBundleInfo->getBundleInfo($entity_type_id));
   +  }
   +
   

   This is the bugfix. Let's not fix bugs here, let's *only* clean up existing code.

3. +++ b/tests/src/Kernel/Context/FieldResolverTest.php
   @@ -150,37 +56,106 @@ class FieldResolverTest extends JsonapiKernelTestBase {
   +    $cases = [
   +      ['field_test1', 'field_test1'],
   +      ['field_test2', 'field_test2'],
   +      ['field_test3', 'field_test3'],
   +
   +      ['field_test_ref1.entity.field_test1', 'field_test_ref1.field_test1'],
   +      ['field_test_ref1.entity.field_test2', 'field_test_ref1.field_test2'],
   +      ['field_test_ref2.entity.field_test1', 'field_test_ref2.field_test1'],
   +      ['field_test_ref2.entity.field_test2', 'field_test_ref2.field_test2'],
   +      ['field_test_ref3.entity.field_test1', 'field_test_ref3.field_test1'],
   +      ['field_test_ref3.entity.field_test2', 'field_test_ref3.field_test2'],
   +
   +      ['field_test_ref1.entity.field_test_text', 'field_test_ref1.field_test_text'],
   +      ['field_test_ref1.entity.field_test_text.value', 'field_test_ref1.field_test_text.value'],
   +      ['field_test_ref1.entity.field_test_text.format', 'field_test_ref1.field_test_text.format'],
   +      ['field_test_ref2.entity.field_test_text', 'field_test_ref2.field_test_text'],
   +      ['field_test_ref2.entity.field_test_text.value', 'field_test_ref2.field_test_text.value'],
   +      ['field_test_ref2.entity.field_test_text.format', 'field_test_ref2.field_test_text.format'],
   +    ];
   +
   

   We should convert this to a @dataProvider. That'd make it even clearer.

Making the code easier to understand for more people is worthwhile, right?

+10

except if it's making the code conform to core's coding standards, that's always okay

+1

I agree this case is mostly that, I wanted to clarify that upfront with:

To make myself clear: I think this patch is an improvement and we should commit it.

Yep, I figured, but I just wanted to check some of the nuances with you :) Glad to see we're once again on the same page!

+++ b/src/Context/FieldResolver.php
@@ -189,4 +247,17 @@ class FieldResolver {
+  protected function getAllBundlesForEntityType($entity_type_id) {

This method is still being added, but no longer being used. Should be removed.

Once that's fixed, this is RTBC IMHO.

👏🏽🚢

#11: :D

Now #2920194: Inclusion of nested relationships fails for bundle-specific fields can continue!