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:
- very solid test coverage
- 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
- 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.
- There are known architectural issues, for which there are follow-ups — see #2842148: The path for JSON API to "super stability"
- All coding standards are met.
- There are no known security or data integrity issues.
- There's no disruption of stable functionality elsewhere in Drupal.
- There is functional test coverage for the primary use case, with no test failures.
- There is no basic API documentation — because the JSON API module offers no PHP API. It offers an API via routes+controllers+responses, i.e. it offers a HTTP API, not a PHP API. (This is similar to BigPipe, see #2835604: BigPipe provides functionality, not an API: mark all classes & interfaces @internal.) Regarding how to use this HTTP API, there is http://jsonapi.org/format/ + http://jsonapi.org/examples/ + https://www.youtube.com/playlist?list=PLZOQ_ZMpYrZsyO-3IstImK1okrpfAjuMZ
- There are no user interfaces, so there's no need for planning/validating them.
- All critical issues have been resolved, most notably #2829398: Clean up JsonApiResource: the annotation, plugin type, plugin manager, plugin implementations, the dynamic routes generator and the request handler.
- Follow up issues documented and filed for outstanding work: see #2842148: The path for JSON API to "super stability"
- Timeframe for completing the remaining work: the goal is to complete all remaining work by Drupal 8.4, so that it can be marked
beta
(or evenstable
) in 8.4, and to flush out remaining problems by 8.5, at which point it should be rock-solid.
Comments
Comment #2
wim leersDone!
Comment #3
e0ipsoRegarding 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
Comment #4
wim leersYes, this!
Or, what could be interesting too:
assert(). A production site doesn't runassert()and hence wouldn't validate JSON API responses. But any development site would, and hence problems would be uncovered during development :)Comment #5
wim leersMost 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.
Comment #6
e0ipsoI 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.jsonfor this? Do you have any PHP json-schema validator of choice?Comment #7
wim leersDrupalCI doesn't support installing additional libraries via composer (yet) :(
Comment #8
wim leersBUT 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 usesettings.local.phpand hence have assertions enabled).Comment #9
hampercm commentedCreated #2840677: Validate responses against the JSON API specification for adding validation against the JSON API spec
Comment #10
wim leersAdded #2840948: Remove interfaces of JSON API-specific normalizers and #2840967: \Drupal\jsonapi\Encoder\JsonEncoder should extend the serialization module's JsonEncoder, not Symfony's.
Comment #11
wim leersAdded #2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources, which almost seems like a blocker to landing JSON API in core.
Comment #12
wim leers#2840948: Remove interfaces of JSON API-specific normalizers and #2840967: \Drupal\jsonapi\Encoder\JsonEncoder should extend the serialization module's JsonEncoder, not Symfony's landed, yay!
Comment #13
wim leersComment #14
e0ipsoAfter some live discussion, we decided that this is a core issue that will not block JSON API progress towards core inclusion.
Comment #15
wim leersAlso, #2829398: Clean up JsonApiResource: the annotation, plugin type, plugin manager, plugin implementations, the dynamic routes generator and the request handler is now completely done — it took 8 child issues!
A good thing is that #2829740: Clean up Drupal\jsonapi\Configuration\Resource(Config|Manager)(Interface) can be closed too, it was done as part of the refactoring in #2829398: Clean up JsonApiResource: the annotation, plugin type, plugin manager, plugin implementations, the dynamic routes generator and the request handler. :)
Comment #16
wim leers#2842378: Remove all remaining interfaces in the JSON API module that have only one implementation landed too :)
Comment #17
skyredwangSince 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
Comment #18
hampercm commented@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.
Comment #19
wim leersI was able to close #2829746: Clean up \Drupal\jsonapi\LinkManager\LinkManager(Interface) :)
Comment #20
wim leers#2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources is ready for review and will hopefully be committed soon.
#2829977: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary is also ready for review, but is blocked on #2841027.
Progress! :)
Comment #21
wim leers#2841027: JSON API format should only be used on JSON API routes, but can currently also be used for REST resources is in!
Comment #22
wim leersTo properly solve #2829977: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary we also need the follow-up #2855693: Remove \Drupal\jsonapi\Controller\RequestHandler::renderJsonApiResponse(), add \Drupal\jsonapi\EventSubscriber\ResourceResponseSubscriber. Made that a child issue too.
Comment #23
e0ipso#2829977: SerializableHttpException and Drupal\jsonapi\Error\* are not necessary is in.
Comment #24
wim leers#2831137: Remove the need for ?_format=api_json: assume the use of the 'api_json' format for routes managed by JSON API landed too. That means this issue is done! Great work, everyone!
Now see you over at #2842148: The path for JSON API to "super stability".