Contrib.social

Symfony ControllerArgumentsEvent is not really usable

Open on Drupal.org →
Created on 9 August 2024, 6 months ago

Problem/Motivation

Subscribing to KernelEvents::CONTROLLER_ARGUMENTS results in sadness.

Steps to reproduce

Subscribe. Realize there's nothing to work with because EarlyRenderingControllerWrapperSubscriber has hidden both the controller and the arguments in a closure. It's possible to punch through that with reflection but it's brittle and super ugly.

  $controller = $event->getController();
  $arguments = $event->getArguments();
  if ($controller instanceof \Closure) {
    $rf = new \ReflectionFunction($controller);
    $variables = $rf->getClosureUsedVariables();
    if (isset($variables['controller']) && isset($variables['arguments'])) {
      $controller = $variables['controller'];
      $arguments = $variables['arguments'];
    }
  }

Proposed resolution

Use a wrapper class instead of a closure. This would result no in change in the logic, closure is just syntactic sugar over a class anyways. So the above event subscriber now would look like this:

  $controller = $event->getController();
  $arguments = $event->getArguments();
  if (is_array($controller) && $controller[0] instanceof ControllerWrapper) {
    $controller = $controller[0]->getController();
    $arguments = $controller[0]->getArguments();
  }

Further, the argument resolver could be decorated to do the unwrapping and so now the subscriber is

  $controller = $event->getController();
  if (is_array($controller) && $controller[0] instanceof ControllerWrapper) {
    $controller = $controller[0]->getController();
  }

Remaining tasks

Decide on the namespaces of the classes. I threw them in Routing. That's not where they ought to be.

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Active

Version

11.0 🔥

Component
Base  →

Last updated about 5 hours ago

Created by

🇨🇦Canada Charlie ChX Negyesi 🍁Canada

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @Charlie ChX Negyesi
  • Merge request !9147what about this → (Open) created by nicxvan
  • Pipeline finished with Failed
    6 months ago
    Total: 152s
    #249460
  • Comment 6 months ago →
    🇨🇦Canada Charlie ChX Negyesi 🍁Canada
  • Pipeline finished with Success
    6 months ago
    Total: 3059s
    #249480
  • Status changed to Needs review 5 months ago
  • Comment 5 months ago →
    🇨🇦Canada Charlie ChX Negyesi 🍁Canada
  • Status changed to Needs work 5 months ago
  • Comment 5 months ago →
    🇺🇸United States smustgrave

    Thanks for reporting. Will need test coverage also

  • Status changed to Needs review 5 months ago
  • Comment 5 months ago →
    🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    Needs a review first. I am not wasting my time on tests for features that won't be accepted.

    And for this? Tests are a waste of time anyways, there's nothing you can meaningfully test which doesn't tie the test to the implementation so tightly it's just a rewording of the implementation. But yes, I will write that pointless test to satisfy the pointless checklists.

    But first, it needs a review.

  • Comment 5 months ago →
    🇺🇸United States smustgrave

    Ok good luck with that

  • Status changed to Closed: won't fix 5 months ago
  • Comment 5 months ago →
    🇨🇦Canada Charlie ChX Negyesi 🍁Canada

    OK then.

  • Comment 5 months ago →
    🇺🇸United States smustgrave

    You can keep open I’m just not one to review this change

  • Status changed to Needs review 5 months ago
  • Comment 5 months ago →
    🇳🇿New Zealand quietone

    I think there is a misunderstanding here. This is asking for a review of the idea before doing all related tasks. And we have a tag just for that, Needs architectural review → . I am adding the tag now. I hope that helps get the review.

    And finally, this is not an issue I can review.

  • Comment 3 months ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

  • Status changed to Needs work 3 months ago
  • Comment 3 months ago →
    🇮🇩Indonesia el7cosmos 🇮🇩 GMT+7

    This is a valid, both \Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent::getArguments and \Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent::setArguments won't work.

  • Comment 3 months ago →
    🇮🇩Indonesia el7cosmos 🇮🇩 GMT+7

    I just added a test case to validate the bug

  • Pipeline finished with Failed
    3 months ago
    Total: 1283s
    #313877
  • Pipeline finished with Failed
    3 months ago
    Total: 555s
    #313913
  • Pipeline finished with Failed
    3 months ago
    Total: 140s
    #313927
  • Pipeline finished with Success
    3 months ago
    Total: 664s
    #313932
  • Comment 3 months ago →
    🇺🇸United States nicxvan

    This still needs architectural review.

  • Comment 3 months ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Left some comments, thanks for working on this, it is a nice DX improvement

  • Pipeline finished with Failed
    3 months ago
    Total: 413s
    #319481
  • Pipeline finished with Success
    3 months ago
    Total: 6190s
    #319488
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024