Ignore repeated slashes in the incoming path like Drupal <= 7

These are the removed hunks from the other patch to turn #291 back into #289, so will be our starting point when we get that in.

And, adding Needs Tests so we can add some more cases to check and prevent regressions.

The issue still exists in 8.4. Applying the patch fixes the issue (Tested /node//[nid], /node/[nid]/, and /node//[nid]//). The functional tests are passing. Moving to RTBC

Not a blocking issue, just a comment: The update to remove the duplicate and trailing slashes could be changed from

$parts = preg_split('@/+@', $request->getPathInfo(), NULL, PREG_SPLIT_NO_EMPTY);
$path = '/' . implode('/', $parts);

to
$path = preg_replace(['{//+}', '{/^}'], ['/', ''], $request->getPathInfo());
I didn't roll a new patch as I have no idea which one would be faster. It may well just be a personal preference.

Fired off the updated tests with no failures. Makes sense on the preg_split, thanks!

One minor note on the following comment as it doesn't read smoothly. I recommend `Note that no test case is included for paths with multiple leading slashes because they are redirected and cannot be used to create Request objects.` or something similar.

+    // Note that paths with multiple leading slashes get redirected, and also
+    // cannot be used to create a Request object, so multiple leading slashes
+    // are not part of the text cases.

updated the patch. only change is the suggested comment edit. not sure what the failure from the test bot is about

Failure was b/c https://www.drupal.org/node/2863416#comment-12062889 so NR is def OK here.

I think this looks good. My involvement was minimal with this change; going to read this applied later today and likely set RTBC.

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -146,22 +146,23 @@ public function __construct(Connection $connection, StateInterface $state, Curre
-    $cid = 'route:' . $request->getPathInfo() . ':' . $request->getQueryString();
+    // routes. We can not yet convert the path to lowercase since wildcard path
+    // portions may be case sensitive if they contain data like a base64 encoded
+    // token. We remove repeated and trailing slashes to normalize the path.
+    $parts = preg_split('@/+@', $request->getPathInfo(), NULL, PREG_SPLIT_NO_EMPTY);
+    $path = '/' . implode('/', $parts);
+    $cid = 'route:' . $path . ':' . $request->getQueryString();
     if ($cached = $this->cache->get($cid)) {

More thinking out loud here, but this will result in some stale data in the cache, but I don't think this is a real problem for an edge-case and the entries will be GCed anyway. IOW, I think a post-update hook to clear is overkill.

Played with this applied.

Two things, not sure if we need to go back to NW.

1. If Internal Page Cache is enabled, and a path like '/node/////1' gets cached as a 404, the patch will still serve up a 404 until a cache clear. I think we may want to consider clearing that bin (if enabled) in a post-update hook.

2. Does anyone recall why we aren't doing a RedirectResponse to 301 to the proper slashified path? We will serve up the proper route, but at the path with the wonky slashes? By itself, this may not be a problem, but there are a few side effects with breadcrumbs:

?

While the patch works, I don't think that it's implemented as it should be.

If you got to ///node/[nid] you are redirected to /node/[nid]. I believe the same thing should happen if you visit /node///[nid] as is mentioned by pwolanin in #13. This should also fix mpdonadio's breadcrumb problem in #12.

Re-roll for 8.6.x. This includes a rather significant change in the form of RouteProvider::normalizePath(), which I introduced to deal with the introduction of RouteProvider::getRouteCollectionCacheId() since the last iteration of this patch.

I will look into redirecting duplicated non-leading slashes as well.

This implements the redirect approach as discussed in #12 and #13.

I think this is a lot cleaner, primarily because it keeps our API pure, and we move processing invalid paths to a layer dedicated to handling input.

1. Reproducing. Creating an article, landed on /node/1: works. /node//1: 404.
2. Applying patch: curl -l https://www.drupal.org/files/issues/2018-03-28/drupal_2852361_19.patch | git apply -v
3. Applied clean.
4. drush cr
5. Retry node//1: success, I am redirected to /node/1.
6. Few more for good measure. /node/1//, /node//1//, both work, but I am redirected to /node/1/ (trailing backslash). I assume this is fine.

Since this issue has successful tests, and has no UI changes (no screenshot needed), I'm moving it to RTBC. Apologies if that's not the correct action.

That looked like an unrelated failure.

I don't see how this is a major bug. Also the current fix makes it a minor release only fix and one that needs a change record.

+++ b/core/core.services.yml
@@ -1210,8 +1210,8 @@ services:
-  redirect_leading_slashes_subscriber:
-    class: Drupal\Core\EventSubscriber\RedirectLeadingSlashesSubscriber
+  redirect_successive_slashes_subscriber:
+    class: Drupal\Core\EventSubscriber\RedirectSuccessiveSlashesSubscriber

similarity index 79%
rename from core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashesSubscriber.php

rename from core/lib/Drupal/Core/EventSubscriber/RedirectLeadingSlashesSubscriber.php
rename to core/lib/Drupal/Core/EventSubscriber/RedirectSuccessiveSlashesSubscriber.php

Can't do this in bug fix. Imo it'd be better to just do the changes to RedirectLeadingSlashesSubscriber and then do the service rename and class rename in another issue.

So keep RedirectLeadingSlashesSubscriber but make it handle trailing slashes as well? Would a docblock update and change record be sufficient, or do we want to document this another way too?

This patch addresses the feedback given in #26. Thanks! I also added a change record.

I'm not sure I agree with #26. This is extremely painful UX-wise and has caused a lot of pain since the major changes from D8 menu/routing sprint. It also creates D7 upgrade path problems with old links suddenly becoming broken. There should be some related issues about the leading slashes issues; I didn't find them but they're out there.

Setting major for now.

Rerolled #29

Tested patch #38 and verified it worked see screenshots

This patch I'm uploading is the same as #38 but noticed there was no tests only patch.

Not sure why this didn't upload. Again SAME as #38

If tests-only patch fails and this passes I'll mark RTBC

Looks like everything passed and test-only failed.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x

We could use a follow-up for the class rename.

I made the followup, change the CR to make it easier to read and published it.