Skip to content
Snippets Groups Projects

what about this

what about this
issue/drupal-3467228:3467228-symfony-controllerargumentsevent-is into 11.x
4 unresolved threads
Open nicxvan requested to merge issue/drupal-3467228:3467228-symfony-controllerargumentsevent-is into 11.x
4 unresolved threads

Closes #3467228

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
core/lib/Drupal/Core/Routing/ControllerWrapper.php 0 → 100644
32 * The return value of the controller.
33 *
34 * @throws \LogicException
35 * When early rendering has occurred in a controller that returned a
36 * Response or domain object that cares about attachments or cacheability.
37 *
38 * @see \Symfony\Component\HttpKernel\HttpKernel::handleRaw()
39 */
40 public function wrapControllerExecutionInRenderContext(...$arguments) {
41 $context = new RenderContext();
42
43 $controller = $this->controller;
44 $response = $this->renderer->executeInRenderContext($context, function () use ($controller, $arguments) {
45 // Now call the actual controller, just like HttpKernel does.
46 return call_user_func_array($controller, $arguments);
47 });
  • Comment on lines +44 to +47
    Lee Rowlands
    Lee Rowlands @larowlan ·
    Maintainer

    Do we want to clean this up a bit while we're here?

    Suggested change
    Applied
    43 $response = $this->renderer->executeInRenderContext($context, function () use ($controller, $arguments) {
    44 // Now call the actual controller, just like HttpKernel does.
    45 return call_user_func_array($controller, $arguments);
    46 });
    43 // Now call the actual controller, just like HttpKernel does.
    44 $response = $this->renderer->executeInRenderContext($context, static fn () => call_user_func_array($controller, $arguments));
    45 });
  • Luhur Abdi Rizal changed this line in version 8 of the diff

    changed this line in version 8 of the diff

  • Please register or sign in to reply
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on the diff
    • core/lib/Drupal/Core/Routing/ControllerWrapper.php 0 → 100644
    75 // If a non-Ajax Response or domain object is returned and it cares about
    76 // attachments or cacheability, then throw an exception: early rendering
    77 // is not permitted in that case. It is the developer's responsibility
    78 // to not use early rendering.
    79 elseif ($response instanceof AttachmentsInterface || $response instanceof CacheableDependencyInterface) {
    80 throw new \LogicException(sprintf('The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Ensure you are not rendering content too early. Returned object class: %s.', get_class($response)));
    81 }
    82 else {
    83 // A Response or domain object is returned that does not care about
    84 // attachments nor cacheability; for instance, a RedirectResponse. It is
    85 // safe to discard any early rendering metadata.
    86 }
    87 }
    88
    89 return $response;
    90 }
    • Comment on lines +49 to +90
      Lee Rowlands
      Lee Rowlands @larowlan ·
      Maintainer

      Oof, nested elseif/if - do we want to leave things in a better place while we're here?

      Suggested change
      47 // If early rendering happened, i.e. if code in the controller called
      48 // RendererInterface::render() outside of a render context, then the
      49 // bubbleable metadata for that is stored in the current render context.
      50 if (!$context->isEmpty()) {
      51 /** @var \Drupal\Core\Render\BubbleableMetadata $early_rendering_bubbleable_metadata */
      52 $early_rendering_bubbleable_metadata = $context->pop();
      53
      54 // If a render array or AjaxResponse is returned by the controller, merge
      55 // the "lost" bubbleable metadata.
      56 if (is_array($response)) {
      57 BubbleableMetadata::createFromRenderArray($response)
      58 ->merge($early_rendering_bubbleable_metadata)
      59 ->applyTo($response);
      60 }
      61 elseif ($response instanceof AjaxResponse) {
      62 $response->addAttachments($early_rendering_bubbleable_metadata->getAttachments());
      63 // @todo Make AjaxResponse cacheable in
      64 // https://www.drupal.org/node/956186. Meanwhile, allow contrib
      65 // subclasses to be.
      66 if ($response instanceof CacheableResponseInterface) {
      67 $response->addCacheableDependency($early_rendering_bubbleable_metadata);
      68 }
      69 }
      70 elseif ($response instanceof CacheableResponseInterface) {
      71 $response->addCacheableDependency($early_rendering_bubbleable_metadata);
      72 }
      73 // If a non-Ajax Response or domain object is returned and it cares about
      74 // attachments or cacheability, then throw an exception: early rendering
      75 // is not permitted in that case. It is the developer's responsibility
      76 // to not use early rendering.
      77 elseif ($response instanceof AttachmentsInterface || $response instanceof CacheableDependencyInterface) {
      78 throw new \LogicException(sprintf('The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Ensure you are not rendering content too early. Returned object class: %s.', get_class($response)));
      79 }
      80 else {
      81 // A Response or domain object is returned that does not care about
      82 // attachments nor cacheability; for instance, a RedirectResponse. It is
      83 // safe to discard any early rendering metadata.
      84 }
      85 }
      86
      87 return $response;
      88 }
      47 return $this->applyRenderContextToResponse($response, $context);
      48 }
      49
      50 /**
      51 * Applies render context to response.
      52 *
      53 * @param \Symfony\Component\HttpFoundation\Response $response
      54 * Response.
      55 * @param \Drupal\Core\Render\RenderContext $context
      56 * Render context.
      57 *
      58 * @return \Symfony\Component\HttpFoundation\Response
      59 * Response with applied cacheability.
      60 */
      61 protected function applyRenderContextToResponse(Response $response, RenderContext $context): Response {
      62 // If early rendering happened, i.e. if code in the controller called
      63 // RendererInterface::render() outside of a render context, then the
      64 // bubbleable metadata for that is stored in the current render context.
      65 if ($context->isEmpty()) {
      66 return $response;
      67 }
      68 /** @var \Drupal\Core\Render\BubbleableMetadata $early_rendering_bubbleable_metadata */
      69 $early_rendering_bubbleable_metadata = $context->pop();
      70
      71 // If a render array or AjaxResponse is returned by the controller, merge
      72 // the "lost" bubbleable metadata.
      73 if (is_array($response)) {
      74 BubbleableMetadata::createFromRenderArray($response)
      75 ->merge($early_rendering_bubbleable_metadata)
      76 ->applyTo($response);
      77 return $response;
      78 }
      79
      80 if ($response instanceof AjaxResponse) {
      81 $response->addAttachments($early_rendering_bubbleable_metadata->getAttachments());
      82 // @todo Make AjaxResponse cacheable in
      83 // https://www.drupal.org/node/956186. Meanwhile, allow contrib
      84 // subclasses to be.
      85 if (!$response instanceof CacheableResponseInterface) {
      86 return $response;
      87 }
      88 }
      89
      90 if ($response instanceof CacheableResponseInterface) {
      91 $response->addCacheableDependency($early_rendering_bubbleable_metadata);
      92 }
      93
      94 // If a non-Ajax Response or domain object is returned and it cares about
      95 // attachments or cacheability, then throw an exception: early rendering
      96 // is not permitted in that case. It is the developer's responsibility
      97 // to not use early rendering.
      98 if ($response instanceof AttachmentsInterface || $response instanceof CacheableDependencyInterface) {
      99 throw new \LogicException(sprintf('The controller result claims to be providing relevant cache metadata, but leaked metadata was detected. Ensure you are not rendering content too early. Returned object class: %s.', get_class($response)));
      100 }
      101
      102 // A Response or domain object is returned that does not care about
      103 // attachments nor cacheability; for instance, a RedirectResponse. It is
      104 // safe to discard any early rendering metadata.
      105 return $response;
      106 }
    • Please register or sign in to reply
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
    • core/modules/system/tests/modules/early_rendering_controller_test/src/EarlyRenderingControllerArgumentSubscriber.php 0 → 100644
    1 <?php
    2
    3 declare(strict_types=1);
    4
    5 namespace Drupal\early_rendering_controller_test;
    6
    7 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
    8 use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent;
    9 use Symfony\Component\HttpKernel\KernelEvents;
    10
    11 class EarlyRenderingControllerArgumentSubscriber implements EventSubscriberInterface {
    12
    13 public function onKernelController(ControllerArgumentsEvent $event): void {
    14 $arguments = $event->getArguments();
    15 if (!count($arguments)) {
  • Lee Rowlands
    Lee Rowlands @larowlan started a thread on an old version of the diff
    • core/modules/system/tests/modules/early_rendering_controller_test/early_rendering_controller_test.services.yml
    3 3 autoconfigure: true
    4 4 test_domain_object.view_subscriber:
    5 5 class: Drupal\early_rendering_controller_test\TestDomainObjectViewSubscriber
    6 early_rendering_controller_argument_subscriber:
    7 class: Drupal\early_rendering_controller_test\EarlyRenderingControllerArgumentSubscriber
  • Lee Rowlands
    Lee Rowlands @larowlan ·
    Maintainer

    This feels like a nice DX cleanup. I think we can make some of the moved code much simpler to grok as well, elseif/nested if etc - the original code is pretty difficult to follow.

  • Luhur Abdi Rizal added 1 commit

    added 1 commit

    • cf6dc554 - Apply 3 suggestion(s) to 3 file(s)

    Compare with previous version

  • Luhur Abdi Rizal added 1 commit

    added 1 commit

    Compare with previous version

    • Please register or sign in to reply