[regression] ResourceTypeRepository is significantly slower in JSON:API 2, becomes noticeable when handling hundreds of interlinked resources

Thanks for this feedback @yobottehg, I think this is priceless!

Do you think you can share a site dump so we can debug locally? If not, do you think you can come up with a minimal site that exposes this issue?

We really appreciate the report, and we really want to fix it. However, as you well know, it is very hard to fix a performance issue that depends on content and configuration you don't have.

Fascinating. We actually made JSON:API 2.x significantly faster and more efficient than 1.x, specifically for includes. So … yeah, very surprised! It'd be awesome if you could provide a dump like @e0ipso asked! 🙏

Thank you so much for testing JSON:API 2.x! We really really appreciate it :)

@Wim Leers I think we made it faster for some use cases and slower in other. I think there's not an inherent trade-off in this case, and we can actually make it faster across the board.

I attempted to install this dump. I hacked core so I'd always be logged in as user 1 (I couldn't get my regenerated root password to be accepted and didn't want to waste time on debugging that). I installed the necessary modules. I got Warning: Invalid argument supplied for foreach() in Drupal\jsonapi_extras\Entity\JsonapiResourceConfig->getFieldMapping() (line 135 of modules/jsonapi_extras/src/Entity/JsonapiResourceConfig.php). warnings upon rebuilding jsonapi routes (the config doesn't seem to be compatible with the latest JSON:API Extras). For some reason, every page load seems to not properly finish but keep loading forever; the server doesn't seem to terminate (I guess this is where the dragons are).

