Problem/Motivation

See #2836165: New experimental module: JSON API and #2757967: API-first initiative.

Proposed resolution

Drop in of the JSON API module. After that, a plan with next steps towards stability is detailed in #2842148: [PP-2] Next steps: the path to stability for the JSON API module.

Remaining tasks

  1. Provide a patch.
  2. Review the code. Some review + clean-up/refactoring has been done already by Wim Leers (rest.module maintainer) in #2829327: Review JSON API module for core-readiness.

User interface changes

No user interface changes.

API changes

No API changes. Only additions isolated in the experimental module.

Data model changes

No data model changes.

Comments

e0ipso created an issue. See original summary.

e0ipso’s picture

e0ipso’s picture

Status: Active » Needs review

Kicking off tests.

e0ipso’s picture

Issue summary: View changes
e0ipso’s picture

It seems that jsonapi tests are being executed by Drupal CI and passing along with all other tests.

Wim Leers’s picture

Issue summary: View changes

Left a comment at #2836165-23: New experimental module: JSON API pointing people here.

Status: Needs review » Needs work

The last submitted patch, 7: 2843147--experimental-module-patch--7.patch, failed testing.

e0ipso’s picture

Status: Needs work » Needs review
dawehner’s picture

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

e0ipso’s picture

I created #2844373: Address core feedback 2843147#comment-11872253 to address feedback in #11 before porting back here.

Status: Needs review » Needs work

The last submitted patch, 12: 2843147--experimental-module-patch--12.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
375.88 KB
3.9 KB

It seems patch #12 includes much more than reported on the interdiff. Starting again from #9 I applied the patch to fix #2841109.

dagmar’s picture

Really nice to see this issue, but now this module could be part of the core I think #2775205: [PP-1] [FEATURE] Discus how to make JSON API use text input formats for long text fields (which is blocked by #2751325: All serialized values are strings, should be integers/booleans when appropriate) becomes much more important.

My understanding is rest module and jsonapi takes different approaches to fix the problem with processed values.

So if #2626924: [PP-1] Expose TextItems' "processed" property when normalizing is fixed rest.module and jsonapi module is included without significant changes, fix that would require a significant amount of work.

Wim Leers’s picture

#16:

My understanding is rest module and jsonapi takes different approaches to fix the problem with processed values.

Well, neither has an actual solution yet.

Depending on how we solve #2626924: [PP-1] Expose TextItems' "processed" property when normalizing, we could fix it for JSON API and REST in one go.

Also, the JSON API module will be experimental when it is added to core. So it's totally fine if that's not supported yet. We can add it later. In fact, there are lots of such missing things in core REST, yet that has also been shipping (even as "stable"). See #2794263: REST: top priorities for Drupal 8.3.x for the many things that need to be fixed in core REST.

e0ipso’s picture

e0ipso’s picture

The patch above will fail because it's generated with my composer based drupal-project (which has the git root at /core). /sigh

This patch should address this.

If anyone has a good workflow to deal with this, I'm interested. Otherwise I'll write a custom script.

The last submitted patch, 18: 2843147--experimental-module-patch--18.patch, failed testing.

The last submitted patch, 21: 2843147--interdiff--19-21.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 21: 2843147--experimental-module-patch--21.patch, failed testing.

dagmar’s picture

Status: Needs work » Needs review
FileSize
376.67 KB

It seems the patch was created outside the core/ folder.

Applied with git apply --directory=core and re-rolled.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 26: 2843147--experimental-module-patch--26.patch, failed testing.

mallezie’s picture

Seems the latest patch, attached some git magic. (Or not related changes from an core rebase).

e0ipso’s picture

I definitely need to take the time to automate this process… Here's another attempt.

e0ipso’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 29: 2843147--experimental-module-patch--29.patch, failed testing.

Wim Leers’s picture

The fail in #29 is because we'll need to update core's composer.json, rather than adding our dependency in core/modules/jsonapi/composer.json.

