Problem/Motivation

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

Proposed resolution

Drop in of the JSON:API module.

Remaining tasks

In no particular order,

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.

Added dependencies

Production: None!
Dev (require-dev):

  • justinrainbow/json-schema:
    • Code quality: documented, comprehensive tests
    • Maintainership of the package: recent releases (latest release was in Jan. 2019); apparently actively maintained.
    • Security policies of the package: unknown
    • Expected release and support cycles: undocumented; sporadic releases
    • Other dependencies it'd add, if any:
          "require": {
              "php": ">=5.3.3",
              "marc-mabe/php-enum": "2.3.1",
              "icecave/parity": "1.0.0"
          }

Migration from the contrib module

The ideal migration path is:

  1. update from 8.x-1.x to 8.x-2.x while you're on Drupal 8.5 or Drupal 8.6; this may require some changes to JSON:API clients
  2. update from Drupal 8.5 or 8.6 + JSON:API 8.x-2.x to Drupal 8.7 and remove (rm -rf) the contrib jsonapi module (this is exactly what we told sites using the contrib bigpipe module when they upgraded to Drupal 8.1)

For sites that don't want to or cannot invest in updating from JSON:API 8.x-1.x to 8.x-2.x: they can continue to keep the jsonapi contrib module installed, the same 8.x-1.x version. This overrides core's jsonapi module. IOW: even though JSON:API is part of core, this makes Drupal ignore the core module and pick the contrib module instead. @gabesullice manually vetted this.

More information wrt JSON:API module branches and how they're supported

The JSON:API module has 2 branches in contrib: 8.x-1.x and 8.x-2.x. The 8.x-1.x branch has been minimally maintained (security updates only) since June 2018, since 1.22. That branch was first maximally stabilized.
To comply with the JSON:API spec completely, we had to make certain changes that are disruptive to some JSON:API 8.x-1.x users/clients. That's why we opened the 8.x-2.x branch. This new branch is explicitly compatible with not only Drupal 8.6 (current stable) and Drupal 8.7 (the core minor we're targeting for including JSON:API), but also Drupal 8.5 (which currently receives only security updates). This means that JSON:API 8.x-2.x is supported on all secure Drupal 8.x releases. Which in turn means that every JSON:API 8.x-1.x user (which must currently use Drupal 8.5 or 8.6) can first update to 8.x-2.x. And then update to Drupal 8.7. This gives them maximal freedom to update.
The 8.x-2.x branch of the JSON:API contrib module will continue to receive security updates for as long as Drupal 8.6 receives security updates. And so will the JSON:API 8.x-1.x branch.

Release notes snippet

JSON:API is now included in core. Sites currently using the 8.x-2.x branch of the JSON:API contributed module should remove the contributed module from the codebase (rm -rf modules/jsonapi) when updating to Drupal 8.7, as the contributed module will not receive further updates aside from security fixes. Sites using the 8.x-1.x branch may require changes to API clients, but the contributed module may be left in place with Drupal 8.7 until any needed conversions are complete.

