Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hotfixing request stack mis-handling (https://www.drupal.org/node/261… #100

Merged
merged 2 commits into from May 10, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -4,7 +4,6 @@

use Drupal\Core\DependencyInjection\DependencySerializationTrait;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\Core\Routing\ResettableStackedRouteMatchInterface;
use Drupal\Core\Url;
use Drupal\graphql_block\BlockResponse;
use Drupal\graphql_core\GraphQL\FieldPluginBase;
Expand Down Expand Up @@ -68,6 +67,14 @@ public function resolveValues($value, array $args, ResolveInfo $info) {
}

$response = $this->httpKernel->handle($request, HttpKernelInterface::SUB_REQUEST);

// TODO:
// Remove the request stack manipulation once the core issue described at
// https://www.drupal.org/node/2613044 is resolved.
while ($this->requestStack->getCurrentRequest() === $request) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, nice fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

¯_(ツ)_/¯

$this->requestStack->pop();
}

if ($response instanceof BlockResponse) {
foreach ($response->getBlocks() as $block) {
yield $block;
Expand Down
8 changes: 8 additions & 0 deletions modules/graphql_core/src/Plugin/GraphQL/Fields/Context.php
Expand Up @@ -128,6 +128,14 @@ public function resolveValues($value, array $args, ResolveInfo $info) {
$this->languageManager->reset();

$response = $this->httpKernel->handle($request, HttpKernelInterface::SUB_REQUEST);

// TODO:
// Remove the request stack manipulation once the core issue described at
// https://www.drupal.org/node/2613044 is resolved.
while ($this->requestStack->getCurrentRequest() === $request) {
$this->requestStack->pop();
}

if ($response instanceof ContextResponse) {
yield $response->getContext()->getContextValue();
}
Expand Down
1 change: 1 addition & 0 deletions src/Cache/Context/QueryCacheContext.php
Expand Up @@ -4,6 +4,7 @@

use Drupal\Core\Cache\CacheableMetadata;
use Drupal\Core\Cache\Context\CacheContextInterface;
use Drupal\Core\Routing\StackedRouteMatchInterface;
use Symfony\Component\HttpFoundation\RequestStack;
use Youshido\GraphQL\Parser\Parser;

Expand Down
10 changes: 5 additions & 5 deletions src/Cache/ResponsePolicy/DenyMutation.php
Expand Up @@ -3,7 +3,7 @@
namespace Drupal\graphql\Cache\ResponsePolicy;

use Drupal\Core\PageCache\ResponsePolicyInterface;
use Drupal\Core\Routing\ResettableStackedRouteMatchInterface;
use Drupal\Core\Routing\StackedRouteMatchInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Youshido\GraphQL\Execution\Context\ExecutionContext;
Expand All @@ -16,25 +16,25 @@ class DenyMutation implements ResponsePolicyInterface {
/**
* The route match.
*
* @var \Drupal\Core\Routing\ResettableStackedRouteMatchInterface
* @var \Drupal\Core\Routing\StackedRouteMatchInterface
*/
protected $routeMatch;

/**
* Constructs a new request policy instance.
*
* @param \Drupal\Core\Routing\ResettableStackedRouteMatchInterface $routeMatch
* @param \Drupal\Core\Routing\StackedRouteMatchInterface $routeMatch
* The route provider service.
*/
public function __construct(ResettableStackedRouteMatchInterface $routeMatch) {
public function __construct(StackedRouteMatchInterface $routeMatch) {
$this->routeMatch = $routeMatch;
}

/**
* {@inheritdoc}
*/
public function check(Response $response, Request $request) {
if ($this->routeMatch->getCurrentRouteMatch()->getRouteName() !== 'graphql.request') {
if ($this->routeMatch->getRouteMatchFromRequest($request) !== 'graphql.request') {
return NULL;
}

Expand Down
10 changes: 5 additions & 5 deletions src/Cache/ResponsePolicy/DenyPageCache.php
Expand Up @@ -3,7 +3,7 @@
namespace Drupal\graphql\Cache\ResponsePolicy;

use Drupal\Core\PageCache\ResponsePolicyInterface;
use Drupal\Core\Routing\ResettableStackedRouteMatchInterface;
use Drupal\Core\Routing\StackedRouteMatchInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

Expand All @@ -15,25 +15,25 @@ class DenyPageCache implements ResponsePolicyInterface {
/**
* The route match.
*
* @var \Drupal\Core\Routing\ResettableStackedRouteMatchInterface
* @var \Drupal\Core\Routing\StackedRouteMatchInterface
*/
protected $routeMatch;

/**
* Constructs a new request policy instance.
*
* @param \Drupal\Core\Routing\ResettableStackedRouteMatchInterface $routeMatch
* @param \Drupal\Core\Routing\StackedRouteMatchInterface $routeMatch
* The route provider service.
*/
public function __construct(ResettableStackedRouteMatchInterface $routeMatch) {
public function __construct(StackedRouteMatchInterface $routeMatch) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for those changes!

$this->routeMatch = $routeMatch;
}

/**
* {@inheritdoc}
*/
public function check(Response $response, Request $request) {
if ($this->routeMatch->getCurrentRouteMatch()->getRouteName() !== 'graphql.request') {
if ($this->routeMatch->getRouteMatchFromRequest($request) !== 'graphql.request') {
return static::DENY;
}

Expand Down
33 changes: 31 additions & 2 deletions src/Controller/RequestController.php
Expand Up @@ -15,6 +15,7 @@
use Drupal\graphql\Reducers\ReducerManager;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\HttpKernelInterface;
use Youshido\GraphQL\Schema\AbstractSchema;
Expand Down Expand Up @@ -66,6 +67,13 @@ class RequestController implements ContainerInjectionInterface {
*/
protected $reducerManager;

/**
* The request stack service.
*
* @var \Symfony\Component\HttpFoundation\RequestStack
*/
protected $requestStack;

/**
* Constructs a RequestController object.
*
Expand All @@ -79,15 +87,26 @@ class RequestController implements ContainerInjectionInterface {
* The config service.
* @param \Symfony\Component\HttpKernel\HttpKernelInterface $httpKernel
* The http kernel service.
* @param \Symfony\Component\HttpFoundation\RequestStack $requestStack
* The request stack service.
* @param \Drupal\Core\Render\RendererInterface $renderer
* The renderer service.
*/
public function __construct(ContainerInterface $container, AbstractSchema $schema, ReducerManager $reducerManager, Config $config, HttpKernelInterface $httpKernel, RendererInterface $renderer) {
public function __construct(
ContainerInterface $container,
AbstractSchema $schema,
ReducerManager $reducerManager,
Config $config,
HttpKernelInterface $httpKernel,
RequestStack $requestStack,
RendererInterface $renderer
) {
$this->container = $container;
$this->schema = $schema;
$this->reducerManager = $reducerManager;
$this->config = $config;
$this->httpKernel = $httpKernel;
$this->requestStack = $requestStack;
$this->renderer = $renderer;
}

Expand All @@ -101,6 +120,7 @@ public static function create(ContainerInterface $container) {
$container->get('graphql.reducer_manager'),
$container->get('config.factory')->get('system.performance'),
$container->get('http_kernel'),
$container->get('request_stack'),
$container->get('renderer')
);
}
Expand Down Expand Up @@ -145,7 +165,16 @@ public function handleBatchRequest(Request $request, array $queries = []) {
$subRequest->setSession($session);
}

return $this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST);
$output = $this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST);

// TODO:
// Remove the request stack manipulation once the core issue described at
// https://www.drupal.org/node/2613044 is resolved.
while ($this->requestStack->getCurrentRequest() === $subRequest) {
$this->requestStack->pop();
}

return $output;
}, $queries);

// Gather all responses from all sub-requests.
Expand Down
18 changes: 11 additions & 7 deletions src/EventSubscriber/CacheSubscriber.php
Expand Up @@ -10,7 +10,7 @@
use Drupal\Core\Cache\Context\CacheContextsManager;
use Drupal\Core\PageCache\RequestPolicyInterface;
use Drupal\Core\PageCache\ResponsePolicyInterface;
use Drupal\Core\Routing\ResettableStackedRouteMatchInterface;
use Drupal\Core\Routing\StackedRouteMatchInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\HttpFoundation\Response;
Expand Down Expand Up @@ -98,7 +98,7 @@ class CacheSubscriber implements EventSubscriberInterface {
* A policy rule determining the cacheability of a request.
* @param \Drupal\Core\PageCache\ResponsePolicyInterface $responsePolicy
* A policy rule determining the cacheability of the response.
* @param \Drupal\Core\Routing\ResettableStackedRouteMatchInterface $routeMatch
* @param \Drupal\Core\Routing\StackedRouteMatchInterface $routeMatch
* The current route match.
* @param \Symfony\Component\HttpFoundation\RequestStack $requestStack
* The request stack.
Expand All @@ -112,7 +112,7 @@ class CacheSubscriber implements EventSubscriberInterface {
public function __construct(
RequestPolicyInterface $requestPolicy,
ResponsePolicyInterface $responsePolicy,
ResettableStackedRouteMatchInterface $routeMatch,
StackedRouteMatchInterface $routeMatch,
RequestStack $requestStack,
CacheBackendInterface $responseCache,
CacheBackendInterface $metadataCache,
Expand All @@ -136,12 +136,14 @@ public function __construct(
* The event to process.
*/
public function onRouteMatch(GetResponseEvent $event) {
if ($this->routeMatch->getCurrentRouteMatch()->getRouteName() !== 'graphql.request') {
$request = $event->getRequest();
$routeMatch = $this->routeMatch->getRouteMatchFromRequest($request);

if ($routeMatch->getRouteName() !== 'graphql.request') {
return;
}

// We do not need to cache batch requests locally.
$request = $event->getRequest();
if (empty($request->attributes->get('query'))) {
return;
}
Expand Down Expand Up @@ -173,7 +175,10 @@ public function onRouteMatch(GetResponseEvent $event) {
* The event to process.;
*/
public function onResponse(FilterResponseEvent $event) {
if ($this->routeMatch->getCurrentRouteMatch()->getRouteName() !== 'graphql.request') {
$request = $event->getRequest();
$routeMatch = $this->routeMatch->getRouteMatchFromRequest($request);

if ($routeMatch->getRouteName() !== 'graphql.request') {
return;
}

Expand All @@ -188,7 +193,6 @@ public function onResponse(FilterResponseEvent $event) {
}

// We do not need to cache batch requests locally.
$request = $event->getRequest();
if (empty($request->attributes->get('query'))) {
return;
}
Expand Down