Follow-up from #2224777: Unlimited allowed length of path in menu router allows DoS attack on Drupal 8.

Problem/Motivation

Route of all fitness values are processed for regex match and passed through for final matching. In Drupal 7 the code ordered by fitness and had a limit 1, so we only got the optimal route.

In Drupal 8, we are trying to use a 2 element index, but it can never be used by MySQL because of the DESC ordering:

mysql> EXPLAIN SELECT name, route, fit FROM router WHERE pattern_outline IN('admin/structure', 'admin') ORDER BY fit DESC, name ASC;
+----+-------------+--------+-------+---------------------+---------------------+---------+------+------+-----------------------------+
| id | select_type | table  | type  | possible_keys       | key                 | key_len | ref  | rows | Extra                       |
+----+-------------+--------+-------+---------------------+---------------------+---------+------+------+-----------------------------+
|  1 | SIMPLE      | router | range | pattern_outline_fit | pattern_outline_fit | 767     | NULL |    2 | Using where; Using filesort |
+----+-------------+--------+-------+---------------------+---------------------+---------+------+------+-----------------------------+
1 row in set (0.00 sec)

Even changing to fit ASC and removing name we get a filesort - so we would be safer sorting in PHP.

Also, we don't have a web test to check that a request with many path parts doesn't cause a 503.

Proposed resolution

Fix Route Provider to do the query based on the number of parts, which is a condition we can add to the where clause and use an index

add a test.

Remaining tasks

review patch

User interface changes

N/A

API changes

none. This change is internal to the router and should not be disruptive to any other existing code.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because .it's a follow-up to a prior bug
Issue priority Major because it's in the critical path for every request
Prioritized changes The main goal of this issue is performance
Disruption Not disruptive for core/contributed and custom modules/themes
CommentFileSizeAuthor
#93 mergediff.txt1.34 KBneclimdul
#91 further_optimize-2228217-91.patch9.64 KBjosephdpurcell
#85 2228217-85.interdiff.txt3.25 KBneclimdul
#85 further_optimize-2228217-85.patch9.46 KBneclimdul
#82 further_optimize-2228217-82.patch9.36 KBneclimdul
#82 2228217-82.interdiff.txt2.42 KBneclimdul
#78 further_optimize-2228217-78.patch9.05 KBneclimdul
#78 2228217-78.interdiff.txt4.23 KBneclimdul
#69 increment.txt502 bytespwolanin
#69 2228217-69.patch7.68 KBpwolanin
#64 interdiff.txt430 byteslauriii
#64 further_optimize-2228217-64.patch7.67 KBlauriii
#55 microbench.php_.txt1.98 KBneclimdul
#51 increment.txt1.01 KBpwolanin
#51 2228217-51.patch7.67 KBpwolanin
#50 output.txt1.27 KBneclimdul
#50 microbench.php_.txt1.93 KBneclimdul
#47 microbench.php_.txt1.19 KBneclimdul
#41 increment.txt1.5 KBpwolanin
#41 2228217-41.patch7.73 KBpwolanin
#39 increment.txt3.47 KBpwolanin
#39 2228217-39.patch7.74 KBpwolanin
#33 2228217-33.patch9.16 KBpwolanin
#25 2228217-25.patch9.75 KBecrown
#19 2228217-19.patch11 KBpwolanin
#19 increment.txt859 bytespwolanin
#9 2228217-9.patch10.97 KBpwolanin
#9 increment.txt1.08 KBpwolanin
#8 2228217-7.patch10.72 KBpwolanin
#8 increment.txt7.55 KBpwolanin
#6 2228217-6.patch171.48 KBpwolanin
#6 increment.txt7.55 KBpwolanin
#5 2228217-5.patch4.95 KBpwolanin
#5 increment.txt3.54 KBpwolanin
#3 2228217-3.patch2.41 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue tags: -beta blocker

remove tag.

pwolanin’s picture

Issue summary: View changes
Issue tags: -Security improvements +Performance
pwolanin’s picture

Status: Active » Needs review
FileSize
2.41 KB

First patch, shifts route sorting to PHP so we never have a filesort, and reject any routes with less than maximal fitness.

