Make Drupal handle incoming paths in a case-insensitive fashion for routing

Most CMSs are case sensitive so I'm not sure if I would consider this a bug or an unexpected feature of using symfony.

If we want to maintain case insensitivity, seems like we'd need to intervene very early? Maybe overriding the request class or manipulating the globals.

See previous issue: #368093: Block visibility settings are case sensitive ?

According to the W3C (http://www.w3.org/TR/1999/REC-html401-19991224/types.html#h-6.4):

"URIs in general are case-sensitive. There may be URIs, or parts of URIs, where case doesn't matter (e.g., machine names), but identifying these may not be easy. Users should always consider that URIs are case-sensitive (to be on the safe side)."

See also #333054: Make all varchar columns case sensitive (regression) (was make page cache case sensitive) and before that #99970: Page cache matching is case insensitive.

I'm not really that bothered whether URLs are case sensitive or not, but we have various assumptions in core / pending core issues which want URLs to be case sensitive or not, and that's been changed without any of those cases being considered at all.

I'll be honest, I never knew we were doing anything case-insensitive. Paths are case-sensitive. Strings are by default case-sensitive in PHP. So routing is case-sensitive. It never occurred to me to consider that anything should be case-insensitive.

Yes, anything relating to paths should be case-sensitive, across the board, everywhere. If that's not the case, it's a bug.

The new routing system also changed the behavior or being able to append any kind of argument which are just ignored,
so things like node/123/foobar just works.

If people want to have insensitivity, what about make it possible and provide a potential contrib module?

Some features just shouldn't be. We shouldn't concern ourselves with it. The entire system is swappable to a ridiculous degree by virtue of using the DIC for everything, so you could do whatever wacky stuff you want from contrib.

I'm thinking this is a won't fix.

You can also do this with mod_spelling in Apache. It comes by default, but is not usually enabled.

RE #8, That would be a bug-fix. Responding to arbitrary URLs makes it easy for someone to SEO DoS your site.

+1 to letting this "feature" die.

This is a critical bug not because of the behaviour change but because there's no upgrade path for it. Upgrade paths are not a won't fix and I wish people would read the issue before posting.

I don't know what exactly might need to be done for the upgrade path, but again that's because it's not been discussed yet. For example do we need to go in and lowercase every system path in the URL alias table? What about block visibility rules?

@catch - it sounds like the path rules already lowercase - we should check.

Are we converting path aliases to be route based or always path? I guess the way it's built now it has to be path, though that leave a lot of fragility if route path patterns are changed?

@pwolanin, no-one's really looking at that or discussed it etc.. There's #1553358: Figure out a new implementation for url aliases which is looking at the CMI implications which also hasn't been updated in a long time.

Re-titling so it's more obvious what the bug is.

I don't know what an upgrade path here would even look like... What's the task to be done here? What data do we care about? (We don't care about paths in content blobs because, well, I don't think we ever really have.)

@Crell - yes that's the point. The change was made by accident, and no-one has yet done the research to see exactly which subsystems were affected, hence why it's a critical upgrade path issue because it's not even known yet how much may be broken.

However, two examples were given in comment #11 which you appear to be ignoring. Perhaps you could have checked those instead of posting another contentless follow-up?

I spent a couple of minutes testing path aliases to confirm the bug.

An alias pointing to a system path of /User/Register works just fine on Drupal 7. Upgrade to Drupal 8 and it's a 404.

While testing I also discovered an unrelated bug, but that'll affect anyone trying to re-save an alias with an incorrect path (assuming they don't fix it). Opened #2086559: Adding an invalid path alias shows an exception in the UI for that.

On system paths in blobs, we'll need to decide about that (as well as incoming links which may break), but it's not the case that we've never cared about those, given the critical regression in Drupal 7 that's been open for months dealing with a similar issue #1934568: Allow sites using the 'image_allow_insecure_derivatives' variable to have partial protection from the Drupal 7.20 security issue. A commented out .htaccess rule that checks for a lower case version of incoming paths and redirects to that if necessary might be enough for external links and blobby content.

As well as path aliases, {menu_links} also stores system paths as a string and we have interfaces that let you enter that manually. I haven't tested what happens on the upgrade path for those yet, given that's tied up with access checks I'd expect it to be worse than a 404.

This is further complicated by the fact that we don't enforce lower case router paths in Drupal 7, so it's valid to have route names like admin/Content - meaning that simply lowercasing all router paths stored everywhere could potentially break legitimate aliases/links.

Having a look through D8 upgrade path issues now we have agreement on moving to migrate. Unfortunately this one is still going to be an issue since the old data might need transformation.

No longer blocks 8.0 due since the migration path can go in later, but it'll still need massaging of data during the migration.

Discussing in person with Crell and dawehner at Drupalcon LA. We were thinking that this is a doc issue and not something core should directly handle. There can be a contrib module to make path handling case-insensitive if some site needs that.

In D8 aliased and non-alised handling shoudl be consistent, so we should file a bug if the handling of aliased paths is not case sensitive also.

I spoke with this at DrupalCon LA with @alexpott and @pwolanin.

We did a quick survey of several big websites that also use dynamically-generated pages, both PHP and not, for example:

* WordPress (25% of the web): http://ma.tt/2015/05/wearable-gadgets/ vs. http://ma.tt/2015/05/wearable-Gadgets/ - case insensitive.
* Microsoft (ASP.NET): http://www.microsoft.com/en-us/default.aspx vs. http://www.microsoft.com/en-us/DeFault.aspx - case insensitive.
* Wikipedia (#7 website in the world): http://en.wikipedia.org/wiki/Main_Page vs. http://en.wikipedia.org/wiki/main_page - case insensitive.

Mind you, lots of other ones are not: Facebook, Google, etc. but they also tend to write their own stuff vs. use a generated dynamic system.

Additionally, no matter what we do, GOOGLE.COM and google.com both take you to the same place in your browser. So the expectation from non-technical users is likely to be that URLs are case insensitive; it's very difficult to explain that "if it comes after the slash then case matters, else..."

And finally, the current behaviour is not hurting anyone (unlike the ability to put infinite /something/made/up/here paths), and breaking this behaviour has a lot of ripple effects. For example, sites having to put manual redirects in, sites having to contact 3rd party websites/customers/etc. and ask them to change links, etc. Lots of disruption for not a lot of gain.

Therefore, we agreed to fix this by keeping the existing behaviour from Drupal 7.

The comments above happened as I was performing triage of this issue at DrupalCon Los Angeles. I've updated the issue summary and added a beta evaluation.

While I did not perform a D6/D7 to D8 upgrade to reproduce the issue, I did confirm that handling for node paths is case sensitive, and that node paths such as NODE/1 or Node/1 404 now when they didn't previously. I also noticed that this behavior is not consistent with how path aliases are handled, and kgoel and I opened #2489484: Path and alias case sensitivity should be consistent in core.

kgoel and I discussed this with alexpott, pwolanin, crell, and dawehner, who all agree this issue is still relevant and still major, and alexpott discussed the issue with webchick, as she outlined in #21.

Note that there are still some edge cases we need to document. "mbstring is a non-default extension." So, for example if PHP has mbstring enabled then the Drupal Unicode::strtolower() uses: http://php.net/manual/en/function.mb-strtolower.php

In this case Ä -> ä

But in the fallback in Unicode::strtolower() it only maps accented characters, so it seems like Ä is left untouched, as well as many others (Cyrillic, etc).

As pwolanin noted during the sprint, string comparisons are case-insensitive in MySQL, but are not in other databases. D8 does not currently convert URL aliases to lowercase before saving them in the database -- if we change this behavior, do we want to note it in the help text below the URL alias field on the node page?

Here's a preliminary patch. It runs Unicode::strtolower() on incoming paths, and on URL aliases before they're saved. It doesn't make any changes to help text.

The testbot revealed that some data in the URL needs to be case-sensitive, e.g. the Base64-encoded data in /admin/config/system/actions/add/RlNfIL8R9iZFfGM45liyQlw3nkJlc3vvhbAGQN5bYUc. Per pwolanin, it looks like we need to convert to lowercase in getRoutesByPath() in RouteProvider, and also in RouteCompiler, as well as deeper in the alias manager than the first patch. PathProcessor needs to be reverted because routing is happening after path processing.

tested locally:

* patch applied cleanly
* /USER gives a 404, /user works normally
* /NODe/1 gives a 404, /node works normally
* I created a URL alias to node/1 called "Case-Sensitive-Alias". It points to node/1 when entered as /case-sensitive-alias and Case-SENSITIVE-alias.
* one-time login link token gives an "invalid token" error when I muck with the case, and works fine when I leave it as drush generated it.

Looks like the Path tests need to be updated, though.

We should run the tests through PostgreSQL and SQLite too.

Edit: specifically ohthehugemanatee's fourth test regarding URL aliases.

@marcvangend asked on the closed duplicate #2489484: Path and alias case sensitivity should be consistent in core whether this issue should be critical, as that one was filed as critical. Per discussion in #23, I agree with webchick and alexpott that this is not critical because:

• There are workarounds (e.g. .htaccess rules).
• It can't actually result in any data integrity or security issues, just a whole lot of embarrassing broken links.
• We can fix it in a patch release safely.

That said, it's definitely a gross major that we should fix sooner rather than later. :P

Thank you to the major triage sprint participants for digging this out again and to everyone who discussed it at DrupalCon.

@ohthehugemanatee - Now that I'm back from my short post-DrupalCon vacation, I can take a crack at updating tests. My experience with tests is pretty limited, though, so it might be several days. Is there a documented process somewhere for modifying existing tests, so that regressions aren't introduced?

@xjm - I defintely agree that this is not critical, but it's definitely going to be a big gotcha for site owners. If this doesn't make it into 8.0.0, there should definitely be some sort of documentation of the issue and possible workarounds. An .htaccess rule will work for Apache, but Nginx sites (including notably anyone on Pantheon) would need an alternate method -- maybe a regex match and 301 at the top of settings.php? I'm thinking there should probably be a short acknowledgement of the issue in UPGRADE.txt (I don't see a "Known Issues" section there now -- will there be?), linking to a new D.o documentation page with more detail and sample .htaccess/settings.php code. I'm happy to write a draft for this, but not sure about process. Would that be a child issue of this, a totally separate issue, and would it be created now, or when/if it's determined that this isn't going into 8.0.0?

