Issue #3419693 by mondrake, alexpott, smustgrave, catch: Log error instead of...

Issue #3419693 by mondrake, alexpott, smustgrave, catch: Log error instead of trigger_error in Drupal\Core\EventSubscriber\RedirectResponseSubscriber::checkRedirectUrl

(cherry picked from commit 25697a17)

core/core.services.yml

@@ -1384,7 +1384,7 @@ services:
     class: Drupal\Core\EventSubscriber\ResponseGeneratorSubscriber
   redirect_response_subscriber:
     class: Drupal\Core\EventSubscriber\RedirectResponseSubscriber
-    arguments: ['@unrouted_url_assembler', '@router.request_context']
+    autowire: true
   redirect_leading_slashes_subscriber:
     class: Drupal\Core\EventSubscriber\RedirectLeadingSlashesSubscriber
   request_close_subscriber:

core/lib/Drupal/Core/EventSubscriber/RedirectResponseSubscriber.php

@@ -7,24 +7,18 @@
 use Drupal\Core\Routing\LocalRedirectResponse;
 use Drupal\Core\Routing\RequestContext;
 use Drupal\Core\Utility\UnroutedUrlAssemblerInterface;
+use Symfony\Component\DependencyInjection\Attribute\AutowireServiceClosure;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Response;
-use Symfony\Component\HttpKernel\KernelEvents;
 use Symfony\Component\HttpKernel\Event\ResponseEvent;