CommentFileSizeAuthor
#185 jsonapi-2843147-185.patch1.32 MBWim Leers
#173 jsonapi-2843147-173.patch1.31 MBWim Leers
#171 jsonapi-2843147-171.patch1.31 MBWim Leers
#171 create_jsonapi_patch-171.sh_.txt2.23 KBWim Leers
#163 jsonapi-2843147-161.patch1.29 MBWim Leers
#163 create_jsonapi_patch-161.sh_.txt2.23 KBWim Leers
#154 jsonapi-2843147-154.patch1.26 MBWim Leers
#154 create_jsonapi_patch-154.sh_.txt2.23 KBWim Leers
#153 JSON_API___Drupal_org.png44.21 KBxjm
#150 jsonapi-2843147-150.patch1.26 MBWim Leers
#150 create_jsonapi_patch-150.sh_.txt2.07 KBWim Leers
#144 jsonapi-2843147-144.patch1.26 MBWim Leers
#144 create_jsonapi_patch-144.sh_.txt2.07 KBWim Leers
#141 jsonapi-2843147-141.patch1.24 MBWim Leers
#141 create_jsonapi_patch-141.sh_.txt2.07 KBWim Leers
#138 jsonapi-2843147-138.patch1.24 MBWim Leers
#138 create_jsonapi_patch-138.sh_.txt2.07 KBWim Leers
#132 create_jsonapi_patch-131.sh_.txt1.44 KBWim Leers
#131 jsonapi-2843147-131.patch1.24 MBWim Leers
#131 interdiff.txt26.16 KBWim Leers
#114 jsonapi-2843147-114.patch1.25 MBWim Leers
#114 create_jsonapi_patch-114.sh_.txt1.44 KBWim Leers
#109 jsonapi-2843147-109.patch1.29 MBWim Leers
#109 create_jsonapi_patch-109.sh_.txt1.44 KBWim Leers
#106 jsonapi-2843147-106.patch1.29 MBgabesullice
#106 create_jsonapi_patch-106.sh_.txt1002 bytesgabesullice
#104 jsonapi-2843147-104.patch1.28 MBgabesullice
#104 create_jsonapi_patch-104.sh_.txt57.72 KBgabesullice
#103 jsonapi-2843147-103.patch1.29 MBWim Leers
#101 jsonapi-2843147-101.patch1.29 MBWim Leers
#101 create_jsonapi_patch-101.sh_.txt1.4 KBWim Leers
#99 jsonapi-2843147-99.patch1.15 MBWim Leers
#96 jsonapi-2843147-8.x-2.0-rc4-96.patch1.14 MBWim Leers
#96 create_jsonapi_patch-96.sh_.txt1.4 KBWim Leers
#95 jsonapi-2843147-2.0-rc3-95.patch1.06 MBWim Leers
#95 create_jsonapi_patch-95.sh_.txt1.4 KBWim Leers
#92 jsonapi-2843147-2.0-rc3.patch1.07 MBWim Leers
#83 jsonapi-2843147-83-b5d9167fde8db6d46cbabdcdd668d8af0e899067.patch1.06 MBWim Leers
#79 create_jsonapi_patch-79.sh_.txt1.4 KBWim Leers
#79 jsonapi-2843147-79-8.x-2.0-rc2.patch1.06 MBWim Leers
#78 create_jsonapi_patch-78.sh_.txt1.3 KBWim Leers
#78 jsonapi-2843147-78-8.x-2.0-rc2.patch1.07 MBWim Leers
#70 jsonapi-2843147-70--7865fae2a2e3d9c0f991e673356bde9dc5a410f1.patch1 MBWim Leers
#67 create_jsonapi_patch-67.sh_.txt1.3 KBWim Leers
#67 jsonapi-2843147-67--c5f7a5eeb8afb012d91c9500c3d741b84a01a962.patch1 MBWim Leers
#62 jsonapi-2843147-62--c034235345899cc998170e7f1aa576d7abdca0f6.patch1 MBWim Leers
#62 create_jsonapi_patch-62.sh_.txt1006 bytesWim Leers
#55 jsonapi-2843147-55.patch929.91 KBWim Leers
#55 create_jsonapi_patch-55.sh_.txt1.14 KBWim Leers
#53 jsonapi-2843147-53.patch929.93 KBWim Leers
#53 create_jsonapi_patch-53.sh_.txt1.14 KBWim Leers
#51 jsonapi-2843147-51.patch929.25 KBWim Leers
#51 create_jsonapi_patch-51.sh_.txt420 bytesWim Leers
#34 2843147--experimental-module-patch--34.patch408.75 KBe0ipso
#29 2843147--experimental-module-patch--29.patch408.92 KBe0ipso
#26 2843147--experimental-module-patch--26.patch4 MBe0ipso
#26 2843147--interdiff--24-26.txt42.96 KBe0ipso
#24 2843147--experimental-module-patch--24.patch376.67 KBdagmar
#21 2843147--experimental-module-patch--21.patch375.24 KBe0ipso
#21 2843147--interdiff--19-21.patch31.13 KBe0ipso
#19 2843147--experimental-module-patch--19.patch375.88 KBe0ipso
#19 2843147--interdiff--15-19.txt0 bytese0ipso
#18 2843147--experimental-module-patch--18.patch374.45 KBe0ipso
#18 2843147--interdiff--15-18.txt830 bytese0ipso
#15 2843147--interdiff--9-15.txt3.9 KBdagmar
#15 2843147--experimental-module-patch-15.patch375.88 KBdagmar
#12 2843147--experimental-module-patch--12.patch439.08 KBe0ipso
#12 2843147--interdiff--9-12.txt3.94 KBe0ipso
#9 2843147--experimental-module-patch--9.patch375.65 KBe0ipso
#9 2843147--interdiff--7-9.txt1.2 KBe0ipso
#7 2843147--experimental-module-patch--7.patch375.7 KBe0ipso
#5 Default Simpletest Runner (patches, contrib, d7) #298695 Console [Jenkins] 2017-01-13 12-11-51.png383.66 KBe0ipso
#2 2843147--experimental-module--2.patch374.31 KBe0ipso
Support from Acquia helps fund testing for Drupal Acquia logo

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

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-2] [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: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata 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: Include processed text in normalizations: "text" field type's "processed" computed property should be non-internal and carry cacheability metadata, 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
e0ipso’s picture

Status: Closed (outdated) » Active

This is back on the table.

webchick’s picture

Version: 8.4.x-dev » 8.6.x-dev
Related issues: +#2836165: New experimental module: JSON API
gabesullice’s picture

As of #2836165-48: New experimental module: JSON API, the idea of moving JSON API into core was accepted. Which I believe means that this is no longer a Feature Request, but a plan.

I've added 3 news tasks to the issue summary:

  1. Decide on stability of the module in core
  2. Provide a migration plan for users of the existing contrib module
  3. Provide feasible targets for core to "dog food" this API

The first is about whether it is in alpha/beta/RC or stable status. Personally, I believe that the module is at beta/RC or above. It already have a stable release in contrib (and has had one for a long time).

The second is about how sites which already have the module installed can uninstall the contrib version and install the core version. Fortunately, the JSON API module has zero-configuration and no public PHP APIs. This should be a very simple migration.

The third is, as I understand it, not a blocker for core inclusion except insofar as we have generally agreed upon plan. The JavaScript initiative already has a use case for this. They are working on a React.js implementation of the permissions page, which already makes use of the contrib JSON API module to determine which permissions are enabled for each role.

Finally, I've "unfinished" the provide a patch task because much has changed since the initial patch was provided. There are still just a small handful of ongoing issues (mostly test-related) in #2931785: The path for JSON API to core. Once those are complete, we can submit a new patch with the proposed "drop in" of the JSON API module into Drupal core (woot!)

gabesullice’s picture

Issue summary: View changes
gabesullice’s picture

Issue summary: View changes
e0ipso’s picture

I have some questions about the next steps in the process:

  1. Should we post in this issue saying that code review has started / about to start?
  2. How is the review going to happen if we keep adding code behind the scenes? Doesn't each commit potentially invalidate the review in progress? Do we need a static patch posted here?
  3. How can others contribute to the review?

I will halt non-essential issues from being committed in order to ease the review process and help prioritize essential fixes. Please let me know how to best assist with this.

webchick’s picture

I think probably the soonest we get a "needs review" patch over here, the better, generally-speaking. Transparency and visibility are important early in the development cycle. Ideally, this patch would come along with guidance on what types of review you're looking for: e.g. architectural, security, "particularly curious what people think about how this part foos the bazzes" or whatever. :) If you're worried about unnecessarily wasting peoples' time reviewing stuff that's about to be invalidated, feel free to also add whatever caveats (e.g. "this doesn't include blah and blah and blah issue yet" or "ignore all the stuff about blargle, we're ripping it out")

Usually what happens is people do the work over wherever they want (d.o sandbox, contrib module, github repo, whatever) and then update the main issue when there's something of note that's changed and worthy of another review. As it gets closer to "done" then this patch might get updated every commit.

Does that make sense?

e0ipso’s picture

It makes perfect sense. Thanks @webchick!

Wim Leers’s picture

Core committer @effulgentsia is already reviewing the JSON API module's code. I'll provide a core patch here, which will simply contain the JSON API module in its current state.

Wim Leers’s picture

Status: Active » Needs review
FileSize
420 bytes
929.25 KB

Here we go! 🎉

Ideally, this patch would come along with guidance on what types of review you're looking for: e.g. architectural, security, "particularly curious what people think about how this part foos the bazzes" or whatever. :)

@effulgentsia is already doing architectural review. Architectural review also includes/implies security review. Note that jsonapi.api.php documents the rationale behind the architecture, and describes what the actual API surface is!

We're aware that there are still a few fairly obvious gaps — that's why we have #2931785: The path for JSON API to core and have been working on addressing those. Now that the majority of those is addressed though, it makes sense to put this patch up for review.

(Note that in getting to this stage, we already did several JSON API security releases in the last few weeks: https://www.drupal.org/project/jsonapi/releases/8.x-1.10 + https://www.drupal.org/project/jsonapi/releases/8.x-1.14.)

Status: Needs review » Needs work

The last submitted patch, 51: jsonapi-2843147-51.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.14 KB
929.93 KB
  1. One of the failures is due to core/composer.json not being updated. Fixed. (And updated shell script.)
  2. Almost all of the failures are due to:
    Remaining deprecation notices (1)
    
      1x: \Drupal\Core\Routing\Enhancer\RouteEnhancerInterface is deprecated in Drupal 8.5.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal\Core\Routing\EnhancerInterface. See https://www.drupal.org/node/2894934
    

    Created #2957271: [>=8.5] Fix RouteEnhancerInterface deprecation errors for this. Until that's committed, letting the shell script apply that patch.

  3. 13 coding standards violations. Created #2957274: Fix coding standard violations: 8 according to JSON API's testing, 13 according to core's testing for this. Until that's committed, letting the shell script apply that patch.

Status: Needs review » Needs work

The last submitted patch, 53: jsonapi-2843147-53.patch, failed testing. View results

Wim Leers’s picture

So, #53:

  1. has 65 instead of 66 failures thanks to #53.1 🎉
  2. still has 65 failures for the same reason as #53.2, because I only updated one of the two route enhancers, oops! 😅 Fixed in #2957271-3: [>=8.5] Fix RouteEnhancerInterface deprecation errors, and updated the script to use that patch instead.
  3. 0 coding standards violations! 🎉
Wim Leers’s picture

Uhm, 113 passes, great, but that's not everything. Apparently that's all the functional JS tests.

Looks like something in testing is broken:

00:01:40.929   ERROR: PHPUnit testing framework version 6 or greater is required when running on PHP 7.2 or greater. Run the command 'composer run-script drupal-phpunit-upgrade' in order to fix this.

This is very strange. Retesting.

Status: Needs review » Needs work

The last submitted patch, 55: jsonapi-2843147-55.patch, failed testing. View results

Wim Leers’s picture

Now all tests ran. Weird.

The new failures are all also deprecation-related:

  742x: Method Symfony\Component\Serializer\Serializer::supportsNormalization() will have a third `$context = array()` argument in version 4.0. Not defining it is deprecated since Symfony 3.3.
    267x in BlockContentTest::testGetIndividual from Drupal\Tests\jsonapi\Functional
    267x in BlockContentTest::testPostIndividual from Drupal\Tests\jsonapi\Functional
    196x in BlockContentTest::testPatchIndividual from Drupal\Tests\jsonapi\Functional
    11x in BlockContentTest::testGetRelationships from Drupal\Tests\jsonapi\Functional
    1x in BlockContentTest::testDeleteIndividual from Drupal\Tests\jsonapi\Functional

  22x: Method Symfony\Component\Serializer\Serializer::supportsDenormalization() will have a fourth `$context = array()` argument in version 4.0. Not defining it is deprecated since Symfony 3.3.
    22x in BlockContentTest::testPatchIndividual from Drupal\Tests\jsonapi\Functional

Will fix those upstream as well.

e0ipso’s picture

Thanks for re-starting this @Wim Leers! Besides the composer thing I believe I also had to add VERSION to the .info.yml in the patches from a year ago. In addition to that we'll want to clarify if core modules can have their own composer.json or those should be bubbled up to core/composer.json.


supportsNormalization() will have a third `$context = array()` argument

Finally! I was tempted to do that in https://preview.npmjs.com/package/symfony-serializer a while ago.

dawehner’s picture

Just some small comments while skimming through the code.

  1. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,151 @@
    + *
    + * The JSON API module does provide a PHP API to generate a JSON API
    + * representation of entities:
    + *
    + * @code
    + * \Drupal::service('jsonapi.entity.to_jsonapi')->serialize($entity)
    + * @endcode
    + *
    

    To reduce the API surface of the core module I think it would be fine with moving this to jsonapi_extras. It is not something every decoupled site needs. This way new features can be added more easy

  2. +++ b/core/modules/jsonapi/jsonapi.module
    @@ -0,0 +1,59 @@
    +/**
    + * Implements hook_entity_base_field_info().
    + *
    + * @todo This should probably live in core, but for now we will keep it as a
    + * temporary solution. There are similar unresolved efforts already happening
    + * there.
    + *
    + * @see https://www.drupal.org/node/2825487
    + */
    +function jsonapi_entity_base_field_info(EntityTypeInterface $entity_type) {
    

    I just realized that this issue is closed alrady

Wim Leers’s picture

#60: Completely agreed on both counts!

  1. We've discussed this on a call with the maintainers, and agree, but it's technically a BC break, so it belongs in the next major version of JSON API: #2952293: Branch next major: version 2, requiring Drupal core >=8.5.
  2. I've been working on removing this in #2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it, but because it represents a BC break, we're thinking it belongs in the next major version of JSON API: #2952293: Branch next major: version 2, requiring Drupal core >=8.5.

So: both things are already in progress, but thank you for confirming that these should happen before JSON API is moved into core!

Wim Leers’s picture

Time to get this going again. Since #55, here's what happened:

  1. Latest release at #55: JSON API 1.14
  2. Latest release today: JSON API 1.22
  3. 69 commits: ($ git log --oneline --since "March 30 2018 14:21 CET" | wc -l)
  4. Comprehensive test coverage completed (#2953318: Comprehensive JSON API integration test coverage phase 4: collections, filtering and sorting + #2953321: Comprehensive JSON API integration test coverage phase 5: nested includes and sparse field sets + #2972808: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships)
  5. Getting the test coverage to that point revealed some security vulnerabilities (1.16), and many before it (1.14, 1.10 …)
  6. Ported many of the core REST improvements in the past 1.5 years to JSON API (1.15)
  7. Many, many, many bugfixes, and much, much clean-up for future maintainability (1.16, 1.17, 1.18, 1.19, 1.20, 1.21, 1.22)

That's a lot, isn't it? :)

But there's more! All of the above happened on the 8.x-1.x branch. As described in #2952293: Branch next major: version 2, requiring Drupal core >=8.5 (and mentioned in #61), we have many reasons to start a 8.x-2.x branch. (That branch was created months ago, but we kept them identical for months.)
Why wait so long? Because we wanted all >6000 JSON API users to be able to gently migrate from JSON API 1.x (on Drupal <=8.5) to JSON API 2.x (on Drupal >=8.5). And what better way to do that than to write comprehensive test coverage, and fixing all known problems that that surfaced?
That's what we've been doing the past few months! This massively reduces the risk of adding JSON API to Drupal core. We outlined a plan of must-have issues before going into Drupal core: #2931785: The path for JSON API to core — and they're all DONE as of today! Dozens of bugs have been flushed out and fixed before they ever entered core. Important: in the past 6–8 weeks we've noticed a steep drop in the number of bug reports and support requests that have been filed against the JSON API module!

After having been tasked with maturing core's REST API, and finding the less-than-great state that was in when Drupal 8 shipped, and having experienced how hard it is to improve it or even just fix bugs, this was a hard requirement for me. I hope it gives core committers the same feeling of relief as it gives me, to see that JSON API will on day one be in much better shape.

The other reason why it's in much better shape, is that the JSON API module now has no API surface other than the HTTP API! No PHP API (its sole API was dropped in the 2.x branch: #2982210: Move EntityToJsonApi service to JSON API Extras) at all, only the HTTP API as specified by http://jsonapi.org/format/.

TL;DR: JSON API in contrib today is more stable, more reliable, more feature-rich than core's REST API. And it does so while strongly complying with the JSON API spec: it's far less of a Drupalism than core's REST API.

So, with pride, and with lots of sweat (no blood and no tears fortunately), @gabesullice, @e0ipso and I present you this massively improved core patch! 🤘 💦 🎉 🍻 ❤️

EDIT: P.S.: 668K bytes of the 1.0M of bytes that this patch contains are for test coverage. That's 2/3rds!

Status: Needs review » Needs work
Wim Leers’s picture

Status: Needs work » Needs review

Most of those 48 fails aren't actual fails, but deprecation errors:

fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-1339.xml:
PHPunit Test failed to complete; Error: PHPUnit 6.5.8 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\jsonapi\Functional\ActionTest
...S...                                                             7 / 7 (100%)

Time: 1.14 minutes, Memory: 4.00MB

OK, but incomplete, skipped, or risky tests!
Tests: 7, Assertions: 194, Skipped: 1.

<SNIP>

Remaining deprecation notices (48)

  46x: Method Symfony\Component\Serializer\Serializer::supportsNormalization() will have a third `$context = array()` argument in version 4.0. Not defining it is deprecated since Symfony 3.3.
    35x in ActionTest::testCollection from Drupal\Tests\jsonapi\Functional
    11x in ActionTest::testGetIndividual from Drupal\Tests\jsonapi\Functional

  2x: Method Symfony\Component\Serializer\Serializer::supportsDenormalization() will have a fourth `$context = array()` argument in version 4.0. Not defining it is deprecated since Symfony 3.3.
    2x in ActionTest::testGetIndividual from Drupal\Tests\jsonapi\Functional

We'll get that fixed :)

Moving back to Needs review.

e0ipso’s picture

From the IS:

Decide on stability of the module in core

From #62:

JSON API in contrib today is more stable, more reliable, more feature-rich than core's REST API

I think that means that we'd want to attempt to include the JSON API module with a high stability level.


So, with pride, and with lots of sweat (no blood and no tears fortunately), @gabesullice, @e0ipso and I present you this massively improved core patch! 🤘 💦 🎉 🍻 ❤️

So much pride! This was a long journey, that I walked (almost) alone for a couple of years. Then @Wim Leers and @gabesullice joined and carried this to the finish line. Such a beautiful collaboration!

Wim Leers’s picture

I think that means that we'd want to attempt to include the JSON API module with a high stability level.

+1

I don't see why it would not be stable right from the start: https://www.drupal.org/core/experimental#stable. Precisely because it's already battle-hardened, comprehensively integration test-covered, spec-following-rather-than-NIH-Drupalism and very few bug reports are being filed. The API surface is only the HTTP API, and that is merely following http://jsonapi.org/format/ as closely as possible. So only changes that make it comply with the spec less would be BC breaks.

Wim Leers’s picture

Status: Needs review » Needs work
webchick’s picture

Wim Leers’s picture

5 failures due to deprecation errors remained, because #2982964: Add a drupalci.yml to JSON API to match Drupal core's and fix all surfaced deprecation errors only fixed those in 8.5. #2983196: Fix all deprecation errors in Drupal 8.6 just fixed those in 8.6 too.

Should be 100% green now!

xjm’s picture

(@effulgentsia and @xjm co-authored this comment.)

It's really awesome to see the progress here on JSON API!

@xjm and @effulgentsia discussed this with other core committers (@webchick, @Dries, @larowlan, @catch) and with the JSON API module maintainers. Based on what we learned in these discussions, we've decided to target this issue for an early feature in 8.7 rather than 8.6. Therefore, we will will set it 8.7 in a few days when we branch 8.7. Reviews and comments are still welcome in the meantime, whether in this issue, or as individual issues in the jsonapi issue queue.

Feel free to stop reading this comment here, or continue reading if you want to know why it's being bumped to 8.7.

First, we want to give a huge applause for everything that everyone working on the jsonapi contrib module has done. In the last 3-4 months alone (since 8.5.0 was released and #44 was written):

  • Over 100 issues in the contrib project have been closed.
  • There are currently only 36 open issues, only 7 of which are bug reports.
  • Per #62, the remaining bug fixes require breaking backwards compatibility for users of the 1.x module, so a final 1.x release has been released, and new features and BC-breaking bug fixes are now happening in the 2.x branch.
  • Also per #62, an amazing amount of test coverage has been written and correspondingly there's been a drop in new bug reports and support requests getting filed.
  • The module is now extremely well-documented, both in the API documentation and in the Drupal.org handbook.

Given all of the above, why not commit #70 to core now, prior to 8.6 alpha? Well,

  1. We generally prefer to commit significant new core features early in the release cycle for the minor, rather than toward the end. This means that this month and the next couple are the best time to commit 8.7.x features.
  2. To minimize the disruption to contrib, API consumers, and sites of moving a stable module from core to contrib, we'd like to have it as a stable module in 8.7.0, rather than an experimental module in 8.6.0.
  3. Per above, we're not yet done breaking BC. The mentioned spec compliance issues still need more work.
  4. While we're still potentially evolving the API, it's helpful to continue having the module in contrib for faster iteration and feedback.
  5. Since the 2.x branch of JSON API was just branched, there are virtually no sites using it yet (only 23 as compared with the 6000 using 1.x). An alpha release of JSON API 2.x once we're ready will give us some quick real-world testing of the final API that we're targeting for core.
  6. As @lauriii pointed out, an additional advantge of allowing a bit more time for API changes is that it allows more time for the Javascript Modernization Initiative, which depends on JSON API, to help validate that JSON API includes everything we need to have a fully decoupled admin frontend within Drupal core itself. (We wouldn't block the module addition on the other initiative, but it's an added bonus given the other reasons to target 8.7.)
  7. While the module has reached maturity in contrib, we still need the final reviews and signoffs for the core patch. Given the quality of the contrib module this should go well, but it is a 1 MB patch (with 668K of tests, but that still means 300K+ of code to review.) :) We want to give our review of this code the attention it deserves.

None of the above aside from the last point are hard blockers to adding an experimental module to core. Users who prefer the stability of the 1.x module could continue to use it from contrib, thereby overriding the one in core. However, in the case of jsonapi, I think there's something odd about telling site builders to experiment with the one in core, but if they want to use it in production, to downgrade to the one in contrib. I think that people who are actually interested in using jsonapi on their sites would be better off going to the contrib project page and making an explicit 1.x or 2.x decision from there.

Meanwhile, we see what issues, if any, people run into when upgrading from 1.x to 2.x. When we're ready to commit it to core, we'll consider it at least beta stability (rather than alpha).

Once again, really fantastic work here.

e0ipso’s picture

Title: Add experimental JSON API module » Add JSON API as stable module into core

Changing title based on

To minimize the disruption to contrib, API consumers, and sites of moving a stable module from core to contrib, we'd like to have it as a stable module in 8.7.0, rather than an experimental module in 8.6.0.

Wim Leers’s picture

Those hopefully reach people that aren't following this issue!

pwolanin’s picture

I'm surprised this is considered viable without being able to create a new revision of a node (or have it created by default). That's a basic need for tracking authorship and compliance.

Wim Leers’s picture

I'm surprised this is considered viable without being able to create a new revision of a node (or have it created by default). That's a basic need for tracking authorship and compliance.

I understand it's frustrating that this isn't supported yet. But Drupal 8 core shipped with the rest module that was far less capable than the jsonapi module in its current state. The issue for adding revision support to REST (#2946972: EntityResource: revisions support) has almost no followers, and in fact, it was only created 4.5 months ago. At least the JSON API module is structured in such a way that we can layer on revision support later; in core REST this is much more difficult to achieve.

In other words: why block JSON API on a feature that REST hasn't had for years, whereas JSON API already has many capabilities that REST doesn't have, and has a better DX?

That being said: we will do our utmost best to get #2969022: Support content revisioning completed before JSON API is added to Drupal 8.7! My point is just that it doesn't make sense to postpone JSON API going into core until it achieves a North Star State.

Also: thank you for speaking up! ❤️🎉 The above is just my opinion. This sort of constructive criticism is EXACTLY what this issue needs! Please add more of it! Hold us accountable. Help ensure what this issue does makes sense for Drupal at large.

e0ipso’s picture

The above is just my opinion.

I fully agree on the thoughts shared by @Wim Leers in #75.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Title: Add JSON API as stable module into core » Add JSON:API as stable module into core
FileSize
1.07 MB
1.3 KB

I last got this going again in #62, 8 days shy of 5 months ago.

We released JSON:API 2.0-RC2 late last night: https://www.drupal.org/project/jsonapi/releases/8.x-2.0-rc2. We believe it'll be the last RC before we release 2.0. So it's time we have a core patch here again.

As explained in https://wimleers.com/blog/state-of-jsonapi-2018-10, JSON:API 2.x:

  1. is 100% spec compliant, strictly so, to ensure future maintainability and evolvability (no open spec compliance issues!, addresses #71.3)
  2. has significantly improved performance
  3. has even better test coverage
  4. has 0 known bugs! (except for 2 which are duplicates of core bugs)
  5. is ready for new features, such as file uploads (100% done) and revisions (90% done)
  6. has 0 known blockers for the Admin UI & JavaScript Modernisation Initiative (@drpal said so last week), and they're looking forward to revision support
  7. is therefore definitely ready in my humble opinion to go in as stable (addresses #71.2) — what we've been doing for the past 11 months is effectively making the public API surface rock solid
  8. has >300 sites using it today and >200 for over a month (addresses #71.5)

So that brings us to the key remaining point: #71.7:

While the module has reached maturity in contrib, we still need the final reviews and signoffs for the core patch. Given the quality of the contrib module this should go well, but it is a 1 MB patch (with 668K of tests, but that still means 300K+ of code to review.) :) We want to give our review of this code the attention it deserves.

Let's get this going! (I know that @effulgentsia already started doing this, but I don't know how far he got.)

So here's a new core patch, adding JSON API 2.0-RC2 on top of core 5731bc7dab92a79d6292be65b156c91e5fa7ebea. 676KB of the 1090KB patch is test coverage.

Wim Leers’s picture

Shortly after posting it, I realized that this patch could be simpler:

  1. drupalci.yml is not necessary
  2. A specific core requirement is not necessary
  3. A bunch of BC layers (for 8.5 and 8.6) are no longer necessary.

Created a patch for points 2 and 3 at #3015325: [ignore] support issue for the core patch. Updated the shell script to perform point 1 and apply the patch for points 2 and 3.

The patch is now a bit smaller: 1083 KB instead of 1090 KB.

The last submitted patch, 78: jsonapi-2843147-78-8.x-2.0-rc2.patch, failed testing. View results

Wim Leers’s picture

Hah, the wonderfully precise hook_help() test coverage detected that a few instances of JSON API were left, that should've been changed to JSON:API. Thanks, @jhodgdon! This single failure will be fixed at #3015343: Follow-up for #3007274: s/JSON API/JSON:API/ in *.module files, once that lands, I'll update the patch here.

Status: Needs review » Needs work

The last submitted patch, 79: jsonapi-2843147-79-8.x-2.0-rc2.patch, failed testing. View results

Wim Leers’s picture

#3015343: Follow-up for #3007274: s/JSON API/JSON:API/ in *.module files landed. Commit b5d9167fde8db6d46cbabdcdd668d8af0e899067. This patch should then be green :)

Status: Needs review » Needs work
Wim Leers’s picture

Status: Needs work » Needs review

A failure in the PhpTransliterationTest unit test. Huh?! I don't see how this could be caused by JSON:API. Seems like a random failure. Retesting.

effulgentsia’s picture

Category: Plan » Feature request
Priority: Normal » Major

Awesome progress!

Although this issue hasn't yet gotten the formal sign-offs for the Plan, #71 is at the very least an informal approval from both a release manager and a framework manager, and the iteration of patches since then I think squarely puts this in the Build phase of the info graphic in https://www.drupal.org/core/community-initiatives#approval-process. Therefore, I'm recategorizing this as a Feature request, but leaving the "Needs framework manager review" and "Needs release manager review" tags until those reviews are done.

Also, raising the priority to Major, since it's the top item in the API-first roadmap for 8.7.

e0ipso’s picture

Exciting times!

Although this issue hasn't yet gotten the formal sign-offs for the Plan, #71 is at the very least an informal approval

Do we still need formal sign-off for this?

borisson_’s picture

I had a couple of minutes over lunch to glance at this, I got maybe ~5% into this patch but I found some nits to pick, not sure if I should've created a jsonapi issue for this instead.

  1. +++ b/core/modules/jsonapi/jsonapi.module
    @@ -0,0 +1,82 @@
    +    // @todo: only do this when relationship fields are updated, not just any field.
    +    JsonApiRoutes::rebuild();
    ...
    +    // @todo: only do this when relationship fields are updated, not just any field.
    +    JsonApiRoutes::rebuild();
    

    Usually @todo comments are linked to a drupal.org issue, so we can track when the todo needs to be removed and we know that this is still an open issue. One should be created and linked here.

    Also, the comment is > 80cols long.

  2. +++ b/core/modules/jsonapi/jsonapi.services.yml
    @@ -0,0 +1,199 @@
    +  jsonapi.serializer_do_not_use_removal_imminent:
    +    class: Drupal\jsonapi\Serializer\Serializer
    +    calls:
    +      - [setFallbackNormalizer, ['@serializer']]
    +    arguments: [{  }, {  }]
    

    Does this mean that this is part of the bc layer? Should this be removed instead?

  3. +++ b/core/modules/jsonapi/jsonapi.services.yml
    @@ -0,0 +1,199 @@
    +  # Deprecated services.
    +  serializer.normalizer.htt_exception.jsonapi:
    +    alias: serializer.normalizer.http_exception.jsonapi
    +    deprecated: The "%service_id%" service is deprecated. You should use the 'serializer.normalizer.http_exception.jsonapi' service instead.
    

    Should we remove this before we get this in core?

  4. +++ b/core/modules/jsonapi/jsonapi.services.yml
    @@ -0,0 +1,199 @@
    +  # Forward compatibility.
    +  # @todo Remove in Drupal 8.6 (assuming it contains https://www.drupal.org/project/drupal/issues/2926508).
    

    Comment should be updated, and should link to a new issue (I think, or the one linked from here should be updated)

Wim Leers’s picture

I had a couple of minutes over lunch to glance at this

Thanks!

I found some nits to pick, not sure if I should've created a jsonapi issue for this instead.

Strictly speaking that's not necessary; all problems (and nits) reported here I have been fixing (and will continue to do) in the contrib module. But for the sake of focus, it would be wonderful if nits could be reported to the JSON:API issue queue, to keep the conversation in this issue about the things that make a material difference. So ideally this issue would primarily cover architectural questions.

effulgentsia’s picture

Do we still need formal sign-off for this?

Good question. Thanks for asking. The implementation still needs framework and release manager review, hence those tags.

In terms of a Plan, the things that could be considered part of the planning, per https://www.drupal.org/core/initiative-proposal-template, include:

  • If this is a large undertaking, is it sufficiently resourced? At this point, the patch is already nearly done, so the question is moot.
  • Are there core issues that are required to be fixed as part of this (e.g., blockers and/or bugs that would be made worse by having this in core)? I think at this point, the answer to this is no.
  • What significant features are must-have, vs. should-have, could-have, etc.? That's sometimes helpful to get alignment on before writing all the code, but at this point, those questions have all been decided in the contrib queue, so the patch here can just be reviewed as-is.
  • What will be supported for / required of the current users of the contrib module? I think it would be good for the issue summary to be updated to include this (i.e., expand the Provide a migration plan for users of the existing contrib module. bullet point). For example, is the plan to mark the 1.x branch unsupported prior to 8.7's release, and essentially make it impossible to upgrade core to 8.7 without first upgrading contrib jsonapi to 2.x? Or is there a need and way to keep 1.x supported beyond the 8.7 release?
  • The issue summary currently has a bullet point for Propose a plan for core to "dog food" this new API. I'd actually be fine with removing that from the scope of this issue. In my opinion, we can commit this to core prior to any such plan existing. Anyone disagree with that?

Given the above, I think for this issue, we don't need a separate step of signing off on the plan prior to signing off on the patch. But I'm adding the "Needs issue summary update" tag for the question about contrib migration.

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

#90: thanks! Updated the issue summary.

Wim Leers’s picture

We released JSON:API 2.0-RC3 last night: https://www.drupal.org/project/jsonapi/releases/8.x-2.0-rc3.

So here's a new core patch, adding JSON API 2.0-RC3 on top of core 55f13aacb5a348c5db49042d2abca04bc126eda4.

Wim Leers’s picture

--- /dev/null
+++ b/core/modules/jsonapi/src/ForwardCompatibility/Normalizer/DateTimeIso8601Normalizer.php
…
--- /dev/null
+++ b/core/modules/jsonapi/src/ForwardCompatibility/Normalizer/DateTimeNormalizer.php
…
--- /dev/null
+++ b/core/modules/jsonapi/src/ForwardCompatibility/Normalizer/TimestampNormalizer.php

These classes are "forward compatibility classes" — they are being added to core in #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API. They will be removed from this patch once #2926508 lands. #2926508 was blocked on #3002164: DateTimeIso8601::getDateTime() does not specify the storage timezone when constructing DrupalDateTime object, which landed yesterday! 🎉

This means #2926508 is a blocker for this issue.

#2926508 makes@FieldType=timestamp + @FieldType=datetime + @FieldType=daterange consistent, by making @DataType=timestamp + @DataType=datetime_iso8601 consistent.

jibran’s picture

First of all, I'm so happy that jsonapi is going to be part of Drupal core. There are only 16 open bugs/tasks under jsonapi 2.x branch which shows the hardwork and dedication of jsonapi maintainers. 👏🏽👏🏽👏🏽👏🏽👏🏽

All the generic Normalizers should be moved to Core or appropriate modules before this module goes stable in Drupal core.

Here are some observations:

  1. +++ b/core/modules/jsonapi/jsonapi.module
    @@ -0,0 +1,82 @@
    +  if (floatval(\Drupal::VERSION) < 8.6 && $entity_type->id() == 'taxonomy_term') {
    

    Core doesn't have to support this. I think this can remain jsonapi 2.x branch.

  2. +++ b/core/modules/jsonapi/jsonapi.services.yml
    @@ -0,0 +1,206 @@
    +  jsonapi.serializer_do_not_use_removal_imminent:
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent, priority: 1 }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent, priority: 1 }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent }
    ...
    +      - { name: jsonapi_normalizer_do_not_use_removal_imminent, priority: 1 }
    ...
    +    arguments: ['@jsonapi.serializer_do_not_use_removal_imminent', '%serializer.formats%']
    ...
    +    arguments: ['@jsonapi.serializer_do_not_use_removal_imminent']
    ...
    +    arguments: ['@jsonapi.serializer_do_not_use_removal_imminent', '@logger.channel.jsonapi', '@module_handler', '@app.root']
    

    Are we going to remove there before adding the module to the core? I scan the jsonapi issue queue and unable to find any related issue.

  3. +++ b/core/modules/jsonapi/src/JsonapiServiceProvider.php
    @@ -0,0 +1,55 @@
    +    // @todo Remove when we stop supporting Drupal 8.5.
    +    if (floatval(\Drupal::VERSION) < 8.6) {
    

    Same as above.
    Core doesn't have to support this. I think this can remain jsonapi 2.x branch.

  4. /jsonapi
    

    In the links section of the response, we show all the available(non internal) entities and bundles on the site. Isn't it information disclosure to show all the available entities and bundles of a site?

Wim Leers’s picture

Thanks for the review! 🙏 🎉

There are only 16 open bugs/tasks under jsonapi 2.x branch

Yep! And many of them are actually about refactoring to make things even better in the future. :) There's only one open bug report for something very small.
We indeed worked very hard to fix all bugs and refactor the code to be as maintainable as possible.

  1. Good catch! Updated the patch in #3015325: [ignore] support issue for the core patch to remove all 7 of these. We recently had to add more core minor-specific expectations/work-arounds. We indeed don't need them in core :)
  2. Excellent question 👍 The issues you're looking for are #2994473: [META] JSON API's normalizers support schema tracking, to guarantee comprehensive schema + #3014283: Use ResourceTypeFields to statically determine the field normalizer + #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem. The problem is that the Symfony serialization component that core uses and JSON:API also uses is insufficient in several respects. That's why many months ago, in early 2018, even in JSON:API 1.x, we locked down the ability to add JSON:API-specific @FieldType normalizers. First we were only warning (hat is what you're seeing there), then we made it actually impossible/unsupported (see \Drupal\jsonapi\Serializer\Serializer::__construct()) in JSON:API 2.x. That tag name is just an internal implementation detail at this point. It's locked down. It's not an API. Nobody can use it. We might as well rename the tag to foobarbaz and it'd still not be an API change. We've worked hard to ensure that all @FieldType-level normalizer logic is not an API.
    How do you then add JSON:API support for your own field types, I hear you thinking? Well, field types in the end always use properties (attributes in JSON:API terminology — and this is also why we don't allow per-field type normalizers: to ensure they're all normalized consistently, complying with the JSON:API spec). Properties in Drupal's Entity/Field API are modeled as @DataType plugins. And data type normalizers are supported/respected by JSON:API. The great thing is that those are format-agnostic: they work for both core REST and JSON:API. For more than a year, we've been shifting all of Drupal's API-First infrastructure in that direction — see #2926507: Discourage @FieldType-level normalizers, encourage @DataType-level normalizers, to strengthen the API-First ecosystem.
  3. See 1.
  4. Great question! 👏 No, it isn't. 🙂 Knowing which concepts are accessible via the API is one thing, actually being able to read/view them is another. The same question was raised for when cache tags were added to core and exposed via a HTTP header. That allows you to know that those things exist, but it still doesn't allow you to read them. If it'd be a problem here, then we'd need to rip out the cache tags headers too.

Updated core patch and script, adding JSON API 2.0-RC3 + #3015325-8: [ignore] support issue for the core patch on top of core b2160caa3dcf867d4850429d9ea9f693cddc04b5.

Wim Leers’s picture

With https://www.drupal.org/sa-contrib-2018-081 now shipped in https://www.drupal.org/project/jsonapi/releases/8.x-2.0-rc4, there now finally is no longer this only-known-by-core-committes hidden blocker. The only remaining blocker is publicly visible and already is RTBC: #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API.
RC4 will become 2.0. We will ship 2.0 in the first week of January 2019 rather than today because it'll reach more people — many people are taking time off for the holidays in the next week.

Updated core patch and script, adding JSON API 2.0-RC4 + #3015325-12: [ignore] support issue for the core patch on top of core bda5967ecab9efc707b09a4570cc20bd8d61a0c3.

dww’s picture

Exciting! Great work. Thanks for all your efforts, the great approach this whole initiative has been using, and the solid results!

Started reading the enormous patch, and found some extremely minor nits in core/modules/jsonapi/jsonapi.api.php:

+ * specification. By its own definition, the JSON:API specification is "is a

double "is". ;)

+ * a later specification. Remember that he specification stipulates that future

s/he/the/

Otherwise, I love the jasonapi.api.php file and what it represents. ;) No more time today for further reviews, but this is clearly very close to RTBC.

Thanks!
-Derek

Wim Leers’s picture

#97: On behalf of all API-First Initiative contributors: thanks! — it means a lot coming from you! 😊 I'll make sure to get those nits fixed upstream — issue created: #3021873: Two nits in jsonapi.api.php.

Wim Leers’s picture

JSON:API 2.0 (stable!) was just released: https://www.drupal.org/project/jsonapi/releases/8.x-2.0. 🎉

So here's a new core patch, adding JSON API 2.0 on top of core a013f0538cca1edabc4a59f7d3cf09055247f194.

Wim Leers’s picture

Issue summary: View changes

I noticed that one portion of the contrib migration question in #90 was not yet answered in the issue summary. Fixed that.

Wim Leers’s picture

JSON:API 2.1 was just released: https://www.drupal.org/project/jsonapi/releases/8.x-2.1. Updated core patch using that release, on top of core d6688ebf3c536178531ff01959057f1c4dc1785d.

The increase in patch size is because of two new features (file uploads & revisions), which come with a lot of test coverage. The file uploads functionality includes a forward port of #2940383: Unify file upload logic of REST and JSON:API.

EDIT: oh and FYI: 1700 sites are now running JSON:API 2.x, up from 400 a month ago — see https://wimleers.com/blog/state-of-jsonapi-2019-01.

effulgentsia’s picture

Status: Needs review » Needs work

Needs work for the PHP 5.6 failure (see #101's test results).

Wim Leers’s picture

Fixed in #3027501: [regression] Follow-up for #2995960 and #2992833: syntax errors in PHP 5.5 & 5.6. This core patch is JSON:API 2.1 + #3027501 (JSON:API ea0633233a5c4b81683959dd8398741b93a41d5c) on top of the same core commit.

Queued tests against PHP 5.5, 5.6, 7.0, 7.1 and 7.2. Also queued tests against SQLite & PostgresQL.

This sadly went unnoticed because despite our repeated attempts to configure automated testing to use the PHP versions we specify, it keeps changing it. Testing against multiple core minors and/or multiple PHP versions doesn't seem to work reliably. 😔 I've reported this before. It probably isn't a high priority right now, because the DA is working on the GitLab integration.

gabesullice’s picture

The two failures in #103 were caused by:

  1. The patch failing to update Drupal's composer.lock file (I believe)
  2. Some of JSON:API's tests had an implicit dependency on the sort order of MySQL 5.5 when no sort is explicitly provided to the query. MySQL 8 and PostgreSQL do not have the same default ordering. This has been addressed by #3027626: Add sorts to our collection tests that are inspecting values in the collection, to not be dependent on per-DB default order

If there is a preferred way to update composer dependencies in a patch like this, please let us know!

Status: Needs review » Needs work

The last submitted patch, 104: jsonapi-2843147-104.patch, failed testing. View results

gabesullice’s picture

Looks like I still hadn't gotten the composer thing right in #104. I got on a call with @effulgentsia, who helped me figure out the right way to do that (thanks!)

Status: Needs review » Needs work

The last submitted patch, 106: jsonapi-2843147-106.patch, failed testing. View results

effulgentsia’s picture

pushd core; composer require -v --dev --no-update justinrainbow/json-schema; popd

This is great. It's adding that package dependency to core/composer.json correctly, but the remaining test failure is due to nothing yet adding the "drupal/jsonapi": "self.version", line. (The #103 patch has it, but I don't know if/how that was automated.)

Wim Leers’s picture

#108: Yep, #104's update to the script lost the manual update to core's composer.json, which is declaring the presence of the jsonapi module.

effulgentsia’s picture

Status: Needs review » Needs work
Issue tags: -Needs framework manager review

I've now spent enough time reading over the module code that I now feel comfortable removing the "Needs framework manager review" tag!

In the course of doing so, a few significant WTFs popped out at me. I've made each of them child issues of this one. You can look at the live list of them in the Child issues block in the right sidebar, but here's the current snapshot:

I or other reviewers may find more between now and RTBC, and between RTBC and commit.

As a tip for anyone else wanting to help review, I find it easiest to review the 2.x branch tip of the contrib module, with #3015325: [ignore] support issue for the core patch applied on top (which removes some legacy Drupal 8.5 and 8.6 cruft), rather than the patch on this issue. That way, as things get committed there, you're always reviewing the latest. That said, I'm also setting this to NW as there have been commits there since #109, so the patch here could use a refresh.

I truly appreciate all the great work being done by the JSON:API maintainers, reviewers, testers, issue filers, and all other contributors, and look forward to this landing in core!!!

e0ipso’s picture

@effulgentsia thanks for the in-depth review! Hopefully we can get those issues merged soon and then have this patch updated.

SO CLOSE! 🎉

effulgentsia’s picture

All the issues in #110 are now fixed except for #3029897: [PP-1] Do not overload Symfony's normalize() method to return objects; define a new interface for that., which will take a little while to untangle its dependencies. So, I think the next step here is to upload an up-to-date patch with where we're currently at. Either before or after that's uploaded, I'd like to add some docs that explain where the JSON:API normalizer architecture currently is, what's being attempted in #3029897: [PP-1] Do not overload Symfony's normalize() method to return objects; define a new interface for that., and what the eventual goal is in #3028080: Add CacheableNormalization for Normalizer to return Normalized value and Cacheablity. I think those docs will be both helpful to any additional people reviewing this patch, and to release managers in evaluating the "Needs release manager review" tag for this issue, from the perspective that until those issues are complete, we're not ready to commit to backwards compatibility (of the normalizer API) for the very small set of modules (like JSON:API Extras) that require adjusting the JSON:API output at any level other than a @DataType normalizer (which is the only level for which there is a supported public API). I'll work on those docs tomorrow.

As long as a release manager approves, I'm fine with committing this to core with that work still undone and without the corresponding BC promise, as long as there's discoverable issues open (which there are) for eventually getting to a supported public API for adding normalizers to any level (or otherwise customizing that level's output) for which there are known legitimate use-cases to do so.

effulgentsia’s picture

Of likely interest to release managers for when they review this: #3032787: [META] Start creating the public PHP API of the JSON:API module

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.44 KB
1.25 MB

New core patch, using jsonapi commit 8b164fbaf82257e26e47bf03b9cc4b6f95cfa3f4. Patch size is smaller because #3015325: [ignore] support issue for the core patch is now removing even more, thanks to @effulgentsia!


I'm not going to reiterate the benefits of JSON:API over REST, we had #2836165: New experimental module: JSON API for that. I want to provide some context about the depth and scope of review the JSON:API module has received.

The short version: I've been nudging it towards core-level quality for about two years. I am confident that not only is JSON:API simpler to set up, richer in features and more pleasant to use, it also is far better prepared for the future thanks to both the spec it follows and the module's code being designed for future evolution. More detail below.

I believe this should be RTBC, because:

I opened #2829327: Review JSON API module for core-readiness on November 21, 2016. In January 2017 I finished the initial review of the code base, resulting in #2842148: The path for JSON API to "super stability".

At the time, I had spent the better part of a year making core's REST API stable-ish (I blogged about my first week with core's REST API, shortly after D8 shipped: https://wimleers.com/blog/restless-week).

The JSON:API module had only been around for a number of months at that time: @e0ipso was working a lot on it after his dayjob hours (the first commit took place in May 2016, the first alpha was in October 2016). Initially, it was not much more than some overrides for core's HAL+JSON format. (But to support the full feature set and potential of https://jsonapi.org/format/, the REST module's architecture was too simplistic.)

Rather than helping out with stabilizing JSON:API, I had to instead stabilize Drupal 8's REST API, which suffered from an overwhelming number of bugs. However, many of those bugs were not in the REST module itself, but in the underlying subsystems (Entity API, Field API, Typed Data API, routing system, request processing system, and so on). Those subsystems were originally not designed to power HTTP APIs, so there were some leaky abstractions, some wrong assumptions, and so on.

Given those known problems, it didn't make sense to work on getting JSON:API into Drupal core if we knew it was going to suffer from those same problems, and it'd effectively double the number of BC layers that core would have to provide.

So, for most of 2016, 2017 and a good chunk of 2018, we shipped lots of bugfixes in Drupal 8.2, 8.3, 8.4, 8.5 and 8.6. In 8.5 we reached a good level of maturity and 8.6, we finally were able to mark rest.module "maintainable" (~45 open issues in May 2018 and the same number today, with very little effort).

I'd estimate that at least half of the effort in maturing rest.module involved improving the underlying foundations. Fixing those underlying foundations also helped https://www.drupal.org/project/jsonapi and https://www.drupal.org/project/graphql. For example: just last week we landed #3026264: UserAccessControlHandler::checkFieldAccess() denies access to mail field without varying by the ‘user’ cache context, which affects all three!

In leading that effort, I've also learned a lot about unforeseen consequences in the design choices made for the REST module, and I was determined to help ensure we wouldn't repeat those. So that's the summary of the last year of my professional life: ensuring JSON:API is much more stable for API clients (wrt both bugs and BC breaks) than the REST module could possibly be, despite a richer feature set and while still allowing the internals to be changed so new features can be added. The recently added revision support was as uneventful as it should be and a nice validation of that :)

This means that the JSON:API is already significantly more mature than rest.module was in Drupal 8.0.0! Combined with the fact that it provides far more capabilities, even more test coverage than the REST module and empowers the Drupal ecosystem to do even more interesting things, I can't wait to see where we take this!

@effulgentsia said given that extensive scrutiny that we've applied and have effectively been preparing for core for more than a year, it'd be fair for us to RTBC this patch. So … 🦙🛥😎🍻

effulgentsia’s picture

@effulgentsia said given that extensive scrutiny that we've applied and have effectively been preparing for core for more than a year, it'd be fair for us to RTBC this patch.

Yep. Specifically, I've seen the 3 primary JSON:API maintainers: @Wim Leers, @gabesullice, and @e0ipso, thoroughly reviewing each other's work in the project queue, so +1 to the RTBC. Speaking of which, next time this patch is rerolled, can we add a MAINTAINERS.txt entry to the patch? Are all 3 of you staying on as maintainers once this is in core?

xjm’s picture

Amazing to see this RTBC! Starting a release management review:

(Edit: Moving the BC policy bit into its own section.)

BC policy

+++ b/core/modules/jsonapi/jsonapi.api.php
@@ -0,0 +1,369 @@
+ * HTTP API: URLs and JSON response structures are considered part of this
+ * module's public API. However, inconsistencies with the JSON:API specification
+ * will be considered bugs. Fixes which bring the module into compliance with
+ * the specification are *not* guaranteed to be backwards compatible.
+ *
+ * What this means for developing consumers of the HTTP API is that *clients
+ * should be implemented from the specification first and foremost.* This should
+ * mitigate implicit dependencies on implementation details or inconsistencies
+ * with the specification that are specific to this module.
+ *
+ * To help develop compatible clients, every response indicates the version of
+ * the JSON:API specification used under its "jsonapi" key. Future releases
+ * *may* increment the minor version number if the module implements features of
+ * a later specification. Remember that the specification stipulates that future
+ * versions *will* remain backwards compatible as only additions may be
+ * released.

Interesting. I think we probably need to discuss/verify that we're okay with this for BC policy for this module in core.

(Nit: "backwards-compatible" with a hyphen.)

Added dependencies

+++ b/core/composer.json
@@ -63,7 +63,8 @@
+        "justinrainbow/json-schema": "^5.2"

We should evaluate this package against the following criteria:

  • Code quality
  • Maintainership of the package
  • Security policies of the package
  • Expected release and support cycles
  • Other dependencies it'd add, if any

This information can be added to the issue summary.

Documentation gate

The in-code API documentation is excellent overall!

  1. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * @see http://jsonapi.org/
    + *
    + *
    + * @section resources Resources
    

    Double blank lines in code fail our phpcs rules. TBD if double blank lines in docblocks do, but I assume the principle is the same.

  2. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * The JSON:API specification requires special handling for resources
    + * (entities), relationships between those resources (entity references) and
    + * resource IDs (entity UUIDs), it must override some of the Serialization
    + * module's normalizers for entities and fields (most notably, entity
    

    Run-on sentence here. I think this is supposed to say:

    ...special handling for resources (entitties), relationships between those resoures (entity references), and resource IDs (entity UUIDs). It must override some of the Serialization module's normalizers...

  3. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * normalizers at the "DataType" plugin level. This is a level below "FieldType"
    + * plugins. Normalizers which are not implemented at this level will not be used
    + * by the JSON:API module.
    + *
    + * A benefit of implementing normalizers at this lower level is that they will
    + * work automatically for both the JSON:API module and core's REST module.
    

    "Plugin levels" is unclear / not a familiar concept. I think this is talking about inheritance. If so, can we talk about it more directly, i.e. they must implement normalizers that extend DataTypeSomethingSomething or that implement DataTypeSomethingSomethingInterface?

    The use of "level below" and "lower level" is part of what's confusing. "Level below" implies a level down the inheritance chain (I think?) but "lower level" usually refers to code that runs earlier.

  4. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * @section revisions Resource Versioning
    

    Nit: "versioning" should be lowercase.

  5. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * still specification compliant).
    

    Nit: specification-compliant.

  6. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * In doing so, JSON:API module should be maximally compatible with other
    + * systems.
    

    In doing so... In doing what? Not clear from context. Would be better as "By [implementing revision support in this fashion]..." (Is that what this is saying?)

  7. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * A version identifier is a string with enough information to load a
    + * particular revision. The version negotiator component names the negotiation
    + * mechanism for loading a revision. Currently, this can be either @code id
    + * @endcode or @code rel @endcode. The @code id @endcode negotiator takes a
    + * version argument which is the desired revision ID. The @code rel @endcode
    + * negotiator takes a version argument which is either the string @code
    + * latest-version @endcode or the string @code working-copy @endcode.
    

    @code and @endcode can't be used inline like this. On api.d.o this will split the paragraph up with each code snippet getting a whole new div. It's best to use single quotes instead for inline docs like this, or backticks if single quotes are being used already in a different way.

  8. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * In future, other negotiators may be developed. For instance, a negotiator
    + * which is UUID, timestamp or workspace based.
    

    This is unclear (maybe just on account of punctuation) and has a typo. Is the following accurate?

    In the future, other negotiatiors may be developed, such as negotiatiors that are UUID-, timestamp-, or workspace-based.

  9. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * @see https://www.drupal.org/project/jsonapi/issues/3009588.
    + *
    + * @see https://tools.ietf.org/html/rfc5829
    

    The blank line between these will probably need to be removed.

  10. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * - Custom field normalization is not supported; only normalizers at the
    + *   "DataType" plugin level are supported (these are a level below field
    + *   types).
    

    Same question as above about "plugin levels".

  11. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * The integration tests test the same common cases and edge cases using
    + * @code \Drupal\Tests\jsonapi\Functional\ResourceTestBase @endcode, which is a
    + * base class subclassed for every entity type that Drupal core ships with. It
    + * is ensured that 100% of Drupal core's entity types are tested thanks to
    + * @code \Drupal\Tests\jsonapi\Functional\TestCoverageTest @endcode.
    

    Same note about @code blocks. For class, function/method, and variable names, we can just use them inline with no delimiter.

  12. +++ b/core/modules/jsonapi/jsonapi.api.php
    @@ -0,0 +1,369 @@
    + * Please note, *normalizers are internal implementation details.* While
    

    We don't use the word "Please" in docs or UI text. It almost all cases the word can be removed. Here I would just say "Note that normalizers are..."

  13. +++ b/core/modules/jsonapi/jsonapi.info.yml
    @@ -0,0 +1,7 @@
    +description: Provides a JSON:API standards-compliant API for accessing and manipulating Drupal content and configuration entities.
    
    new file mode 100644
    index 0000000000..f1256bfb69
    

    This could use a little work as site builder-facing documentation. How about:
    "Exposes entities as a JSON:API-specification-compliant web API."

    Edit: changed my recommendation above a little more; site builders don't need to care that content and configuration entities are two different things.

    We'll also want to consider how to help the user choose between this and REST given just their two descriptions, but I think the above helps with that.

  14. +++ b/core/modules/jsonapi/jsonapi.module
    @@ -0,0 +1,303 @@
    +      $output .= '<p>' . t('The JSON:API module is a fully compliant implementation of the <a href=":spec">JSON:API Specification</a>. By following shared conventions, you can increase productivity, take advantage of generalized tooling, and focus on what matters: your application. Clients built around JSON:API are able to take advantage of its features such as efficiently caching responses, sometimes eliminating network requests entirely. For more information, see the <a href=":docs">online documentation for the JSON:API module</a>.', [
    

    The third sentence here is a bit run-on and hard to read. How about:

    Clients built around JSON:API are able to take advantage of features like efficient response caching, which can sometimes eliminate network requrests entirely.

  15. +++ b/core/modules/jsonapi/jsonapi.module
    @@ -0,0 +1,303 @@
    +        ':docs' => 'https://www.youtube.com/playlist?list=PLZOQ_ZMpYrZsyO-3IstImK1okrpfAjuMZ',
    

    Hmmm, usually this should link to a handbook page on Drupal.org. I don't think linking a youtube video is very accessible. We should instead create that d.o handbook page and can link or embed the video there.

  16. +++ b/core/modules/jsonapi/jsonapi.module
    @@ -0,0 +1,303 @@
    +      $output .= '<dt>' . t('General') . '</dt>';
    +      $output .= '<dd>' . t('JSON:API is a particular implementation of REST that provides conventions for resource relationships, collections, filters, pagination, and sorting, in addition to error handling and full test coverage. These conventions help developers build clients faster and encourages reuse of code.') . '</dd>';
    +      $output .= '</dl>';
    

    I don't think we need to talk about test coverage in user-facing help documentation. (It makes sense in the developer docs in the API since that section goes on to tell developers about their own tests.)

Several of the above points from reviewing the docs gate stuff are blocking, but I'll leave this at RTBC so that it's visible to other committers for review while those docs updates are made.

colan’s picture

We'll also want to consider how to help the user choose between this and REST given just their two descriptions, but I think the above helps with that.

That's a very good point, @xjm. We should make it explicit outside of this context as well. Maybe on a developer docs page? I don't want to lose site of this because...

I've been using the REST API since D8 was out, but have been tracking this module as it's been progressing. And I still don't understand what value it brings over REST. The contrib project page states that it implements the spec. So then I looked at the spec, and it shed some light on what it does, but still doesn't tell my why it's better than REST. Is it? What's the difference? Why would I use one and not the other? Does REST not do the the things that this module claims to do?

As someone who does lots of API work, this isn't clear to me. So it definitely won't be clear to anyone else. Let's be sure to document this somewhere (outside of the code here).

e0ipso’s picture

@colan this is prominently explained in the documentation of the module. It's one of the big sections on the documentation homepage. https://www.drupal.org/docs/8/modules/jsonapi

I'm not sure we can find a more prominent place for it once this lands into core.

Notice that this is the official way Drupal.org surfaces documentation. Maybe there's a better way to do it, and that's a d.o feature request that I will +1 if you end up creating an issue for it.

colan’s picture

Thanks, found it on the API Overview sub-page. When this gets moved to the core section from contrib we'll be all good:

Unlike the Drupal Core REST module, these paths are not configurable and are all enabled by default. Unlike core REST the JSON:API is not simply a format like JSON or HAL+JSON. It encompasses a much broader set of rules about how your API will work. It dictates which HTTP methods should be used, what HTTP response codes should be returned under specific circumstances, the format of your response body, and the linkage between resources.

xjm’s picture

Ah yes, so https://www.drupal.org/docs/8/modules/jsonapi is what the hook_help() should link instead of a youtube video. :) Edit: Or whatever the equivalent URL will be in the core handbook when we move it. Thanks @e0ipso!

I think it'd also be worth adding a short section to both modules' hook_help() specifically linking https://www.drupal.org/docs/8/modules/jsonapi/jsonapi-vs-cores-rest-module since that's the first choice site builders are going to make.

xjm’s picture

BC policy part 2

  1. +++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
    @@ -0,0 +1,277 @@
    + * @internal
    

    For all the @internal, we should document why the thing is internal and what the consequences of using it despite the @internal would be. For some of these, there's a specific reason already given on the class docs that can just be moved; for others, it's probably a single sentence that we just add under each that's a boiled-down version of what's in the API docs about BC. It could even link the API doc section about it, actually.

    Note that marking things @internal still isn't a totally free pass in terms of BC, because we've discovered over the course of Drupal 8's life that people can and will use internal things in their own code despite our warnings, but when sites update and trip over an internal BC break, they blame "Drupal 8 being unstable". This is why we added https://www.drupal.org/core/deprecation#internal to our BC and deprecation policy. So even though these things are marked internal, we should still plan to provide best-effort BC via the deprecation process if we need to change them in the future.

  2. +++ b/core/modules/jsonapi/src/Access/TemporaryQueryGuard.php
    @@ -0,0 +1,603 @@
    + * This class will be removed when new Drupal core APIs have been put in place
    + * to make it obsolete.
    

    This paragraph here can simply be moved to under the @internal for the class. Edit: And we should probably deprecate it instead of removing it when the time comes and make it simply wrap the new core API.

  3. +++ b/core/modules/jsonapi/src/Access/TemporaryQueryGuard.php
    @@ -0,0 +1,603 @@
    + * @deprecated
    

    This is what we call a "wishful thinking" deprecation (deprecating before the replacement API is committed). We don't do this in core anymore. :) The @deprecated should simply be removed for now and added back according to the normal process once the new core API is added.

  4. +++ b/core/modules/jsonapi/src/LinkManager/LinkManager.php
    @@ -0,0 +1,202 @@
    + * @deprecated
    

    Ditto.

xjm’s picture

Let's cover the easy core gates next...

Usability gate

No UI. N/A aside from what's already covered above for the docs gate.

Accessibility gate

No UI. N/A aside from what's already covered above for the docs gate.

Testing gate

I didn't read all the tests. But wow. Let's just say...

✅ Passed

Actually I'm not sure that's quite strong enough. How about...

✅✅✅ PASSED!!!!

xjm’s picture

Title: Add JSON:API as stable module into core » Add JSON:API to core as stable module
xjm’s picture

Title: Add JSON:API to core as stable module » Add JSON:API to core as a stable module
xjm’s picture

BC policy part 3

The project page for JSON API has:

Extend JSON:API

This module is zero configuration. Once you enable the module, you are done.

If you need to customize your API you may be interested in the JSON:API Extras module.

So that sounds to me like a clear counterpoint to the API visibility strategy currently exposed for this module and underscores #122,1,

Wim Leers’s picture

Wow, thanks for the super fast detailed feedback! 🙏 👏 😃

Added dependencies

Two remarks before I do the work to gather that information:
this is in the require-dev section of composer.json, not in require
we could definitely live without this, but it'd mean we'd lose the ability for any site to validate generated JSON:API responses against https://jsonapi.org/'s schema. (See \Drupal\jsonapi\EventSubscriber\ResourceResponseValidator.)

Documentation gate

All being addressed in #3033343: Address Drupal core documentation gate feedback from @xjm. Waiting for feedback from @gabesullice and @e0ipso, expecting that to land later today. One thing worth calling out here in the core issue queue:

  1. ✅ (But why didn't this trigger phpcs fails then? 🤔)

BC policy

Working on that now.

xjm’s picture

✅ (But why didn't this trigger phpcs fails then? 🤔)

I looked into this too; some of the things in our documented standard nabout overall docblock/docgroup section ordering and formatting and the like look like they still haven't made our way into our phpcs rules. I look forward to the day that computers just handle most of this for us. :P

Two remarks before I do the work to gather that information:
this is in the require-dev section of composer.json, not in require

Ah, this is great news. I think we should still document the things I mentioned for it, but clearly highlight in the IS that it's a dev dependency only. Missed that when I was looking it over.

Wim Leers’s picture

I look forward to the day that computers just handle most of this for us. :P

Indeed 🤖

Ah, this is great news. I think we should still document the things I mentioned for it, but clearly highlight in the IS that it's a dev dependency only.

🙂 Will do!

Wim Leers’s picture

BC policy

#117:

Interesting. I think we probably need to discuss/verify that we're okay with this for BC policy for this module in core.

Note that we already have such a BC policy elsewhere in Drupal core: bigpipe.module does the same. Both for JSON:API and BigPipe, the responses are the API not the code that generates those responses.

#126: It's up to the JSON:API Extras contrib module to make changes as JSON:API's implementation changes. That's what's been happening for the past year or two.

#122.1: For most core modules the statement the responses are the API not the code that generates those responses isn't true. And consequently I would argue that an @internal for JSON:API carries is more meaningful: it truly is an implementation detail.

Over the course of 2018, a lot of implementation details have changed. A lot of code that was marked @internal was changed, renamed, moved to different parts of the codebase, or even removed altogether. The JSON:API module is designed to grow in features. Thanks to this approach, we were able to add for example revision support. If we'd hadn't explicitly used @internal as we have, we wouldn't have been able to do that.

Examples of features we probably want to add in the future: translations (#2794431: [META] Formalize translations support), partial caching (#2819335: Resource (entity) normalization should use partial caching), alternative pagination strategies (was just created), schema introspection (#2822768: [PP-1] Add information about the schema in the json:api resource), delta includes (#3006217: [upstream] Ability to include a specific delta), fetching all includes at once (#2997915: [Profile addition] Ability to fetch *all* includes), full text search (#2991729: Ability to define a "full text search" filter, e.g. using Elasticsearch), contrib module-provided hypermedia links (#3014704: Expose API to allow links handling for entities from other modules) and more. That last one is an example of an explicit PHP API.

We specifically have #3032787: [META] Start creating the public PHP API of the JSON:API module for this: to not blindly declare the current codebase as the API, but to gradually grow an intentionally designed public PHP API, based on real-world use cases and feedback. This is how pretty much every other software library, software framework and software platform does it: private APIs first, developers using those fully knowing that. I hope we can do the same for this module.

we've discovered over the course of Drupal 8's life that people can and will use internal things in their own code despite our warnings, but when sites update and trip over an internal BC break

When Drupal is serving the end user application, I think it makes sense: developers just want to override "That One Thing" to get the job done. But in the case of JSON:API, Drupal is serving as the API for the end user application. The changes/overrides then tend to happen inside the end user application that consumes JSON:API.

Wim Leers’s picture

Yay, #2926508: Add DateTimeNormalizer+TimestampNormalizer, deprecate TimestampItemNormalizer: @DataType-level normalizers are reusable by JSON:API landed yesterday, which was one of the two blockers to landing JSON:API in core! Updated #3015325-32: [ignore] support issue for the core patch to remove the forward compatibility layer, and now the core patch is again a bit smaller :)

Also, all docs feedback was addressed in #3033343-10: Address Drupal core documentation gate feedback from @xjm, and I credited that commit to you, @xjm, since I was able to simply copy/paste many of your suggestions, so you'd already done much of the work — thanks! :)

Wim Leers’s picture

Forgot to upload the updated script that rolled #131's patch.

e0ipso’s picture

Speaking of which, next time this patch is rerolled, can we add a MAINTAINERS.txt entry to the patch? Are all 3 of you staying on as maintainers once this is in core?

I'm +1 on this. I really hope @Wim Leers and @gabesullice are in as well, we're a kick ass team together! (of course I already know their answers, but I'll let them say 😛)

gabesullice’s picture

I'm in.

@e0ipso 🤜💥🤛 /me 🤜 @Wim Leers?

Wim Leers’s picture

Of course 🤜💥🤛 — also: 🕺🦙

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 131: jsonapi-2843147-131.patch, failed testing. View results

catch’s picture

On the bc policy, I think it's sensible but one question:

On building to the specification rather than the module though, how does that work for people in practice if they run up against a bug - should they look for things how they should be, then if they don't find what then fall back to the actual behaviour the module is doing pre-fix? If that's the case it might be worth expanding the comment to include advice - i.e. if you code to the spec but the module has a bug that breaks this, you're a bit stuck.

I personally don't mind marking everything @internal and expanding it later explicitly, this is primarily not a module people will integrate with (like bigpipe) and it doesn't store any data/define any entities either. It's slightly different from marking things as @internal that are obvious extension points 'just in case'.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.07 KB
1.24 MB

https://www.drupal.org/sa-core-2019-003 caused the release of https://www.drupal.org/project/jsonapi/releases/8.x-2.3, so we now need a reroll. JSON:API 11f4b19fbe5768eeb4a02c08e42b43cbf918ab07 on top of core d4fa72e7cf2cee035100a5c62c762d9af161d409.

#137: 👌👍 Regarding spec compliance bug fixes: discussed this with catch in chat. Improved documentation in #3035085: Clarify client expectations wrt server spec compliance fixes, with @catch's +1.

The requested MAINTAINERS.txt additions have been added to the shell script.

gabesullice’s picture

+++ b/core/MAINTAINERS.txt
@@ -248,6 +248,11 @@ JavaScript
+JSON:API
+- Gabe Sullice 'gabesullice' https://www.drupal.org/u/gabesullice
+- Mateu Aguiló Bosch 'e0ipso' https://www.drupal.org/u/e0ipso
+- Wim Leers 'Wim Leers' https://www.drupal.org/u/wim-leers

Ubernit: Let's go by the order that each of us got commit permissions to the contrib module, not alphabetically.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 138: jsonapi-2843147-138.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.07 KB
1.24 MB

#139: somebody on Slack pointed out that the goal is to eventually list these alphabetically, so let's just keep it like this for now.


#138 was green.

Until #2244513: Move the unmanaged file APIs to the file_system service (file.inc) landed two days ago, which introduced deprecations in 8.7.0 that cannot be fixed in JSON:API itself (since it needs to continue to work with Drupal 8.5.x and 8.6.x), which we fixed in #3035666: Drupal core compatibility: file_* functions deprecated in Drupal >= 8.7. Also, if #2940383: Unify file upload logic of REST and JSON:API had already landed (the last blocker for this issue), this reroll wouldn't have been necessary.

Rerolled using jsonapi 3e062ac4cf674ef91238c0545ecad112f62c1e81 plus Drupal core ce4034eef635036a57ea14e0b9d7593450a59c7b with #3015325-39: [ignore] support issue for the core patch.

gabesullice’s picture

Issue summary: View changes

Added a brief section on dependencies to the IS. justinrainbow/json-schema is the only dependency and it's in the composer.json dev section.

Wim Leers’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

#142 updated the issue summary as requested.

I also just updated other parts of the issue summary. Removing tag.

Wim Leers’s picture

#2940383 was the last blocker. As of #2940383-60: Unify file upload logic of REST and JSON:API, it was decided that it'd be better to not provide an "file field uploader" service that both core REST and JSON:API use, but to instead work on such a service that works for not only REST and JSON:API, but also UI file uploads. In other words: the decision is to wait until after this lands so we can "get it right the first time", thanks to the rule of three.

Therefore this issue now has no more blockers!

As far as I can tell, the only remaining bit of feedback that was not yet addressed here, is the backwards compatibility policy feedback from @xjm. That was addressed last night in #3033359: Address Drupal core BC policy gate feedback from @xjm.

So: attached is a core patch with all feedback addressed 🥳

Core patch: jsonapi 4ed6779ca3034cd790585c565471f38c005fd354 plus Drupal core f5cd28b7b877e4093eb83b8472a49e368019549e with #3015325-43: [ignore] support issue for the core patch.

dww’s picture

Re: #144: "with all feedback addressed"

Almost. ;) If JSON:API is not using #2940383: Unify file upload logic of REST and JSON:API there's unaddressed feedback in the JSON:API - specific version of the file uploader service (*sigh*). E.g. jsonapi/src/ForwardCompatibility/FileFieldUploader.php is still calling basename() directly, perhaps leaking FDs in various error conditions, etc. I asked in Slack about this but you might have missed it. ;) Should I post the review here or in the JSON:API queue?

Wim Leers’s picture

Ah! Thanks for being so diligent, @dww :) And that is of course exactly why #2940383 exists: to prevent parallel implementations of the same things. This nicely illustrates that :)

Should I post the review here or in the JSON:API queue?

It'd be great if you could file an issue in the JSON:API issue queue for that. If you do it here (which is also fine), we'll just have to convert your comment into an issue.

dww’s picture

Okay then, added #3035866: Update \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader to be in sync with \Drupal\file\Plugin\rest\resource\FileUploadResource as a major "blocker" child. See you there. ;)

Cheers,
-Derek

p.s. Does that mean this should be pp-1 and postponed? I don't want to mess with the status since everyone's worked so hard to get to RTBC already...

gabesullice’s picture

I've created #3036210: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses at the request of @effulgentsia and @xjm to further delve into #142.

dww’s picture

Update from my files-related deep-dive:

#3035866: Update \Drupal\jsonapi\ForwardCompatibility\FileFieldUploader to be in sync with \Drupal\file\Plugin\rest\resource\FileUploadResource is now committed.

#3036251: JSON:API TemporaryJsonapiFileFieldUploader::streamUploadData() can call fclose(FALSE) is RTBC and awaiting a commit of the corresponding bug report for REST in core (also RTBC): #3036197: REST FileUploadResource::streamUploadData() can call fclose(FALSE)

So I'm signing off on my file-related concerns for this (at least until we can revisit via #2940383: Unify file upload logic of REST and JSON:API).

s/imaginary-postponed/RTBC/ ;)

Thanks!
-Derek

Edit: p.s. although we will need another patch re-rolled that includes the fixes.

Wim Leers’s picture

The last submitted patch, 144: jsonapi-2843147-144.patch, failed testing. View results

xjm’s picture

I reviewed the docs gate fixes. Looks good to me! Just one thing I think we should improve further:

+++ b/jsonapi.module
@@ -84,13 +84,16 @@ function jsonapi_help($route_name, RouteMatchInterface $route_match) {
+      $output .= '<dd>' . t('There is a <a href=":comparison">comparison</a> page that compares the RESTful Web Services module and the JSON:API module.', [
+        ':comparison' => 'https://www.drupal.org/docs/8/modules/jsonapi/jsonapi-vs-cores-rest-module',
+      ]) . '</dd>';

I would make this a little more clear to the uninitiated, maybe something like:

The <a href="">JSON:API</a> and <a href="">RESTful Web Services</a> modules serve similar purposes. <a href="">Read the comparison of the RESTFul Web Services and JSON:API modules</a> to determine the best choice for your site.

And then let's add that same paragraph to rest's hook_help() too, in this patch.

xjm’s picture

Issue summary: View changes
FileSize
44.21 KB

Maintenance and technical debt

Security

As of this writing, all known security vulnerabilities with the module have been resolved. However, the current "no-config" design does result in increased attack surface, including an increased attack surface compared to core REST. There is a blocking security hardening discussion still underway for this:
#3035979: [DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities

(Interested parties should chime in over there. We'll continue to review the discussion in that issue.)

Performance gate

Since this is new code that doesn't touch anything outside the added module, presumably this won't regress any existing functionality. However, I haven't reviewed this patch for any of the specifics of https://www.drupal.org/core/gates#performance and will leave this to the framework managers.

Dependencies continued

We are reviewing this in #3036210: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses and should consider that issue a blocker for this one.

Other stuff

  1. Regarding #130, #137, and the added docs for @internal: I'm comfortable with all that so long as it's understood that we will still ask for core's deprecation process to be used even for this internal code, as per: https://www.drupal.org/core/deprecation#internal

  2. +++ b/composer.lock
    @@ -3954,6 +4020,7 @@
                 ],
    +            "abandoned": true,
                 "time": "2015-10-02T06:51:40+00:00"
    

    Uh what?

    I'm assuming this is some side effect of rebuilding composer.lock and totally unrelated to JSON:API? Maybe having to do with our use of an old version of PHPUnit? But would be good to know why that's happening here and what impact this would have on Composer builds if any.

  3. +++ b/core/modules/jsonapi/src/Access/TemporaryQueryGuard.php
    @@ -0,0 +1,613 @@
    + * @deprecated These additional security measures should eventually reside in
    + *   the Entity API subsystem but were introduced here to address a security
    + *   vulnerability. The following two issues should obsolesce this class:
    + *
    + * @see https://www.drupal.org/project/drupal/issues/2809177
    + * @see https://www.drupal.org/project/drupal/issues/777578
    + *
    

    As per my previous review in #122, we should only add the @deprecated after the replacements have been added to core. This can be converted to an @todo instead.

Wim Leers’s picture

#152: ✅— done in #3037682: Address additional Drupal core documentation gate feedback from @xjm and uploaded patch for rest.module to #3015325-47: [ignore] support issue for the core patch and updated the script to apply it.
#153:

Maintenance and technical debt

There are three issues that concern me based on their titles:

The second issue was marked as a critical bug by an (understandably frustrated!) Drupal user. It wasn't actually a critical bug. We were giving the issue some time. I've now closed it and updated the issue in another contrib module issue whose patch was causing this, to propose a patch change that would avoid the problem :)

The first and third are about translation and revision support. I've updated their issue summaries. For your convenience, here is a summary for each:

  • The most often needed functionality wrt revisions has been completed. You can use it today. You can use it to have a decoupled authoring UX and generate a preview. Updated the meta issue to accurately reflect the current status: #2795279-47: [PP-2] [META] Revisions support.
  • For translations, it's "just" a matter of deciding what the official mechanism for language negotiation is going to be, rather than relying on Drupal's automatic translating of entity parameters in routes. There hasn't been significant progress, and fairly few comments have been left, presumably because the current automagic behavior provided by core satisfies the most basic needs. The few comments left by community members relying on that automatic behavior do indicate that there is a need to formalize this. This issue cannot remove the current automatic behavior (that'd be too disruptive), but users would have to update to the formal/official mechanism if they want the edge cases to be handled in a consistent, sensible manner.

So: revision support is present insofar as possible (read-only, only certain entity types, due to the Entity Revision API not being fully fleshed out, the Workflow Initiative ran into this as well), translation support is present thanks to "entity" route parameter upcasting (read-only, edge cases not handled in a way that's great for a web service).

There are 67 open issues, including two criticals and 12 majors.

I want to call out that of those 65 (down from 67) open issues, 31 are feature requests. 7 are plans. 25 are tasks (usually about refactoring something). Only 2 are bugs. In other words: the functionality that we do have is reliable, the bulk of the issues are about adding more features :)

Also, 2 criticals and 12 majors sounds relatively scary, but … per the above, it's really just one critical (for adding JSON:API 1.1 support when that spec is finalized, we've already vetted that we're indeed fully compliant, the remaining work is about communicating which profiles on top of the base specification our implementation provides). I triaged the issue queue again and we're now down to 10 majors. There is only one major bug.

Performance gate

I'll just point out that I've worked on lots of issues making the REST module faster, and all of those have also been applied to JSON:API. JSON:API just like REST takes advantage of Dynamic Page Cache and Page Cache (and has thorough test coverage). JSON:API's performance is much better than when REST's was when Drupal 8.0.0 shipped.

The key difference wrt performance between REST and JSON:API is that JSON:API supports collections and filtering of collections. This is mapped directly to Entity Field queries. So JSON:API benefits of years of maturing and optimizing that existing API.

Other stuff

  1. See next comment.
  2. Yeah this is definitely not something we did. I think whichever next core commit updates composer.lock will end up adding this. It's because https://github.com/sebastianbergmann/phpunit-mock-objects/ has been abandoned, since it was merged into PHPUnit itself. We see this appear because we're still using an older version of phpunit. 100% unrelated to this issue :)
  3. ✅ Makes sense, done together with #152
Wim Leers’s picture

Pair-written by @gabesullice and I.
#153.1:

It seems like there's a contradiction between https://www.drupal.org/core/d8-bc-policy#bc and https://www.drupal.org/core/deprecation#internal. In the former, it says When we do make changes in either minor or patch releases, we will make reasonable efforts to provide a backward compatibility layer (BC) for anything not marked @internal. and in the latter it says However, as a best practice, we will still deprecate internal APIs first to reduce disruption and make the process easier to understand.. I'm trying to reconcile these two statements and I think what it's saying when/where there's a real API, then that needs to be carefully deprecated.

However, the JSON:API module does not have any "internal APIs". There are no hooks, no interfaces, no plugin types, no service tags that JSON:API uses to achieve its functionality.

Perhaps there's a misunderstanding about what code is and isn't an internal API?

dww’s picture

xjm’s picture

Thanks @Wim Leers!

I included tasks and feature requests in the issue list because #2794431: [META] Formalize translations support for example was filed as a feature request. It's since been corrected, but I always look at all the issues myself to look for things I consider essential features. :) The bullet was just informational about how I'm doing my ongoing review of the technical debt, not positive or negative. The work to date shows that the module is really well maintained!

Regarding #155: The internal API policy was written in 2014, and those words you highlight were added by someone who is not a part of the community anymore. The deprecation policy was written in 2017 and updated in both 2017 and 2018 to take into account our experiences with what happened with actual minor releases from 2016 on. So lots of the internal API policy needs to be updated; AFAIK it doesn't even mention deprecation (or if it does, it does so only barely).

The deprecation practice for internal APIs is actively enforced, within reason. Sometimes providing BC and deprecation actually is dangerous or makes the DX really bad, and in those cases we justify the break, write a CR, and consider it for release notes. So it's best to start by simply asking "How can we provide BC?" and then trying that with best effort. Exceptions are discussed on a case-by-case basis on issues where they come up.

Wim Leers’s picture

@effulgentsia, @xjm, @gabesullice and I had a call about #155 last night, after she wrote #157. It boils down to @xjm being convinced that it will not be a blocker to progress, but @gabesullice and I not being convinced of that. @xjm has since improved https://www.drupal.org/core/d8-bc-policy (thanks! 👏🙏), which certainly reduces our concerns.

Just now, I had a chat with @catch. He pointed to #3029197: [symfony 5] Class implements "Symfony\Component\DependencyInjection\ResettableContainerInterface" that is deprecated since Symfony 4.2, use "Symfony\Contracts\Service\ResetInterface" instead as an example that even dropping an interface entirely is possible. He pointed out that changing something about the container obviously is less obscure than JSON:API internals 🙂

I also had a chat with @e0ipso to catch him up.

@e0ipso, @gabesullice and I would have preferred for it to be objective and crystal clear for JSON:API, but it's going to remain a subjective decision. Given the extra context we've been given, we're okay with moving forward as-is. I consider #155 / #153.1 addressed.

I wrote this alone, but I believe I've captured @e0ipso's and @gabesullice's opinions too.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 154: jsonapi-2843147-154.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

I am not seeing the actual test fail in #159 and have this same thing happen in another issue today so I am assuming this is drupalci hiccup.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 154: jsonapi-2843147-154.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.23 KB
1.29 MB
effulgentsia’s picture

Version: 8.8.x-dev » 8.7.x-dev

Last I heard, the release managers are ok with giving this an extension until beta, given that it's only held up on #3035979: [DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities.

webchick’s picture

I was asked to weigh in from a product manager POV on revisions and translations support.

Since we do not have "100% full compatibility with each and every one of the ever-ballooning matrix of core features" as one of the Drupal core gates, I can't see holding this feature up on that, especially since REST has the same limitations. As long as we are not introducing new bugs, particularly those that would be classified as "critical" such as data loss/security bugs, I'm in favour of an iterative approach to adding features to core, and not denying 100% of users access to something awesome because it doesn't yet meet 20% of users' needs, for example.

Nevertheless, Integration issues such as these, where feature A and feature B come into contact and there's friction, tend to be some of the most painful, confusing, and frustrating for our users, so it is very good to take them seriously. I gave https://www.drupal.org/docs/8/modules/jsonapi/revisions and https://www.drupal.org/docs/8/modules/jsonapi/translations a once-over. I'm impressed with the straight-forwardness of the docs to help developers understand what limitations exist in these areas, as well as the general plan for resolving them in the future.

So +1 from me on the product side for these (well-documented) limitations not standing in the way of this feature.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 163: jsonapi-2843147-161.patch, failed testing. View results

xjm’s picture

The test fail is in Drupal\Tests\jsonapi\Functional\MenuLinkContentTest so I think it's legit. https://www.drupal.org/pift-ci-job/1226063

plach’s picture

voleger’s picture

amateescu’s picture

I provided a fix for those test failures in #3015325-49: [ignore] support issue for the core patch, but I'll let Wim post a new patch here because it seems to be quite an involved process :)

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.23 KB
1.31 MB

#165: 👍

#170: thanks again for that, @amateescu, and thanks @xjm, @plach and @voleger — all of you were credited in #3039235: 8.7.x: update anything needed now that taxonomy terms are revisionable :)


Since #163, the following 3 issues were fixed/committed:

  1. the aforementioned #3039235: 8.7.x: update anything needed now that taxonomy terms are revisionable to update JSON:API's test suite's expectations for Term and MenuLinkContent's data model additions
  2. #3039966: Document the extent of JSON:API's revision support. (a sibling issue of #3037804: Document the extent of JSON:API's multilingual support.), requested by @xjm
  3. #3039568: Add a read-only mode to JSON:API (the consensus on how to solve #3035979: [DISCUSSION] Should a facility be added to JSON:API which limits automatic exposure of all entities — @effulgentsia is going to file further follow-ups to explore what more Drupal should do.)

Fresh patch, JSON:API 39c29e2b41a05bc9dde06e66f7e2978f433b010d + #3015325-53: [ignore] support issue for the core patch + Drupal 9f6c7c1e547b9a21335e5800d0a68d1efa079cf7.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 171: jsonapi-2843147-171.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.31 MB

Apparently DrupalCI for contrib modules will gladly ignore tests that are misplaced, but not for Drupal core, which is how it went unnoticed that I'd put the new test that #3039568: Add a read-only mode to JSON:API added in the correct place but assigned it a slightly incorrect namespace. 😭

Fixed that in #3040186: Follow-up for #3039568: ReadOnlyModeUpdateTest's namespace is defined incorrectly, causing the test not to run, and that's the only change compared to #171.

Fresh patch, JSON:API d5abca9f600000e142516ae24e0058bdfcb9111d + #3015325-53: [ignore] support issue for the core patch + Drupal 9f6c7c1e547b9a21335e5800d0a68d1efa079cf7.

Wim Leers’s picture

Issue summary: View changes

I made a stupid mistake in #91: I wrote "uninstall" but I should have written "remove".

Also wrote a release notes snippet, catering to both possible update path scenarios: update from the contrib JSON:API module at version 8.x-2.x or from version 8.x-1.x.

Wim Leers’s picture

Issue summary: View changes

@xjm pointed out that for some sites, it may be preferable to remove the contrib module after updating to Drupal 8.7.

xjm’s picture

Issue summary: View changes

Simplifying the release note a bit after discussion with @Wim Leers.

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
xjm’s picture

Alright, consider this release-manager-reviewed!

I checked over the queue one more time. The notewothy remaining issues are:

  1. #3039959: Documentation review of the "Security Considerations" page -- @greggles and I have both reviewed it now, and so I think it's "done enough" for core inclusion. (Feedback still welcome, of course.)
  2. #3040025: [meta] Decide on and implement additional security hardenings for JSON:API and other HTTP APIs -- Non-blocking improvements to future releases. (Some might be blocking for creating a decoupled site install profile or such, down the line.)
  3. #3039730: Include paths are resolved for every resource in a resource collection, instead of once per unique resource type -- Definite opportunity for a perf improvement there, but since no existing functionality is regressed and the module is off by default I think it is not a perf gate blocker. (Might also be a blocker for a decoupled install profile.)

Beyond that, all my concerns have been addressed. I also checked with @catch and while he hasn't done a full review of all the everything himself, he's comfortable with this being untagged.

I did notice that we don't have a CR attached here yet, so let's add that.

Great work!

e0ipso’s picture

@xjm is it worth mentioning in the release notes that sites with 8.x-2.x will work without any additional modifications upon contrib deletion? Maybe:

JSON:API is now included in core. Sites currently using the 8.x-2.x branch of the JSON:API contributed module should remove the contributed module from the codebase (rm -rf modules/jsonapi) when updating to Drupal 8.7, as the contributed module will not receive further updates aside from security fixes. No further changes are necessary to sites currently using the 8.x-2.x branch. Sites using the 8.x-1.x branch may require changes to API clients, but the contributed module may be left in place with Drupal 8.7 until any needed conversions are complete.

Wim Leers’s picture

Issue tags: -Needs change record

#179:

  1. 👍
  2. 👍
  3. Makes sense. I agree performance can be improved there, and I'm sure it can also be improved in other places in JSON:API. This is exactly why we spent a lot of time getting the architecture right and pay a lot of attention to limiting the API surface: to be able to refactor implementation details to address needs as they come up, whether that's new features or performance improvements :)
    I'm already working on it!

    P.S.: in that particular issue, we're dealing with including eighty relationships, which are in total referencing about forty different entity types/bundles. It's about optimizing an extreme case.

