The Acquia Content Hub module isn't compatible with other modules that rely on URL manipulation such as the CDN module. Thanks to Wim Leers who identified the culprit in \Drupal\acquia_contenthub\ContentHubInternalRequest::getEntityCdfByInternalRequest() with the way we generate sub-requests:

$request = Request::create($url);

which should be:

      $master_request = $this->requestStack->getCurrentRequest(); // @todo inject the RequestStack service into this service.
      $request = Request::create($url, 'GET', $master_request->query->all(), $master_request->cookies->all(), [], $master_request->server->all());
CommentFileSizeAuthor
#11 2942312-11-internal-request-stack.test-only.patch11.57 KBscor
#11 2942312-11-internal-request-stack.patch20.17 KBscor
#9 2942312-7-internal-request-stack.test-only.patch7.4 KBscor
#9 2942312-7-internal-request-stack.patch11.73 KBscor
#8 interdiff.txt4.88 KBscor
#7 2942312-7-internal-request-stack.txt11.73 KBscor
#5 2942312-4-internal-request-stack.test-only.patch7.12 KBscor
#5 2942312-4-internal-request-stack.patch11.45 KBscor
#2 internal-requests-fix.patch4.33 KBabarrios

Comments

scor created an issue. See original summary.

abarrios’s picture

abarrios commented
StatusFileSize
new internal-requests-fix.patch4.33 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs work
Issue tags: +Needs tests

Looks good, that's indeed the fix I proposed.

Would be great if we could add integration test coverage, that would have caught this problem.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2826137: CDN is serving files with wrong path

Discovered via #2826137: CDN is serving files with wrong path.

scor’s picture

scor commented
Status: Needs work » Needs review
StatusFileSize
new 2942312-4-internal-request-stack.patch11.45 KB
new 2942312-4-internal-request-stack.test-only.patch7.12 KB
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs tests

Hurray, tests!

  1. +++ b/tests/src/Unit/ContentHubInternalRequestTest.php
@@ -0,0 +1,182 @@
+    $config_factory = $this->getMock('Drupal\Core\Config\ConfigFactoryInterface');

    FYI: ConfigFactoryInterface::class would be much friendlier for refactoring. 

  2. +++ b/tests/src/Unit/ContentHubInternalRequestTest.php
@@ -0,0 +1,182 @@
+    $this->loggerFactory = $this->getMock('Drupal\Core\Logger\LoggerChannelFactoryInterface');
+    $this->kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
...
+    $this->requestStack = $this->getMockBuilder('Symfony\Component\HttpFoundation\RequestStack')
+      ->disableOriginalConstructor()
+      ->getMock();

    FYI: $this->prophesize(…) is MUCH nicer to work with than the old-school mocks.

scor’s picture

scor commented
StatusFileSize
new 2942312-7-internal-request-stack.txt11.73 KB

Replaced some of the mock() with prophesize().

scor’s picture

scor commented
StatusFileSize
new interdiff.txt4.88 KB

Interdiff for previous patch.

scor’s picture

scor commented
StatusFileSize
new 2942312-7-internal-request-stack.patch11.73 KB
new 2942312-7-internal-request-stack.test-only.patch7.4 KB

doh! /me learns how to use drupal.org...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

It's just been too long :)

You haven't converted all of them to use prophesize() but that is fine — it really was just meant as an FYI to hopefully make future maintenance simpler!

scor’s picture

scor commented
StatusFileSize
new 2942312-11-internal-request-stack.patch20.17 KB
new 2942312-11-internal-request-stack.test-only.patch11.57 KB

We found another scenario not covered by the previous patches.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Glad to see this is helping stabilize the module in even more places!

Should the module perhaps have a service to handle internal requests? Then the same carefully crafted code can be reused everywhere, without the need to keep it all consistent.

didebru’s picture

didebru
Primary language German
commented

Is this issue related to this (warning/error) message?
Site is running on Drupal 8.5.0 with CDN enabled.

Declaration of Drupal\acquia_contenthub\Controller\ContentHubEntityRequestHandler::handle(Drupal\Core\Routing\RouteMatchInterface $route_match, Symfony\Component\HttpFoundation\Request $request) should be compatible with                                                                        [warning]
Drupal\rest\RequestHandler::handle(Drupal\Core\Routing\RouteMatchInterface $route_match, Symfony\Component\HttpFoundation\Request $request, Drupal\rest\RestResourceConfigInterface $_rest_resource_config) in include() (line 37 of
modules/contrib/acquia_contenthub/src/Controller/ContentHubEntityRequestHandler.php). include('/home/dschmidt/workspace/mylife/docroot/modules/contrib/acquia_contenthub/src/Controller/ContentHubEntityRequestHandler.php') (Line: 444)
Composer\Autoload\includeFile('/home/dschmidt/workspace/mylife/docroot/modules/contrib/acquia_contenthub/src/Controller/ContentHubEntityRequestHandler.php') (Line: 322)
Composer\Autoload\ClassLoader->loadClass('Drupal\acquia_contenthub\Controller\ContentHubEntityRequestHandler')
spl_autoload_call('Drupal\acquia_contenthub\Controller\ContentHubEntityRequestHandler')
ReflectionMethod->__construct('\Drupal\acquia_contenthub\Controller\ContentHubEntityRequestHandler', 'handle') (Line: 123)
Drupal\Core\Entity\EntityResolverManager->setParametersFromReflection(Array, Object) (Line: 208)
Drupal\Core\Entity\EntityResolverManager->setRouteOptions(Object) (Line: 48)
Drupal\Core\EventSubscriber\EntityRouteAlterSubscriber->onRoutingRouteAlterSetType(Object, 'routing.route_alter', Object)
call_user_func(Array, Object, 'routing.route_alter', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('routing.route_alter', Object) (Line: 184)
Drupal\Core\Routing\RouteBuilder->rebuild() (Line: 83)
Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild() (Line: 1156)
drupal_flush_all_caches() (Line: 52)
drupal_rebuild(Object, Object) (Line: 302)
drush_cache_rebuild() (Line: 422)
_drush_invoke_hooks(Array, Array) (Line: 231)
drush_command() (Line: 199)
drush_dispatch(Array) (Line: 67)
Drush\Boot\BaseBoot->bootstrap_and_dispatch() (Line: 66)
drush_main() (Line: 12)
scor’s picture

scor commented
Status: Needs review » Fixed
Related issues: +#2952380: Create service to handle internal requests

This was fixed with commit 90ca4c37 and released in 8.x-1.23.

@Insasse We fixed this issue as well as support for Drupal 8.5 in the new 8.x-1.23 release. Could you install this new version and let us know if the error is still happening? Please open a new ticket if you see errors.

didebru’s picture

didebru
Primary language German
commented

Thanks @scor 1.23 fixed it for me.

Status: Fixed » Closed (fixed)

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