Don't hardcode `/jsonapi/` in FormatSetter, read JSON API base path from container parameter instead

Nice catch! This will make sites using jsonapi_extras to customize the prefix behave correctly. IOW: today, those sites behave incorrectly.

I think we should postpone this on #2949632: Make ResourceTypeRepository aware of the path prefix.

👍 Very thoughtful!

Oops.

IOW: today, those sites behave incorrectly.

This prediction came true: #2969398: When using JSON API Extras' ability to change the base path, 4xx errors returned for using an invalid field in filter or sort returns HTML response rather than JSON.

This is still blocked on #2949632: Make ResourceTypeRepository aware of the path prefix, which has been assigned to @e0ipso for a few weeks now, because it needs a coordinated release of jsonapi_extras.

#2949632: Make ResourceTypeRepository aware of the path prefix landed!

Patch still applies cleanly. Retesting…

#6 was against HEAD, #2949632: Make ResourceTypeRepository aware of the path prefix renamed that method. Of course that couldn't work.

I wanted to RTBC and commit this patch. But actually, having updated this patch, I realized something:

+++ b/jsonapi.services.yml
@@ -127,6 +127,8 @@ services:
   jsonapi.http_middleware.format_setter:
     class: Drupal\jsonapi\StackMiddleware\FormatSetter
+    calls:
+      - [setJsonApiBasePath, ['@jsonapi.resource_type.repository']]
     tags:
       # Set priority to 201 so it happens right before the page cache
       # middleware (priority 200)has the opportunity to respond.

This is making a middleware dependent on another service.

That other service needs a boatload of entity services to be constructed, which is expensive. And because this middleware runs before the page cache middleware, that means every single page cache hit is going to be made slower by this. All of that, just for JSON API. So I tested the before/after of this patch (using ab -c 1 -n 100 http://d8/foobar — i.e. a trivial 404 page that does not at all involve JSON API):

HEAD (before)

Requests per second:    216.77 [#/sec] (mean)
Time per request:       4.613 [ms] (mean)
Time per request:       4.613 [ms] (mean, across all concurrent requests)
Transfer rate:          2112.83 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     4    4   0.5      4       7
Waiting:        4    4   0.5      4       7
Total:          4    5   0.5      4       7

Patch (after)

Requests per second:    180.25 [#/sec] (mean)
Time per request:       5.548 [ms] (mean)
Time per request:       5.548 [ms] (mean, across all concurrent requests)
Transfer rate:          1757.09 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     5    5   0.4      5       7
Waiting:        4    5   0.4      5       6
Total:          5    5   0.4      5       7

"Only" 1 millisecond difference, but that's 20% slower, and only ~80% of the number of requests per second. That's a pretty big difference.

Normally, I would not care. But we're trying to move this into Drupal core. And the intent is for new Drupal UIs to be built atop JSON API. We can't have all of them be slowed down by this.

If we'd implement this using a container parameter, then this performance problem would go away: no more services to construct.

And we're now at a unique time and place: #2949632: Make ResourceTypeRepository aware of the path prefix already means jsonapi_extras will need to be updated (it hasn't been updated yet). JSON API Extras can still make this customizable, but it will have to do this through a container service modifier, for example how \Drupal\language\LanguageServiceProvider::alter() sets the language.default_values container parameter.

Theoretically this is out of scope for this issue; but it's the necessary change (necessary for correctness) that this issue's patch was making that was causing the performance problem. Hence it's IMHO in scope.

So here's a completely alternative patch.

Unfortunately, @e0ipso tagged a release of JSON API before this was resolved, which led to a JSON API Extras bug report: #2982133: No longer works with JSON API >=1.21 (so either 1.21 or 1.22).

So this:

And we're now at a unique time and place: #2949632: Make ResourceTypeRepository aware of the path prefix already means jsonapi_extras will need to be updated (it hasn't been updated yet).

is no longer true.

I assigned #2982133 in the jsonapi_extras queue to myself.

Patch posted for jsonapi_extras at #2982133-4: No longer works with JSON API >=1.21 (so either 1.21 or 1.22).

I wasn't able to apply 2971745-15.patch. I realized that several commits have been added to 8.x-1.x branch since this patch was submitted.

We need to update the patch.

I assigned #2982133 in the jsonapi_extras queue to myself.

👏🏽 Yay! Thanks for that @Wim Leers!

Unfortunately, @e0ipso tagged a release of JSON API before this was resolved, which led to a JSON API Extras bug report

The tag was intentional despite the bug introduced in extras. I was hoping to get to the bug sooner, but I couldn't. Evil me thinks that if stuff is not broken ever you don't get contributions. We can see it as a way to attract new blood 😅

In general I like the approach of the patch. The only think I'm hesitant to sign-off on is the removal of the public method. Despite the white lie of making everything @internal I believe that other modules will need the base path information. In particular (at least) Open API will need it. Although you can still get the parameter directly from the container, so I don't feel strongly about it if you want to remove the method. It will add an additional break to JSON API Extras and Open API, but that may be OK according to the reasoning above.

Rebased #15. Not sure what went wrong there.

Evil me thinks that if stuff is not broken ever you don't get contributions. We can see it as a way to attract new blood 😅

:D No problem. But it left the JSON API module in the awkward place of trying to not break JSON API Extras, and being blocked on your feedback, and then you tagged a release and hence breaking it anyway! We were trying to do The Right Thing :)

The only think I'm hesitant to sign-off on is the removal of the public method.

My concern with doing that is mostly that it's not clear what the impact of that method is: is that the source of truth, or the container parameter? I'd rather avoid any ambiguity there if possible.

I'd be okay with retaining it to make things easier for JSON API Extras/Open API. But … Open API actually won't be affected, because it's ready JSON API Extras' settings directly:

  /**
   * {@inheritdoc}
   */
  public function getBasePath() {
    return parent::getBasePath() . $this->getJsonApiBase();
  }

  /**
   * Determine the base for JsonApi's endpoint routes.
   *
   * @return string
   *   The url prefix used for all jsonapi resource endpoints.
   */
  public function getJsonApiBase() {
    $root = '/jsonapi';
    if ($this->moduleHandler->moduleExists('jsonapi_extras')) {
      $root = '/' . $this->configFactory
        ->get('jsonapi_extras.settings')
        ->get('path_prefix');
    }
    return $root;
  }

And JSON API Extras is already broken by #2949632-34: Make ResourceTypeRepository aware of the path prefix having shipped in the 1.21 release of JSON API that you tagged a few days ago. So changing it again won't actually impact JSON API Extras!

Retitling and bumping priority per #14+#15+#20.

RTBC'ing per #20.