Convert most direct usages within module code of routing related request attributes to use RouteMatchInterface instead

Going to look at this.

I looked at breadcrumbs first, and realized it was a sizable change on its own (and an API change), so I split it out to #2292025: Convert BreadcrumbBuilderInterface applies() and build() to use RouteMatch.

A bunch of remaining usages are because we don't have something like #1837388: Provide a ParamConverter than can upcast any entity..

I also opened a couple child issues.

Here is what I have so far.

"yeah, you need all the semicolons"
- @drumm

Fixed some tests.

Rerolled and added more fixes.

There are a bunch of weird problems with config_translation, I might need to split those out to a new issue. Removing for now.

Discussed with @tim.plunkett and @effulgentsia. We'll add @todos in this patch for the remaining issues Tim hasn't solved yet here (c_t, quickedit, maybe others) and then should be in good shape for beta with this.

With these changes and the child issues, we cover everything except the explicit test coverage for $request->attributes.

Just reviewed a bit. Classical example of doing too much in a match, honestly.

1. +++ b/core/lib/Drupal/Core/Authentication/AuthenticationManager.php
   @@ -178,7 +179,7 @@ public function cleanup(Request $request) {
   -    $route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);
   +    $route = RouteMatch::createFromRequest($request)->getRouteObject();
   

   Can we somehow move the authentication after routing? AccessSubscriber::onKernelRequestAccessCheck happens after routing already