Draft change record created: https://www.drupal.org/node/3041438 (got inspiration from https://www.drupal.org/node/2924128 and https://www.drupal.org/node/2968491 — the change records for Layout Builder & Workspaces).

effulgentsia’s picture

Thanks for the CR. I plan to commit this once we're out of commit freeze unless there's commit-blocking feedback between now and then.

e0ipso’s picture

Woo! @effulgentsia I'll be drumrolling until the commit.

webchick’s picture

Commit freeze lifts Thursday, so make sure you've got gloves/mittens to go along with your drumroll. ;)

YAY! Thanks, @effulgentsia!

Wim Leers’s picture

Since #173/#179, two bugs were fixed, and both come with an explicit regression test :). I've also worked quite a bit on #3039730: Include paths are resolved for every resource in a resource collection, instead of once per unique resource type (which was called out by @xjm in #179.3): it now has a clearer title, tighter scope and simpler patch. It's now blocked on feedback from the person who reported it, to test it in their pretty extreme scenario. It's really cool to see JSON:API being used in such scenarios: it helps discover opportunities for improvement that may otherwise have gone unnoticed!

Attached patch = JSON:API fb7bf82c7acec2a2576bc01c2b51374593155d02 + #3015325-53: [ignore] support issue for the core patch + Drupal d0c3e6432099e571b5c3f8de8d8d866cb803dec1.

