[Contrib blocker] SubRequestBuffer service went missing in 4.x

[pfrenssen]

In the transition from 3.x to 4.x the SubRequestBuffer service was removed but we never got round to restoring it. The service was removed as part of the work in #812 which was one of the last PRs that was blocking the first 4.x alpha release.

It was probably just forgotten in the rush to create the alpha. Evidence for this is that we still have the service definition in graphql.services.yml, but the service class is missing.

This is a major (or even critical) issue since this is blocking migrations from GraphQL 3.x to 4.x. Also the GraphQL Core Schema contrib module depends on this service and cannot move out of beta until this is restored.

[dulnan]

There's a few issues with the subrequest feature in general. I ran into problems where the subrequest was served from cache. The SubrequestSubscriber subscribes to KernelEvents::REQUEST and then does this:

$request->attributes->set('_controller', '\Drupal\graphql\Controller\SubrequestExtractionController:extract');

The extraction controller is never actually called when the subrequest is served from dynamic page cache and thus the provided callback is never executed, resulting in the data producer using the buffer returning null.

To clarify: Not the actual subrequest is cached: You have to first visit the URL in the browser, e.g. /node/123 and then use the subrequest buffer for this URL for it to return null.

I have "fixed" this temporarily by subscribing to KernelEvents::RESPONSE and executing the callback in there. This works in any case, but the subrequest is now actually executed completely, incl. rendering a response, which is quite wasteful.

There's a proper fix here probably, like bypassing the dynamic page cache for subrequests. I haven't looked into it in detail yet.

I assume that this bug was already present in V3. Is it possible that this feature was supposed to be removed entirely? It probably makes sense to get it into a usable state first and fixing the cache bug.

[Kingdutch]

Hey @pfrenssen @dulnan ,

If I look at the work done in #812 then I can see subrequests mostly being used originally as working around a core limitation to support request batching, which found a different solution. So at least in my interpretation, not removing the service definition is the bug and the removal of the buffer itself was intentional.

Subrequests are also not something I've needed in my own implementations using the GraphQL module, so I'm curious to learn more about the use-case and what problems they solve :)

This is a major (or even critical) issue since this is blocking migrations from GraphQL 3.x to 4.x. Also the GraphQL Core Schema contrib module depends on this service and cannot move out of beta until this is restored.

Is there anything preventing the buffer from being implemented in the graphql_core_schema module itself? The buffers are an implementation of the Dataloader pattern.

As can be seen in the EntityLoad DataProducer, a buffer can be retrieved as service and the magic of a buffer is in how the resolver it returns is deferred. This pattern allows multiple loads to be queued, which will then only be loaded once when the $resolve callable is called and subsequent calls to other resolve values for the same buffer service will load directly from the buffer.

So the implementation of buffers is entirely supported by GraphQL's use of promises and it's breadth-first field resolution. Implementing a new buffer does not have to happen in the GraphQL module but should be possible entirely in contrib.

[dulnan]

Subrequests are also not something I've needed in my own implementations using the GraphQL module, so I'm curious to learn more about the use-case and what problems they solve :)

It‘s needed for all things that directly (or indirectly through hooks) use the current route match, for example: Breadcrumbs, language switch links, metatags, (some) views, webforms.
I think it doesn‘t technically have to be a buffer. I see it more as something like "executeInRenderContext" is. But it‘s possible I‘ve just continued to use functionality from 3.x to solve this problem without thinking about how it could be solved differently. But I don‘t see any other way than to issue a subrequest to be able to for example get breadcrumbs for a desired route via GraphQL. computed_breadcrumbs also does it this way.

Is there anything preventing the buffer from being implemented in the graphql_core_schema module itself?

Indeed, this is something that has been done in the meantime, to remove the dependency on this patch, so that the module is soon ready for a first stable release. I also see that graphql_compose has done the same and included the missing file in their module.

[almunnings]

getCurrentRequest and breadcrumbs, /shakes fist at cloud/. The worst.

Only way around it is adding/popping the request stack, which is what the buffer is doing, so it is a utility we can use, and want.