Installing a partially copy/pasted codebase where I'm warned of there being dragons doesn't seem to be working very well :( Could you install https://blackfire.io/, reproduce your performance issues, and then share the Blackfire.io performance profiles? Thank you very much! 🙏

Sorry for that i thought it would be easier to share :(

Don't worry, it's fine! I do appreciate you taking the time to create that dump. Unfortunately it was just not sufficient to reproduce this without figuring out dozens more setup details particular to your site.

Here a blackfire profile:

🎉🎉🎉

Anything else i can do?

A blackfire profile of the same request (the same URL) against JSON:API 1.x would be super valuable!

Analyzing Blackfire profile…

This jumped out to my eye immediately. This would be a JSON:API Extras issue insufficently mitigated in #3007091: Performance issue in ConfigurableResourceType.

This other performance problem will be fixed in #3014283: Use ResourceTypeFields to statically determine the field normalizer.

There seems to be an interaction with https://www.drupal.org/project/imageapi_optimize from the image styles core patch, according to:

I think reducing the number of image styles can improve performance greatly. Try with https://www.drupal.org/project/consumer_image_styles

From the Blackfire profile in #10:

1. 1519 non-reference fields and 316 reference fields normalized — please update to the tip of the 8.x-2.x branch, #3011099: Only serialize sparse_fieldset fields in \Drupal\jsonapi\Normalizer\EntityNormalizer::normalize() landed 2 weeks ago which should massively improve performance for sparse fieldsets and bring this number down a lot. We're waiting for one more issue to land before tagging RC2.
2. 9.74% of exclusive wall time is spent in \Symfony\Component\Serializer\Serializer::getNormalizer()! -> #3014283-3: Use ResourceTypeFields to statically determine the field normalizer + #3011497-8: [upstream] Too many calls to \Symfony\Component\Serializer\Serializer::getNormalizer().
3. 21.92% of inclusive wall time is spent in Drupal\image\Plugin\DataType\ComputedImageStyleDerivatives::computeImageStyleMetadata(), and 11.03% of that is spent in crop_file_url_alter(). This means https://www.drupal.org/project/crop is responsible for ~11% of the response time! But of course, this is only happening because you have #2825812: ImageItem should have an "derivatives" computed property, to expose all image style URLs applied (yay!), without crop, it'd still be >10% of the response time. However, once you're running a version of JSON:API 2 with #3011099: Only serialize sparse_fieldset fields in \Drupal\jsonapi\Normalizer\EntityNormalizer::normalize() applied, much of this will probably disappear.
4. 1081 calls to \Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository::get(), leading to 40.5% of incluse wall time! 96% of that 40,5% is spent in unserialize()… So we may want to revisit the introduction of static caching in a memory backend, which was introduced in #2984886: Trigger route rebuild when new bundles/fields are added/removed.

So:

1. Please update to commit 29a0e42d882afe5aa31fea9c61e6635bf42bf7a2 or later. Although quite possibly you're already on that version. If you aren't yet, this should cause a significant change.
2. The next biggest thing to address is point 4. PoC patch attached. Please apply and report back (with a new Blackfire profile)! (But please test the previous thing separately first if you aren't already running that version.) I already have some ideas for making this much faster, but to get us started, here's a super naïve initial approach. (A key problem is that we're computing all ResourceType value objects for every request, we can easily change that.)

#14: I'm not sure it is a JSON:API Extras perf bug, I think it probably is a JSON:API perf bug! See #18.4.

#15: yep, I already commented on that issue to that effect, and also see #18.2 :)

#17: yep, I also pointed that out in #18.3. But none of this would be computed if those fields would be omitted using a sparse fieldset thanks to #3011099: Only serialize sparse_fieldset fields in \Drupal\jsonapi\Normalizer\EntityNormalizer::normalize()!

#16: thanks for that 1.x profile! Now I have the data I need :)

It's clear based on the 2.x profile in #10 and the 1.x profile in #16 that BY FAR the biggest problem for sure is the problem I described in #18.4, and provided a rough PoC patch for. It was nearly free in 1.x, and now it's super expensive.

The 1.x profile is 23.3 s, the 2.x one is 32.7. 40% of 32.7 s is 13 s. If you subtract that, you end up at 19 s. Fixing only the ResourceTypeRepository's performance will make JSON:API 2 faster for @yobottehg!

Retitling for that. Will work on this tomorrow.

So we may want to revisit the introduction of static caching in a memory backend, which was introduced in #2984886: Trigger route rebuild when new bundles/fields are added/removed

Exactly my thinking. We also added the static cache service in that JSON:API Extras issue. I planned to resolve that via an object/class property or some legacy use of drupal_static.

Do i understand it wrong that i archive the same but disabling all that fields via jsonapi_extras?

That's correct, disabled fields are not serialized either. You are good in this regard.

Because I did this and otherwise i need to statically define all fields on the query and for all includes

A more important question is: why do you even need such a complex query? Are you trying to get all the data on the site in a single request?

Looks already a lot better.

Great! :) 17.2 seconds, I predicted 19. Great :)

Blackfire.io supports comparing two profiles. Could you share the URL where #23 and #10 are being compared?

I'll work on this more today. But this is a task, not a bug. Since everything is working correctly, just slower than it should. Correctness is the most important thing.

First: the Blackfire.io profiles posted here are now also helping the Crop module: #2975722: Many slow queries for lookup up crop of a image_style + file :)

#27: thanks for https://blackfire.io/profiles/compare/93e76c97-1f74-4b74-9be4-430c3d6528...() — that confirms that #18 makes this not 40% faster like I predicted in #20, but 47%!
Could you also create a comparison between the JSON:API 1.x profile from #16 and the patched JSON:API 2.x profile from #23? That'd allow us to see whether So the same request took 3 times as long as with jsonapi 1 and used 3 times the amount of memory. Is this known? is now fixed!

No this is just some part but the part with the most includes etc. I fetch all data for the node type "project" into the Model in the consumer. Then i save this to a store for reuse on detail pages etc.

It may be "just some part", but then you say "fetch all data for […]", which is pretty much answering "yes" to Are you trying to get all the data on the site in a single request? 😃 It may not be fetching 100% of the data managed in Drupal, but it sounds like you're still fetching 100% of the data for a particular use case, for client-side caching.
Subsequent requests should be cached by Dynamic Page Cache, and should result in <100 ms response times. Can you confirm this is the case?

If so, I think a many-second response time may actually be acceptable in this case.

Thanks for the extra info @yobottehg. I am not convinced I support the combo of Paragraphs + JSON:API but it has proven very useful to uncover the performance bottlenecks.

As i think one slow request and subsequent faster responses are better than multiple not so slow requests.

There is no golden rule for this. Imagine that you split the results into three requests A + B + C. If you can re-use A in some other pages and B in other, then it makes sense to split it in three. You'll be leveraging HTTP's cache (in multiple layers). However, as you can see there is a lot of craft that goes into this and it does not refactor easily.

Maybe off-topic, but managed to move to 2.x. Currently using jsonapi 2.x rc1. What I can see is that ResourceTypeRepository::all() is a slow function from my profiling it shows that the bottleneck there is coming from the static cache service :(. Replacing that with a plain PHP private variable solves it (see metrics below).

When checked deeper the overhead was coming from the unserialize that the cache layer is calling internally and the wake-up that gets executed multiple times on the cached resources.

Replacing that with a private property lowered (when profiler is running):

1. Response time from 12.4 to 4.8 seconds.
2. Memory usage from 109MB to 19MB.

Profiling output before:

Profiling output after:

Maybe we can work-around this with a single flag to be cached in the global cache and when the flag is invalid / missing - regenerate the private property, as it's SO Much faster sparing the unserialization calls.

Expect a PoC patch shortly.

Here is the patch. I've moved the cache service to store a simple TRUE value, we are using it as a simple flag. When the flag is invalid (missing cache) - we regenerate the private cache in the class itself. This way we can still clear that cache from outside the class and sparing the heavy unserialize calls that are generating the overhead mentioned above.

Performance is comparable to the after benchmarks from the previews comment, but we still have the external cache clear of the static cache service.

Though this conflicts with the patch from #2992833: Add a version negotiation to revisionable resource types

I'd like to remove the additional level of indirection.

If we assume a resource type does not change during a request (it really shouldn't) we should be good without cache invalidation and a protected property would suffice. Additionally, we would be able to remove a service dependency.

Here is the new version of this :) - as requested - no static cache, clean and simple. I am expecting same performance as the after benchmark. Have not bench-marked this...

No time to mess with the tests. Anyone can take over :)

The approach of using a hand-written static cache is a treacherous road to tread ;)

We've already tried that once and it eventually evolved into the current code. #35 fails because there are situations where a genuine cache backend is required to support cache invalidation during a request.

The underlying problem here isn't using a cache bin, it's really my naive use of cache.static. When I added it, I assumed it was simply an array-backed cache and didn't realize its docblock said:

Should be used for unit tests and specialist use-cases only, does not store cached items between requests.... The functions ::prepareItem()/::set() use unserialize()/serialize(). It behaves as an external cache backend to avoid changing the cached data by reference.

What we actually want is the MemoryCache backend which implements MemoryCacheInterface That interface promises exactly what we'd expect:

Objects stored must be the same instance when retrieved from cache, so that this can be used as a replacement for protected properties.

Beyond just replacing cache.static with the right in-memory cache backend, this implements a backend chain so that resource types don't need to be recalculated on every request (this should mean this issue isn't just fixing a performance regression, it will actually result in a performance improvement!).

