Follow-up to #3277784, related to #3352384

Problem/Motivation

#3277784 introduced this with commit a894a04b:

    // Route defaults that do not start with a leading "_" are also
    // parameters, even if they are not included in path or host patterns.
    foreach ($route->getDefaults() as $name => $value) {
      if (!isset($raw_variables[$name]) && substr($name, 0, 1) !== '_') {
        $raw_variables[$name] = $value;
      }
    }

Route parameters, however, might end up in TitleResolver, which is pulling all route parameters considering them safe as translation arguments:

if (($raw_parameters = $request->attributes->get('_raw_variables'))) {
  foreach ($raw_parameters->all() as $key => $value) {
    $args['@' . $key] = $value ?? '';
    $args['%' . $key] = $value ?? '';
  }
}

and

$route_title = $this->t($title, $args, $options);

So route defaults are now required to be scalars, as otherwise t() fails with a TypeError.
However (@larowlan):

I think default arguments can totally be arrays and as such that automagic '@arg' '%arg' should be checking if $value is a scalar - i.e. that sounds like a bug in TitleResolver in my book

This hit e.g. the CiviCRM module which defined a route default args being an array.

Steps to reproduce

You can reproduce the issue with Drupal 9.5.9 by declaring a new Route object with an array in its default argument, e.g.

in a module's modulename.routing.yml:

route_callbacks:
  - '\Drupal\modulename\Routing\Routes::getRoutes'

in the module's src/Routing/Routes.php:

class Routes {
  public function getRoutes() {
    $route_collection = new \Symfony\Component\Routing\RouteCollection();
    return $route_collection->add('/my-path', [
      '_title' => 'Page Title',
      '_controller' => 'Drupal\modulename\Controller\ModuleController:run',
      'my_default_arg' => ['some' => 'array'],
    ]);
    return $route_collection;
  }
}

Proposed resolution

TitleResolver should check for route parameters being a scalar before copying them as translation arguments:

if (($raw_parameters = $request->attributes->get('_raw_variables'))) {
  foreach ($raw_parameters->all() as $key => $value) {
    if (is_scalar($value) {
      $args['@' . $key] = $value ?? '';
      $args['%' . $key] = $value ?? '';
    }
  }
}

or variables cast-able to string (while that might not make sense for all types).

Issue fork drupal-3358402

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jensschuppe created an issue. See original summary.

totten’s picture

totten commented

Yeah, that `Route` metadata seems to go through several copy/filter steps, and I don't know the history on all of them, but you only need one of them to be more selective.

TitleResolver should check for route parameters being a scalar before copying them as translation arguments:

+1 - Sounds sensible to me. That seems like the place where you go from a broader domain (all possible arguments to a page-controller) to a narrower domain (values that can be interpolated into a string).

lisotton made their first commit to this issue’s fork.

lisotton opened merge request !3953

lisotton’s picture

lisotton commented

I created a MR applying what was suggested by @jensschuppe.

The problem I see is that using the function "is_scalar", it will not enter even when the value is null and before if the value was null, it was using an empty string.

What do you think is the best option?
- Not set the value if is null or array
- Leave the value as empty string if is null and not set the value if is array
- Set the value as empty string if both null or array

socialnicheguru’s picture

socialnicheguru commented
Status: Active » Needs review
jensschuppe’s picture

jensschuppe commented

Thanks @lisotton for your MR! I'd opt for keeping current behavior when the value is null, i. e. set it to the empty string. I'm not sure about non-scalar values though. If the empty string was meant to be a fallback, I'd say it should be the default value for every non-scalar value. If it was meant to be the string representation of null, non-scalar values should have their own defaults or be ignored. But maybe this should be answered by someone more familiar. Historically, only scalar values or null would have been "allowed" anyway ...

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented

I'm still trying to track down how it is related but I also have a regression related to #3277784: copyRawVariables should support default route parameters -- in my case I've got a view with a path and menu entry and the rendered menu item no longer sets in_active_trail to TRUE when accessing the view on a page with the menu.

Reverting #3277784: copyRawVariables should support default route parameters resolves the issue.

Will try to see if I can create clean reproduction steps as well...

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented

I am able to reproduce the issue from #8 with the following steps on a fresh install of Drupal 9.5.x (using drush si in my case):

  1. Log in as admin.
  2. Navigate to /admin/structure/views/add.
  3. Add a view with options:
    • View name: Site content
    • Create a page: checked
    • Create a menu link: checked
    • Menu: Main navigation
  4. Navigate to /site-content.
  5. Inspect "Site content" main navigation item with browser tools and confirm class primary-nav__menu-link--active-trail is not present on a element.
  6. Revert changes from #3277784: copyRawVariables should support default route parameters (or just comment out core/lib/Drupal/Core/Routing/Enhancer/ParamConversionEnhancer.php#L71-75).
  7. Rebuild caches.
  8. Navigate to /site-content.
  9. Inspect "Site content" main navigation item with browser tools and confirm class primary-nav__menu-link--active-trail is present on a element.

Still not sure if this belongs here or in a separate issue. Happy to create a new issue if it anyone thinks it should be separate.

znerol’s picture

znerol commented

In my case I've got a view with a path and menu entry and the rendered menu item no longer sets in_active_trail to TRUE when accessing the view on a page with the menu.

Observed exactly the same issue today with a custom menu/route after rolling out a recent build. Glad that you found a repro relying on core only. I ended up reverting #3277784: copyRawVariables should support default route parameters to work around the issue for the moment.

znerol’s picture

znerol commented

I think that $this->routeMatech->getRawParameters()->all() returns a different set of key-value pairs which in turn leads to an empty result from $this->menuLinkManager->loadLinksByroute() in MenuActiveTrail::getActiveLink():

  public function getActiveLink($menu_name = NULL) {
    [...]
      $route_parameters = $this->routeMatch->getRawParameters()->all();

      // Load links matching this route.
      $links = $this->menuLinkManager->loadLinksByRoute($route_name, $route_parameters, $menu_name);
      // Select the first matching link.
      if ($links) {
        $found = reset($links);
      }
    [...]
  }
lisotton’s picture

lisotton commented

I had similar issues in many places where I manipulate retrieve the menu tree.

I have some routes that share the same Controller and Action, but in the route declaration it declares default parameters. For example I have the route mymodule.news_list (/news) which have the controller ListPageController::entryPoint with a default parameter called type with value news-list. Then I have another route called mymodule.articles_list (/articles) which shares the same controller, but the default parameter type with value articles-list.

Before when I was calling \Drupal::service('plugin.manager.menu.link')->loadLinksByRoute('news_list'), without any second second parameter (route params), it was returning me the menu links, but with the change in 9.5.9, I'm enforced to call \Drupal::service('plugin.manager.menu.link')->loadLinksByRoute('news_list', ['type' => 'news-list').

For me this broke many small things, like breadcrumbs, custom menu blocks, etc.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

It sounds like there are two separate issues here.

The first one (the warning from title controller) is covered by this patch.

Can we get a separate issue to address the menu active trail issue?

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented
Related issues: +#3359511: [regression] missing menu active trail since Drupal 9.5.9, +#3277784: copyRawVariables should support default route parameters

Make sense. Thanks for chiming it, @larowlan. I have moved the active menu trail issue to #3359511: [regression] missing menu active trail since Drupal 9.5.9.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Thanks, watching both actively to try and get these in asap

quietone’s picture

quietone commented
Title: Possible regression with Drupal 9.5.9 - route defaults are now automatically route parameters » [regression] route defaults are now automatically route parameters

Changing title per Special titles.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe the issue on this particular issue has been addressed and issue summary matches what is in the MR.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work

Can we get a version that applies cleanly to 11.x and 10.1.x too

thanks!

totten opened merge request !4129

totten’s picture

totten commented

Added a forward-port (from !3953 on 9.5.x to !4129 on 10.0.x). The merge-conflict was fairly small. The new test passed locally, and I confirmed that one specific error scenario (re: civicrm-core@5.60) worked with this fix.

Aside: This is the first time I've posted a patch to drupal core, so I'm not too familiar with the workflows. But I can say that the merge-conflict should be resolved -- you can cherry-pick the 10.0.x patch on 10.1.x or 11.x.

totten opened merge request !4145

totten opened merge request !4146

totten’s picture

totten commented
Status: Needs work » Reviewed & tested by the community

Added forward ports for 10.1.x and 11.x. Tests still passing.

  • catch committed 9f71d94a on 10.0.x 
    Issue #3358402 by totten, lisotton, jensschuppe, larowlan, znerol: [...

  • catch committed 05dfdaa2 on 10.1.x 
    Issue #3358402 by totten, lisotton, jensschuppe, larowlan, znerol: [...

  • catch committed 75d3d2d1 on 11.x 
    Issue #3358402 by totten, lisotton, jensschuppe, larowlan, znerol: [...

  • catch committed 1cb7e24e on 9.5.x 
    Issue #3358402 by totten, lisotton, jensschuppe, larowlan, znerol: [...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, cherry-picked to 10.1.x and 10.0.x, committed the 9.5.x MR to 9.5.x, thanks!

Status: Fixed » Closed (fixed)

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