2. +++ b/core/lib/Drupal/Core/Controller/DialogController.php
   @@ -93,7 +94,8 @@ public function dialog(Request $request, $_content, $modal = FALSE) {
   +    $title = isset($page_content['#title']) ? $page_content['#title'] : $this->titleResolver->getTitle($request, $route_match->getRouteObject());
   

   Can we just have two methods on the title resolver, one using the request + route?

3. +++ b/core/lib/Drupal/Core/EventSubscriber/AccessSubscriber.php
   @@ -64,17 +65,16 @@ public function onKernelRequestAccessCheck(GetResponseEvent $event) {
   +    $route_match = RouteMatch::createFromRequest($request);
   ...
   -    if (!$request->attributes->has(RouteObjectInterface::ROUTE_OBJECT)) {
   -      // If no Route is available it is likely a static resource and access is
   -      // handled elsewhere.
   +    if (!$route_object = $route_match->getRouteObject()) {
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
   @@ -121,6 +122,7 @@ public function onKernelRequestDetermineSiteStatus(GetResponseEvent $event) {
      public function onKernelRequestMaintenance(GetResponseEvent $event) {
        $request = $event->getRequest();
   +    $route_match = RouteMatch::createFromRequest($request);
   

   Does that mean that the route match should be maybe part of the response event (maybe as part of the request *hides*)

4. +++ b/core/lib/Drupal/Core/Routing/AdminContext.php
   @@ -62,7 +61,7 @@ public function isAdminRoute(Route $route = NULL) {
      protected function getRouteFromRequest() {
        $request = $this->requestStack->getCurrentRequest();
        if ($request) {
   -      return $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT);
   +      return RouteMatch::createFromRequest($request)->getRouteObject();
        }
      }
   

   Here we should inject the current route match as dependency, shouldn't we?

Can we somehow move the authentication after routing?

Yes we should, but that's not too easy mostly because maintenance mode, see #2286971: Remove dependency of current_user on request and authentication manager and its child-issues.

I will extract a couple of the usages here in #2330363: Enhance the controller resolver to get a route match class

Another related issue #2327387: Add the current route match to FormBase

#2238981: Change controllers to receive route parameters in their signature rather than via $request->attributes is another subissue

So according to #18 this issue should be set to postponed with a [pp-2] prefix?

Still needs an issue summary. I'm not sure how much core has changed since #15, but there might be better ways to do this now.

This is kind of a similar to #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API or at least a child.

A hell lot of the instances got converted in the meantime.

This is a list of things which is not covered yet by this patch:

modules/webprofiler/src/Form/DatabaseFilterForm.php:68:    $profile = $this->getRequest()->attributes->get('profile');
modules/webprofiler/src/DataCollector/RequestDataCollector.php:32:      $_content = $request->attributes->get('_content');
modules/webprofiler/src/DataCollector/RequestDataCollector.php:39:          $request->attributes->get('_form'),
modules/page_manager/src/EventSubscriber/RouteParamContext.php:71:          $context->setContextValue($request->attributes->get($route_context_name));
modules/devel/src/Access/SwitchAccess.php:50:    $name = $request->attributes->get('name');
modules/devel/src/Form/SettingsForm.php:32:    $current_path = $request->attributes->get('_system_path');
core/lib/Drupal/Core/Entity/HtmlEntityFormController.php:46:    return $request->attributes->get('_entity_form');
core/lib/Drupal/Core/Entity/HtmlEntityFormController.php:75:      $entity = $request->attributes->get($entity_type);
core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php:100:    $system_path = $request->attributes->get('_system_path');
core/lib/Drupal/Core/EventSubscriber/DefaultExceptionHtmlSubscriber.php:112:        $sub_request->attributes->set('exception', $request->attributes->get('exception'));
core/lib/Drupal/Core/EventSubscriber/ExceptionLoggingSubscriber.php:50:    $this->logger->get('access denied')->warning(String::checkPlain($request->attributes->get('_system_path')));
core/lib/Drupal/Core/EventSubscriber/ExceptionLoggingSubscriber.php:63:    $this->logger->get('page not found')->warning(String::checkPlain($request->attributes->get('_system_path')));
core/lib/Drupal/Core/EventSubscriber/ContentFormControllerSubscriber.php:68:    if ($form = $request->attributes->get('_form')) {
core/lib/Drupal/Core/EventSubscriber/ContentControllerSubscriber.php:51:    if (!$request->attributes->get('_format')) {
core/lib/Drupal/Core/Routing/RouteMatch.php:84:    if ($request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)) {
core/lib/Drupal/Core/Routing/RouteMatch.php:86:      if ($raw = $request->attributes->get('_raw_variables')) {
core/lib/Drupal/Core/Routing/RouteMatch.php:90:        $request->attributes->get(RouteObjectInterface::ROUTE_NAME),
core/lib/Drupal/Core/Routing/RouteMatch.php:91:        $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT),
core/lib/Drupal/Core/Routing/Enhancer/AuthenticationEnhancer.php:58:    $auth_provider_triggered = $request->attributes->get('_authentication_provider');
core/lib/Drupal/Core/Routing/RouteProvider.php:116:      $path = '/' . $request->attributes->get('_system_path');
core/lib/Drupal/Core/Routing/UrlMatcher.php:34:      $path = '/' . $request->attributes->get('_system_path');
core/lib/Drupal/Core/Form/FormSubmitter.php:142:      $url = $this->urlGenerator->generateFromPath($request->attributes->get('_system_path'), array(
core/lib/Drupal/Core/Controller/TitleResolver.php:60:      if (($raw_parameters = $request->attributes->get('_raw_variables'))) {
core/lib/Drupal/Core/Controller/ControllerResolver.php:92:    if (!$controller = $request->attributes->get('_controller')) {
core/lib/Drupal/Core/Controller/ControllerResolver.php:175:    if ($request->attributes->has('_raw_variables') && $raw = $request->attributes->get('_raw_variables')->all()) {
core/lib/Drupal/Core/Controller/HtmlFormController.php:45:    return $request->attributes->get('_form');
core/lib/Drupal/Core/Controller/ControllerResolverInterface.php:24:   *   The controller attribute like in $request->attributes->get('_controller')
core/modules/content_translation/content_translation.module:505:    \Drupal::service('content_translation.synchronizer')->synchronizeFields($entity, $entity->language()->getId(), $attributes->get('source_langcode'));
core/modules/views/src/Plugin/views/argument_default/Raw.php:96:    $path = $this->view->getRequest()->attributes->get('_system_path');
core/modules/system/src/Tests/HttpKernel/StackKernelIntegrationTest.php:48:    $this->assertEqual($request->attributes->get('_hello'), 'world');
core/modules/system/src/Tests/HttpKernel/StackKernelIntegrationTest.php:49:    $this->assertEqual($request->attributes->get('_previous_optional_argument'), 'test_argument');
core/modules/system/src/Plugin/Condition/RequestPath.php:146:    $path = $request->attributes->get('_system_path');
core/modules/system/tests/modules/httpkernel_test/src/HttpKernel/TestMiddleware.php:51:     $request->attributes->set('_previous_optional_argument', $request->attributes->get('_optional_argument'));
core/modules/system/tests/modules/theme_test/src/EventSubscriber/ThemeTestSubscriber.php:35:    $current_path = $request->attributes->get('_system_path');
core/modules/system/tests/modules/theme_test/src/EventSubscriber/ThemeTestSubscriber.php:60:    $current_path = $request->attributes->get('_system_path');
core/modules/rest/src/Access/CSRFAccessCheck.php:57:    $cookie = $request->attributes->get('_authentication_provider') == 'cookie';
core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/RequestMatcher.php:161:            if (!preg_match('{'.$pattern.'}', $request->attributes->get($key))) {
core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Request.php:733:        if ($this !== $result = $this->attributes->get($key, $this, $deep)) {
core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Tests/RequestTest.php:42:        $this->assertEquals('bar', $request->attributes->get('foo'), '->initialize() takes an array of attributes as its third argument');
core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Tests/RequestTest.php:289:        $this->assertEquals('json', $dup->attributes->get('_format'));
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/EventListener/LocaleListener.php:91:        if ($locale = $request->attributes->get('_locale')) {
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/EventListener/FragmentListener.php:70:        $request->attributes->set('_route_params', array_replace($request->attributes->get('_route_params', array()), $attributes));
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Tests/EventListener/FragmentListenerTest.php:73:        $this->assertEquals(array('foo' => 'bar', '_controller' => 'foo'), $request->attributes->get('_route_params'));
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Tests/DataCollector/RequestDataCollectorTest.php:45:        $this->assertRegExp('/Resource\(stream#\d+\)/', $attributes->get('resource'));
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Tests/DataCollector/RequestDataCollectorTest.php:46:        $this->assertSame('Object(stdClass)', $attributes->get('object'));
core/vendor/symfony/http-kernel/Symfony/Component/HttpKernel/Controller/ControllerResolver.php:52:        if (!$controller = $request->attributes->get('_controller')) {

Alright.

Let's see, how many failures are left.

Fixed some of the tests.

.

ups, my fail.

1. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -92,6 +93,7 @@ public function getRouteParameters(Request $request) {
   +    $route_match = RouteMatch::createFromRequest($request);
   

   @effulgentsia says in #16 that RouteMatch::createFromRequest() is not a good pattern to use, so I wonder if we should inject the $route_match service instead?

2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
   @@ -341,10 +342,11 @@ protected function isRouteActive($current_route_name, $route_name, $route_parame
   +    $route_match = RouteMatch::createFromRequest($request);
   

   Same here.

3. +++ b/core/modules/block_content/src/Plugin/Menu/LocalAction/BlockContentAddLocalAction.php
   @@ -21,16 +22,19 @@ class BlockContentAddLocalAction extends LocalActionDefault {
   +    $route_match = RouteMatch::createFromRequest($request);
   

   This wouldn't be needed if we do 1., because we'd have the route match available from the parent class.

4. +++ b/core/modules/config_translation/src/ConfigEntityMapper.php
   
   +++ b/core/modules/config_translation/src/ConfigMapperInterface.php
   
   +++ b/core/modules/config_translation/src/ConfigNamesMapper.php
   
   +++ b/core/modules/config_translation/tests/src/Unit/ConfigNamesMapperTest.php
   

   Maybe it would be easier if we leave all the Config*Mapper changes to #2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch?

5. +++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
   @@ -21,15 +22,11 @@ class MenuLinkAdd extends LocalActionDefault {
   +    $route_match = RouteMatch::createFromRequest($request);
   

   Same as 1. and 3.

Also related to the points above, the change notice for RouteMatch (https://www.drupal.org/node/2295317) lists If you have a request available, you can just RouteMatch::createFromRequest($request). as the first way of using it. Based on #16, should we update the change notice to say that we need to use RouteMatch::createFromRequest($request) only when we have a $request and no way of injecting the route match service? I'd be really curious to see an example of that.

Okay, let's first figure out #2294157: Switch getOptions() and getRouteParameters() within LocalActionInterface and LocalTaskInterface to use RouteMatch as it deals with the fact whether / how to use the route match.

Update this issue to be postponed

Alright, a recent reroll.

Worked on a proper issue summary

+++ b/core/modules/field_ui/src/DisplayOverviewBase.php
@@ -190,7 +190,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $entity_t
-      $bundle = $bundle ?: $this->getRequest()->attributes->get('_raw_variables')->get($this->bundleEntityTypeId);
+      $bundle = $bundle ?: $this->getRouteMatch()->getRawParameter($this->bundleEntityTypeId);

I'm also removing this one in #2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects :)

Nice, so RTBC ? ;)

I was more like hoping you would prefer to keep that hunk out of the patch, but apparently not :P

It looks like #36.4 is still not done. And here's another in-depth review:

1. +++ b/core/lib/Drupal/Core/Routing/AdminContext.php
   @@ -24,13 +24,20 @@ class AdminContext {
      protected $requestStack;
   

   Looks like we can remove this member now.

2. +++ b/core/modules/content_translation/src/Controller/ContentTranslationController.php
   @@ -10,6 +10,7 @@
    use Symfony\Component\HttpFoundation\Request;
   

   Pretty sure this is unused now :)

3. +++ b/core/modules/field_ui/src/Controller/FieldConfigListController.php
   @@ -22,18 +23,17 @@ class FieldConfigListController extends EntityListController {
   +   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   +   *   The current route match.
   +   * @return array A render array as expected by drupal_render().
   +   * A render array as expected by drupal_render().
   

   Something went wrong here...

4. +++ b/core/modules/rest/src/RequestHandler.php
   @@ -31,11 +31,13 @@ class RequestHandler implements ContainerAwareInterface {
   +   * @return \Symfony\Component\HttpFoundation\Response The response object.
   +   * The response object.
   

   Same here.

5. +++ b/core/modules/system/tests/modules/entity_test/src/Controller/EntityTestController.php
   @@ -10,6 +10,8 @@
   +use Drupal\Core\Routing\RouteMatch;
   

   This one doesn't seem to be needed.

6. +++ b/core/modules/system/tests/modules/form_test/src/FormTestControllerObject.php
   @@ -10,6 +10,7 @@
   +use Symfony\Component\HttpFoundation\Request;
   
   @@ -34,11 +35,11 @@ public static function create(ContainerInterface $container) {
   -  public function buildForm(array $form, FormStateInterface $form_state, $custom_attributes = NULL) {
   +  public function buildForm(array $form, FormStateInterface $form_state, Request $request = NULL, $custom_attributes = NULL) {
   ...
   -    $form['request_attribute']['#markup'] = $this->getRequest()->attributes->get('request_attribute');
   +    $form['request_attribute']['#markup'] = $request->attributes->get('request_attribute');
   

   I realize this is test code but it tests exactly the thing we're trying to move away from. Is it worth keeping it?

7. +++ b/core/modules/system/tests/modules/paramconverter_test/src/TestControllers.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Routing\RouteMatch;
   

   Not used?

8. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
   @@ -30,7 +30,7 @@ class UserLoginBlock extends BlockBase {
   -    $route_name = \Drupal::request()->attributes->get(RouteObjectInterface::ROUTE_NAME);
   +    $route_name = \Drupal::routeMatch()->getRouteName();
   

   In all the other places you injected the current_route_match service, any reason for not doing it here too?

9. +++ b/core/modules/user/src/Plugin/LanguageNegotiation/LanguageNegotiationUserAdmin.php
   @@ -10,6 +10,7 @@
   +use Drupal\Core\Routing\RouteMatch;
   
   @@ -117,7 +118,8 @@ protected function isAdminPath(Request $request) {
   -      if (!$route_object = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)) {
   +      $route_match = RouteMatch::createFromRequest($request);
   +      if (!$route_object = $route_match->getRouteObject()) {
   

   This plugin already extends ContainerFactoryPluginInterface, why not inject the service instead?

10. +++ b/core/tests/Drupal/Tests/Core/Menu/LocalTaskDefaultTest.php
    @@ -11,6 +11,7 @@
    +use Symfony\Component\DependencyInjection\ParameterBag\ParameterBag;
    

    Unrelated I think.

Thank you for your review!!!

I'm also removing this one in #2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects :)

Alright, let's get rid of that hunk

Looks like we can remove this member now.

Good catch!

Pretty sure this is unused now :)

Fair

I realize this is test code but it tests exactly the thing we're trying to move away from. Is it worth keeping it?

In terms of test coverage it makes sense to convert to the more often used API.

In all the other places you injected the current_route_match service, any reason for not doing it here too?

Fixed that.

This plugin already extends ContainerFactoryPluginInterface, why not inject the service instead?

Well, the problem is that you need the route match for the passed in request. Expanded the stacked route match interface to allow that.

Unrelated I think.

Yup

Maybe it would be easier if we leave all the Config*Mapper changes to #2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch?

Given that the other issue doesn't have any proper patch yet,

Maybe it would be easier if we leave all the Config*Mapper changes to #2295255: Convert ConfigMapperInterface::populateFromRequest() to use RouteMatch?

Okay, then let's do that.

Okay, breaking the login form is a bad idea :)

Okay, breaking the login form is a bad idea :)

Yes, don't do that! :)

The last remaining test fail is quite interesting and I think it shows a flaw in the way (or timing) RequestMatch objects are created, because they don't have custom request attributes added by an event. Now I'm glad I brought up #43.6.

Well, to be clear here, the route match just contains parameters which are part of the URL, not arbitrary request attributes.

In this test we do update arbitrary request attributes, so well let's keep it, the test ensure that you can inject the request object.

Right, that explanation makes sense. I found a few more really small nitpicks which are not worth blocking a critical and can be fixed on commit if you don't want to roll a new patch.

1. +++ b/core/lib/Drupal/Core/Routing/StackedRouteMatchInterface.php
   @@ -6,6 +6,7 @@
    namespace Drupal\Core\Routing;
   +use Symfony\Component\HttpFoundation\Request;
   

   Missing newline.

2. +++ b/core/lib/Drupal/Core/Routing/StackedRouteMatchInterface.php
   @@ -36,5 +37,14 @@ public function getMasterRouteMatch();
   +   * @param \Symfony\Component\HttpFoundation\Request
   +   *   The request.
   +   * @return \Drupal\Core\Routing\RouteMatchInterface|NULL
   +   *   THe matching route match, or NULL if there is no matching one.
   

   Same here.

3. +++ b/core/modules/user/src/Plugin/Block/UserLoginBlock.php
   @@ -22,15 +24,68 @@
   +  /**
   +   * Creates an instance of the plugin.
   +   *
   +   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
   +   *   The container to pull out services used in the plugin.
   +   * @param array $configuration
   +   *   A configuration array containing information about the plugin instance.
   +   * @param string $plugin_id
   +   *   The plugin ID for the plugin instance.
   +   * @param mixed $plugin_definition
   +   *   The plugin implementation definition.
   +   *
   +   * @return static
   +   *   Returns an instance of this plugin.
   +   */
   +  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
   

   Can be just {@inheritdoc}.

Thank you for the RTBC, but there is no reason to not fix these nitpicks.

Missing newline.

Don't blame the patch, blame storm :)

1. +++ b/core/core.services.yml
   index d5193e7..febffd9 100644
   --- a/core/lib/Drupal/Core/Controller/ControllerResolver.php
   
   --- a/core/lib/Drupal/Core/Controller/ControllerResolver.php
   +++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
   

   Talking @dawehener in IRC he says the changes to this are a bug - do we need to add tests for this?

2. +++ b/core/lib/Drupal/Core/Controller/ControllerResolver.php
   @@ -166,21 +170,6 @@ protected function doGetArguments(Request $request, $controller, array $paramete
   -    // The parameter converter overrides the raw request attributes with the
   -    // upcasted objects. However, it keeps a backup copy of the original, raw
   -    // values in a special request attribute ('_raw_variables'). If a controller
   -    // argument has a type hint, we pass it the upcasted object, otherwise we
   -    // pass it the original, raw value.
   

   Should we not keep the comment?

Talking @dawehener in IRC he says the changes to this are a bug - do we need to add tests for this?

Note: ControllerResolverTest is changed as part of this patch.

Should we not keep the comment?

It is not valid anymore ...

Thanks @dawehner.

Committed 1e08b50 and pushed to 8.0.x. Thanks!

Thank you a ton alex!

A bit sad to see that the fancy new "Credit & committing" stuff is still not used at its full potential (e.g. for reviewers)..

@amateescu, one issue is that the form is pure JS (it looks like you can save a list of who to credit, but actually nothing is saved). I think there's an issue to improve this but I wasn't able to locate it.

#2830631: Remove code that tries to use _raw_variables for route argument resolution as it does not work is saying the changes to ControllerResolver in terms of not using the raw parameters constitutes a bug.