🇩🇪Germany @jensschuppe

I added some more adjustments which seems to cover all places that display values of the CMRF Reference field, including forms (also multi-value fields on multi-step webforms), editing forms of submissions, display of submissions in both, table and single entity view.

There might be a configuration option needed for whether to actually query every field value when displaying a lot of them, as this causes API requests for each value.

@jaapjansma, @kainuk looking forward to your comments.

jensschuppe → changed the visibility of the branch multipleValues to hidden.

jensschuppe → changed the visibility of the branch submissionDisplayValues to hidden.

jensschuppe → created an issue.

jaapjansma → credited jensschuppe → .

The Issue fork branch in #3344945 already contains all those changes, see the branch diff. Please mark the other issue as RTBC so that the MR can be merged by maintainers.

Fixed another Drupal 10 incompatibility in the issue fork branch (Denote Drupal 10 compatibility for sub-theme template).

@AdlerM There's an issue fork with a Drupal-10-compatible version in #3344945 which includes these automated compatibility fixes and adjustments for deprecated base themes.

Fixed another Drupal 10 incompatibility with Issue #3344945 Replace removed function `file_create_url()` in the issue fork branch.

Fixed another Drupal 10 incompatibility with Issue #3344945 Drupal 10: Fix Twig syntax error Unknown "spaceless" tag in the issue fork branch.

Webform using Drupal\datetime\Plugin\Field\FieldType\DateTimeItemInterface without checking for the DateTime module being installed is truly an undocumented module dependency, which does not seem necessary regarding the patch. The patch fixes that problem.

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 ...

#3352384 seems to cover another issue, see #3358402 🐛 [regression] route defaults are now automatically route parameters Fixed instead.

Realised this might be a different bug, so created a new issue for that: #3358402 🐛 [regression] route defaults are now automatically route parameters Fixed .

jensschuppe → created an issue.

The patch in #3 is not really handling the root of the problem, as things ending up in htmlspecialchars() might not be cast-able to a string, because route defaults are not restricted to any set of data types.

The bug seems to be in the TitleResolver class, which just copies all route parameters, and those include all route defaults whose key does not start with an underscore _ since Drupal 9.5.9. At least there's a mismatch of assumptions now that route default arguments automatically become route parameters and route parameters have been considered safe as title translation arguments.

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;
  }
}

TitleResolver will then do this (for routes without a _title_callback and with a _title):

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

and consequently:

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

And this is the very place the ['some' => 'array'] will eventually be passed into htmlspecialchars() which expects a string, but any type might get passed in, potentially not cast-able to string.

This was not the case prior to Drupal 9.5.9, as defaults from the route with a key not known to Drupal have not been copied as route parameters and would thus not have been inside $request->attributes->get('_raw_variables').

The commit introducing that new behavior is a894a04b.

I would say this is at least Major, as this might be considered a regression, and affected modules end up with no workaround. Also quoting @larowlan in #3277784 🐛 copyRawVariables should support default route parameters Fixed :

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

@larowlan There's a new issue for the problem: #3352384 ✨ Add Exception for TypeError Argument must be String in \Drupal\Component\Utility\Html escape{} Needs work

I also think that the bug is in TitleResolver, at least in conjunction with the new behavior of Drupal 9.5.9 that all route defaults whose keys do not start with an underscore _ are now automatically considered route parameters and thus get copied as arguments for title translation.

Sorry for posting in a closed issue, just want to be sure this didn't introduce a regression in some form ...

The CiviCRM module has been doing this in a route_callbacks callback:

$route = new Route(
  '/' . $path . '/{extra}',
  [
    '_title' => isset($item['title']) ? $item['title'] : 'CiviCRM',
    '_controller' => 'Drupal\civicrm\Controller\CivicrmController::main',
    'args' => explode('/', $path),
    'extra' => '',
  ],
  [
    '_access' => 'TRUE',
    'extra' => '.+',
  ]
);

Notice the args key within the second argument ($defaults) passed to the Route constructor - all keys not starting with a _ inside that parameter are now (due to this change) eventually being treated as translation parameters (in Drupal/Core/Controller/TitleResolver.php:67), causing an array being considered a string and eventually resulting in a TypeError: htmlspecialchars(): Argument #1 ($string) must be of type string, array given in htmlspecialchars() (Line 432 in /path/to/drupal/web/core/lib/Drupal/Component/Utility/Html.php).

Is this args key being an array just wrong here or did this issue introduce a regression? Is there documentation on what is allowed to be passed into a Route's defaults?

The PR got merged. Thanks @jitendrapurohit for reviewing!

jensschuppe → created an issue.

The issue fork now contains three commits on its 8.x-2.x branch:

• Issue #3296630 Automated Drupal 10 compatibility fixes
• Issue #3344945 Depend on the Classy contrib theme
• Issue #3344945 Denote Drupal 10 compatibility

I.e. including the automated patch created by Drupal rector in #3296630, depending on the now-contrib Classy theme (keeping current base theme dependency) and adding ^10 to core_version_requirement.

This should make the theme installable with Drupal 10, we were at least able to do this successfully.

The Classy contrib theme does not depend on the Stable contrib theme so my comment above can be ignored.

If it's not desirable to introduce a dependency to a contrib theme, there's some more work to do, as described in the change records.

See Pull Request on GitHub. There's an issue fork in the Drupal repository as well for consistency reasons in our composer.json files.

jensschuppe → created an issue.

jensschuppe → created an issue.

There might be extra work necessary if the dependency to the Stable base theme is to be removed as well, since Stable got deprecated in 9.5.0, too, and Classy is based on Stable. See the Change Record → for removing Stable from Core.

jensschuppe → created an issue.

I added some mitigation for not breaking submissions for select type elements, as the default format in the configuration form would be Default (value), which maps to the option label instead of its value. So for those elements, there should now be a default format of Raw value instead, as otherwise this would have introduced a regression.

We're currently testing this, especially whether the default config form value is being selected correctly, and whether select fields without a configured submission format still behave correctly.

jensschuppe → created an issue.

jensschuppe → created an issue.