Address core feedback 2843147#comment-11872253

Pasting feedback for readability

This is just some review while going through some of the code.

1. +++ b/core/composer.json
   index 0000000..ab01a11
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/jsonapi/README.md
   

   Note: We don't yet use markdown files in core ... I'm fine with adding some, its much easier to read.

2. +++ b/core/modules/jsonapi/README.md
   @@ -0,0 +1,24 @@
   +## Compatibility
   +
   +This module is compatible with Drupal core 8.2.x and higher.
   
   +++ b/core/modules/jsonapi/jsonapi.info.yml
   @@ -0,0 +1,8 @@
   +  - drupal:system (>=8.2)
   

   I guess we talk about 8.3.x as of this core patch.

3. +++ b/core/modules/jsonapi/jsonapi.module
   @@ -0,0 +1,65 @@
   +      $spec_link = Link::fromTextAndUrl(t('JSON API Specification'), $spec_url);
   ...
   +          '@spec' => $spec_link->toString(),
   +          '@online' => $youtube_link->toString(),
   

   Nitpick: Afaik we use <a href="">:url</a> instead of generating links, in help texts.

4. +++ b/core/modules/jsonapi/jsonapi.services.yml
   @@ -0,0 +1,98 @@
   +  serializer.normalizer.entity_reference_item.jsonapi:
   ...
   +      - { name: normalizer, priority: 21 }
   +  serializer.normalizer.field_item.jsonapi:
   ...
   +      - { name: normalizer, priority: 21 }
   +  serializer.normalizer.scalar.jsonapi:
   ...
   +      - { name: normalizer, priority: 5 }
   

   For easier readability: Could we reorder the entries in this file to be ordered by priority somehow?

5. +++ b/core/modules/jsonapi/jsonapi.services.yml
   index 0000000..686fca8
   --- /dev/null
   
   --- /dev/null
   +++ b/core/modules/jsonapi/phpcs.xml
   

   Nice!