No interdiff provided because this patch was started from scratch.

@ndobromirov, can you profile the attached patch?

+++ b/jsonapi.services.yml
@@ -149,6 +149,24 @@ services:
+  jsonapi.cache.resource_types:
+    class: Drupal\Core\Cache\CacheBackendInterface
+    factory: cache_factory:get
+    arguments: [jsonapi_resource_types]
+    tags:
+      - { name: cache.bin, default_backend: jsonapi.cache.persisted_memory }

Looks like the jsonapi_test_collection_count needs to clear the resource type repository cache on install. Makes sense, it modifies how resource types are generated.

In fixing the above, I realized that cache bins require a specific naming format: cache.{bin_name} so that one can call \Drupal::cache($bin_name)->invalidateAll().

That led me to realize I wasn't creating the cache services correctly and that I needed to create a factory for the "persisted memory" backend this introduces.

Will do, some time today.

Hmmm, I'd like to understand the treason you are talking about with the in memory cache. That's a super common pattern.

@e0ipso, heh, I think you misunderstood me.

Treacherous:

• a: likely to betray trust
• b: providing insecure footing or support
• c: marked by hidden dangers, hazards, or perils

I meant "treacherous" as in the b + c definitions, not as in a.

I was trying to say that using a property on a singleton as a cache can bite you in unexpected ways. Cache invalidation is hard ;)