I can write a script to automate this. Like I already said in the past: you should be able to use something like the script I used in #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts.

e0ipso’s picture

Thanks @Wim Leers. That script is not really fitting for me because I installed my local using the drupal-composer/drupal-project. That sets my core git root in DRUPAL_ROOT/core and not DRUPAL_ROOT. Besides, having to merge the module's composer into core's composer.json makes things slightly more difficult.

It's not that it cannot be solved, it's just that it feels like a pointless chore to maintain this issue while the core committer's focus is on other pressing matters, and all the discussion is happening in the contrib module.

e0ipso’s picture

Status: Needs work » Needs review
FileSize
408.75 KB

Alright, let's give this other one a shot!

Status: Needs review » Needs work

The last submitted patch, 34: 2843147--experimental-module-patch--34.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Closed (outdated)

Per https://twitter.com/webchick/status/839240958105432065, since March 7, the decision was communicated by core committers (sadly not in a very clear way, sadly not here, sadly not in the "ideas" issue queue…) that no additional experimental modules would be added in 8.4.x.

Consequently, the JSON API module must remain a contrib module for now. Which also has a bright side: we can iterate much faster in contrib. And we're already seeing a massive uptick in JSON API usage: from 240 on March 5, to 982 on April. In between those two times, the first beta release of JSON API was published, and this seems to ad adoption significantly. So, we're getting lots of contrib validation before moving it into core, which is better anyway.

The mere existence of this issue, and the publicity around it, have already helped a lot with awareness. That's in the end the key reason for wanting to add JSON API to Drupal core: to make more people aware, to lower the bar to find this module, which we believe is a better fit/offers a better DX than the REST module for many (most?) use cases.

For now, closing this issue — I will reopen this once it's possible again to add an experimental module to Drupal core.

Berdir’s picture

@Wim That tweet *does* include API-First.. isn't json API part of that initative? This isn't something I'm specifically invested in or anything, just that the tweet as far as I see doesn't really seem to be an argument against? :)

Wim Leers’s picture

The tweet mentions API-First. Yes, JSON API is part of the API-first initiative (which btw still is not an official initiative… :/).

However, you need to look at the slides that that tweet links to. See slide 9. That says this for 8.4:

As a developer, I want stable releases of JSON API and OAuth2 modules in contrib that I can leverage in my production websites (stable contrib).

Keyword: contrib.

The reason API-first is mentioned, is that webchick means that core committers would like to see JSON API become fully stable in contrib first. The other reason it's mentioned is the work on the REST+Serialization+HAL modules in Drupal core:

As a developer, I would like to see various limitations in the core Serialization/REST API resolved for improved functionality and DX. (stable core)


So, while confusing, it's not actually a contradiction… But yes, the communication around all this is very confusing and very vague.

Berdir’s picture

Ok, I was too lazy to click on the link, that makes more sense :)

PS: Tried pinging you in IRC to discuss something else, pong me when you have a few minutes to discuss a maybe crazy idea :)

Wim Leers’s picture

@e0ipso was confused by #36, much like @Berdir.

11:44:36 <e0ipso> WimLeers: I just saw your comment about no more experimental modules in 8.4.x
11:44:36 <drupalbot> e0ipso: 40 min 8 sec ago <alvar0hurtad0> tell e0ipso ping
11:45:22 <e0ipso> WimLeers: is there a public announcement about that?
15:43:04 <WimLeers> e0ipso: RE: no experimental modules: the announcement/core committer plan about that is linked from webchick's tweet that I linked to
15:43:16 <WimLeers> e0ipso: And I even told you this via IRC weeks ago?
15:44:08 <e0ipso> WimLeers: I had a ghost for several weeks. You may have been communicating to that
15:44:13 <WimLeers> e0ipso: hahahahahahaha
Wim Leers’s picture

Issue tags: -API-First Initiative +API-First Initiative