6. +++ b/core/modules/jsonapi/src/Access/CustomParameterNames.php
   @@ -0,0 +1,59 @@
   +  public function access(Request $request) {
   +    $json_api_params = $request->attributes->get('_json_api_params', []);
   

   Wow, its sad that we don't use the controller resolver, so we cannot easily pull values out of the request attributes.

7. +++ b/core/modules/jsonapi/src/Access/CustomParameterNames.php
   @@ -0,0 +1,59 @@
   +
   +    foreach (array_keys($json_api_params) as $name) {
   +      if (strpbrk($name, '+,.[]!”#$%&’()*/:;<=>?@^`{}~|')) {
   +        $valid = FALSE;
   +        break;
   +      }
   +
   +      if (strpbrk($name[0], '-_ ') || strpbrk($name[strlen($name) - 1], '-_ ')) {
   +        $valid = FALSE;
   +        break;
   

   Can we point to the place in the space where this is defined?

8. +++ b/core/modules/jsonapi/src/Context/CurrentContext.php
   @@ -0,0 +1,142 @@
   +    $params = $this
   +      ->requestStack
   +      ->getCurrentRequest()
   +      ->attributes
   +      ->get('_json_api_params');
   +    return (isset($params[$parameter_key])) ? $params[$parameter_key] : NULL;
   +  }
   

   Note: We could use ->get("_json_api_params[$parameter_key]", NULL, TRUE) instead of this isset

9. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
   @@ -0,0 +1,735 @@
   +use Drupal\jsonapi\Error\SerializableHttpException;
   +use Drupal\jsonapi\Error\UnprocessableHttpEntityException;
   ...
   +      $exception = new UnprocessableHttpEntityException();
   

   Is there a reason why these exceptions are in the Error and not Exception namespace?

10. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
    @@ -0,0 +1,735 @@
    +    $params = $request->attributes->get('_route_params');
    +    $query = $this->getCollectionQuery($entity_type_id, $params['_json_api_params']);
    

    As somewhere else on the patch, this could use $request->attributes->get('_route_params[_json_api_params]', NULL, TRUE)

11. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
    @@ -0,0 +1,735 @@
    +    // We request N+1 items to find out if there is a next page for the pager. We may need to remove that extra item
    +    // before loading the entities.
    +    $pager_size = $query->getMetaData('pager_size');
    +    if ($has_next_page = $pager_size < count($results)) {
    +      // Drop the last result.
    +      array_pop($results);
    +    }
    

    <3 mini pagers!

12. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
    @@ -0,0 +1,735 @@
    +    if (!($field_list = $entity->get($related_field)) || !$this->isRelationshipField($field_list)) {
    ...
    +      if (!$destination_field_list = $destination->get($field_name)) {
    +        throw new SerializableHttpException(400, sprintf('The provided field (%s) does not exist in the entity with ID %d.', $field_name, $destination->id()));
    +      }
    

    ... When $field_name doens't exist, it throws an exception, according to the documentation on \Drupal\Core\Entity\FieldableEntityInterface::get

13. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
    @@ -0,0 +1,735 @@
    +    if ($origin instanceof ContentEntityInterface && $destination instanceof ContentEntityInterface) {
    

    I'm wondering whether we should check for FieldableEntityInterface instead, as these are the methods called here

14. +++ b/core/modules/jsonapi/src/Controller/EntityResource.php
    @@ -0,0 +1,735 @@
    +    $field_type_manager = \Drupal::service('plugin.manager.field.field_type');
    

    Don't we have this service injected?

1. +++ b/core/MAINTAINERS.txt
   @@ -243,6 +243,9 @@ Installer
   +JSON-API
   
   +++ b/core/modules/jsonapi/jsonapi.info.yml
   @@ -0,0 +1,8 @@
   +name: JSON API
   

   Ubernitpick: What's the reason for this dash?

2. +++ b/core/modules/jsonapi/src/Normalizer/NormalizerBase.php
   @@ -0,0 +1,51 @@
   +    return in_array($format, $this->formats) && parent::supportsNormalization($data, $format);
   ...
   +    if (in_array($format, $this->formats) && (class_exists($this->supportedInterfaceOrClass) || interface_exists($this->supportedInterfaceOrClass))) {
   

   Just for sanity reason I would also use $strict=TRUE as the third parameter.

3. +++ b/core/modules/jsonapi/src/Normalizer/Value/EntityNormalizerValue.php
   @@ -0,0 +1,147 @@
   +    $this->includes = array_reduce($this->includes, function ($carry, $includes) {
   +      return array_merge($carry, $includes);
   +    }, []);
   

   Nitpick: You can also use array_reduce($this->includes, 'array_merge', []) here.

4. +++ b/core/modules/jsonapi/src/Normalizer/Value/JsonApiDocumentTopLevelNormalizerValue.php
   @@ -0,0 +1,175 @@
   +    $this->includes = array_reduce($this->includes, function ($carry, $includes) {
   +      array_walk($includes, function ($include) {
   +        $this->addCacheableDependency($include);
   +      });
   +      return array_merge($carry, $includes);
   +    }, []);
   

   Isn't it just sad how OOP code just doesn't really compose together. Note: You could use array_walk($includes, [$this, 'addCacheableDependency']) to save some lines.

5. +++ b/core/modules/jsonapi/src/Query/GroupOption.php
   @@ -0,0 +1,157 @@
   +      };
   

   Wow, I had no idea that this is actually possible

6. +++ b/core/modules/jsonapi/src/Query/GroupOption.php
   @@ -0,0 +1,157 @@
   +      // If we already know that we have the child, skip evaluation and return.
   +      if (!$has_child) {
   +        // Determine if this group or any of its children match the $id.
   +        $has_child = ($group->id() == $id || $group->hasChild($id));
   +      }
   +      return $has_child;
   +    }, FALSE);
   

   I wonder whether we should use return $has_child || $group_id == $id | $group->hasChild($id)

   I thought PHP actually lazy evaluates in this case.

7. +++ b/core/modules/jsonapi/src/Resource/EntityCollection.php
   @@ -0,0 +1,89 @@
   +   * @var array
   +   */
   +  protected $entities;
   +
   ...
   +   * @param array $entities
   

   Can we document using EntityInterface[]

8. +++ b/core/modules/jsonapi/src/Routing/Param/JsonApiParamInterface.php
   @@ -0,0 +1,41 @@
   +  /**
   +   * The key name.
   +   *
   +   * @var string
   +   */
   +  const KEY_NAME = NULL;
   

   I couldn't find any usecase for this, was this basically meant as abstract constant or so?

9. +++ b/core/modules/jsonapi/tests/src/Unit/Routing/JsonApiParamEnhancerTest.php
   @@ -0,0 +1,97 @@
   +  public function testApplies() {
   +    $object = new JsonApiParamEnhancer($this->prophesize(EntityFieldManagerInterface::class)->reveal());
   +    $route = $this->prophesize(Route::class);
   +    $route->getDefault(RouteObjectInterface::CONTROLLER_NAME)->will(new ReturnPromise([Routes::FRONT_CONTROLLER, 'lorem']));
   +
   +    $this->assertTrue($object->applies($route->reveal()));
   +    $this->assertFalse($object->applies($route->reveal()));
   +  }
   

   Wow, that's cool

The patch in #6 looks good!