So, I'm all for an in-memory cache, just not a property as a cache, especially when a tool already exists (MemoryCache) to do what we need.

^ This is the issue which introduced the current cache bin.

If an entity reference field or bundle is added, then a new route needs to be created. So we need to trigger a route rebuild. The route rebuild runs after the response is sent (Kernel::TERMINATE). That means it runs during the lifetime of a request, which caused a problem: new routes would never appear because the resource type repository was using the resource types calculated at the start of the request (prior to the field being added) . That's because it was using the $all class property as a pseudo-cache with no way to invalidate it.

That's because it was using the $all class property as a pseudo-cache with no way to invalidate it.

Yep, this was the essence of the problems before #2984886: Trigger route rebuild when new bundles/fields are added/removed. That issue was surprisingly tricky. I didn't like the solution that we ended up committing, but the different approach I was working on got stuck.
(Also I don't remember off the top of my head if my approach would've been better — just saying that there was a different approach in that issue that may be helpful here, it may serve as inspiration.)

I think I understand the problem now. Thanks for being patient, I have my head deep elsewhere.

We need invalidation because:

1. User adds a bundle, adds an entity reference field, or changes entity reference settings.
2. Those changes have implications on routes, so we signal for route rebuild.
3. We build the route definitions from (a) the resource types themselves and (b) the data on them.

We invalidate cahe or we get a previously cached version of the resource types otherwise. That leads to bad routes, which is undesirable.

My question is. If the request (1) is not JSON:API-managed, should we have a previously cached version of ResourceTypeRepository::all()?

I assume that it may be called outside of JSON:API-managed requests in order to build the router in Routes. If that is true, I think we should avoid caching when building routes. Something like ResourceTypeRepository::all($cache: FALSE) inside of Routes. Why? Because then we can dump all the hard cache invalidation stuff and reduce the dependency footprint by using a property cache.

@e0ipso, I hear your desire to take a simpler approach, but that would make turn this issue into a bigger refactoring instead of a bugfix.

This regression was caused because I used the wrong cache bin. If I had used a cache bin which used the MemoryCache class vs. the MemoryBackend class, there'd be nothing to see here.

Perhaps it was my fault for going one step further and also introducing a persistent cache. So, if you'd be more amenable to it, I'd happily move that piece to a new issue. That said, I'm a little bit surprised that you're not more excited by the patch in #40. It should result a nice boost for JSON:API Extras since having a persistent cache will mean that there will no longer be a need to load its configuration entities on every uncached request.

but that would make turn this issue into a bigger refactoring instead of a bugfix.

I'm not sure I'm explaining myself. I think the changes I'm proposing are pretty minimal.

That said, I'm a little bit surprised that you're not more excited by the patch in #40. It should result a nice boost for JSON:API Extras since having a persistent cache will mean that there will no longer be a need to load its configuration entities on every uncached request.

On the contrary, it complicates things in JSON:API Extras and given that we're maintaining compatibility with both versions at the same time, it's very challenging. The config entities are already cached individually, they load fast. But now I'll have to also take into account invalidation of the ConfigurableResourceType caches. ConfigurableResourceType already has a static cache, so the cache layers cake becomes a mess.

