Problem/Motivation

While investigating whether https://www.drupal.org/node/2642848 needs to be forward ported (it needs to) I found the call chain to whitelist rebuild be absurdly long and of course such a long call chain ought to contain a bug: system_path_delete does not pass the source so the whitelist is always rebuilt on alias delete. That's quite a performance hit.

Proposed resolution

Trival: pass the $path['source']

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 2645036_11.patch3.44 KBchx
#7 2645036_7.patch3.61 KBchx
#5 path_source.patch1.09 KBchx
#3 path_source.patch1020 byteschx
path_source.patch464 byteschx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx created an issue. See original summary.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g at Diputació de Barcelona commented
Status: Needs review » Reviewed & tested by the community
chx’s picture

chx CreditAttribution: chx at Tag1 Consulting commented
Title: Performance: system_path_delete doesn't pass the source » Performance: system_path_* doesn't pass the source
FileSize
path_source.patch1020 bytes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: path_source.patch, failed testing.

chx’s picture

chx CreditAttribution: chx at Tag1 Consulting commented
Status: Needs work » Needs review
FileSize
path_source.patch1.09 KB

Grr!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Component: other » system.module
Issue tags: +Performance, +Needs tests

Wow, great find! Looks like a sane (and simple!) fix. Still needs test coverage though.

chx’s picture

chx CreditAttribution: chx at Tag1 Consulting commented
Issue tags: -Needs tests +SprintWeekend2016
FileSize
2645036_7.patch3.61 KB
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented 
  1. +++ b/core/modules/system/system.module
@@ -1404,22 +1404,24 @@ function system_block_view_system_main_block_alter(array &$build, BlockPluginInt
-function system_path_update() {
...
+function system_path_update($path) {
...
-function system_path_insert() {
...
+function system_path_insert($path) {

    For those concerned: hook_path_update/hook_path_insert was already passing the $path, it just wasn't used here. 

  2. +++ b/core/modules/system/tests/src/Kernel/PathHooksTest.php
@@ -0,0 +1,64 @@
+    $alias_manager->cacheClear(Argument::any())->shouldBeCalledTimes(1);
+    $alias_manager->cacheClear($source)->shouldBeCalledTimes(1);
...
+    $alias_manager->cacheClear(Argument::any())->shouldBeCalledTimes(2);
+    $alias_manager->cacheClear($source)->shouldBeCalledTimes(1);
+    $alias_manager->cacheClear($new_source)->shouldBeCalledTimes(1);
...
+    $alias_manager->cacheClear(Argument::any())->shouldBeCalledTimes(1);
+    $alias_manager->cacheClear($new_source)->shouldBeCalledTimes(1);

    This is a nice trick. +1 

  3. +++ b/core/modules/system/tests/src/Kernel/PathHooksTest.php
@@ -0,0 +1,64 @@
+    $alias_manager->checkProphecyMethodsPredictions();
...
+    $alias_manager->checkProphecyMethodsPredictions();
...
+    $alias_manager->checkProphecyMethodsPredictions();

    I've never once used this. I don't think it's needed (i.e., it is automatic)? If you don't have these, and the code does something to violate the shouldBeCalledTimes() expectations, doesn't the test fail

chx’s picture

chx CreditAttribution: chx at Tag1 Consulting commented

Sure, the test fails without checkProphecyMethodsPredictions but at the same time your assert count goes up if it's there. So, I dunno, I'd rather keep it. Explicit is better than implicit.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Needs work 
+++ b/core/modules/system/tests/src/Kernel/PathHooksTest.php
@@ -0,0 +1,64 @@
+  public function testPathHooks() {

The reason why you need to call checkProphecyMethodsPredictions is because you have 1 test for 3 different things.

Just split it up, then you don't need to deal with that.

chx’s picture

chx CreditAttribution: chx at Tag1 Consulting commented
Status: Needs work » Needs review
FileSize
2645036_11.patch3.44 KB

Splitting saves us little: you need to insert to update and to delete. And it has nothing to do with the checkProphecyMethodsPredictions calls. The reason I didn't want to remove it was -- it's not in the destructor so I didn't see clearly how it gets checked and the byzantine overengineered phpunit is makes it very hard to find where it is but actually phpunit does the same check eventually. Sane people would expect it in tearDown where the phpspec docs recommend it but phpunit, no, for some bizarre reason it calls TestResult::run (why does the result run the test?) which eventually threads its way back to the TestCase::runBare where runBare has a verifyMockObjects which since 3.5.0 also happens to verify prophecies despite its name. So much fun...

Removed the explicit calls. I am glad this is well documented.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
Status: Needs review » Reviewed & tested by the community

Seems alright for me now. Splitting it up would be nice but I see the point of being a bit of pointless.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 9aff04c and pushed to 8.0.x and 8.1.x. Thanks!

  • alexpott committed 6badeae on 
    Issue #2645036 by chx: Performance: system_path_* doesn't pass the...

  • alexpott committed 9aff04c on 
    Issue #2645036 by chx: Performance: system_path_* doesn't pass the...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#2642848: path_delete() is broken for nonexisting paths, +#1025904: path_delete() cannot handle multiple aliases

Per IS.

Status: Fixed » Closed (fixed)

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

zaporylie’s picture

zaporylie
he/him
Primary language Polish
CreditAttribution: zaporylie at Ny Media AS commented
Related issues: +#2917505: Drupal\Tests\system\Kernel\PathHooksTest uses incorrect @group

I have created a very simple followup to this old-ish issue: #2917505: Drupal\Tests\system\Kernel\PathHooksTest uses incorrect @group