I have converted path into lowercase in RouteCompiler.php but I think path needs to be converted into lowercase somewhere else beside RouteProvider and RouteCompiler since node/1 works but NoDe/1 or NODE/1 returns 404. I need to debug more.

1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
   @@ -50,7 +51,7 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
   -      'alias' => $alias,
   +      'alias' => Unicode::strtolower($alias),
   

   This is strtolower-ing on save, but I don't see anywhere that's strtolower-ing on lookup. Unless that's already in the code base somewhere? Might this be what's causing some of the test fails?

2. +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
   @@ -51,7 +51,7 @@ public static function compile(Route $route) {
   -      $symfony_compiled->getRegex(),
   +      $symfony_compiled->getRegex() . 'i',
   

   What's the performance impact of a case-insensitive regex? It may be faster to instead:

   1) strtolower() the route path when dumping the routes.

   2) strtolower the incoming path in the route provider.

   3) Do the normal case-sensitive regex.

   That way the regex does less work but the result is the same. It's probably simple enough that we can try both approaches and benchmark.

@Crell - you want to do strtolower in #2 just for finding the routes? We have to be careful to preserve case for the portions that are slugs unless we want to prohibit base64 password reset URLs and the like.

However, that seems like it defeats the point of the issue - so maybe tokens or anything else case sensitive needs to move to a query string?

Ugh. You're right, some tokens could break that way. My concern is the performance of a case-insensitive regex. Some google spelunking led me to this post by Tim Bray. See the section "on case and diacritics":

http://www.tbray.org/ongoing/When/200x/2003/10/11/SearchI18n

So IMO we cannot switch to a case-insensitive regex without benchmarks saying the cost is acceptable. If the cost is not acceptable, we either need a plan B or to stick with case-sensitivity. Moving tokens to the query string would work for things like cron or password resets that we control, but it sounds like something that may bite contrib badly if they don't know about it.

Anyone want to run a benchmark and prove me wrong to worry about performance here? I'd be very happy if you did. :-)

How much overhead is the regex change? Or we just need to benchmark?

pwolanin: The article I linked says a lot, but I think we just need benchmarks to say. It shouldn't be too difficult to do both and see. If the performance difference is large then that makes the decision for us.

This needed reroll.

pwolanin and I tested node with path node/1 and NoDE/1 where node/1 works but NoDE/1 returns 404 with the #45 patch. We debugged and found where it's failing....