IMO the proposed approach brings in unnecessary complexity that bleeds over other projects. The proposed alternative is not a bigger refactoring (unless I'm missing something) and results in a simpler approach (KISS, YAGNI and Occam’s Razor).

Testing the patch from 40 is conflicting for me against the patch I am using from #2992833: Add a version negotiation to revisionable resource types comment 225.
Pushing a re-base on top of that. Hopefully it will apply with composer.

The removal of the static cache from there seems to break the jsonapi_extras... I am getting the following error.

PHP Fatal error:  Uncaught TypeError: Argument 1 passed to Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType::setStaticCache() must implement interface Drupal\Core\Cache\CacheBackendInterface, null given, called in /docroot/modules/contrib/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php on line 188 and defined in /docroot/modules/contrib/jsonapi_extras/src/ResourceType/ConfigurableResourceType.php:97
Stack trace:
#0 /docroot/modules/contrib/jsonapi_extras/src/ResourceType/ConfigurableResourceTypeRepository.php(188): Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType->setStaticCache(NULL)
#1 /docroot/modules/contrib/jsonapi/src/ResourceType/ResourceTypeRepository.php(94): Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository->createResourceType(Object(Drupal\Core\Config\Entity\ConfigEntityType), 'block')
#2 [internal function]: Drupal\jsonapi\ResourceType\ResourceTypeRepository->Drupa in /docroot/modules/contrib/jsonapi_extras/src/ResourceType/ConfigurableResourceType.php on line 97

As the static cache was introduced in jsonapi 2.x. From what I can see we are just using the protected scope of the static cache dependency in jsonapi_extras. Injecting that dependency along to the concrete configurable resource types.

It get's injected manually in 1.x. For 2.x we expect it to be there.

If we follow that direction, there should be a follow-up for extras module as well.

I have a fix for the extra that will be 1.x and 2.x compliant.
Should I open the follow-up?

Overall the method seems to be as fast as the simpler implementation.

The request is half a second overall slower, mainly because the chained fast is not good at handling the cache scenario that this was used in the jsonapi_extras. The cache service instance needs to be propagated along the way to there.

Note that I am testing cold cache scenarios, so from the patch in 40 I am getting level 1 speedups only. I think if everything is there it should be properly seeded up.

Still here is the top level run report and method overview:

And the method.

The getting of resources is same speed, likely faster in the long run, due to bulk reads.
The request is half a second slower - mainly due to some 2K DB queries chained fast is doing to level 2 (DataBase) storage every time. Likely due to APCu size limits abused in jsonapi_extras.

I suggest to have just this instead in services and drop the custom factory

cache.jsonapi_resource_types:
    class: Drupal\Core\Cache\MemoryCache\MemoryCache

Here is what I got with that - even less memory and even faster...

Adding to needs work, as this needs to be considered.

1. Are we going to break jsonapi_extras by changing the property?
2. Should we go with eoipso's proposal that seems the fastest :)?

Here is the patch.

Benchmarks for it should be same as the one above (or better).
No regression in jsonapi_extras can be seen.

This is implementation of option 2 above.
I am seeing about 60% speed-up on a particular home page :D

One more plus here - this does not conflict with #2992833: Add a version negotiation to revisionable resource types

Neat! Regardless of the direction we go, simple caching will improve from ≈13s to ≈4.5s!

So apparently this is not cache tags aware, or I am missing meta-information to trigger the invalidation in the new bucket in here.

\Drupal::service('cache_tags.invalidator')->invalidateTags(['jsonapi_resource_types']);

For some reason the above is not aware about our new cache bucket...

Here is a random patch trying to fix that :)

Patch files should be generated...

Ooooh, I ❤️ #62. That'a a really nice, minimal change set. I hope tests pass!