Since having fit in the index provides on benefit, let's just remove it.

Status: Needs review » Needs work

The last submitted patch, 3: 2228217-3.patch, failed testing.

pwolanin’s picture

Assigned: Unassigned » pwolanin
Status: Needs work » Needs review
FileSize
3.54 KB
4.95 KB

Fixes the test case text and adds another.

Discussing with tim.plunkett, it seems that based on the interface and the regex match, we should not actually limit to max fitness, though that seems a bit weird to me.

So, the optimization (of sorts) that remains is doing the sort in PHP so that we avoid a filesort in mysql.

Remaining work is to add a web test for a very long path. I'll get to that shortly.

pwolanin’s picture

FileSize
7.55 KB
171.48 KB

This moves code around in the RouterTest test- no actual changes, so you can see the additions in the next patch.

Turns a hard-to-read function call with closure to a simple foreach.

Alters some logic in the dumper and the route provider so we can skip the regex while getting basically the same 1st pass filtering of routes.

Status: Needs review » Needs work

The last submitted patch, 6: 2228217-6.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
7.55 KB
10.72 KB

oops, diffed against 8.x instead of origin/8.x - ignore #6 since it won't apply

pwolanin’s picture

FileSize
1.08 KB
10.97 KB

Adds test for long paths.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -291,22 +295,39 @@ protected function getRoutesByPath($path) {
    +    // The >= check on number_parts allows us to match routes with optional
    +    // trailing wildcard parts as long as the pattern matches, since we
    +    // dump the route pattern without those optional parts.
    +    $routes = $this->connection->query("SELECT name, route, fit FROM {" . $this->connection->escapeTable($this->tableName) . "} WHERE pattern_outline IN (:patterns) AND number_parts >= :count_parts", array(
    +      ':patterns' => $ancestors, ':count_parts' => count($parts),
         ))
    

    Shouldn't we also optimize getCandidateOutlines() now? Because it returns ancestors with different length, which are excluded by :count_parts anyway and can never match? So in getCandidateOutlines() we should check the length of the binary digits and if it does not match we can exclude the mask. We are already doing that for masks that are longer than the path, so I think we can now also do that for masks that are shorter than the current path?

  2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -291,22 +295,39 @@ protected function getRoutesByPath($path) {
    +    // We sort by fit and name in PHP to avoid a SQL filesort.
    +    usort($routes, array($this, 'routeProviderRouteCompare'));
    

    Why? What is bad about an SQL filesort? Please expand the comment.

  3. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -291,22 +295,39 @@ protected function getRoutesByPath($path) {
       /**
    +   * Comparison function for usort on routes.
    +   */
    +  public function routeProviderRouteCompare(array $a, array $b) {
    

    Bad naming. The method name should tell us what it does, not where it is used. Suggestions: sortRoutesByFitness() or compareRoutesByFitnees().
    Same for the comment, suggestion: "Comparison callback for sorting routes by fitness."

  4. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -291,22 +295,39 @@ protected function getRoutesByPath($path) {
    +    // Reverse sort from highest to lowest fit.
    +    return (int) $a['fit'] < (int) $b['fit'] ? 1 : -1;
    

    Why do we have to cast to "int" here? We can always be sure that we have numbers as fitness, right? Is this for robustness? Please add a comment.

The last submitted patch, 8: 2228217-7.patch, failed testing.

The last submitted patch, 9: 2228217-9.patch, failed testing.

pwolanin’s picture

postponed on https://drupal.org/node/1366020 which should fix the search module test fails by moving keywords out of the path to a query string.

pwolanin’s picture

The cast to int for $a['fit'] is because all values that come from the database are actually PHP strings.

We cannot optimize the candidate outlines now, since we need to match routes with trailing optional path parts - this is why we have the >= check in the query.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -291,22 +295,39 @@ protected function getRoutesByPath($path) {
+    // We sort by fit and name in PHP to avoid a SQL filesort.
+    usort($routes, array($this, 'routeProviderRouteCompare'));

I somehow doubt that we actually still need the order by name. This was introduced when we had no proper router filter mechanism for accept headers etc. In the longrun it feels OK to skip that ORDER.

 SELECT name, route FROM router WHERE pattern_outline IN ('foo') ORDER BY fit DESC; 
+----+-------------+--------+------+---------------------+---------------------+---------+-------+------+-------------+
| id | select_type | table  | type | possible_keys       | key                 | key_len | ref   | rows | Extra       |
+----+-------------+--------+------+---------------------+---------------------+---------+-------+------+-------------+
|  1 | SIMPLE      | router | ref  | pattern_outline_fit | pattern_outline_fit | 767     | const |    1 | Using where |
+----+-------------+--------+------+---------------------+---------------------+---------+-------+------+-------------+
pwolanin’s picture

@Dawehner - the result must be deterministic, so I think it's important to sort by name also.

klausi’s picture

@pwolanin: but PHP converts strings containing numbers for us automatically when using ">" operators, so the cast should not be necessary?

pwolanin’s picture

@klausi. Hmm, ok. Shows how little I trust PHP is be sane :)
http://www.php.net/manual/en/language.operators.comparison.php

However, it seems like there is no guarantee what it will do with the route names.

pwolanin’s picture

FileSize
859 bytes
11 KB
jhodgdon’s picture

Status: Postponed » Needs review
dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -279,10 +279,14 @@ public function getRoutesByPattern($pattern) {
    -    // filtered out by empty().
    -    $parts = array_values(array_filter(explode('/', $path), function($value) {
    -      return $value !== NULL && $value !== '';
    -    }));
    +    // filtered out by empty(). Values may be empty if the incoming path
    +    // has multiple slashes in a row, or a leading or trailing slashes.
    +    $parts = array();
    +    foreach (explode('/', $path) as $value) {
    +       if ($value !== NULL && $value !== '') {
    +         $parts[] = $value;
    +       }
    +    }
     
    

    This is an unneeded change for this issue as well as micro optimization. There will be people which actually want to use anonymous functions because they think it is more readable.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterTest.php
    @@ -36,12 +36,9 @@ public static function getInfo() {
    -  }
     
    -  /**
    -   * Confirms that our default controller logic works properly.
    -   */
    -  public function testDefaultController() {
    
    @@ -107,18 +118,21 @@ public function testControllerPlaceholdersDefaultValuesProvided() {
     
    -  /**
    -   * Checks that dynamically defined and altered routes work correctly.
    -   *
    -   * @see \Drupal\router_test\RouteSubscriber
    -   */
    -  public function testDynamicRoutes() {
    

    Can't we at least keep some doTestDefaultController methods? Keeping them separated by method makes it much better to read.

Status: Needs work » Needs review

ecrown queued 19: 2228217-19.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 19: 2228217-19.patch, failed testing.

ecrown’s picture

Issue tags: +SprintWeekend2015

i am working on this issue

ecrown’s picture

FileSize
9.75 KB

This is a re-roll of the patch at #19

ecrown’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: 2228217-25.patch, failed testing.

Status: Needs work » Needs review

ecrown queued 25: 2228217-25.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2228217-25.patch, failed testing.

Status: Needs work » Needs review

ecrown queued 25: 2228217-25.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2228217-25.patch, failed testing.

mgifford’s picture

Assigned: pwolanin » Unassigned

Just unassigning issues that haven't been developed for a bit in the D8 queue.

pwolanin’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
9.16 KB

re-roll, but need an update function for index change and to address dawehner's comments.

Status: Needs review » Needs work

The last submitted patch, 33: 2228217-33.patch, failed testing.

Fabianx’s picture

Fabianx’s picture

Looks great to me, not able to RTBC, but I like the approach to remove the not-useful SQL sort here and add another condition.

I assume index already takes into account number_parts?

pwolanin’s picture

@Fabianx - the patch adds the number of parts to the index, so that's why an update function is needed

pwolanin’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
3.47 KB

fixed test for changed in class constructor, add update hook, roll out test changes not essential for the issue.

pwolanin’s picture

Issue summary: View changes
pwolanin’s picture

FileSize
7.73 KB
1.5 KB

A couple code comment fixes.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Could we fill out the disruption section of the beta evaluation.

From the parts I do understand this is RTBC and test coverage looks great, but I can neither confirm nor deny if this needs a change record and how contrib is or is not affected.

Thanks!

neclimdul’s picture

Status: Reviewed & tested by the community » Needs review

Neat!

  1. +++ b/core/lib/Drupal/Core/Routing/RouteCompiler.php
    @@ -43,7 +43,9 @@ public static function compile(Route $route) {
    -    $num_parts = count(explode('/', trim($pattern_outline, '/')));
    +    // We count the number of parts including any optional trailing parts. This
    +    // allows the RouteProvider to filter candidate routes more efficiently.
    +    $num_parts = count(explode('/', trim($route->getPath(), '/')));
    

    This looks like maybe a bug fix? Not really clear what this change is accomplishing.

  2. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -299,10 +299,14 @@ public function getRoutesByPattern($pattern) {
    -    // filtered out by empty().
    -    $parts = array_values(array_filter(explode('/', $path), function($value) {
    -      return $value !== NULL && $value !== '';
    -    }));
    +    // filtered out by empty(). Values may be empty if the incoming path
    +    // has multiple slashes in a row, or a leading or trailing slash.
    +    $parts = array();
    +    foreach (explode('/', $path) as $value) {
    +       if ($value !== NULL && $value !== '') {
    +         $parts[] = $value;
    +       }
    +    }
    

    Didn't dawehner ask for this change to be removed in #21.1? I find the closure easier to read personally so unless there's a appreciable performance gain can we do that?

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

The change from pattern outline to number of parts is deliberate so we include the count of optional parts. Otherwise, if you have a path with 2 optional trailing parts, and you supply the 1st but not the second the route wouldn't match.

I personally find the closure + filter not readable, so I think the foreach is much more readable and self-evident in what's happening in that we are assembling an array of the non-empty parts. So, I do not think it should be changes as in the patch. I don't think clever code is necessarily clear or better code.

pwolanin’s picture

Issue summary: View changes
Fabianx’s picture

Thanks for filling out disruption section, I am also +1 to remove the closure. Is is not only more readable, but also slightly more performant and this is in the critical path.

neclimdul’s picture

FileSize
1.19 KB
+++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
@@ -299,10 +299,14 @@ public function getRoutesByPattern($pattern) {
-    // filtered out by empty().
-    $parts = array_values(array_filter(explode('/', $path), function($value) {
-      return $value !== NULL && $value !== '';
-    }));
+    // filtered out by empty(). Values may be empty if the incoming path
+    // has multiple slashes in a row, or a leading or trailing slash.
+    $parts = array();
+    foreach (explode('/', $path) as $value) {
+       if ($value !== NULL && $value !== '') {
+         $parts[] = $value;
+       }
+    }

Well, not warm cache critical path. This only gets hit when caches are cold and the path hasn't been routed yet.

If we're going to optimize it, why not just handle it with string operations?

// Filter duplicate slashes and leading or trailing slashes. These would lead
// to empty path parts.
$clean_path = trim(str_replace('//', '', $path), '/');
$parts = explode('/', $clean_path);

Comments and code are both clearer IMHO.

Quick benchmark script with a largish path. You can adjust it with a smaller or larger path.

RouteProvider::getRoutesByPath():
str_replace and trim: 5.86
Closure: 68.08
Loop: 7.4
neclimdul’s picture

PHP 7 is a bit more dramatic getting close to twice as fast. Previous benchmark was Ubuntu's build of PHP 5.6.4.

~/tmp/usr/bin/php microbench.php.txt
RouteProvider::getRoutesByPath():
str_replace and trim: 2.85
Closure: 8.16
Loop: 4.28
pwolanin’s picture

@neclimdul. The string operation fails for foo//bar, so let's leave it as the loop.

To do it correctly as string operations you need to use preg_replace() I think.

neclimdul’s picture

FileSize
1.93 KB
1.27 KB

typo in the str_replace but foo///bar would still have failed.

Did some more benches, peter suggested preg_split, we bounced some tests around. Look preg_split is a pretty solid winner when sending abusive paths. The closure is miserable in those cases too so I'd say its actually good we replace that with something.

pwolanin’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.67 KB
1.01 KB

Here's with preg_split

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC if tests pass.

neclimdul’s picture

+1 RTBC

Crell’s picture

Concur with RTBC, as there's even some new tests to ensure nothing breaks.

That said, re #47/ #48, if you're seeing that dramatic a difference between a loop and a closure it probably means you have xdebug on. XDebug does very unpleasant things to the performance of stack calls. At least on 5.4, the last time I benchmarked it, the difference without XDebug was measurable but not enough to care about. Order of magnitude difference means XDebug is on, so benchmarks are not reliable at all.

neclimdul’s picture

FileSize
1.98 KB

Better data:

#  php -v
PHP 5.6.4-4ubuntu6.2 (cli) (built: Jul  2 2015 15:29:28) 
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2014 Zend Technologies
    with Zend OPcache v7.0.4-dev, Copyright (c) 1999-2014, by Zend Technologies
# php -i | grep xdebug
# php microbench.php
RouteProvider::getRoutesByPath():
Path: 
Closure: 91.85
Loop: 43.18
preg_split: 43.05

Path: /0/1/ ///2/
Closure: 276.01
Loop: 150.26
preg_split: 109.77

Path: d
Closure: 107.04
Loop: 50.07
preg_split: 51.38

Path: d/d
Closure: 135.47
Loop: 65.72
preg_split: 66.57

Path: d/d/d
Closure: 164.11
Loop: 82.39
preg_split: 78.89

Path: d//d///d
Closure: 230.97
Loop: 121.73
preg_split: 81.77

Path: /d/d/d/d////d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d
Closure: 791.71
Loop: 438.42
preg_split: 338.08

Path: /d/d/d/d///////////////////////////////////////////d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d
Closure: 1550.91
Loop: 820.22
preg_split: 319

Path: d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d/d
Closure: 7014.68
Loop: 3989.67
preg_split: 3300.12

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 51: 2228217-51.patch, failed testing.

neclimdul’s picture

Status: Needs work » Reviewed & tested by the community

Looks random:

Segmentation fault
FATAL Drupal\user\Tests\UserCancelTest: test runner returned a non-zero error code (139).

Fabianx queued 51: 2228217-51.patch for re-testing.

alexpott’s picture

I thought about asking for an upgrade path test but considering it is so simple I think it's not worth it.

Committed 7d5e65d and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 7d5e65d on 8.0.x
    Issue #2228217 by pwolanin, ecrown, neclimdul, Fabianx, dawehner, klausi...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
alexpott’s picture

Status: Fixed » Needs work

Should have asked for that update test.

Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key was too long; max key length is 767 bytes: ALTER TABLE {router} ADD INDEX `pattern_outline_parts` (`pattern_outline`, `number_parts`); Array ( ) in db_add_index() (line 659 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/includes/database.inc).

reverting.

  • alexpott committed bc62529 on 8.0.x
    Revert "Issue #2228217 by pwolanin, ecrown, neclimdul, Fabianx, dawehner...
lauriii’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
7.67 KB
430 bytes

This is how I fixed this earlier when I was working on other upgrade path and this was blocking me to proceed.

alexpott’s picture

+++ b/core/modules/system/system.install
@@ -946,7 +946,7 @@ function system_schema() {
+      'pattern_outline_parts' => array('pattern_outline', 'number_parts'),

Need to fix this too

pwolanin’s picture

Doe we support UTF-8 patterns or do routes, or could we make that an ASCII column?

Crell’s picture

I'm not sure. Sounds like something we need a test for. I don't think we can require ASCII-only paths, though, as that would be rather rude to UTF-8 path using international users. (UTF-8 domain names and paths are a legit thing according to IETF and ICANN, even if they're rarely used outside of China.)

pwolanin’s picture

Domain names don't really matter here, and in paths the request should be URL encoded if UTF-8, but I suspect PHP gets the decoded strong which would have the UTF-8 chars.

pwolanin’s picture

Here's the schema fix

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

Crell’s picture

Status: Reviewed & tested by the community » Needs work

191 is a magic number. In context I have NFI what it means or why it is. It needs either a constant or a comment. (Or Constant Comment, if you're a tea drinker.)

neclimdul’s picture

be prepared to see it a lot. mysql max index length is 767 bytes. 767/4 = 191. Which means the max varchar you can index is 191 characters long or you need to do this hack.

I think the better question is why isn't this working and we need to hard code it?
http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Database/Dri...

Crell’s picture

A comment in the code to that effect with a @see would be sufficient. Character encoding bites (pun intended), but we need to at least document with a reference where there's a magic number. (A constant is the other option if it's logical and reasonable to do, as then the constant can have the documentation.)

neclimdul’s picture

My point was I'm not sure the changes in #64and #69 are actually not correct. #51 seems like it was actually fine because we should not be hard coding assumptions about mysql into our database schema. e.g. I don't think postgres has this limitation and could index the full field.

There seems to be a bug in the Database API that would be a blocker for this issue. The linked code is from #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) and was clearly written to handle this case, it just seems from alex's comment that it isn't. Likely since testbot didn't catch this, db_add_index() doesn't trigger the code and so is bugged.

edit: typo

neclimdul’s picture

neclimdul’s picture

neclimdul’s picture

Assigned: Unassigned » neclimdul
Status: Postponed » Needs work

Unblocked. Doing a quick reroll of the previously committed patch.

neclimdul’s picture

Status: Needs work » Needs review
FileSize
4.23 KB
9.05 KB

Something like this maybe. I'm not sure how to assert that the index was applied without Database API supporting schema introspection which I'm under the impression it currently doesn't (hence the delay on the patch that blocked this).

neclimdul’s picture

Sorry for the weirdness in the interdiff. its a mergediff (update function name conflict) + spec addition to index add + the test kinda all in one.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
  1. +++ b/core/modules/system/src/Tests/Routing/RouteProviderTest.php
    @@ -316,7 +316,46 @@ function testOutlinePathMatchDefaultsCollision2() {
    -      $this->assertNull($routes->get('eep'), 'Noin-matching route was not found.');
    +      $this->assertNull($routes->get('eep'), 'Non-matching route was not found.');
    

    Wasn't Noin one of the Dwarfs from The Hobbit?

catch’s picture

Status: Reviewed & tested by the community » Needs work
I'm not sure how to assert that the index was applied without Database API supporting schema introspection which I'm under the impression it currently doesn't

It does for indexes, just not for anything else.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

    Also a few minor things:

  1. +++ b/core/lib/Drupal/Core/Routing/RouteProvider.php
    @@ -311,22 +309,37 @@ protected function getRoutesByPath($path) {
    +  public function routeProviderRouteCompare(array $a, array $b) {
    

    Why a public method and not a closure for the sort?

  2. +++ b/core/modules/system/src/Tests/Routing/RouterTest.php
    @@ -197,6 +197,15 @@ public function testRouterMatching() {
    +    $suffix = '/d/r/u/p/a/l';
    

    heh

  3. +++ b/core/modules/system/src/Tests/Update/RouterIndexOptimizationTest.php
    @@ -0,0 +1,35 @@
    +    $this->pass('Update did not fail.');
    

    Per-above this could check for the index.

  4. +++ b/core/modules/system/system.install
    @@ -1203,3 +1203,25 @@ function system_update_8001(&$sandbox = NULL) {
    +  db_drop_index('router', 'pattern_outline_fit');
    +  db_add_index('router', 'pattern_outline_parts', ['pattern_outline', 'number_parts'], [
    

    Both functions are deprecated.

neclimdul’s picture

1) seems likely. This has the possibility of being called a lot so this seems a reasonable micro-optimization.
2) :-D
3) Done
4) Sure, we don't provide a connection to updates so the alternative isn't a lot better though.

Note, I don't know if this will work. I'm getting failures locally on any update tests using drupal-8.bare.standard.php.gz.

neclimdul’s picture

Status: Needs work » Needs review

man I've been having trouble scrolling back up to change the status recently. sorry guys.

Status: Needs review » Needs work

The last submitted patch, 82: further_optimize-2228217-82.patch, failed testing.

neclimdul’s picture

I need a break from this issue. Me and simpletest are not on good terms right now.

neclimdul’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/system.install
@@ -1203,3 +1203,33 @@ function system_update_8001(&$sandbox = NULL) {
 
+/**
+ * Change the index on the {router} table.
+ */
+function system_update_8002() {
+  $database = \Drupal::database();
+  $database->schema()->dropIndex('router', 'pattern_outline_fit');
+  $database->schema()->addIndex(
+    'router',
+    'pattern_outline_parts',
+    ['pattern_outline', 'number_parts'],
+    [
+      'fields' => [
+        'pattern_outline' => [
+          'description' => 'The pattern',
+          'type' => 'varchar',
+          'length' => 255,
+          'not null' => TRUE,
+          'default' => '',
+        ],
+        'number_parts' => [
+          'description' => 'Number of parts in this router path.',
+          'type' => 'int',
+          'not null' => TRUE,
+          'default' => 0,
+          'size' => 'small',
+        ],
+      ],
+    ]
+  );
+}

This update exists - sorry.

dawehner’s picture

Issue tags: +Quickfix, +Novice

.

josephdpurcell’s picture

Working on an update now.

josephdpurcell’s picture

Status: Needs work » Needs review
FileSize
9.64 KB

Rerolling.

Conflict was:

<<<<<<< HEAD
 * Removes the system.filter configuration.
 */
function system_update_8002() {
  \Drupal::configFactory()->getEditable('system.filter')->delete();
  return t('The system.filter configuration has been moved to a container parameter, see default.services.yml for more information.');
=======
 * Change the index on the {router} table.
 */
function system_update_8002() {
  $database = \Drupal::database();
  $database->schema()->dropIndex('router', 'pattern_outline_fit');
  $database->schema()->addIndex(
    'router',
    'pattern_outline_parts',
    ['pattern_outline', 'number_parts'],
    [
      'fields' => [
        'pattern_outline' => [
          'description' => 'The pattern',
          'type' => 'varchar',
          'length' => 255,
          'not null' => TRUE,
          'default' => '',
        ],
        'number_parts' => [
          'description' => 'Number of parts in this router path.',
          'type' => 'int',
          'not null' => TRUE,
          'default' => 0,
          'size' => 'small',
        ],
      ],
    ]
  );
>>>>>>> wip

Resolution is:

/**
 * Removes the system.filter configuration.
 */
function system_update_8002() {
  \Drupal::configFactory()->getEditable('system.filter')->delete();
  return t('The system.filter configuration has been moved to a container parameter, see default.services.yml for more information.');
} 

/**
 * Change the index on the {router} table.
 */
function system_update_8003() {
  $database = \Drupal::database();
  $database->schema()->dropIndex('router', 'pattern_outline_fit');
  $database->schema()->addIndex(
    'router',
    'pattern_outline_parts',
    ['pattern_outline', 'number_parts'],
    [
      'fields' => [
        'pattern_outline' => [
          'description' => 'The pattern',
          'type' => 'varchar',
          'length' => 255,
          'not null' => TRUE,
          'default' => '',
        ],
        'number_parts' => [
          'description' => 'Number of parts in this router path.',
          'type' => 'int',
          'not null' => TRUE,
          'default' => 0,
          'size' => 'small',
        ],
      ],
    ]
  );
Crell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Quickfix, -Novice

An interdiff would have been sufficient. :-) Thanks.

neclimdul’s picture

FileSize
1.34 KB

Awesome thanks. Interdiff's are tricky when rerolling conflicts because the previous patch doesn't apply.

Tip: Since it looks like you where doing a merge which is also how I handle these as well, you can resolve the conflict and then do the diff. Git will give a mergediff which is pretty clear at showing both the conflict and resolution in one file. Example output for #91 attached.

Derp, I didn't rename the second function when making the mergediff but you get the point. #91 is correct.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Second time lucky! Committed 7730866 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 7730866 on 8.0.x
    Issue #2228217 by pwolanin, neclimdul, lauriii, ecrown, josephdpurcell,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.