-use Symfony\Component\HttpFoundation\RedirectResponse;
-use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpKernel\KernelEvents;
 
 /**
  * Allows manipulation of the response object when performing a redirect.
  */
 class RedirectResponseSubscriber implements EventSubscriberInterface {
 
-  /**
-   * The unrouted URL assembler service.
-   *
-   * @var \Drupal\Core\Utility\UnroutedUrlAssemblerInterface
-   */
-  protected $unroutedUrlAssembler;
-
   /**
    * Whether to ignore the destination query parameter when redirecting.
    *
@@ -32,22 +26,22 @@ class RedirectResponseSubscriber implements EventSubscriberInterface {
    */
   protected bool $ignoreDestination = FALSE;
 
-  /**
-   * The request context.
-   */
-  protected RequestContext $requestContext;
-
   /**
    * Constructs a RedirectResponseSubscriber object.
    *
-   * @param \Drupal\Core\Utility\UnroutedUrlAssemblerInterface $url_assembler
+   * @param \Drupal\Core\Utility\UnroutedUrlAssemblerInterface $unroutedUrlAssembler
    *   The unrouted URL assembler service.
-   * @param \Drupal\Core\Routing\RequestContext $request_context
+   * @param \Drupal\Core\Routing\RequestContext $requestContext
    *   The request context.
+   * @param \Closure $loggerClosure
+   *   A closure that wraps the 'logger.channel.php' service.
    */
-  public function __construct(UnroutedUrlAssemblerInterface $url_assembler, RequestContext $request_context) {
-    $this->unroutedUrlAssembler = $url_assembler;
-    $this->requestContext = $request_context;
+  public function __construct(
+    protected UnroutedUrlAssemblerInterface $unroutedUrlAssembler,
+    protected RequestContext $requestContext,
+    #[AutowireServiceClosure('logger.channel.php')]
+    protected \Closure $loggerClosure,
+  ) {
   }
 
   /**
@@ -87,13 +81,14 @@ public function checkRedirectUrl(ResponseEvent $event) {
         }
         catch (\InvalidArgumentException $e) {
           // If the above failed, it's because the redirect target wasn't
-          // local. Do not follow that redirect. Display an error message
-          // instead. We're already catching one exception, so trigger_error()
-          // rather than throw another one.
-          // We don't throw an exception, because this is a client error rather than a
-          // server error.
+          // local. Do not follow that redirect. Log an error message instead,
+          // then return a 400 response to the client with the error message.
+          // We don't throw an exception, because this is a client error rather
+          // than a server error.
           $message = 'Redirects to external URLs are not allowed by default, use \Drupal\Core\Routing\TrustedRedirectResponse for it.';
-          trigger_error($message, E_USER_ERROR);
+          /** @var \Psr\Log\LoggerInterface $logger */
+          $logger = ($this->loggerClosure)();
+          $logger->error($message);
           $safe_response = new Response($message, 400);
         }
         $event->setResponse($safe_response);

core/phpstan-baseline.neon

@@ -2724,14 +2724,6 @@ parameters:
 			count: 1
 			path: tests/Drupal/Tests/Core/Entity/EntityUrlTest.php
 
-		-
-			message: """
-				#^Call to deprecated method expectError\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:
-				https\\://github\\.com/sebastianbergmann/phpunit/issues/5062$#
-			"""
-			count: 2
-			path: tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php
-
 		-
 			message: """
 				#^Call to deprecated method expectWarning\\(\\) of class PHPUnit\\\\Framework\\\\TestCase\\:

core/tests/Drupal/Tests/Core/EventSubscriber/RedirectResponseSubscriberTest.php

@@ -4,12 +4,13 @@
 
 namespace Drupal\Tests\Core\EventSubscriber;
 
+use Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher as EventDispatcher;
 use Drupal\Core\EventSubscriber\RedirectResponseSubscriber;
 use Drupal\Core\Routing\TrustedRedirectResponse;
 use Drupal\Core\Utility\UnroutedUrlAssemblerInterface;
 use Drupal\Tests\UnitTestCase;
+use Psr\Log\LoggerInterface;
 use Symfony\Component\DependencyInjection\Container;
-use Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher as EventDispatcher;
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Event\ResponseEvent;
@@ -36,12 +37,21 @@ class RedirectResponseSubscriberTest extends UnitTestCase {
    */
   protected $urlAssembler;
 
+  /**
+   * The mocked logger closure.
+   */
+  protected \Closure $loggerClosure;
+
   /**
    * {@inheritdoc}
    */
   protected function setUp(): void {
     parent::setUp();
 
+    $this->loggerClosure = function (): LoggerInterface {
+      return $this->prophesize(LoggerInterface::class)->reveal();
+    };
+
     $this->requestContext = $this->getMockBuilder('Drupal\Core\Routing\RequestContext')
       ->disableOriginalConstructor()
       ->getMock();
@@ -82,7 +92,7 @@ public function testDestinationRedirect(Request $request, $expected) {
     $response = new RedirectResponse('http://example.com/drupal');
     $request->headers->set('HOST', 'example.com');
 
-    $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
+    $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext, $this->loggerClosure);
     $dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
     $event = new ResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
     $dispatcher->dispatch($event, KernelEvents::RESPONSE);
@@ -122,11 +132,11 @@ public function testDestinationRedirectToExternalUrl($request, $expected) {
     $kernel = $this->createMock('Symfony\Component\HttpKernel\HttpKernelInterface');
     $response = new RedirectResponse('http://other-example.com');
 
-    $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
+    $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext, $this->loggerClosure);
     $dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
     $event = new ResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
-    $this->expectError();
     $dispatcher->dispatch($event, KernelEvents::RESPONSE);
+    $this->assertSame(400, $event->getResponse()->getStatusCode());
   }
 
   /**
@@ -139,7 +149,7 @@ public function testRedirectWithOptInExternalUrl() {
     $request = Request::create('');
     $request->headers->set('HOST', 'example.com');
 
-    $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
+    $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext, $this->loggerClosure);
     $dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
     $event = new ResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
     $dispatcher->dispatch($event, KernelEvents::RESPONSE);
@@ -170,11 +180,11 @@ public function testDestinationRedirectWithInvalidUrl(Request $request) {
     $kernel = $this->createMock('Symfony\Component\HttpKernel\HttpKernelInterface');
     $response = new RedirectResponse('http://example.com/drupal');
 
-    $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext);
+    $listener = new RedirectResponseSubscriber($this->urlAssembler, $this->requestContext, $this->loggerClosure);
     $dispatcher->addListener(KernelEvents::RESPONSE, [$listener, 'checkRedirectUrl']);
     $event = new ResponseEvent($kernel, $request, HttpKernelInterface::SUB_REQUEST, $response);
-    $this->expectError();
     $dispatcher->dispatch($event, KernelEvents::RESPONSE);
+    $this->assertSame(400, $event->getResponse()->getStatusCode());
   }
 
   /**