Thanks for diving into this @ndobromirov! That's awesome.

Opened a core one to audit the usage of cache.static as it appears to be deadly slow...

@ndobromirov, does your patch in #62 work without breaking JSON API Extras?

This is RTBC from me, but I think @e0ipso should give a +1.

@e0ipso, since this a a regression on the 2.x branch, can we commit this and open a follow up for your proposal? Given that it would make things easier for JSON API Extras, I'm not opposed to it.

My Patch was ok with jsonapi_extras, as I did not change the protected property name.
Yours would have been as well if you didn't change cacheStatic to cache in the repository class.

I am pretty sure this implements the proposal :).

@ndobromirov, does your patch in #62 work without breaking JSON API Extras?

We are adding a container pass to add the @static.cache when using with 1.23, so we'll need to update it regardless.

I am committing this as is.

@eoipso update is one thing, breaking change - another :).
Here is the follow-up: #3016725: Do not use cache.static

I love the simplicity, but … this does not work on Drupal 8.5. \Drupal\Core\Cache\MemoryCache\MemoryCache was only introduced in 8.6 + 8.7 about six months ago.

We can still do this, but on 8.5 we'll need to make \Drupal\jsonapi\JsonapiServiceProvider alter it to fall back to the behavior of HEAD. So then JSON:API 2.x will not have this performance improvement, but that's okay. Yet another reason to move on from 8.5 to 8.6.

Just queued a test against Drupal 8.5, it should fail.

+++ b/jsonapi.services.yml
@@ -141,6 +141,11 @@ services:
+    tags: [{ name: cache.bin }]

Also, why do we need this? If we need it, let's document it.

Hmm. Are we supporting 8.5 on the 2.x branch? I honestly don't remember what we decided, if we decided it at all.

Are we supporting 8.5 on the 2.x branch?

My recollection was that we were supporting 8.6+ for now (for obvious reasons), but we'll move to 8.7 when it gets into core.

In any case, I think this will not be a problem when we get #3016733: Simplify ResourceTypeRepository; use a protected property in place of an in-memory cache bin. in place. Just lets hold off any tagging until then and 8.5 users will be safe.

@Wim

Also, why do we need this? If we need it, let's document it.

This is needed, because otherwise the \Drupal::service('cache_tags.invalidator')->invalidateTags(['jsonapi_resource_types']); is not finding our new cache service and thus not invalidating the data in there. You can see test failures in #55 same as #35, vs #62 that all passes.

I can document that and have a follow up. I get the backwards compatibility thing mentiond and will have a patch for that as well.

My recollection was that we were supporting 8.6+ for now (for obvious reasons), but we'll move to 8.7 when it gets into core.

Quoting 2.x's jsonapi.info.yml:

dependencies:
  - drupal:system (>=8.5.4)

The security team now supports both Drupal 8.5 and 8.6. JSON:API 2.x supporting both of those means we make it easier for people to make the transition. Because many sites will skip 8.6 and go from 8.5 to 8.7.

If JSON:API 2 goes into Drupal 8.7 core, that'd mean those sites have to not only deal with core disruptions (hopefully none!), but also the guaranteed JSON:API 1.x to 2.x disruptions.

If JSON:API 2 does work on 8.5, then those sites could first work on the transition to JSON:API 2 — today already actually — and then make the jump to 8.7 with pretty much zero disruption.

So … I'm hesitant to retrace our steps on that front now.

. Just lets hold off any tagging until then and 8.5 users will be safe.

Hm … so you're saying that this issue/commit was just a temporary stopgap solution until #3016733: Simplify ResourceTypeRepository; use a protected property in place of an in-memory cache bin. lands, and #3016733 blocks a 2.0 stable release?

Here is the patch proposed in #70.

It's not passing, although the patch does look good.

Error: Class 'Drupal\jsonapi\Reference' not found

Ahh!