🥁

Wim Leers’s picture

Might I also suggest crediting everyone in https://www.drupal.org/node/2723491/committers? They all contributed to this patch!

effulgentsia’s picture

+1 to crediting everyone in https://www.drupal.org/node/2723491/committers. First, checking the checkboxes for the people already on this issue. Next, I'll add everyone else.

effulgentsia credited nuez.

effulgentsia’s picture

effulgentsia’s picture

effulgentsia’s picture

effulgentsia’s picture

effulgentsia’s picture

effulgentsia’s picture

  • effulgentsia committed e59ec93 on 8.8.x
    Issue #2843147 by Wim Leers, e0ipso, gabesullice, dagmar, xjm,...
effulgentsia’s picture

The 8.8.x branch is now unfrozen, so pushed to there!!!!

The 8.7.x branch is still frozen, so leaving at RTBC for cherry picking to it tomorrow.

Brilliant work, everyone! Sorry, that's a massive understatement, but I don't have enough superlatives to write into this comment to give it proper justice.

I'm super excited to see a whole new era of decoupled sites, slick apps, and everything else that this will enable!

xjm’s picture

Wim Leers’s picture

🥳

It's kinda hard to believe this is happening!

  • xjm committed b2f88e3 on 8.7.x authored by effulgentsia
    Issue #2843147 by Wim Leers, e0ipso, gabesullice, dagmar, xjm,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Backported to 8.7.x! Congratulations everyone and thanks for all your work. 🎉🌎🍻💫

Gábor Hojtsy’s picture

🎉🎉🎉 Congrats and thanks all! 🎉🎉🎉

Status: Fixed » Closed (fixed)

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