Per #2757967: API-first initiative, we'd like to move the JSON API module into Drupal 8.3/8.4. For that to happen, the module needs to be in a excellent shape. (It looks like it already is though!)

We need:

  1. very solid test coverage
  2. since this is following a spec (http://jsonapi.org/format/): compliance with the spec, test coverage verifying that compliance, and code comments linking to the spec where relevant
  3. compliance with core best practices wrt naming, structure

Why do this now, while there is not even a Drupal core issue to add this to Drupal core? Because if we do this now, before the JSON API contrib module enters the beta phase, we minimize (and hopefully eradicate) the need for BC breaks, which will make it easier for those sites who are using the contrib module to upgrade to a Drupal core version that includes this module.

Wim Leers has taken on this review.


After the initial review above, and having finished most of it (by January 9, 2017), it's time to look at the exact requirements that must be met. See https://www.drupal.org/core/experimental#requirements.

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Done!

e0ipso’s picture

Regarding 2. We could have all responses be checked against the specification schema before sending them out to the user. That would ensure that any response we deliver complies with the standard. I'm unsure of the performance toll on doing this.

If we don't want to check every response, we can have this schema checks on the functional tests.

http://jsonapi.org/schema

Wim Leers’s picture

If we don't want to check every response, we can have this schema checks on the functional tests.

Yes, this!

Or, what could be interesting too: assert(). A production site doesn't run assert() and hence wouldn't validate JSON API responses. But any development site would, and hence problems would be uncovered during development :)

Wim Leers’s picture

Most of these issues don't need to block JSON API from being added to Drupal core as an experimental module.

Also: fixing them now, before the module is committed to Drupal core, will lead to them being fixed faster, since you don't need to go through the core review process, and it won't need to be committed by a core committer. It'll also mean getting this committed to Drupal 8 core as an experimental module will be easier.

e0ipso’s picture

I like the assert() idea, since most of the spec violations will come from convoluted data models that are very very hard to reproduce in the functional tests.

Shall I add https://github.com/justinrainbow/json-schema to the composer.json for this? Do you have any PHP json-schema validator of choice?

Wim Leers’s picture

DrupalCI doesn't support installing additional libraries via composer (yet) :(

Wim Leers’s picture

DrupalCI doesn't support installing additional libraries via composer (yet) :(

BUT NOW IT DOES! :) :) See #2597778: Install composer dependencies for contrib projects before running tests.

So we can now totally add JSON Schema-based validation, to ensure all our JSON API responses are valid. We should update our functional test to use that, and we should use assert() to also validate all JSON API responses just before sending them, only for developers (i.e. those who use settings.local.php and hence have assertions enabled).

hampercm’s picture

Created #2840677: Validate responses against the JSON API specification for adding validation against the JSON API spec

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes
e0ipso’s picture

which almost seems like a blocker to landing JSON API in core.

After some live discussion, we decided that this is a core issue that will not block JSON API progress towards core inclusion.

Wim Leers’s picture

skyredwang’s picture

Since 8.3.0-alpha1 is out already, does this mean JSONAPI won't make it to the 8.3.x branch? According to https://www.drupal.org/core/d8-allowed-changes#patch

hampercm’s picture

@skyredwang Yes, JSON API in core is being held off until 8.4.x. The core committers simply had too much on their plate already to manage getting JSON API in as a core module. On the plus side, this means there will be more time to refine the module before it is committed to core, at which point making changes becomes significantly more time consuming due the core commit process.

Wim Leers’s picture

Wim Leers’s picture

e0ipso’s picture

Wim Leers’s picture

Status: Fixed » Closed (fixed)

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