+++ b/src/JsonapiServiceProvider.php
@@ -20,6 +20,17 @@ class JsonapiServiceProvider implements ServiceModifierInterface, ServiceProvide
+      $definition->setArgument(3, new Reference('cache.static'));

We're just missing a use … statement for this class :)

Here is the missed use statement.

Down to 1 fail but I do not see how can this be related...

@Wim Leers I'm OK with supporting 8.5. I remembered us saying that we only support 8.5 because it's easy, but maybe I'm mixing things up. Your comment makes sense.

Hm … so you're saying that this issue/commit was just a temporary stopgap solution until #3016733: Simplify ResourceTypeRepository; use a protected property in place of an in-memory cache bin. lands, and #3016733 blocks a 2.0 stable release?

Yes. #3016733: Simplify ResourceTypeRepository; use a protected property in place of an in-memory cache bin. should be easy enough. I don't think it'll take long.

So we only committed this for 2.0-RC2 users that really need this, so they can start using HEAD of the branch until 2.0 final? It'd have been simpler to just not do this and only do #3016733 then, because now HEAD is broken on 8.5. :(

This is now blocking #3019389: MediaTest fails on 8.7 since #2956368, which is blocking #3016866: The "me" link breaks the EntryPoint when user resource is internal. IOW this blocks further development of JSON:API.

+++ b/src/JsonapiServiceProvider.php
@@ -20,6 +21,17 @@ class JsonapiServiceProvider implements ServiceModifierInterface, ServiceProvide
+    // Backwards compatibility fix for Drupal 8.5 and older.
+    if (!class_exists('\Drupal\Core\Cache\MemoryCache\MemoryCache')) {

Change to: @todo Remove when we stop supporting Drupal 8.5.
And make this not use a class_exists(), but a core version check, like we do elsewhere.

Changes as suggested in #81. 8.7 tests will fail because of #3019389: MediaTest fails on 8.7 since #2956368.

Whoops! 🤦‍♂️

I have no idea what's going on with UserTest::testRelated on 8.5. That seems unrelated to this issue, but I'm unable to replicate it locally because I have MySQL 8 installed and that's not compatible with Drupal 8.5.

This combines the above patch with #3019389: MediaTest fails on 8.7 since #2956368 so that I can test 8.6 + 8.7. If this passes, I'm going to commit this and #3019389: MediaTest fails on 8.7 since #2956368 as they're blocking development on HEAD. Then, if 8.5 is still broken, I'll open a new issue to discover why UserTest::testRelated is broken on 8.5 (if it's related, we'll name the issue as a followup).

It failed. Too bad :/

What if you upload a revert patch? Does that make 8.5 pass? If so, it *somehow* was caused by the already committed change, and a revert then still is the best option after all…

I didn't see Wim's comment. This was meant to be posted concurrently with the commit but my comment got eaten. It was essentially this...

I'm going to go ahead and commit this because it's the correct thing to do WRT to the service definition. I believe the remaining failure is not a result of this patch, but instead an already present bug that was hidden by the incorrect cache usage (perhaps we're missing an invalidation somewhere).

This patch I committed revealed a new bug in #3019389: MediaTest fails on 8.7 since #2956368 on 8.5 (it was masked by the failure caused by this issue). Once this is in (it is now), we can address that in that issue.

I'm opening a new issue to address the UserTest::testRelated failure on 8.5 since I believe it's out of scope for this issue (if it is, we'll simply name that issue as a followup) to keep this issue from getting even more confusing than it already is.

@Wim Leers, I would prefer not to revert this. I think we all would agree that it probably made more forward progress for more users than it broke. How many users do we really believe are on 8.5 with JSON:API 2.x-dev? Let's close this, then refocus on the very small UserTest fail elsewhere.

This is the issue where we can address the 8.5 failure: #3019506: UserTest::testRelated() fails on Drupal 8.5 because no $reason is given in the Access Denied response

We can rename it as a followup to this one if we discover it's directly related.