protected function matchCollection($pathinfo, RouteCollection $routes)
    {
        foreach ($routes as $name => $route) {
            $compiledRoute = $route->compile();

            // check the static prefix of the URL first. Only use the more expensive preg_match when it matches
            if ('' !== $compiledRoute->getStaticPrefix() && 0 !== strpos($pathinfo, $compiledRoute->getStaticPrefix())) {
                continue;
            }

We think the problem is in matchCollection method in core/vendor/symfony/routing/Matcher/UrlMatcher.php class and it's where route path is alway prefix with node/
*Symfony\Component\Routing\CompiledRoute*staticPrefix = "/node"

I talked this over with KGoel and dug through this at MWDS. Here's what's going on:

Symfony's RouteCompiler (and Drupal's subclass thereof) creates a regex to use for matching the path. However, as a performance optimization it also stores the initial state part of the route (staticPrefix). That is, for a path of /node/{node} it will store "/node", so that it can then do a cheaper strpos() check on the path to filter out unmatching cases (say, /admin/extend) easily. That, however, is still case-sensitive, even if we make the regex case insensitive.

That match is in Symfony's UrlMatcher, so to fix that we would need to subclass UrlMatcher and change that line to a stripos() to make it case-insensitive. That's an annoying amount of work to do, but it should work.

That said, if we're going to do that it would be even more performant to remove that check entirely. In Drupal's case, we have matching effectively spread out across a couple of classes. The path-matching would have mostly happened already back in the RouteProvider. In fact, in the typical case by the time we get to UrlMatcher there will only be one route anyway. Exceptions would be if http and https were routed separately, or REST cases where different methods are routed separately. In those cases, though, the prefix check is still useless because the paths are the same.

Therefore, if we're going to have to subclass UrlMatcher anyway, let's just eliminate that line as redundant. That will resolve the insensitivity issue as well as be a small performance micro-op on its own.

Reroll. Working on addressing #48

+++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
@@ -51,7 +51,7 @@ public static function compile(Route $route) {
+      $symfony_compiled->getRegex() . 'i',

This might be confused as a typo so maybe a comment?

Or RouteCompiler could override that method and also a comment in that method? I think that it would need to basically do the same thing since $regex is private.

Reroll

This is just a quick patch to address #48. Still need to address #50

+++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
@@ -9,6 +9,7 @@
 use Drupal\Core\Path\CurrentPathStack;
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\Routing\Matcher\UrlMatcher;
 use Symfony\Component\Routing\RouteCollection;
 use Symfony\Cmf\Component\Routing\NestedMatcher\UrlMatcher as BaseUrlMatcher;

This is the problematic line. It collides with the class name here. It's not needed, though, since we're already inheriting from that class indirectly.

@crell - thank you for the pointer!

Not sure if this is the right direction for the test changes. Since Random::name() doesn't guarantee an uppercase character, this issue may have random fails.

Good progress, but a little more to do I think

1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
   @@ -59,7 +60,7 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
   +      'alias' => Unicode::strtolower($alias),
   

   Add a code comment here, and in the methods docs to say that it's lower-cased

2. +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
   @@ -53,7 +53,7 @@ public static function compile(Route $route) {
   +      $symfony_compiled->getRegex() . 'i',
   

   If possible, we should not need to do this if we lowercase the input?

3. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +46,46 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +  protected function matchCollection($pathinfo, RouteCollection $routes)
   

   Needs docs especially to explain why it differs from the parent method

If possible, we should not need to do this if we lowercase the input?

Well, this would be just the case, in case routes are defined to be lowercase, right? Nothing ensures that at the moment.

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -59,7 +60,7 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
diff --git a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php

diff --git a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
index d7bd416..ebad3ab 100644

index d7bd416..ebad3ab 100644
--- a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php

--- a/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php

+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
+++ b/core/lib/Drupal/Core/PathProcessor/PathProcessorAlias.php
@@ -7,6 +7,8 @@

@@ -7,6 +7,8 @@
 
 namespace Drupal\Core\PathProcessor;
 
+use Drupal\Component\Utility\Unicode;
+use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Path\AliasManagerInterface;
 use Drupal\Core\Render\BubbleableMetadata;
 use Symfony\Component\HttpFoundation\Request;

This change is not needed at all

What would be also nice is a dedicated test which passes in a non lowercase value to ensure that this usecase still continues to work.

@dawehner - I thought the dumper was lower-casing the route path - if not, that's a bug.

Yes, we should have some test routes with mixed-case paths and make sure they get lowered as expected.

Ok, after discussion with tstockler, dawehner, and kgoel, this changes the RouteBuilder to force all route paths to be lower case. This means that the CompiledRoute regex is already lower case, all patterns are lower case, etc.

Also forcing lower case everywhere in the AliasStorage for consistency, and other fixes to try to get consistency including in the caching.

Need to ask WimLeers about page cache.

From the POV of Page Cache, this is fine. It's just possible that multiple cache entries exist for the same lowercased URL. So the cache hit ratio may then be slightly lower than it would otherwise be.

So, it could be argued this is a potential DoS attack, for those sites that have Page Cache enabled. But… we already have that problem, for example random query strings.

Forgot about the path slug problem. :-(

We need to leave src paths alone in aliases and use the case-insensitive regex because we cannot lower-case bas64 path portions matched as slugs.

Tempted to "won't fix" it. At the very least, it won't actually be case insensitive in all cases.

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -59,7 +60,7 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
-      'alias' => $alias,
+      'alias' => Unicode::strtolower($alias),

@@ -98,6 +99,9 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
+      if ($field == 'alias') {
+        $value = Unicode::strtolower($value);
+      }

@@ -115,6 +119,9 @@ public function delete($conditions) {
+      if ($field == 'alias') {
+        $value = Unicode::strtolower($value);
+      }

Does this patch need an update hook with the storage changes so that any alias that could have been saved with upper case (or migrated in with upper case) is converted to lower case?

Opinions:

* No: Anyone creating alias will find it not working anyway.
* No: Migrate is critical for release, but not beta.
* Yes: It is technically a data model change and being technically correct is the best kind of correct.
* Yes: If the site still has case sensitive aliases stored, then it won't be possible to delete or load the bad data.

I updated the issue summary to use the latest template with data model changes, and uncommented the disruption line based on the current patch.

I also added sqlite and pgsql testbots to run the patch. It looks like SQLite is going to fail on PathAliasTest.

Fixed fail for PostgreSQL and sqlite

with some of route path case insensitivity test

First patch:

1. +++ b/core/modules/path/src/Tests/PathAliasTest.php
   @@ -141,7 +142,7 @@ function testAdminAlias() {
        // Create absolute path alias.
        $edit = array();
        $edit['source'] = '/node/' . $node3->id();
   -    $node3_alias = '/' . $this->randomMachineName(8);
   +    $node3_alias = '/' . Unicode::strtolower($this->randomMachineName(8));
        $edit['alias'] = $node3_alias;
        $this->drupalPostForm('admin/config/search/path/add', $edit, t('Save'));
   

   I looked at this specific test in core currently, and it does not do any assertions. Are we just asserting that the node was saved?

   The assertion was removed in commit e044adb014315580372f8d21a0ed4867ff07842f in #2509300: Path alias UI allows node/1 and /node/1 as system path then fatals. It was removed in patch #27. There wasn't any specific mention either by @dawehner or any reviewers about why that assertion was removed. The assertion comment was "Confirm that the alias was converted to a relative path."

   Maybe this best relegated to a follow-up to discuss whether the "create absolute path alias" should have its assertions added back, or maybe it should be removed now if it has no value?

Next patch:

1. +++ b/core/modules/system/src/Tests/Routing/RouterTest.php
   @@ -211,6 +211,27 @@ public function testRouterMatching() {
   +   * Tests the route path case insensitivity
   

   Should have a full stop. Not critical to the patch.

Haven't addressed #801.

1. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -174,7 +175,9 @@ public function rebuild() {
   -        $route = new Route($route_info['path'], $route_info['defaults'], $route_info['requirements'], $route_info['options'], $route_info['host'], $route_info['schemes'], $route_info['methods'], $route_info['condition']);
   +        // Lowercase the path here so that the events get a consistent path,
   +        // even though we force them all to be lower later.
   +        $route = new Route(Unicode::strtolower($route_info['path']), $route_info['defaults'], $route_info['requirements'], $route_info['options'], $route_info['host'], $route_info['schemes'], $route_info['methods'], $route_info['condition']);
            $collection->add($name, $route);
   
   @@ -182,11 +185,15 @@ public function rebuild() {
   +    // Process the whole collection since we cannot tell what was newly added.
   +    $this->processCollection($collection);
   

   Do we need both?

2. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -221,6 +228,20 @@ public function destruct() {
   +  protected function processCollection(RouteCollection $collection) {
   

   What about renaming the method to something like applyStrToLowerToCollection or something like that. It would be nice if the method name makes it clear what is done here.

3. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -221,6 +228,20 @@ public function destruct() {
   +  protected function processCollection(RouteCollection $collection) {
   +    /** @var \Symfony\Component\Routing\Route $route */
   +    foreach ($collection as $route) {
   +      // Force each path to be lower case.
   

   You can use $collection->getAll(), then you don't need to typehint $route any longer

4. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +46,72 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +   * This version differe from the Symfony parent version in two respects.
   

   is differe a proper english word?

- // Confirm that the alias was converted to a relative path.
- $this->assertNoText($edit['alias'], 'The absolute alias was not found.');
- // The 'relative' alias will always be found.
- $this->assertText(trim($edit['alias'], '/'), 'The relative alias was found.');

Well, the idea of that other issue was that path aliases/sources now starts with a slash, so that particular assertion simply doesn't make any sense anymore. I think given the further test its okay to assume here that the tests got saved.

Addressed #82and some other small fixes.

We have tests, we have the documentation that we are case insensitive, though I think the better place to document that would have been routing.api.php.
I think expanding that a bit would be great!

Adding something to the Router topic in routing.api.php sounds like a good idea to me.

Also... just a question about the current patch. If you use the path UI to add an alias to a path, does it check to see if the alias already exists, and not allow creation if so? I think it does... If so, does it check in a case-insensitive manner? Seems like after this patch, it must, and I am not seeing anything in this patch that changes it (but I may have missed it).

And we should also document this in the help docs and/or page help for the UI for aliases.

In other words...

For developers, I think adding docs to routing.api.php saying that paths and aliases need to be unique, where unique ignores case, is a good idea.

But we also need to consider the Drupal user, and document this for them too. It would affect:
- creating aliases from the Aliases management page
- creating aliases with the alias field or whatever it is for entity aliases (like creating an alias for a node while editing the node)
- places where you actually create paths directly, like in Views... not sure what other modules in Core do the same?

And then my other question is whether this patch also makes sure that when a user is creating an alias or path in the UI via any of these places, is it checked for being unique in a case-insensitive way? This patch needs to make sure that is happening.

Also... just a question about the current patch. If you use the path UI to add an alias to a path, does it check to see if the alias already exists, and not allow creation if so? I think it does... If so, does it check in a case-insensitive manner? Seems like after this patch, it must, and I am not seeing anything in this patch that changes it (but I may have missed it).

This is a good question. \Drupal\path\Form\PathFormBase::validateForm is using $aliasStorage->aliasExists() and $this->aliasManager->getPathByAlias() which we change in the patch:

@@ -184,7 +191,7 @@ public function lookupPathAlias($path, $langcode) {
    */
   public function lookupPathSource($path, $langcode) {
     $args = array(
-      ':alias' => $path,
+      ':alias' => Unicode::strtolower($path),
       ':langcode' => $langcode,
       ':langcode_undetermined' => LanguageInterface::LANGCODE_NOT_SPECIFIED,
     );
@@ -208,7 +215,7 @@ public function lookupPathSource($path, $langcode) {
    */
   public function aliasExists($alias, $langcode, $source = NULL) {
     $query = $this->connection->select('url_alias')
-      ->condition('alias', $alias)
+      ->condition('alias', Unicode::strtolower($alias))
       ->condition('langcode', $langcode);
     if (!empty($source)) {
       $query->condition('source', $source, '<>');

But we also need to consider the Drupal user, and document this for them too. It would affect:
- creating aliases from the Aliases management page
- creating aliases with the alias field or whatever it is for entity aliases (like creating an alias for a node while editing the node)
- places where you actually create paths directly, like in Views... not sure what other modules in Core do the same?

Is there a place to document the above?

For the end user, we use hook_help() to document stuff like this [either in the main help topic for a module or on page-specific help], and/or user interface text (field descriptions and field labels). Also, error messages may need to be updated, such as the one saying a path or an alias already exists when it may look to the user as if this is not the case.

So... I am not sure why it was decided that Drupal 8 URL handling needed to be case-insensitive, other than "Drupal 7 was", but what about accents/diacriticals?

@jhodgdon - sadly, we'd need to change a lot more code to allow us to uniformly use the unicode strlower() method for consistency.

However, I think the case-insenstivie preg_match() and that should be similar in their support of utf-8.

See #21 for discussion of CMS comparisons on this behavior. Maybe that should be copied to the issue summary? See also comment #15

Added some doc

+++ b/core/lib/Drupal/Core/Routing/routing.api.php
@@ -43,7 +43,7 @@
  * - The 'path' line gives the URL path of the route (relative to the site's
- *   base URL).
+ *   base URL). URL path needs to be unique and case-insensitive.

Well, we deal with that automatically, don't we? I think we should instead document what actually happens.

Well, we deal with that automatically, don't we? I think we should instead document what actually happens.

Do you mean explain where we are converting path into lower-case? I wasn't sure if I need to document RouteProvider::getRouteCollectionForRequest in api and explaining that bit would result into complication. I can certainly try if that's what you mean.

with path slug test coverage

This is coming along well! I reviewed the patch and had some mostly docs/coding standards comments:

1. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -221,6 +226,19 @@ public function destruct() {
      }
   +  /**
   

   Nitpick: you need a blank line here between the two methods.

2. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -221,6 +226,19 @@ public function destruct() {
   +   * Apply additional processing to each route in the collection.
   

   This is the documentation for the applyLowerCaseCollection() method. It seems like the docs are too non-specific. It's not "apply additional processing", it's "lower-case each path".

   If you want to have the method be more generic, I think it needs a more generic name, like applyProcessingCollection(), and then this docs line can stay.

   If you want the method to be as it's named, then I think the docs need to be made more specific.

3. +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
   @@ -53,7 +53,8 @@ public static function compile(Route $route) {
   +      $symfony_compiled->getRegex() . 'i',
   

   Um. Is Symfony already using the "u" modifier to indicate it could be Unicode?

4. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -149,7 +150,8 @@ public function __construct(Connection $connection, StateInterface $state, Curre
   +    // routes. We can not yet convert the path to lower case since path portions
   +    // corresponding to path slugs may be case sensitive.
   

   What's a path slug? I don't like seeing terms in documentation that I don't know, and I've never seen this one in Drupal docs before.

5. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -319,7 +321,8 @@ public function getRoutesByPattern($pattern) {
   +   *   The route pattern to search for (contains % as placeholders). Must be
   +   *    lower case.
   

   nitpick: the second line here should not be indented.

   Also... I am a bit confused as to why you require the input to this method to be lower-case. Why not just lower-case it in the method, as is being done in other methods?

6. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -327,7 +330,7 @@ public function getRoutesByPattern($pattern) {
   +    $parts = preg_split('@/+@', Unicode::strtolower($path), NULL, PREG_SPLIT_NO_EMPTY);
   

   In fact, here it looks like you *are* lower-casing $path...

7. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +46,72 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +   *   The path info to be parsed.
   

   info => information

   But... can this be more specific? What exactly is "path information" and how is it parsed?

8. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +46,72 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +   * @return array An array of parameters
   

   The docs ("An array ...") need to be on the next line, indented 2 spaces, ending in .

   But... it says it returns "an array of parameters" -- what does this mean? Oh I guess it's the route information. Let's say that.

9. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +46,72 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +  protected function matchCollection($pathinfo, RouteCollection $routes)
   +  {
   

   nitpick: move { to the previous line

10. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
    @@ -46,4 +46,72 @@ public function finalMatch(RouteCollection $collection, Request $request) {
    +      // We use a case-insensitive comparison.  It would be more consistent
    

    nitpick: extra space before "It".

11. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
    @@ -46,4 +46,72 @@ public function finalMatch(RouteCollection $collection, Request $request) {
    +      // a path slug receives base64 encoded data.
    

    What's a path slug and how does it "receive" anything?

12. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
    @@ -46,4 +46,72 @@ public function finalMatch(RouteCollection $collection, Request $request) {
    +        if ('HEAD' === $method = $this->context->getMethod()) {
    

    Can you put
    $method = ...
    in parens? To me, a line of code like this with === and = in the same line, without () to group the assignment, is hard to read.

    Actually, better yet make the assignment a separate line before the if().

13. +++ b/core/lib/Drupal/Core/Routing/routing.api.php
    @@ -43,7 +43,7 @@
    + *   base URL). URL path needs to be unique and case-insensitive.
    

    Um... Maybe reword this to say:

    Paths are case insensitive, and need to be unique.

14. +++ b/core/modules/locale/src/Tests/LocalePathTest.php
    @@ -72,7 +73,7 @@ public function testPathLanguageConfiguration() {
    -    $english_path = $this->randomMachineName(8);
    +    $english_path = Unicode::strtolower($this->randomMachineName(8));
    

    Are these changes to the tests necessary? It seems like we would want people to be able to enter their preferred case for paths, so that their URLs can have upper-case letters in them, and just make sure that *matching* is considered in a case-insensitive way, right? So the tests should not require that the input is all lower-case. Right?

15. +++ b/core/modules/path/path.module
    @@ -20,13 +20,13 @@ function path_help($route_name, RouteMatchInterface $route_match) {
    +      $output .= '<p>' . t('The Path module allows you to specify an unique and case-insensitive alias, or custom URL, for any existing internal system path. Aliases should not be confused with URL redirects, which allow you to forward a changed or inactive URL to a new URL. In addition to making URLs more readable, aliases also help search engines index content more effectively. Multiple aliases may be used for a single internal system path. To automate the aliasing of paths, you can install the contributed module <a href=":pathauto">Pathauto</a>. For more information, see the <a href=":path">online documentation for the Path module</a>.', array(':path' => 'https://www.drupal.org/documentation/modules/path', ':pathauto' => 'https://www.drupal.org/project/pathauto')) . '</p>';
    

    Here I think I would leave the first sentence as it was, and add a new sentence just afterwards saying something like:

    Aliases are case insensitive, and need to be unique.

16. +++ b/core/modules/path/path.module
    @@ -20,13 +20,13 @@ function path_help($route_name, RouteMatchInterface $route_match) {
    -      $output .= '<dd>' . t('If you create or edit a taxonomy term you can add an alias (for example <em>music/jazz</em>) in the field "URL alias". When creating or editing content you can add an alias (for example <em>about-us/team</em>) under the section "URL path settings" in the field "URL alias". Aliases for any other path can be added through the page <a href=":aliases">URL aliases</a>. To add aliases a user needs the permission <a href=":permissions">Create and edit URL aliases</a>.', array(':aliases' => \Drupal::url('path.admin_overview'), ':permissions' => \Drupal::url('user.admin_permissions', array(), array('fragment' => 'module-path')))) . '</dd>';
    +      $output .= '<dd>' . t('If you create or edit a taxonomy term you can add an unique and case-insensitive alias (for example <em>music/jazz</em>) in the field "URL alias". When creating or editing content you can add an alias (for example <em>about-us/team</em>) under the section "URL path settings" in the field "URL alias". Aliases for any other path can be added through the page <a href=":aliases">URL aliases</a>. To add aliases a user needs the permission <a href=":permissions">Create and edit URL aliases</a>.', array(':aliases' => \Drupal::url('path.admin_overview'), ':permissions' => \Drupal::url('user.admin_permissions', array(), array('fragment' => 'module-path')))) . '</dd>';
    

    I'd just leave this as it was.

17. +++ b/core/modules/path/path.module
    @@ -20,13 +20,13 @@ function path_help($route_name, RouteMatchInterface $route_match) {
    +      $output .= '<dd>' . t('The Path module provides a way to search and view a <a href=":aliases">list of all aliases</a> that are in use on your website. Aliases need to be unique and case-insensitive. Aliases can be added, edited and deleted through this list.', array(':aliases' => \Drupal::url('path.admin_overview'))) . '</dd>';
    

    Change that added sentence to say:

    Aliases are case insensitive, and need to be unique.

18. +++ b/core/modules/path/src/Tests/PathAliasTest.php
    @@ -75,7 +76,7 @@ function testAdminAlias() {
    -    $edit['alias'] = '/' . $this->randomMachineName(8);
    +    $edit['alias'] = '/' . Unicode::strtolower($this->randomMachineName(8));
    

    Again, can't the user enter upper-case? This test should allow that too?

19. +++ b/core/modules/system/src/Tests/Routing/RouterTest.php
    @@ -212,6 +212,44 @@ public function testRouterMatching() {
    +   * Tests the route path and route path with slug case insensitivity.
    

    What's a slug (again)?

20. So.... The changes to the hook_help() for the Path module are good (with a few suggested changes)... I'm not seeing the following, which I think also needs to be done for the end user:
    - Changes to error messages they would get when they try to make an alias that isn't unique
    - Changes to the #description on Path fields in the UI explaining they are case insensitive and need to be unique
    - Similar changes to Views UI, where it lets people create paths for pages and feeds
21. I'm also wondering if (for developers and/or end users) there should be some change to wording in an exception that would occur if you enabled a module that had tried to define a duplicate path to another module's path?

Let me work on fixing those points.

re:
#105.3 The regex in the Symfony route compiler is created as

'regex' => self::REGEX_DELIMITER.'^'.$regexp.'$'.self::REGEX_DELIMITER.'s'.($isHost ? 'i' : '')

So it (oddly) uses the s modifier but not u. I'm not sure if u should be used or not - probably need to add some test coverage.

I can already make utf-8 path aliases like:
/content/右blah

Which comes in the URL as:
/content/%E5%8F%B3blah

Ok, so I bit the bullet and made the code consistently process and match paths using Unicode::strtolower(). The tradeoff is that we need to recover the original variables, so I added some tweaks for that. Really this whole code path needs to be optimized/simplified for the things as a follow-up (maybe 8.1.x).

Changes in response to #105

1. fixed
2. fixed
3. Symfony is not using "u". Maybe an upstream bug or follow-up. Removed "i" now.
4. reworded
5. fixed spacing and wording
6. fixed wording
7. This doxygen was just copied from the parent implementation. Removed the word "info" as it is meaningless here.
8. fixed spacing. The words are copied from the parent class docs. Added the word "route" to clarify.
9. fixed, though in many cases we are leaving it like the parent for later comparison.
10. changed the code and this comment
11. reworded
12. fixed, though often we are leaving these matching the parent method despite violating Drupal code style.
13. This is actually wrong. Paths are NOT unique.
14. This is the easiest way to fix the tests since we need to have a lower case value to compare to the output, and since we have a bug that randomMAchineName() returns mixed case. There are tests verifying that mixed case becomes lower case.
15. changed
16. changed
17. changed
18. Other test cases verify that they are converted
19. reworded and fixed test
20. Added some description text. Views doesn't force paths to be unique afaik - Last one may win.
21. As above, route paths are not unique. This was one of the (dubious) advantages of adopting Symfony routing.

Adding "rc deadline" tag

Silly problem with unserialize - easy to fix.

#108 and #34 "We can fix it in a patch release safely.", are opposite in terms of if this is rc deadline or not.

ok, let's call it "rc target".

There might be some data changes needed for path aliases so better to do it before.

took out the original report from the summary (we can use revisions to see it, and bits were already integrated into the summary)

I will fix these now.

1. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   @@ -143,6 +152,19 @@ public function getRequirements() {
   +   * @return array
   +   *  The path variable names keyed by numeric path part.
   

   nit. indent should be 2 spaces.

2. +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   @@ -223,6 +230,20 @@ public function destruct() {
   +   * Lower case the path for each route in the collection.
   

   Lower cases
   (can we third person verb that?)

3. +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
   @@ -64,6 +64,34 @@ public static function compile(Route $route) {
   +   * Drupal only support path wildcard (variables) that consist of a complete
   +   * part of the path separated by slashes.
   

   ^support^supports

   and

   what does "complete part" mean?

4. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +47,83 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +   * @param RouteCollection $routes
   

   full namespace paths in comments.

   https://www.drupal.org/node/1354#classes

   "If you use a namespace on a class anywhere in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash)."

5. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +47,83 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +   * @return array
   +   *   An array of route parameters
   

   period at end of sentence.

   what kind of array?

   array of strings?

6. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +47,83 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +
   +    foreach ($routes as $name => $route) {
   

   no blank line at beginning of methods.

   and...

   is there test coverage for all the paths through this method?

   it looks really complicated. maybe breaking this up with some helper methods would make it easier to test and understand.

7. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +47,83 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +      // check HTTP method requirement
   +      if ($requiredMethods = $route->getMethods()) {
   +        // HEAD and GET are equivalent as per RFC
   

   Case and punctuation for a sentences.

8. +++ b/core/modules/path/path.module
   @@ -21,12 +21,13 @@ function path_help($route_name, RouteMatchInterface $route_match) {
   +      $output .= '<p>' . t('Aliases must be unique, are converted to lower case, and are matched in a case insensitive fashion');
   

   period at end of sentence.

9. +++ b/core/modules/system/src/Tests/Routing/RouterTest.php
   @@ -212,6 +212,53 @@ public function testRouterMatching() {
   +    $this->assertRaw('MIXEDCASESTRING', 'The correct string was returned because the route was successful.');
   +
   +  }
   

   extra newline.

a.
oh, UrlMatcher::matchCollection() was a copy and paste from symfony, and modified. mmm.

if this was drupal code, the "local" variables would be cased_like_this and not lowerCamelCase
... maybe we want to keep it as close as possible to the parent, so it can be easily diff'ed?
but we did other standard changes... hm.

b.
looks like it is lowercase (not lower case)
http://english.stackexchange.com/questions/59409/lowercase-lower-case-or...
grepping core agrees:
ag "lowercase" core/ | wc -l
274

ag "lower case" core/ | wc -l
8

I will do b. next (so it has it's own interdiff)

just the "lower case" -> "lowercase" (with a refactor/rename of the method to go along with it)

re: #115.3

"what does "complete part" mean?" - a path part is basically the string between a pair of slashes (or what you get by exploding on '/'). So a complete part is tying to emphasize that Drupal only supports a path like /my/{foo}/edit Where the {foo} wildcard/variable is the complete thing between slashes, but Drupal doesn't support /my/{foo}{bar}/edit or /my/{foo}-yow/edit even though it is supported by Symfony.

This does point out a basic mismatch between what we are doing and the Symfony code. Ideally we should stop using a lot of that code and optimize for what we actually want, but that's a bit of a project.

With some test additions to verify that routes added and altered in the dynamic and later events have paths converted to lower case.

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -59,7 +60,7 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
     $fields = array(
       'source' => $source,
-      'alias' => $alias,
+      'alias' => Unicode::strtolower($alias),
       'langcode' => $langcode,
     );
 
@@ -98,6 +99,9 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO

@@ -98,6 +99,9 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
   public function load($conditions) {
     $select = $this->connection->select('url_alias');
     foreach ($conditions as $field => $value) {
+      if ($field == 'alias') {
+        $value = Unicode::strtolower($value);
+      }
       $select->condition($field, $value);
     }

API wise I would have expected that the source would be lowercased as well given that its the actual internal path.

@dawehner - the source of a path alias may include a case-sensitive variable value (e.g. the action ID in an action edit link) so we cannot change those to lower case.

However, it doesn't matter, since if the path processor replaces the incoming aliases with a mixed-case link, the case-insensitive code in the router still resolves it correctly.

Alright. This particular issue doesn't cause the failure sin sqlite/pgsql so I consider this as done.

I think we need an update path to update at least the path aliases.

Given that its not system critical we could use a post_update and just resave all the aliases and be done.

Plus update function and update test.

I think we can use a post_update in which firing hooks is not a problem.

ok

+++ b/core/modules/system/system.post_update.php
@@ -31,5 +31,37 @@ function system_post_update_fix_enforced_dependencies() {
+    }
+    $aliases = $connection->queryRange('SELECT pid, source, alias, langcode FROM {url_alias} ORDER BY pid ASC', $sandbox['current'], $sandbox['current'] + 50)
+      ->fetchAllAssoc('pid', PDO::FETCH_ASSOC);
+

sad world that there is no API for that

+++ b/core/modules/system/src/Tests/Update/PathAliasUpdateTest.php
@@ -0,0 +1,56 @@
+   */
+  protected function setDatabaseDumpFiles() {
+    $this->databaseDumpFiles = [
+      __DIR__ . '/../../../tests/fixtures/update/drupal-8.bare.standard.php.gz',
+    ];
+  }
...
+    $connection = \Drupal::database();
+    $schema = $connection->schema();
+
+    if (!$schema->tableExists('url_alias')) {
+      return;
+    }
+    $fields = array(
+      'source' => '/user',
+      'alias' => '/User/PAge',
+      'langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED,
+    );
+    $query = $connection->insert('url_alias')
+      ->fields($fields);
+    $pid = $query->execute();
+    $this->runUpdates();

Do you mind putting that into a dump file? Its not like we came up with that for fun :)

ok, also fixed Drupal\system\Tests\Update\UpdatePostUpdateTest

Thanks

Tagging rc deadline because I'm not entirely confident this can go in during RC. Just because it can go into a patch release doesn't always mean it is RC-able.

+++ b/core/modules/system/system.post_update.php
@@ -31,5 +31,37 @@ function system_post_update_fix_enforced_dependencies() {
+    $aliases = $connection->queryRange('SELECT pid, source, alias, langcode FROM {url_alias} ORDER BY pid ASC', $sandbox['current'], $sandbox['current'] + 50)

Does the test database have more than 50 aliases in it? If not I think we should add some.

Additionally we should check that something at the beginning and end of the batch has been updated.

I ask this because the last time we had a batched update in core it only updated the first 50 and left the rest.

Added more aliases - now it has 101 to update, and we check each one.

Simple re-roll needed

Its kinda crazy how much we test to be honest.

Personally, I'm +1 to this, but I'd rather @catch make the call on if and when this goes in, so assigning to him. Also raising to Critical, because this affects data within Drupal enough that I want to make sure it gets explicitly evaluated prior to RC than accidentally forgotten.

+++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
@@ -263,7 +263,7 @@ public function alterRoutes(RouteCollection $collection) {
         // We assume that the numeric ids of the parameters match the one from
         // the view argument handlers.
-        foreach ($parameters as $position => $parameter_name) {
+        foreach (array_values($parameters) as $position => $parameter_name) {

Given the code comment, why this change?

@effulgentsia - I made the code change there since Symfony indexes them sequentially, and the array_values() call gives you sequential positions.

I didn't understand the views code well enough to tell why the views argument handlers are numbered sequentially rather than by path position, but given that this is the only existing use of that compiled route method in core, this seemed like the easiest fix.

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
@@ -143,6 +152,19 @@ public function getRequirements() {
   /**
+   * Returns the path variables.
+   *
+   * This differs from the parent method in that the numeric array keys
+   * correspond to a matching path part containing that variable.
+   *
+   * @return array
+   *   The path variable names keyed by numeric path part.
+   */
+  public function getPathVariables() {
+    return $this->mappedPathVariables;
+  }
+

As alternative you could have introduced an additional method.

1. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   @@ -72,6 +80,7 @@ public function __construct($fit, $pattern_outline, $num_parts, $staticPrefix, $
   +    $this->mappedPathVariables = $pathVariables;
   
   @@ -143,6 +152,19 @@ public function getRequirements() {
      /**
   +   * Returns the path variables.
   +   *
   +   * This differs from the parent method in that the numeric array keys
   +   * correspond to a matching path part containing that variable.
   +   *
   +   * @return array
   +   *   The path variable names keyed by numeric path part.
   +   */
   +  public function getPathVariables() {
   +    return $this->mappedPathVariables;
   +  }
   

   This part I don't understand. We set $pathVariables to $this->mappedPathVariables, but the parent::__construct() call sets it to $this->pathVariables as well, which is returned by getPathVariables() by default. So there really is no need to override the method, because it's the same thing. Which means in turn it's completely unnecessary to have the $this->mappedPathVariables variable because it too is completely duplicated. What am I missing?

2. +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
   @@ -64,6 +64,34 @@ public static function compile(Route $route) {
   +   * Drupal only supports path wildcard (variables) that consist of a complete
   +   * part of the path separated by slashes.
   ...
   +  public static function getMappedPathVariables($path, array $variables) {
   

   Would be nice to add a @see to Symfony's RouteCompiler::compilePattern() which - unless I'm mistaken - seems to be where Symfony does it's variable mapping without this requirement.

3. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +47,82 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +   * First, the $pathinfo string is converted to lowercase. In addition, we
   ...
   +      if (!preg_match($compiledRoute->getRegex(), Unicode::strtolower($pathinfo), $matches)) {
   

   This does not seem to be entirely correct. $pathinfo is only converted to lowercase in a single check, not entirely. It is then passed on as-is afterwards. Can we clarify this?

4. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +47,82 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +      // Recover the original value for wildcard (named variable) portions
   +      // of the path, since they may be case-sensitive data like a base64
   +      // encoded token.
   +      $parts = preg_split('@/+@', $pathinfo, NULL, PREG_SPLIT_NO_EMPTY);
   +      foreach ($compiledRoute->getPathVariables() as $position => $variable_name) {
   +        if (isset($matches[$variable_name])) {
   +          $matches[$variable_name] = $parts[$position];
   +        }
   +      }
   

   Here a @see to Drupal's RouteCompiler::getMappedPathVariables() would be nice, which is where the reverse is happening, unless I'm mistaken.

5. +++ b/core/modules/system/src/Tests/Routing/RouterTest.php
   @@ -212,6 +212,71 @@ public function testRouterMatching() {
   +    $this->assertRaw('test2', 'The correct string was returned because the route was successful.');
   ...
   +    $this->assertRaw('test2', 'The correct string was returned because the route was successful.');
   ...
   +    $this->assertRaw('test2', 'The correct string was returned because the route was successful.');
   ...
   +    $this->assertRaw('mixedCASEstring', 'The correct string was returned because the route was successful.');
   ...
   +    $this->assertRaw('mixedcasestring', 'The correct string was returned because the route was successful.');
   ...
   +    $this->assertRaw('MIXEDCASESTRING', 'The correct string was returned because the route was successful.');
   

   Minor, but these assertion messages are not very helpful, nor really accurate. I.e. what does it mean for a route to be successful?

6. +++ b/core/modules/system/tests/modules/router_test_directory/src/RouteTestSubscriber.php
   @@ -7,21 +7,82 @@
   -class RouteTestSubscriber extends RouteSubscriberBase {
   +class RouteTestSubscriber implements EventSubscriberInterface {
   

   Not really important, but this seems unnecessary? Seems totally fine to override getSubscribedEvents() and then just add RoutingEvents::DYNAMIC, but then we still have alterRoutes(), which is a bit nicer.

7. +++ b/core/modules/views/src/Plugin/views/display/PathPluginBase.php
   @@ -263,7 +263,7 @@ public function alterRoutes(RouteCollection $collection) {
   -        foreach ($parameters as $position => $parameter_name) {
   +        foreach (array_values($parameters) as $position => $parameter_name) {
   

   I don't understand this, as $parameters comes from $route->compile()->getPathVariables() so it seems the key would be important? Also I guess this underlines the fact that ::getMappedPathVariables() is in fact not necessary?!

Edit, the previous one was a *massive* crosspost with #120 (which is also the patch I reviewed)

This is not critical, I do think it blocks a meaningful migration path from Drupal 6 per #2075889-11: Make Drupal handle incoming paths in a case-insensitive fashion for routing so tagging for that. With hindsight we should have rolled back the patch that introduced the bug at the time.

I'll try to discuss this issue with the other committers today as to whether we commit it before or during RC, which I think was the intent of effulgentsia bumping it to critical.

It would also be useful to know whether this really is viable for a patch release or not.

#1 and #7 both look substantive, so bumping back to CNR.

re: #145
.1) Yes, this is unnecessary in terms of actual functionality, it's just serving to document the behavior of the keys. I could also just call parent:: to do that

.7) Before this path it happens that Symfony simply makes an incrementing numeric array. There is actually no guarantee of that, so the Views change makes that core more robust/correct in any case.

Re 1) Then please let's just call parent:: and remove the additional variable. It makes sense to keep the function for the docs, but the additional variable is unnecessarily confusing IMO.

Don't really grok the reply to 7) but this shouldn't be held up on me understanding something. I guess an inline comment wouldn't hurt but I won't be a stickler about that.

re: #145
.1) changed
.2) added
.3) changed
.4) added
.5) changed messages
.6) left as-is, since the base class really provides no value here.
.7) Added comment as minor code re-organization.

small nitpick - can be fixed on commit

+++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
@@ -143,6 +144,27 @@ public function getRequirements() {
+   * The Drupal implementation differs from from the parent class in that the

'from from'

doh. fixed.

@pwolanin thanks a lot. Also grok the Views thing now. Thanks for the comment! It's not for me to RTBC this, but I have no further comments. Looks good to me.

Pretty easy.

Oh, bah - I didn't see that the $route variable gets re-assigned within there. That code needs some cleanup, but out of scope for this issue.

Yeah that is not that nice to be honest. The bug fix I applied is minimal (just move around some code) so I set this back to RTBC.

1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
   @@ -208,7 +215,7 @@ public function lookupPathSource($path, $langcode) {
   +      ->condition('alias', Unicode::strtolower($alias))
   

   Why not make this a LIKE condition which is case insensitive anyway?

2. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
   @@ -234,7 +241,7 @@ public function getAliasesForAdminListing($header, $keys = NULL) {
   +      $query->condition('alias', '%' . preg_replace('!\*+!', '%', Unicode::strtolower($keys)) . '%', 'LIKE');
   

   LIKE is already case insensitive so why the strtolower() here?

3. +++ b/core/modules/locale/src/Tests/LocalePathTest.php
   @@ -81,7 +82,7 @@ public function testPathLanguageConfiguration() {
   +    $custom_language_path = Unicode::strtolower($this->randomMachineName(8));
   

   Don't we normalize this on save?

It's not necessarily true that LIKE is case insensitive, but it's hacked that way in the driver Condition class.

That said I think I prefer doing the explicit non case sensitivity (application) rather than implicit non case sensitivity (database).

Doesn't the case sensitivity of database searches depend on the collation settings for the database anyway?

It does but we use that everywhere else in core including critical places like usernames, and had about 5 issues spanning multiple years on how to handle it.

I'd generally have expected a few queries here to be converted to using LIKE() and not many other changes in general.

@catch - I'm confused. I thought from our discussion in IRC you agreed that using the database case insensitivity or LOWER() was not the right thing to do?

We could possibly handle the path alias matching using LIKE instead of forcing to lower case, but for the final match in the routing we are using a regular expression. Fixing this final match behavior was the trickiest part and it happens all in PHP. Also, for the SQL part of routing we use an IN query, so there is no way to make that case insensitive in general.

The cases in tests where we do things like:

$custom_language_path = Unicode::strtolower($this->randomMachineName(8));

*are* exactly because we lower case on save, it's easier to start with a lower case value for later comparison rather than lower casing it later for each comparison.

There are some tests that verify the case change, so we don't need to do it every time.

Here's a tweak to switch to using LIKE in the alias storage.

@catch - I'm confused. I thought from our discussion in IRC you agreed that using the database case insensitivity or LOWER() was not the right thing to do?

We were discussing the update - where we might want to make sure the updated data is exactly the same as it would have been if it was entered directly. I hadn't realised we were lowercasing everything in PHP everywhere else too.

In 7.x it's completely valid to have a path alias of 'Happy' for node/1 and when the URL is generated it will show 'Happy'.

Case insensitive matching for incoming paths doesn't require we force everything to lowercase in storage. This would also mean we don't need an update at all, or any of the string changes.

I'm not sure about routes though.

(I'm sorry if I'm stating things already discussed, I did not try to completely understand all 167 comments, even though I did read through all of them, but I did try to look up things to write a review that's as constructive as possible.)

1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
   @@ -59,7 +60,7 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
   -      'alias' => $alias,
   +      'alias' => Unicode::strtolower($alias),
   
   @@ -98,7 +99,8 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
   +      // Use LIKE for case-insensitive matching.
   +      $select->condition($field, $value, 'LIKE');
   

   It's strange to see both explicit lowercasing when storing and still doing case-insensitive queries when loading.

   I'd expect either:

   • explicit lowercasing both when saving and loading
   • case-insensitive loading

   I guess this goes back to #162.1 and the subsequent comments up to this one.

   Given #168, it seems it may be best to still store whatever alias the user enters, still generate URLs with those potentially uppercase aliases, but just lowercase them when doing actual routing?

   Currently, this patch does more than solving the problem at hand: upgrade path becomes painful, because an old site's uppercased aliases would result in 404s. Rather than only turning those 400s into 200s (which this does), it also prevents uppercased aliases altogether. Which may be sensible, but feels out of scope.

2. +++ b/core/lib/Drupal/Core/Routing/CompiledRoute.php
   --- a/core/lib/Drupal/Core/Routing/RouteBuilder.php
   +++ b/core/lib/Drupal/Core/Routing/RouteBuilder.php
   

   This forces our route-related events to use lowercase paths, sounds sane.

3. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -149,7 +150,9 @@ public function __construct(Connection $connection, StateInterface $state, Curre
        // Cache both the system path as well as route parameters and matching
   -    // routes.
   +    // 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.
   

   Not yet lowercasing here makes sense; we basically need to lowercase as late as possible. It is kinda confusing though (routes' paths are lowercase, at except for the dynamic bits in there), but I guess there's no other way without a huge BC break.

4. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
   @@ -157,9 +160,9 @@ public function getRouteCollectionForRequest(Request $request) {
   -      $path = $path === '/' ? $path : rtrim($request->getPathInfo(), '/');
   ...
   +      $path = $path === '/' ? $path : rtrim($path, '/');
   

   This seems like a separate bugfix that needs test coverage. Add test in a follow-up?

5. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -46,4 +47,83 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +   * This version differs from the Symfony parent version in two respects.
   +   * First, the $pathinfo string is converted to lowercase when matching the
   +   * route path regular expression. In addition, we remove the check against any
   

   So this is the "as late as possible", where lowercasing is appropriate.

6. +++ b/core/lib/Drupal/Core/Routing/routing.api.php
   @@ -43,7 +43,7 @@
   + *   base URL). Paths are handled with case-insensitive matching.
   

   I wonder if this is not an oversimplification then, given that we still support casing for e.g. base64 tokens?

7. +++ b/core/modules/system/system.post_update.php
   @@ -31,5 +31,37 @@ function system_post_update_fix_enforced_dependencies() {
    /**
   + * Load and save all path aliases to make sure they are lower case.
   + */
   +function system_post_update_path_alias_lowercase(&$sandbox = NULL) {
   

   So IMO, per my first point, none of this would/should be necessary.

Let me explicitly state what #169 implies: without the alias changes, this only breaks BC in a very minimal way (route paths can no longer use uppercase characters; if they did, they'd be lowercased automatically), and an upgrade path becomes unnecessary.

Ok, so we will remove the "rc deadline" and consider this for sometime in 8.0.x with no changes to alias storage other than making sure we have case insensitive matching?

Yes I think with no migration path this gets much less risky. Given it's an indirect migrate issue, making it RC target.

#171: I personally think that's the least disruptive solution, assuming I'm not overlooking a strong reason to lowercase aliases.

Yeah especially if we can get rid of some upgrade paths this would be wonderful, on less source of failure.

OK I'm a bit confused too about the case sensitivity issue. Here is what I think should happen, for optimal niceness and usability and kittens:

a) URL resolution should be case insensitive. So for example, if someone goes to URLs like:
example.com/fOo
example.com/FoO
These URLs should work the same, and both should match an alias in the database "foo" or "Foo" or whatever, or a path of "foo" or "Foo" or whatever.

b) We should allow users to enter paths (such as on Views UI when creating a feed or page) and aliases (on the aliases config page or when creating content) that contain upper-case, and we should preserve that when making URLs. So if they alias node/5 as "About", its URL should be example.com/About. And if they create a view display page and want its URL to be "AllTheVideos", they should be able to do that.

c) When someone is creating a path or alias in the UI, we should validate that there is not an existing path or alias that matches what they entered, and when checking this we should use case-insensitive matching. I think whether they're entering a path or an alias, we should check against both (e.g. they shouldn't be able to create an alias of "Admin" since path "admin" exists).

Is that the plan? If not, why not? And then what is the plan?

@jhogdon, I mostly agree except saving and rendering the path they created as mixed case may be problematic if we want to do a case insensitive match. Let me see if I can make it work. But on that point we may still need to force lower case. I don't think in Drupal 7 you would generally be able to create mixed case paths (as opposed to aliases)?

So basically I think to do what jhogdon thinks is correct we'd need to rewrite the route compiler to be our own custom code, maybe documenting that we don't support host patterns or any partial variables.

Is that worth the effort?

Here's a patch starting to convert alias storage to use LIKE queries. We have to use a select object to force the ILIKE in pgsql, I think.

Remove upgrade-related test code and revert lowercase calls in other tests.

If we go for rewriting our route compiler we'll need to significantly expand \Drupal\Tests\Core\Routing\RouteCompilerTest

I tested in Drupal 7:
- In Views you can definitely assign a mixed-case path to a page view display. For instance I made one with path "TeStInG".
- If you also in the same view add a menu item to the display, the menu item URL points to the mixed-case URL you designated.
- You can access the URL via paths testing, TESTING, etc. --- all go to the same page.
- After doing this, the alias page let me create an alias of 'testing'. I put one in with a path of 'node' and an alias of 'testing', and now both TeStInG and testing go to node instead of my view, so that isn't great.
- On the aliases page, I tried to add 2 aliases, one with alias "test1" and one with alias "TEST1". When I tried to save the second one, it said "TEST1" was in use.
- On the aliases page, I made an alias "AbCdE" for a node I had on the site. The "view" link for that node is now "AbCdE" so it respects my mixed-case alias, but "abcde" also will get me there.

So. I think D7 basically works like I suggested in #175.

The only difference is that Views doesn't seem to care/notice if I assign a page view a path that already exists, or that matches an alias that already exists. And alias creation checks for existing aliases that match (using case-insensitive match), but not for existing paths.

- After doing this, the alias page let me create an alias of 'testing'. I put one in with a path of 'node' and an alias of 'testing', and now both TeStInG and testing go to node instead of my view, so that isn't great.

There's an issue for this somewhere, will try to find later. Shouldn' affect anything here though.

There is an issue about taking over paths with aliases - that's a different problem.

Basically - I don't have much time for this now to release, so we might be able to fix up the code with forced lower-case paths to rtbc again, but supporting mixed case paths would need a couple other people to join in on working on the code and rewriting the RouteCompiler logic.

#184

The only difference is that Views doesn't seem to care/notice if I assign a page view a path that already exists, or that matches an alias that already exists

If I understand correctly this is by design at least for views, it allows you to replace core paths with views, might be less of an issue in D8

Taking all paths and aliases that people enter and saving them as lower-case just seems wrong... especially if D7 didn't do this.

#188 I agree a 100%, we should not change it, it will break contrib modules like for instance shurly

@jhogdon - please look at the last patch. It's now leaving aliases alone, but forcing actual route paths to be lower case.

@kgoel - this doesn't seem like the right fix - we are trying to make path aliases work in a case insensitive way at the DB query level.

The real problem was the changes in the base test code in core/modules/simpletest/src/AssertContentTrait.php where the path was forced to lower case. Not sure that should ever have been in the patch.

This interdiff is WRT #179

From #177:

maybe documenting that we don't support host patterns or any partial variables.

Ak, no. :-(

Didn't we move away from LIKE in queries because they're unindexed?

@Crell - not sure what you are saying "no" to. We already don't support partial path part variables, and I don't see much value to host tokens

Fix that failure by reverting the change to that test w.r.t. 8.0.x

Peter and I discussed further in IRC. To expand on my previous comment:

AFAIK we do support host-based routing. We got that as part of the Symfony router import. If we somehow managed to break it along the way, that's a problem with our test coverage. We should add test coverage to verify, and if it's not working now that's a bug we should fix. The same is true of scheme routing. The whole point of adopting a Symfony-based router was to support routing by arbitrary parts of the request, not just the path. Regressing on that now seems short-sighted, wasteful, and ill-advised.

If we're going to explicitly say that paths and aliases are all case-insensitive, then I don't see why it's a problem to fold their case. The net result is the same. Going to foo.com/About will still work as expected, but generated links to it will lower-case, which is more consistent anyway.

I don't see how modules like Shurly would be impacted. If path routing is case insensitive, then such modules could not have both /AbC and /abc anyway. And such modules would be issuing redirects, probably directly from the routing system (just create a route and wire it straight to a redirecting controller), so... I don't see why that's even relevant.

Didn't we move away from LIKE in queries because they're unindexed?

We moved away from LOWER() queries because they're unindexed, to LIKE.