Make Drupal handle path aliases in a consistent and case-insensitive fashion on all database drivers

+++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
@@ -135,8 +135,9 @@ public function languageAliasExists();
-   * @param string[]|null $keys
-   *   (optional) Search keys.
+   * @param string|null $keys
+   *   (optional) Search keyword that may include one or more '*' as a wildcard
+   *   value.

is this correcting incorrect docs, or is it changing the param?

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -140,19 +141,22 @@ public function preloadPathAlias($preloaded, $langcode) {
+    $select = $this->connection->select('url_alias');
+    $select->fields('url_alias', ['source', 'alias']);
+    $select->condition('source', $preloaded, 'IN');
     if ($langcode == LanguageInterface::LANGCODE_NOT_SPECIFIED) {
-      // Prevent PDO from complaining about a token the query doesn't use.
+      // Don't put the same value in the IN query twice.
       unset($args[':langcode']);
-      $result = $this->connection->query('SELECT source, alias FROM {url_alias} WHERE source IN ( :system[] ) AND langcode = :langcode_undetermined ORDER BY pid ASC', $args);
     }
     elseif ($langcode < LanguageInterface::LANGCODE_NOT_SPECIFIED) {
-      $result = $this->connection->query('SELECT source, alias FROM {url_alias} WHERE source IN ( :system[] ) AND langcode IN (:langcode, :langcode_undetermined) ORDER BY langcode ASC, pid ASC', $args);
+      $select->orderBy('langcode', 'ASC');
     }
     else {
-      $result = $this->connection->query('SELECT source, alias FROM {url_alias} WHERE source IN ( :system[] ) AND langcode IN (:langcode, :langcode_undetermined) ORDER BY langcode DESC, pid ASC', $args);
+      $select->orderBy('langcode', 'DESC');
     }
-
-    return $result->fetchAllKeyed();
+    $select->condition('langcode', $args, 'IN');
+    $select->orderBy('pid', 'ASC');
+    return $select->execute()->fetchAllKeyed();

@@ -160,23 +164,26 @@ public function preloadPathAlias($preloaded, $langcode) {
     );
     // See the queries above.
+    $select = $this->connection->select('url_alias');
+    $select->fields('url_alias', ['alias']);
     if ($langcode == LanguageInterface::LANGCODE_NOT_SPECIFIED) {

So i'm wondering whether we really need to switch from simple queries to an object. Is this a requirement to fix the issue?

Another alternative is to just have separate MySQL, Postgres, and SQLite implementations of the AliasStorage class, each optimized accordingly. That's fully supported in core and something we should take more advantage of. :-)

Another alternative is to just have separate MySQL, Postgres, and SQLite implementations of the AliasStorage class, each optimized accordingly. That's fully supported in core and something we should take more advantage of. :-)

+1
I like the idea of separate implementation. It's sounds very straight forward but don't know how easy/difficult it would be to implement this.

kgoel: Quite simple: https://www.palantir.net/blog/d8ftw-customizing-your-back-end

(That's assuming the challenge is cross-DB SQL. If that's not the issue that won't help.)

Looks like real fails on postgres and sqlite.

I discussed this with the other committers, and we decided to raise this to Critical for now.

Because, the upgrade path might require data loss on case-sensitive databases such as Sqlite. For example, on Sqlite, you can in both 7.x and 8.0.0-rc2 create "foo" and "Foo" as aliases to different paths. If we fix this issue in a way that starts disallowing that, then figuring out how to migrate such 7.x databases will be Migrate's responsibility, which is still flagged as an experimental module, so that's ok. But updating existing 8.x databases will be hook_update_N()'s responsibility. And making such an update function delete semi-duplicate-but-not-really aliases might be more palatable during RC than after people deploy sites on 8.0.0. For example, a similar kind of deletion of same-except-for-case records in #1260938: d6 to d7 update fails on file duplicates '#7061 Integrity constraint violation' ended up taking a very long time to land.

That said, if we don't solve this relatively quickly, we may choose to downgrade it rather than hold up 8.0.0's release, but that's a discussion/decision for a later date. Hopefully, it won't come to that.

+++ b/core/modules/path/src/Tests/PathAliasTest.php
@@ -82,6 +83,13 @@ function testAdminAlias() {
+    // Confirm that the alias works in a case-insensitive way.
+    $this->drupalGet(Unicode::strtolower($edit['alias']));
+    $this->assertText($node1->label(), 'Alias works lower case.');
+    $this->assertResponse(200);
+    $this->drupalGet(Unicode::strtoupper($edit['alias']));
+    $this->assertText($node1->label(), 'Alias works upper case.');
+    $this->assertResponse(200);

@@ -93,7 +101,7 @@ function testAdminAlias() {
     // Confirm that the alias works.
-    $this->drupalGet($edit['alias']);
+    $this->drupalGet(Unicode::strtolower($edit['alias']));
     $this->assertText($node1->label(), 'Changed alias works.');
     $this->assertResponse(200);
 
@@ -225,7 +233,7 @@ function testNodeAlias() {

@@ -225,7 +233,7 @@ function testNodeAlias() {
     $this->drupalPostForm('node/' . $node1->id() . '/edit', $edit, t('Save'));
 
     // Confirm that the alias works.
-    $this->drupalGet($edit['path[0][alias]']);
+    $this->drupalGet(Unicode::strtolower($edit['path[0][alias]']));
     $this->assertText($node1->label(), 'Changed alias works.');

Just a general comment, it would be nice to also test all the various changed methods like save() / delete() etc ... lookupPathSource and aliasExists

Doesn't need to be assigned to me.

Just to clarify my previous comment, the amount of additional tests does not really match yet the amount of changed functionality.

Do we actually need the case-insensitivity when looking up alias from source? That's not used for incoming paths.

This is a valid question.

public function aliasExists($alias, $langcode, $source = NULL) {
+ // Use LIKE and NOT LIKE for case-insensitive matching.
$query = $this->connection->select('url_alias')
- ->condition('alias', $alias)
+ ->condition('alias', $this->connection->escapeLike($alias), 'LIKE')
->condition('langcode', $langcode);
if (!empty($source)) {
- $query->condition('source', $source, '<>');
+ $query->condition('source', $this->connection->escapeLike($source), 'NOT LIKE');
}

So yeah that example is certainly not needed ... ideally we would distinct between what you use on a more storage API level
and what you use on runtime, which should have the case insensitive behaviour.

The alias lookup needs to be case insensitive, but the source lookup implies we're storing case insensitive source. Do we actually allow people to enter the source path in random case in 7.x? If we do then maybe that's needed.

We allow to add aliases in mix case in 6.x.

@pwolanin wouldn't we do that in #2075889: Make Drupal handle incoming paths in a case-insensitive fashion for routing ?

Tagging minor version target again, just to make it clear we can't fix this in a patch release.

Looking into the issue for a while

I think its fine to convert all API functions, its more consistent, though a better help text for the validation would be nice
Also adding documentation about the behaviour isn't a bad idea, as well as some general tests.

+++ b/core/modules/path/src/Form/PathFormBase.php
@@ -180,8 +180,16 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+        $form_state->setErrorByName('alias', t('There is an alias with other case-sensitivity: %actual_alias is already in use in this language.', ['%actual_alias' => $stored_alias['alias']]));

Hmm, this sounds a bit verbose, and maybe a bit unclear for some people ? Don't have a better proposal atm though.

Hmm, this sounds a bit verbose, and maybe a bit unclear for some people ? Don't have a better proposal atm though.

Yeah I certainly don't think its at all a good sentence, maybe someone has a better idea?

Fixed the failure.

Reviewing this and having a look at the sqlite/pgsql test failures

Some small changes, test coverage and an attempt at fixing the SQLite/PostgreSQL test failure.

I changed the db_query into a dynamic query as SQLite requires an ESCAPE '\' after the LIKE, which dynamic queries do automatically. I tried to hardcode the the ESCAPE '/' into the db_query but didn't succeed as I kept getting an unknown token error. We don't do LIKE x ESCAPE y anywhere else in core either so the dynamic query may be the best way to go?

I also suspect SQLite has a collation issue as it can't find the upper-case version of some non-ASCII characters (#1518506: Normalize how case sensitivity is handled across database engines) so skipping those in the test when using SQLite for now...

Also what about preloadPathAlias, we skip the case insensitivity there?

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -15,8 +15,9 @@
+ * fields, so the the aliases '/test-alias' and '/test-Alias' are considered

the the

The form error sounds a lot clearer to me already!

This adds case insensitivity to preloadPathAlias (do we need it though?), and adds a todo pointing to a followup I just opened at #2607432: SQLite driver does not allow for case insensitive LIKE comparisons on non-ASCII characters. It also cleans up the dynamic queries a bit.

I just noticed we already tried and then reverted using the query builder in #12 - does the approach in this patch pose a significant performance slowdown? We could go back to hardcoded SQL queries but SQLite has a problem with wildcards in the LIKE there (% and _ need to be escaped, which might require unnecessary changes), and in #12 we said doing database-specific implementations was not worth the complexity.

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -128,90 +144,103 @@ public function delete($conditions) {
     if ($langcode == LanguageInterface::LANGCODE_NOT_SPECIFIED) {
...
+      $select->condition('langcode', LanguageInterface::LANGCODE_NOT_SPECIFIED);
...
     elseif ($langcode < LanguageInterface::LANGCODE_NOT_SPECIFIED) {
...
     if ($langcode == LanguageInterface::LANGCODE_NOT_SPECIFIED) {
...
     elseif ($langcode > LanguageInterface::LANGCODE_NOT_SPECIFIED) {
...
     if ($langcode == LanguageInterface::LANGCODE_NOT_SPECIFIED) {
...
     elseif ($langcode > LanguageInterface::LANGCODE_NOT_SPECIFIED) {

Should we use $langcode_undetermined instead of LanguageInterface::LANGCODE_NOT_SPECIFIED for consistency?

@mradcliffe the original code didn't do so either despite having the variable available and I wanted to avoid unrelated changes, but other than that I don't see why not...

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -128,90 +145,106 @@ public function delete($conditions) {
    */
   public function preloadPathAlias($preloaded, $langcode) {
...
    */
   public function lookupPathSource($path, $langcode) {

So yeah just those 2 queries potentially runs often, on the other hand due to our caching, I guess we need to care less about it

@dawehner the difference between the dynamic query in lookupPathSource vs the db_query is about .3 ms on average on my slow laptop.

For the routing we only do the database query on the first page visit, right? Most pages have a few calls to Url::fromUri() as well, which goes through PathValidator::getPathAttributes() and then does a PathProcessorAlias::getPathByAlias(). So altogether I see some 3-4 calls to these newly introduced dynamic queries per page on average.

As to how to proceed here while still fixing SQLite, we could either use this patch, take the ~1ms performance hit and optimize in another release, or alternatively we could find a way to make LIKE x ESCAPE y work in a db_query.

Interdiff looks great, does anything still needs to happen to the current patch?

Why on earth did we add local variables for constants??

    $langcode_undetermined = LanguageInterface::LANGCODE_NOT_SPECIFIED;

is this expected to change? Is there a tax on the number of characters? Is there some incredible PHP bug we are coding around? I am so baffled.

This looks good to me. If we're really worried about the query building, then switching to database-specific implementations would do it, but then we should reconsider the LIKE/ILIKE optimization we have in the database layer, which is used for a lot of things other than path aliases like user names.

If I mark this RTBC I can't really commit it, but no complaints.

If I mark this RTBC I can't really commit it, but no complaints.

Easily solved :)

As a side note, it seems we are applying the same logic multiple times, what about creating a follow-up to factor it out to an helper method and reuse it?

Thank you peter and stefan

Removing tags now this is both critical and RTBC.

Committed/pushed to 8.0.x, thanks!