Cannot use a / in route parameter

@miiimooo
What is the usecase you have for this particular feature?

Adding a use case I'm facing: I wanted to clean up Facets Pretty Paths to make it add an optional trailing route parameter in a route subscriber, instead of an InboundPathProcessor ; can't really work currently.

Edit: fixed project link.

Attaching a proof of concept patch that works on the official Symfony example, setting to needs review for the bot to check if it breaks something.

The real challenge here is to find an appropriate way to check for route parameters that allows a "/" to be in there. Current patch hardcodes ".*" and ".+" which are the examples provided by symfony for this use case and are easy enough to check, but code in the wild might actually provide a more restrictive regexp to validate the argument.

From the correct root this time.

I wonder how much of a performance hit this is. I guess that when there are a lot of routes, even without having any of them with unlimited parts, this will slow the entire route generation down quite a bit.

I think this is faster, array_filters are supposed to be.

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
@@ -42,6 +47,12 @@ public static function compile(Route $route) {
+    foreach ($route->getRequirements() as $requirement) {
+      if ($requirement == '.*' || $requirement == '.+') {
+        $num_parts = static::UNLIMITED_PARTS;
+      }
+    }

-->

$unlimited_requirements = array_filter($route->getRequirements(), function ($it) {
  return $it === '.*' || $it === '.+';
});

if (count($unlimited_requirements) > 0) {
  $num_parts = static::UNLIMITED_PARTS;
}

Even with that, this would need profiling to make sure that this isn't too big of a hit.

Hi, updated the patch regarding @borisson_ recommendations

Note: all of this is happening after calling Symfony\Routing\RouteCompiler::compile() ; this in in turn calling RouteCompiler::compilePattern which is already doing a preg_match, putting them in a foreach, and calling getRequirement one by one on the matches. Profiling is always a good thing, but I'd be surprised to see a performance hit caused by this patch in the route compilation phase.

Performance wise, I'd be more concerned about the potential hit at runtime ; this patch introducing an OR in RouteCollection::getRoutesByPath means that potentially, everything trying to retrieve a route collection for a pat could take a hit.

I don't know about the performance, but it seems to work for me!

Still needs profiling of the performance folks, thanks

Applied the changes in #16 to Drupal core 8.8.x

#26 seems to be corrupted somehow, so this is a reupload. Also set the correct target (8.8.x)

Re-rolled #27 for 9.2.x

I opened 2 MRs, 1 for 8.9.x and one for 9.2.x. I wrote tests for this. This is working as expected for the given cases, I fixed one more edge case, but I have issues with it in its core. The two improvements I did were:

• I am checking that the parameter exists in the path. Because up to now, you could add a $route->setRequirement('some_dummy_text__not_a_route_parameter', '.*' and would still work.
• Only allow this when the route is ending with the parameter set as '.*' or '.+'. This is because, if you have a path, e.g. /some/{param}/{foo} and the param is set as '.*' and foo is set as '.+', the path '/some/hello/world/blah/blah' might have unexpected results.

In general, I am not sure about the solution. I am not sure whether this should be the way to handle it. I think that since Drupal is handling the query parameters as a string divided by '/', a custom case would require to either encode the parameter, or implement something like https://drupal.stackexchange.com/a/225128 in your custom project.
The reason I am thinking of this is that there are many edge cases.

1. The most basic one is what happens if I want to allow '/' but not anything else. The '.*' is like a keyword in this case. What if I want to only allow digits and '/'? e.g. I want to allow the regex \d{2}/\d{2}/\d{4} (just for the sake of argument) in order to pass a date.
2. What happens if in the example above with the /some/{param}/{foo}, The foo</foo> is not set also to '.*' but to anything else. How do I control that the <code>{param} will not get the whole remaining string and foo will be left blank?
3. What happens when I want more than one parameter to contain multiple slashes?
4. What if I have some kind or route where the {param} is '.*' but I have a sub route '/edit' and so, I don't want it to include a '/' because otherwise I will not be able to access the /some/{param}/edit (this is solvable by my update above).

Anyway, I leave it also for the community to decide. The tests are already green. If the community wants to have it as a locked feature, that only in these specific cases works like this, then I am fine with it.

However, to be fair, Symfony is already allowing this as shown at https://symfony.com/doc/4.2/routing/slash_in_parameter.html
So, again, I wrote tests since this exists here. I still think though this is not the best way for the Drupal system.

Rerolled #34

Hi, I tried the patch from #34 but couldn't get it to work with facets_pretty_paths. The `$unlimited_requirements` method uses the stripped path, which doesn't include the default values. Because of this `substr_compare()` fails.

In my usecase changing the `$changed_path` to `$route->getPath();` fixes the problem.

Also as yet there is no profiling for performance regressions.

1. +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
   @@ -39,6 +44,20 @@ public static function compile(Route $route) {
   +    $unlimited_requirements = array_filter($route->getRequirements(), function ($it, $key) use ($stripped_path) {
   

   What is '$it' meant to mean?

2. +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
   @@ -39,6 +44,20 @@ public static function compile(Route $route) {
   +      // Only the last parameter can be set to include '/' and only if the path ends with this parameter.
   

   That's not how Symfony works, and presumably we're trying to match what it does. The docs say:

   > If the route defines several parameters and you apply this permissive regular expression to all of them, you might get unexpected results. For example, if the route definition is /share/{path}/{token} and both path and token accept /, then token will only get the last part and the rest is matched by path.

   -- which means that you can have parameters after the one containing slashes.

3. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -362,9 +362,10 @@ protected function getRoutesByPath($path) {
   -      $routes = $this->connection->query("SELECT [name], [route], [fit] FROM {" . $this->connection->escapeTable($this->tableName) . "} WHERE [pattern_outline] IN ( :patterns[] ) AND [number_parts] >= :count_parts", [
   +      $routes = $this->connection->query("SELECT [name], [route], [fit] FROM {" . $this->connection->escapeTable($this->tableName) . "} WHERE [pattern_outline] IN ( :patterns[] ) AND ([number_parts] >= :count_parts OR [number_parts] = :unlimited_parts)", [
   

   Stupid idea which I may come to regret, but instead of making this critical path query more complicated, what if we stored a route that allows a slash in its path with a really big number for number_parts?

   Then this wouldn't have a risk of affecting performance.

I've just found that the '.+' requirement actually DOES work for slurping up things into one parameter.

So you can do something like this:

action_link.action_link:
  # The numbered parameters are just dummies, as a workaround for
  # https://www.drupal.org/project/drupal/issues/2741939.
  path: /action-link/{action_link}/{state}/{user}/{parameters}/{param_2}/{param_3}/{param_4}
  defaults:
    _title: 'Action Link'
    _controller: '\Drupal\action_link\Controller\ActionLinkController::action'
    param_2: ''
    param_3: ''
    param_4: ''
  requirements:
    _custom_access: '\Drupal\action_link\Controller\ActionLinkController::access'
    # Allow the 'parameters' parameter to include a '/', mopping up multiple
    # path elements.
    parameters: '.+'

and your controller gets everything in the $parameters function parameter.

@joachim : WRT #45, yes, adding dummy parameters is the workaround currently used in Facets Pretty Path ; the matching RouteSubscriber, adding as much dummies parameters as possible can be found in https://git.drupalcode.org/project/facets_pretty_paths/-/blob/8.x-1.x/sr...

Honestly though, it's not really pretty.

For #44:

#44.1 : Pretty sure that it is coming from @borisson_ comment in this issue (#15, 5 years ago ) and was used as is :) I'd guess it's for $item.

#44.2 : The restriction on the parameter containing a / being the last one wasn't part of the original patch, and was added in #35. I tend to agree that matching Symfony here is best, and as far as I can tell, it should work just fine.

#44.3 : Storing the route with an arbitrarily high number of parts should work. That's what can be achieved today when adding dummy parameters. Given that path is a varchar(255), it doesn't have to be that high to be on the safe side, setting it number_parts to 256 should ensure that it's only matching routes with a wildcard in them. Given that there's an index on (pattern_outline, number_parts), from a performance point of view the OR shouldn't really have an impact, but if that can alleviate performance concerns and get this commited ; that feels like an OK solution :)

re-rolled for 10.1.x

Fix CCF #47

Note: the test that was in #40 is missing from the re-roll in #47

> #44.3 : Storing the route with an arbitrarily high number of parts should work. That's what can be achieved today when adding dummy parameters. Given that path is a varchar(255), it doesn't have to be that high to be on the safe side, setting it number_parts to 256 should ensure that it's only matching routes with a wildcard in them. Given that there's an index on (pattern_outline, number_parts), from a performance point of view the OR shouldn't really have an impact, but if that can alleviate performance concerns and get this commited ; that feels like an OK solution :)

Done.

New MR on 10.1.x.

I've just noticed this in the docs at https://www.drupal.org/docs/8/api/routing-system/parameters-in-routes/us...

> While Symfony allows for more arbitrary use of slugs, Drupal has stricter requirements. In fact, unlike generic Symfony routes, Drupal requires that a slug occupies a complete path part - the portion between two slashes (or everything after the last slash). If you must pass a parameter containing slashes, apply the same trick as in PathProcessorFiles.

PathProcessorFiles should maybe be changed as a follow-on?

The Above patches work only upto 62 url parts after that route will be empty as $ancestors from getCandidateOutlines is having bitwise shift left condition of $end = (1 << $number_parts) - 1; which will skip the execution at https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/lib/Drupal/Core/Routing/RouteProvider.php#L297

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

This could use an issue summary update with proposed solution and remaining tasks. Think it would be a good spot for the profiling to happen also.

Updated the IS.

Core is currently doing for /system/files/ route.

It uses an InboundPathProcessor to get the route of the file, set it to a query parameter and return just the /system/files to allow the route system to match the route, no matter how many / the file has, because now is in a query parameter.

class PathProcessorFiles implements InboundPathProcessorInterface {

  /**
   * {@inheritdoc}
   */
  public function processInbound($path, Request $request) {
    if (str_starts_with($path, '/system/files/') && !$request->query->has('file')) {
      $file_path = preg_replace('|^\/system\/files\/|', '', $path);
      $request->query->set('file', $file_path);
      return '/system/files';
    }
    return $path;
  }

}

Then, in the controller, it gets the target by accessing the file query parameter.

    $target = $request->query->get('file');

I created a new MR that targets 11.x.

Do we have any doc about profiling this kind of change?

I used xhprof to profile the patch on a standard profile install with cache disabled.
I called drush cr --xh-link to generate the reports.

I didn't notice a real impact (only ~1000 microseconds of difference in my tests).
The main difference in the stack trace are the closure that is called by array_filter() and the getRequirements() call. And these calls are fast.
Here are screenshots of the xhprof report. I can't upload the .xhprof files because this extension is not allowed.

RouteCompiler::compile() can also be called during runtime if there is a cache miss on a compiled route but I don't think it's worth profiling (it's only one call here and there as opposed to the hundreds of calls during cache rebuild).