Problem/Motivation

Path aliases are currently represented as arrays, and they are using a custom storage class, which means they can't be integrated in modern workflows built around Drupal's Entity API, such as Content Moderation or Workspaces.

Proposed resolution

Convert path aliases to a new path_alias content entity type. The new entity type will be revisionable and non-translatable, and will also be made publishable in a followup issue: #3007669: Add publishing status to path aliases

In order to keep this non-trivial patch to a reasonable size, no methods from the current path.alias_storage service will be deprecated in this issue. That work will happen in a followup: #2233595: Deprecate the custom path alias storage

For the same reason, other conversion to use the Entity API will also be done in followup issues:

#3009014: Convert path alias migrations to use entity:path_alias destination
#3007832: Convert custom path alias forms to entity forms
#3011807: Convert the path alias admin overview to a list builder

Note about performance

Operating with an entity is heavier than just sending arrays around. This patch doesn't change any of the queries that happen in bootstrap phase (which is most sensitive to performance regressions). All the queries (even functions) stay the same. Same is true for URL generation.

Remaining tasks

Review the patch.

User interface changes

Nope.

API changes

API addition: a new path_alias revisionable content entity type.

Release notes snippet

Custom URL aliases are now provided by a new path_alias revisionable content entity type. The path.alias_storage service has been kept in place for backwards compatibility, and its hooks have been deprecated.

If you have the contrib Pathauto module enabled, you must update to the latest version when you update to Drupal 8.8.0. Failure to update Pathauto when updating core could result in data loss.

CommentFileSizeAuthor
#195 interdiff-195.txt862 bytesamateescu
#195 2336597-195.patch102.84 KBamateescu
#193 interdiff-193.txt857 bytesamateescu
#193 2336597-193.patch102.72 KBamateescu
#191 interdiff-191.txt1013 bytesamateescu
#191 2336597-191.patch102.84 KBamateescu
#188 interdiff-188.txt1.67 KBamateescu
#188 2336597-188.patch102.71 KBamateescu
#186 interdiff-186.txt1.13 KBamateescu
#186 2336597-186.patch102.93 KBamateescu
#184 interdiff-184.txt14.16 KBamateescu
#184 2336597-184.patch102.95 KBamateescu
#173 interdiff-173.txt2.1 KBamateescu
#173 2336597-173.patch101.28 KBamateescu
#171 interdiff-171.txt5.96 KBamateescu
#171 2336597-171.patch101.02 KBamateescu
#164 interdiff-164.txt28.7 KBamateescu
#164 2336597-164.patch101.79 KBamateescu
#162 interdiff-162.txt5.58 KBamateescu
#162 2336597-162.patch106.21 KBamateescu
#158 interdiff-158.txt5.57 KBamateescu
#158 2336597-158.patch106.55 KBamateescu
#156 interdiff-156.txt3.78 KBamateescu
#156 2336597-156.patch108.68 KBamateescu
#146 interdiff-146.txt1.99 KBamateescu
#146 2336597-146.patch109.8 KBamateescu
#142 interdiff-142.txt10.12 KBamateescu
#142 2336597-142.patch109.78 KBamateescu
#141 interdiff-141.txt6.23 KBamateescu
#141 2336597-141.patch107.92 KBamateescu
#138 interdiff-138.txt3.26 KBamateescu
#138 2336597-138.patch102.62 KBamateescu
#136 interdiff-136.txt2.64 KBamateescu
#136 2336597-136.patch101.21 KBamateescu
#135 interdiff-135.txt1.06 KBamateescu
#135 2336597-135.patch101.13 KBamateescu
#132 2336597-132.patch100.07 KBjibran
#129 2336597-129.patch100.02 KBjibran
#129 interdiff-03b230.txt1.49 KBjibran
#127 2336597-127.patch98.93 KBjibran
#127 interdiff-e16680.txt587 bytesjibran
#127 interdiff-0c2df5.txt616 bytesjibran
#125 2336597-125.patch98.85 KBjibran
#125 interdiff-164db8.txt563 bytesjibran
#122 2336597-122.patch98.76 KBjibran
#122 interdiff-f7fb3e.txt3.26 KBjibran
#120 2336597-120.patch95.5 KBjibran
#118 interdiff-118.txt644 bytesamateescu
#118 2336597-118.patch95.65 KBamateescu
#116 2336597-116.patch95.65 KBamateescu
#113 interdiff-113.txt3.61 KBamateescu
#113 2336597-113.patch95.68 KBamateescu
#106 interdiff-106.txt5.06 KBamateescu
#106 2336597-106.patch95.35 KBamateescu
#105 2336597-105.patch94.92 KBWim Leers
#105 interdiff.txt7.79 KBWim Leers
#100 interdiff-100.txt6.15 KBamateescu
#100 2336597-100.patch96.56 KBamateescu
#98 interdiff-98.txt7.03 KBamateescu
#98 2336597-98.patch92.71 KBamateescu
#96 interdiff-96.txt1.85 KBamateescu
#96 2336597-96.patch89.55 KBamateescu
#95 interdiff-95.txt5.53 KBamateescu
#95 2336597-95.patch89.56 KBamateescu
#93 2336597-93-combined.patch88.65 KBamateescu
#92 interdiff-92.txt10.17 KBamateescu
#92 2336597-92-combined.patch90.44 KBamateescu
#92 2336597-92-for-review.patch86.92 KBamateescu
#90 2336597-90-combined.patch81.19 KBamateescu
#90 2336597-90-for-review.patch77.67 KBamateescu
#88 interdiff-88.txt598 bytesamateescu
#88 2336597-88-combined.patch81.19 KBamateescu
#88 2336597-88-for-review.patch77.67 KBamateescu
#87 interdiff-87.txt3.85 KBamateescu
#87 2336597-87-combined.patch81.45 KBamateescu
#87 2336597-87-for-review.patch77.92 KBamateescu
#84 interdiff-84.txt14.19 KBamateescu
#84 2336597-84-combined.patch81.23 KBamateescu
#84 2336597-84-for-review.patch77.7 KBamateescu
#81 interdiff-81.txt2.28 KBamateescu
#81 2336597-81-combined.patch81.49 KBamateescu
#81 2336597-81-for-review.patch77.96 KBamateescu
#78 interdiff-78.txt3.9 KBamateescu
#78 2336597-78-combined.patch81.56 KBamateescu
#78 2336597-78-for-review.patch78.04 KBamateescu
#76 interdiff-76.txt7.11 KBamateescu
#76 2336597-76-combined.patch79.09 KBamateescu
#76 2336597-76-for-review.patch75.69 KBamateescu
#75 interdiff-75.txt3.21 KBamateescu
#75 2336597-75-combined.patch82.47 KBamateescu
#75 2336597-75-for-review.patch79.07 KBamateescu
#73 interdiff-73.txt551 bytesamateescu
#73 2336597-73-combined.patch82.5 KBamateescu
#73 2336597-73-for-review.patch79.1 KBamateescu
#71 interdiff-71.txt1.57 KBamateescu
#71 2336597-71-combined.patch82.55 KBamateescu
#71 2336597-71-for-review.patch82.55 KBamateescu
#69 2336597-69-combined.patch82.44 KBamateescu
#69 2336597-69-for-review.patch79.04 KBamateescu
#63 interdiff-63.txt11.84 KBamateescu
#63 2336597-63-combined.patch82.63 KBamateescu
#63 2336597-63-for-review.patch79.21 KBamateescu
#61 interdiff-61.txt14.77 KBamateescu
#61 2336597-61-combined.patch76.53 KBamateescu
#61 2336597-61-for-review.patch73.11 KBamateescu
#59 interdiff-59.txt1.87 KBamateescu
#59 2336597-59-combined.patch59.66 KBamateescu
#59 2336597-59-for-review.patch58.34 KBamateescu
#56 2336597-55-combined.patch60.97 KBamateescu
#53 interdiff-53.txt14.96 KBamateescu
#53 2336597-53-combined.patch59.15 KBamateescu
#53 2336597-53-for-review.patch56.47 KBamateescu
#51 2336597-51.patch48.57 KBamateescu
#33 interdiff-2336597-25-33.txt3.54 KBsaki007ster
#33 2336597_33.patch107.03 KBsaki007ster
#25 interdiff.txt2.73 KBslashrsm
#25 2336597_25.patch108.17 KBslashrsm
#23 2336597_23.patch107.92 KBslashrsm
#17 interdiff.txt682 bytesslashrsm
#17 2336597_17.patch107.79 KBslashrsm
#15 interdiff.txt3.87 KBslashrsm
#15 2336597_15.patch107.13 KBslashrsm
#12 interdiff.txt5.74 KBslashrsm
#12 2336597_12.patch105.7 KBslashrsm
#11 interdiff.txt25.14 KBslashrsm
#11 2336597_10.patch99.96 KBslashrsm
#9 interdiff.txt21.58 KBslashrsm
#9 2336597_9.patch74.82 KBslashrsm
#6 interdiff.txt19.44 KBslashrsm
#6 2336597_6.patch60.51 KBslashrsm
#1 2336597_1.patch42.39 KBslashrsm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm’s picture

Component: path.module » base system
FileSize
42.39 KB

I looked into this a bit. The more time I spent with it the more it looked that it wouldn't be much harder to go all the way to "alias entities". I realize it is a bit crazy idea, but it gives us a lot of power and flexibility from the other hand. The way I implemented it in this patch doesn't change bootstrap preloads, etc so it shouldn't affect bootstrap performance.

Attached (proof of concept) patch converts path aliases to content entities, continues terminology standardization that we started as part of #2002126: [meta] Refactor the alias subsystem (classes in \Drupal\Core\Path) (path => alias) and adds two minor fixes that are needed to correctly install content entity that is defined in Core namespace.

I managed to install site, create alias for a node and navigate node page. Didn't touch tests, migration, ...

Now tear it apart! Ready, set, go! :)

slashrsm’s picture

Status: Active » Needs review
slashrsm’s picture

I initially made it revisionable but that introduced dependency on user.module due to revision_uid base field so I decided to remove that.

Dave Reid’s picture

Priority: Normal » Major
Issue tags: +Regression, +Contributed project blocker

Re-applying tags and priority from #1854284: Path aliases are no longer arrays and cannot pass additional data to path hooks since this fixes a regression between D7 and D8.

Status: Needs review » Needs work

The last submitted patch, 1: 2336597_1.patch, failed testing.

slashrsm’s picture

Assigned: Unassigned » slashrsm
Status: Needs work » Needs review
FileSize
60.51 KB
19.44 KB

Another step forward but still work in progress.

Berdir’s picture

+++ b/core/modules/filter/src/Tests/FilterDefaultConfigTest.php
@@ -20,11 +20,7 @@ class FilterDefaultConfigTest extends DrupalUnitTestBase {
-
-    // Drupal\filter\FilterPermissions::permissions() calls into url() to output
-    // a link in the description.
-    $this->installSchema('system', 'url_alias');
-

Are you sure that removing them is correct and not replace with installEntitySchema('path') ?

Did not look at anything else yet, just noticed that.

Status: Needs review » Needs work

The last submitted patch, 6: 2336597_6.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
74.82 KB
21.58 KB

Some more progress.

Status: Needs review » Needs work

The last submitted patch, 9: 2336597_9.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
99.96 KB
25.14 KB
slashrsm’s picture

FileSize
105.7 KB
5.74 KB

The last submitted patch, 11: 2336597_10.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: 2336597_12.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
107.13 KB
3.87 KB

Fixed two more tests; only one left. Moved installation of entities in core namespace to install_base_system().

Status: Needs review » Needs work

The last submitted patch, 15: 2336597_15.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
107.79 KB
682 bytes
slashrsm’s picture

Assigned: slashrsm » Unassigned

Yay! I'll try to do some performance testing/profiling to see if this introduces any regression.

+++ b/core/modules/config/src/Tests/ConfigImportAllTest.php
@@ -104,6 +104,7 @@ public function testInstallUninstall() {
     // Check that there are no errors.
+    $this->container->get('kernel')->rebuildContainer();
     $this->assertIdentical($this->configImporter()->getErrors(), array());
 

At this point container ends up with uncomplete list of namespaces. I'm not sure where this rebuild should happen but it apparently fixes the last failing test (field_storage_config entity not found).

slashrsm’s picture

Also, there are still some things that could be done. Edit, delete, listing, ... are currently not working as "real" entity handers. Since all those pages still work as they should I'd rather do this kind of tasks as follow-ups to prevent the patch from growing even bigger.

slashrsm’s picture

Issue tags: +Amsterdam2014
slashrsm’s picture

Title: Convert path aliases to objects » Convert path aliases to full featured entities
Issue summary: View changes
slashrsm’s picture

Did some ab benchmarking and profiling:

ab benchmark

HEAD

➜  modules git:(8.0.x) ✗ ab -n 100 -c 5 http://localhost/drupal8/node
This is ApacheBench, Version 2.3 <$Revision: 1528965 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd,
http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:        Apache/2.4.7
Server Hostname:        localhost
Server Port:            80

Document Path:          /drupal8/node
Document Length:        75627 bytes

Concurrency Level:      5
Time taken for tests:   8.313 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      7673100 bytes
HTML transferred:       7562700 bytes
Requests per second:    12.03 [#/sec] (mean)
Time per request:       415.643 [ms] (mean)
Time per request:       83.129 [ms] (mean, across all concurrent
requests)
Transfer rate:          901.41 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   1.0      0      10
Processing:   233  410 141.9    372     978
Waiting:      222  388 134.4    354     899
Total:        233  410 142.3    372     978

Percentage of the requests served within a certain time (ms)
  50%    372
  66%    418
  75%    437
  80%    474
  90%    525
  95%    927
  98%    958
  99%    978
 100%    978 (longest request)
➜  modules git:(8.0.x) ✗ ab -n 100 -c 5
  http://localhost/drupal8/node-76-article
This is ApacheBench, Version 2.3 <$Revision: 1528965 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd,
  http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:        Apache/2.4.7
Server Hostname:        localhost
Server Port:            80

Document Path:          /drupal8/node-76-article
Document Length:        14348 bytes
Concurrency Level:      5
Time taken for tests:   5.672 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      1538600 bytes
HTML transferred:       1434800 bytes
Requests per second:    17.63 [#/sec] (mean)
Time per request:       283.578 [ms] (mean)
Time per request:       56.716 [ms] (mean, across all concurrent
requests)
Transfer rate:          264.93 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   1.5      0      11
Processing:   196  281  55.6    267     498
Waiting:      183  256  50.3    248     436
Total:        196  281  56.2    267     498

Percentage of the requests served within a certain time (ms)
  50%    267
  66%    288
  75%    309
  80%    316
  90%    354
  95%    418
  98%    460
  99%    498
 100%    498 (longest request)
➜  modules git:(8.0.x) ✗

Patch

➜  drupal8 git:(2336597) ✗ ab -n 100 -c 5
http://localhost/drupal8/node
This is ApacheBench, Version 2.3 <$Revision: 1528965 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd,
http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:        Apache/2.4.7
Server Hostname:        localhost
Server Port:            80

Document Path:          /drupal8/node
Document Length:        76849 bytes

Concurrency Level:      5
Time taken for tests:   8.421 seconds
Complete requests:      100
Failed requests:        0
Total transferred:      7795300 bytes
HTML transferred:       7684900 bytes
Requests per second:    11.87 [#/sec] (mean)
Time per request:       421.071 [ms] (mean)
Time per request:       84.214 [ms] (mean, across all concurrent
requests)
Transfer rate:          903.96 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   2.3      0      17
Processing:   280  416 142.0    384    1017
Waiting:      268  396 139.6    361     977
Total:        280  417 143.1    384    1018

Percentage of the requests served within a certain time (ms)
  50%    384
  66%    412
  75%    442
  80%    455
  90%    546
  95%    875
  98%   1001
  99%   1018
 100%   1018 (longest request)
➜  drupal8 git:(2336597) ✗ ab -n 100 -c 5
  http://localhost/drupal8/node-76-article
This is ApacheBench, Version 2.3 <$Revision: 1528965 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd,
  http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient).....done


Server Software:        Apache/2.4.7
Server Hostname:        localhost
Server Port:            80

Document Path:          /drupal8/node-76-article
Document Length:        3090 bytes

Concurrency Level:      5
Time taken for tests:   5.843 seconds
Complete requests:      100
Failed requests:        99
   (Connect: 0, Receive: 0, Length: 99, Exceptions: 0)
Non-2xx responses:      1
Total transferred:      1487988 bytes
HTML transferred:       1384833 bytes
Requests per second:    17.11 [#/sec] (mean)
Time per request:       292.170 [ms] (mean)
Time per request:       58.434 [ms] (mean, across all concurrent
requests)
Transfer rate:          248.68 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   2.2      0      19
Processing:   210  288  57.8    280     502
Waiting:      195  259  51.1    249     444
Total:        210  289  58.8    280     502

Percentage of the requests served within a certain time (ms)
  50%    280
  66%    303
  75%    318
  80%    324
  90%    355
  95%    385
  98%    499
  99%    502
 100%    502 (longest request)

Profiling

HEAD

= cold cache =

Overall Summary
Total Incl. Wall Time (microsec):       6,448,442 microsecs
Total Incl. CPU (microsecs):            5,007,418 microsecs
Total Incl. MemUse (bytes):             43,580,568 bytes
Total Incl. PeakMemUse (bytes):         44,083,336 bytes
Number of Function Calls:               1,527,405

= warm cache =

Overall Summary
Total Incl. Wall Time (microsec):       415,785 microsecs
Total Incl. CPU (microsecs):            394,189 microsecs
Total Incl. MemUse (bytes):             20,026,248 bytes
Total Incl. PeakMemUse (bytes):         20,597,536 bytes
Number of Function Calls:               93,417

patch

= cold cache =

Overall Summary
Total Incl. Wall Time (microsec):       7,137,121 microsecs
Total Incl. CPU (microsecs):            5,208,773 microsecs
Total Incl. MemUse (bytes):             42,873,256 bytes
Total Incl. PeakMemUse (bytes):         43,418,008 bytes
Number of Function Calls:               1,526,824

= warm cache =

Overall Summary
Total Incl. Wall Time (microsec):       359,257 microsecs
Total Incl. CPU (microsecs):            345,558 microsecs
Total Incl. MemUse (bytes):             19,218,248 bytes
Total Incl. PeakMemUse (bytes):         19,783,384 bytes
Number of Function Calls:               93,299
slashrsm’s picture

Issue summary: View changes
FileSize
107.92 KB

Reroll and summary update.

We could provide BC layer by keeping path.alias_storage service and existing CRUD functions in place and operating against alias entity tables directly.

This issue is contrib blocker and was worked on during Amsterdam, which hopefully makes it a bit more acceptable.

Status: Needs review » Needs work

The last submitted patch, 23: 2336597_23.patch, failed testing.

slashrsm’s picture

Status: Needs work » Needs review
FileSize
108.17 KB
2.73 KB
Fabianx’s picture

Issue tags: +needs profiling

Might need some more profiling, not yet reviewed in depth, yet.

Berdir’s picture

I really doubt that :)

It would give you a default cache tag for the alias, but that's easy enough to build when you don't have an entity too. That's not the hard problem here (that is things like that you can *add* an alias for something that didn't have one before, for example...)

Wim Leers’s picture

Right, but aliases being entities means that we don't have to retrofit aliases with cache tags in some hacky way; they'll be able to use cache tags in exactly the same way as others can today.

jibran’s picture

Awesome patch but we need a beta evaluation criteria and probably a reroll.

manningpete queued 25: 2336597_25.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 25: 2336597_25.patch, failed testing.

saki007ster’s picture

Trying rerolling. Big reroll please review it, not sure if done the right thing here.

saki007ster’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 33: 2336597_33.patch, failed testing.

slashrsm’s picture

I honestly think this will unfortunately need to wait until D9. We could potentially do this in one of 8.X releases, but we'd have to do it in a non BC-breaking way.

Fabianx’s picture

No, as this is BC compatible data structure wise it is internal re-factoring also unblocks some contrib projects, so likely 8.0.x or 8.1.x material.

slashrsm’s picture

I don't think this is BC-compatible. At least how it is done in current patch (underlying data structure and parts of API are significantly different). I discussed this with @berdir in Amsterdam last year and his opinion was that it is not realistic to expect this to go in after feature freeze. That was also the main reason why I stopped working on this patch.

Fabianx’s picture

Yeah, I remembered wrong.

However without the rename of source to path I think it could be BC compatible.

googletorp’s picture

Issue tags: -Needs reroll
slashrsm’s picture

Version: 8.0.x-dev » 8.1.x-dev

This is 8.1.x material I guess.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

Discussed with @catch, @moshe, and @timmillwood on IRC. We all agree that this is a good direction for path aliases, and we agree (so far) on the following implementation points that I want to repeat here so that we have it in writing:

  • This patch should be refactored (rewritten?) so that it provides a completely parallel entity-based implementation of path aliases, for backwards compatibility purposes. It can swap out core services with its own implementation(s) if it needs to, but it should probably be in a completely separate module rather than being grafted onto the core Path module.
  • The path alias entity type does not need to be bundleable, but it should be both versionable and translatable.
  • For maximum flexibility, it would be best if the internal path of an alias was computed by a method on AliasInterface, rather than necessarily being stored as a static value.
  • Because of the computed nature of the internal path, it will be OK to store multiple aliases with the same external path. This will also make it possible to migrate the data in url_alias (which does not enforce uniqueness for the alias) into the new system.

More thoughts and ideas are of course welcome! :)

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nevergone’s picture

And now?

timmillwood’s picture

Before this issue, we're working in #2856363: Path alias changes for draft revisions immediately leak into live site to provide a interim solution to get path aliases working with forward revisions.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Regression, -Needs beta evaluation +Workflow Initiative
FileSize
48.57 KB

This functionality is needed by the Workspace module, so I rewrote the patch with an approach that keeps full BC for now. At least some tests pass, let's see what the testbot has to say.

Status: Needs review » Needs work

The last submitted patch, 51: 2336597-51.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
56.47 KB
59.15 KB
14.96 KB

All the update tests are failing because of #2830326: Broken link to 'Put your site into maintenance mode' on update.php results in WSOD. The problem is that we are trying to generate a link to the maintenance mode page without a working path alias storage (that's being added in an update function), so that issue is a hard blocker for this one.

Other changes in this patch:

- dropped the "publishable" part because I feel that needs a separate issue so it can be discussed properly
- added a quick update path which just creates an entity for each existing entry in the url_alias table. A more elaborate version is needed so we can take advantage of the multilingual capability of the new entity type and group existing per-language aliases into a single path_alias entity somehow
- fixed a bunch of kernel tests that need to install the schema for the new path_alias entity type

Let's see where we're at with all the test fixes.

Status: Needs review » Needs work

The last submitted patch, 53: 2336597-53-combined.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
amateescu’s picture

It would help to attach the patch as well :)

Status: Needs review » Needs work

The last submitted patch, 56: 2336597-55-combined.patch, failed testing. View results

Wim Leers’s picture

@amateescu++

From 181 to 32 failures, nice!

amateescu’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
58.34 KB
59.66 KB
1.87 KB

Finally managed to find a solution for #3006086: update.php should not process path aliases, so we just need a few more test fixes here. The combined patch includes #3006086-16: update.php should not process path aliases.

Status: Needs review » Needs work

The last submitted patch, 59: 2336597-59-combined.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
73.11 KB
76.53 KB
14.77 KB

Updated he combined patch to use the latest one from #3006086: update.php should not process path aliases and added the required REST test coverage.

There should be single fail left, \Drupal\Tests\rest\Functional\Update\EntityResourcePermissionsUpdateTest::testBcEntityResourcePermissionSettingAdded(), and the fail is caused by \Drupal\filter\FilterPermissions::permissions trying to generate a URL with path processing enabled, which means that the blocker issue is maybe not done yet :/

Status: Needs review » Needs work

The last submitted patch, 61: 2336597-61-combined.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
79.21 KB
82.63 KB
11.84 KB

It seems that the REST test coverage check wants the test classes in very specific locations, and I was missing the hal_json parts :/

Also looked into the failure from EntityResourcePermissionsUpdateTest::testBcEntityResourcePermissionSettingAdded() and it's just another example of trying to use Drupal's API before running the database updates, when the system is in an unknown/broken state. The fix is pretty easy though, we can check the output of rest's permission callback directly instead of going through the user permission service.

amateescu’s picture

heddn’s picture

Just had a concern come to me. When running a migration from D6 or D7, today the path alias migration takes really long. Do we expect this to get even worse after converting to a full entity? And do we have tests of a migration after the conversion is in place? I would expect more changes in those tests, so I wonder if we have enough coverage to test this. Or maybe the API changes we have here are transparent enough that those tests don't need to change. But I do see some direct database calls in the kernel tests, so it makes me wonder.

amateescu’s picture

Or maybe the API changes we have here are transparent enough that those tests don't need to change.

That's exactly what's happening :) The current path alias destination plugin works with the current APIs, which are not impacted in any way. It's also a very good test that we are not breaking BC with this patch.

However, we should definitely update the default migrations to use a regular entity destination plugin instead of the custom path_alias one in a followup issue.

heddn’s picture

Ah, my causal glance was in error. On more thorough study, I see that direct db call is against the d6 source. Database::getConnection('default', 'migrate')

Let's make sure we get that follow-up opened? Even though it will be postponed for now.

Great work. It nice to see it working so painlessly on the API front.

benjifisher’s picture

@amateescu, @heddn:

I just opened #3009014: Convert path alias migrations to use entity:path_alias destination to track the migration updates discussed in #65-#67.

amateescu’s picture

This patch needed a quick reroll.

catch’s picture

Not a comprehensive review because I just ran out of time in the middle of it, but some minor things:

  1. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,195 @@
    +    // Invoke the deprecated hook_path_OPERATION() hooks.
    

    This could use ::invokeAllDeprecated() now, see https://www.drupal.org/node/2881531

  2. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,195 @@
    +      $query->condition('alias', '%' . preg_replace('!\*+!', '%', $keys) . '%', 'LIKE');
    

    This should use escapeLike(): https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

  3. +++ b/core/modules/path/tests/src/Functional/PathAliasTest.php
    @@ -368,7 +368,10 @@ public function testNodeAlias() {
    +    $result = \Drupal::entityTypeManager()->getStorage('path_alias')->getQuery()
    

    For consistency this should have an accessCheck(FALSE);

amateescu’s picture

Thanks, @catch! All good points :)

Status: Needs review » Needs work

The last submitted patch, 71: 2336597-71-combined.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
79.1 KB
82.5 KB
551 bytes

Based on the test fails from #71, I think we should deprecate the hooks in a followup issue, where we can also replace all their usages: #3011003: Deprecate the old path alias hooks

catch’s picture

I'm not 100% on doing the hook deprecations in a follow-up because it leaves us with two parallel APIs, but can also see good arguments for keeping the patch below 90kb. Here's a bit more of a review otherwise:

  1. +++ b/core/core.services.yml
    --- a/core/lib/Drupal/Core/Path/AliasStorage.php
    +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    

    This should probably be deprecated - are usages removed here?

  2. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,195 @@
    +  public function lookupPathAlias($path, $langcode) {
    

    It's confusing (to me at least) that lookupPathSource() and lookupPathAlias() have the same method signatures.

  3. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,195 @@
    +    $path = $this->database->escapeLike($path);
    

    This is only used once so could skip assigning it?

  4. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,195 @@
    +      $query->condition('alias', '%' . preg_replace('!\*+!', '%', $this->database->escapeLike($keys)) . '%', 'LIKE');
    

    This is concatenating % before and after $keys as well as using escapeLike(), as well as replacing * with % - should it be stripping * instead?

amateescu’s picture

I'm not 100% on doing the hook deprecations in a follow-up because it leaves us with two parallel APIs

Well, to be honest, all of #3007661: Modernize the path alias system has to be done for 8.7 if we want to end up with a real improvement for the path alias system (i.e. usable with workspaces) :) A patch for everything listed in there would be huge, so I'm trying to split up the work in as many logical pieces as I can. I'm aware that every patch should leave the system in a shippable state but in this case I'm learning this subsystem as I go, and I can't really think of another way do it.

For example, after working on #3007832: Convert custom path alias forms to entity forms in the past couple of days, I figured that we need another issue for converting the path alias admin listing to an entity view builder, and also that some methods from the current alias storage will not be needed anymore in the new entity-based storage (aliasExists(), languageAliasExists() and getAliasesForAdminListing()), but I can only be confident about removing them after those patches are done and reviewed.

1. Usages are not removed here, and I've re-purposed #2233595: Deprecate the custom path alias storage for deprecating the current storage as well as converting its usages.

2. This is just carried over from the current implementation, but I agree that it's very confusing. Fixed.

3. Right, same in lookupPathSource(). Fixed.

4. I think the * character is used as a wildcard in the custom filtering code used by the alias overview form, so we can't strip it :)

amateescu’s picture

Ok, #3011807: Convert the path alias admin overview to a list builder is done and now I'm 100% sure that we don't need aliasExists(), languageAliasExists() and getAliasesForAdminListing() in the new storage.

Status: Needs review » Needs work

The last submitted patch, 76: 2336597-76-combined.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
78.04 KB
81.56 KB
3.9 KB

It would help if I ran at least some tests before posting a patch :/

The interdiffs in #76 and the one from this comment might look confusing, and the reason is that I'm trying to minimize the changes in the "for review" patch now that the path forward for the followups is a bit more clear.

Status: Needs review » Needs work

The last submitted patch, 78: 2336597-78-combined.patch, failed testing. View results

catch’s picture

Discussed with Alex Pott, we both think it's OK to deprecate the hooks in a follow-up.

amateescu’s picture

Status: Needs work » Needs review
FileSize
77.96 KB
81.49 KB
2.28 KB

@catch, great! I'll start working on that part now and it will most likely be ready before this one gets in :)

amateescu’s picture

Turns out that the patch to deprecates the old path hooks is ~10K: #3011003-2: Deprecate the old path alias hooks. Let me know if you would prefer to fold it into this issue or keep it separate.

amateescu’s picture

While waiting for reviews here, I started to work on updating Pathauto for this change: #3012050: Prepare for the conversion of path aliases to entities

amateescu’s picture

A few improvements based on various things that were discovered in all the followup issues.

amateescu’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 84: 2336597-84-combined.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

amateescu’s picture

Status: Needs work » Needs review
FileSize
77.92 KB
81.45 KB
3.85 KB

This should be better.

amateescu’s picture

Status: Needs review » Needs work

The last submitted patch, 88: 2336597-88-combined.patch, failed testing. View results

amateescu’s picture

catch’s picture

I think it's worth folding #3011003: Deprecate the old path alias hooks in here given it's already ready - the hook invocations are changed here then changed again, so it should be less than a 10k addition to this patch.

amateescu’s picture

Sure, let's do that :)

amateescu’s picture

We have a new approach in #3006086: update.php should not process path aliases so let's try a new combined patch as well.

catch’s picture

Status: Needs review » Needs work

I think this needs a re-roll - just committed the update.php patch.

I reviewed again, and only found one thing to comment on otherwise.

+++ b/core/modules/system/system.post_update.php
@@ -178,3 +181,76 @@ function system_post_update_extra_fields(&$sandbox = NULL) {
+function system_post_update_convert_path_aliases_to_entities(&$sandbox = NULL) {

I think we should install the schema in a hook_update_N() then migrate the entities and drop the table in the post update. That way we'll start the post updates with a coherent schema that just needs a data migration.

amateescu’s picture

Status: Needs work » Needs review
FileSize
89.56 KB
5.53 KB

Fixed #94 and added an upgrade path test.

I'll start writing a change record for all of #3007661: Modernize the path alias system because I think it would be more discoverable via search engines than separate ones for each issue from that plan.

amateescu’s picture

The CR is at https://www.drupal.org/node/3013865, added it to the patch.

larowlan’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs change record updates
  1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +66,119 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +    return $path_alias_values;
    

    The issue summary talks about 'passing around arrays' as being a motivation here, but because of the interface we're still doing so. Do we want to add a new interface (and implement it with this class) that allows an entity based return. Otherwise we're not really addressing the original problem.

  2. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +66,119 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
       public function load($conditions) {
    

    Note to self and other reviewers: this is only used by UI elements like forms and not in the critical path, so there is no performance cost of adding the entity load

  3. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +66,119 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +      if ($field === 'source') {
    +        $field = 'path';
    ...
    +      if ($field === 'pid') {
    +        $field = 'id';
    

    Is there a reason why we didn't just align there field names?

  4. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +66,119 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +    $result = $query
    ...
    +    $query = $this->getPathAliasEntityStorage()->getQuery();
    

    do we need accessCheck(FALSE) here too?

  5. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -350,103 +245,23 @@ public function getAliasesForAdminListing($header, $keys = NULL) {
    +  protected function getPathAliasEntityStorage() {
    +    if (!$this->pathAliasEntityStorage) {
    +      $this->pathAliasEntityStorage = $this->entityTypeManager->getStorage('path_alias');
    

    not a huge fan of these 'getters that set things'

  6. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -350,103 +245,23 @@ public function getAliasesForAdminListing($header, $keys = NULL) {
    -  public static function schemaDefinition() {
    

    Removing this is a BC break, its a public function. We need to retain it and trigger an error (and a legacy test to verify as such)

  7. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,114 @@
    +    return ['route_match'];
    

    Should this call the parent and merge the result?

  8. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,147 @@
    +      $this->moduleHandler()->invokeAllDeprecated('Use regular entity hooks instead. See https://www.drupal.org/node/3013865.', 'path_' . $hook, [$values]);
    

    Can we get a more descriptive message here, it is pretty blunt. Something like 'hook_path_delete is deprecated, use hook_ENTITY_TYPE_update for the path_alias entity type instead' - also we need legacy tests for this.

  9. +++ b/core/lib/Drupal/Core/Path/PathAliasStorageInterface.php
    @@ -0,0 +1,71 @@
    +  public function preloadPathAlias($preloaded, $langcode);
    ...
    +  public function lookupPathAlias($path, $langcode);
    ...
    +  public function lookupPathSource($alias, $langcode);
    ...
    +  public function pathHasMatchingAlias($initial_substring);
    

    these are already defined on AliasStorage, is there a way we can re-use with inheritance here?

  10. +++ b/core/lib/Drupal/Core/Path/PathAliasStorageSchema.php
    @@ -0,0 +1,27 @@
    +    $schema[$this->storage->getBaseTable()]['indexes'] += [
    +      'path_alias__alias_langcode_id' => ['alias', 'langcode', 'id'],
    +      'path_alias__path_langcode_id' => ['path', 'langcode', 'id'],
    

    note to self and other reviewers: these match the existing schema indices

  11. +++ b/core/modules/menu_link_content/menu_link_content.module
    @@ -39,10 +39,11 @@ function menu_link_content_menu_delete(MenuInterface $menu) {
    +function menu_link_content_path_alias_insert(EntityInterface $entity) {
    
    @@ -64,23 +65,25 @@ function _menu_link_content_update_path_alias($path) {
    +function menu_link_content_path_alias_update(EntityInterface $entity) {
    ...
    +function menu_link_content_path_alias_delete(EntityInterface $entity) {
    

    So now we get into BC policy grey areas. We don't have a policy on whether hook implementations constitute API in https://www.drupal.org/core/d8-bc-policy

    It is 100% legitimate to use hook_module_implements_alter and remove menu_link_content's implementations here, substituting them with your own custom implementation that may call the original functions. Dynamic entity reference does that for field module. Removing these functions would be a fatal for any modules/custom code that was doing so.

    I discussed with @catch and he noted that hook implementations are considered internal, so I updated the BC policy as such.

    However the removal of these functions (and the system ones) needs to be part of the change record.

  12. +++ b/core/modules/rest/tests/src/Functional/Update/EntityResourcePermissionsUpdateTest.php
    @@ -33,16 +33,13 @@ public function setDatabaseDumpFiles() {
    -    $this->assertEqual([], array_filter($permission_handler->getPermissions(), $is_rest_resource_permission));
    +
    +    $rest_permissions_callback = \Drupal::service('controller_resolver')->getControllerFromDefinition('Drupal\rest\RestPermissions::permissions');
    +    $rest_permissions = array_keys(call_user_func($rest_permissions_callback));
    +    $this->assertEquals([], $rest_permissions);
    

    why are these changes needed? - can we comment as to why we're calling the controller direct and bypassing the user permissions service?

  13. +++ /dev/null
    @@ -1,96 +0,0 @@
    -class UrlAliasFixtures {
    

    According to https://www.drupal.org/core/d8-bc-policy#tests we should be using deprecation here. Modules could be making use of this utility. And that means we need a legacy test too.

  14. +++ b/core/modules/system/system.install
    @@ -1181,11 +1182,6 @@ function system_schema() {
    -  // Create the url_alias table. The alias_storage service can auto-create its
    -  // table, but this relies on exceptions being thrown. These exceptions will be
    -  // thrown every request until an alias is created.
    -  $schema['url_alias'] = AliasStorage::schemaDefinition();
    

    Can you elaborate on how this happens in the installer with this patch? I see a lot of 'installEntitySchema()' changes in tests so I does that mean its no longer automatic, which probably means the original intent of this code (avoid a lot of Exceptions) is no longer occurring?

  15. +++ b/core/modules/system/system.module
    @@ -1404,26 +1405,29 @@ function system_block_view_system_main_block_alter(array &$build, BlockPluginInt
    +function system_path_alias_insert(EntityInterface $entity) {
    +  /** @var \Drupal\Core\Path\PathAliasInterface $entity */
    +  \Drupal::service('path.alias_manager')->cacheClear($entity->getPath());
    ...
    +function system_path_alias_update(EntityInterface $entity) {
    +  /** @var \Drupal\Core\Path\PathAliasInterface $entity */
    +  $alias_manager = \Drupal::service('path.alias_manager');
    +  $alias_manager->cacheClear($entity->getPath());
    ...
    +function system_path_alias_delete(EntityInterface $entity) {
    +  /** @var \Drupal\Core\Path\PathAliasInterface $entity */
    +  \Drupal::service('path.alias_manager')->cacheClear($entity->getPath());
    

    can we do away with these altogether now by using the entity specific cache tag when we write the cache?

  16. +++ b/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
    @@ -0,0 +1,56 @@
    +      __DIR__ . '/../../../../tests/fixtures/update/drupal-8.filled.standard.php.gz',
    

    As we saw with the taxonomy tree update to entity field, we really need to be testing this with a substantial data-set and with alternate languages. Can we build a custom data set that is more expansive for this update test. This issue has the potential to be as disruptive as that one. Once bitten twice shy etc

amateescu’s picture

Status: Needs work » Needs review
FileSize
92.71 KB
7.03 KB

Thanks for the review!

Re #97:

1. We are adding a new class, \Drupal\Core\Path\PathAliasStorage, and the old one is kept only to preserve BC for the previous API. Note that the existing alias storage will be deprecated in #2233595: Deprecate the custom path alias storage.

3. The nice thing about writing new APIs is that we are able to improve things, including naming :) The current field names are based on a discussion with @catch and we both think they make more sense.

4. Yup! Added it.

5. I don't see any other way around that..

6. It's also marked @internal, so no one should rely on it. And in this case, I think a hard break is preferable instead of having modules think that they can still work with a url_alias table and its schema.

7. Not sure. I wanted to minimize the changes that might be problematic performance-wise in this patch. But we can discuss it in a followup.

8. \Drupal\Core\Extension\ModuleHandler::triggerDeprecationError() already provides a better and explicit message regarding the deprecated hook implementations, but I changed it a bit anyway to be more specific.

9. I don't think so. The existing storage interface clashes with the new entity storage-based one at least on load(), save() and delete().

11. Added a note in the change record about the hook implementations that have been renamed :)

12. Added a comment to explain why.

13. I found only one usage of this class in contrib: http://grep.xnddx.ru/search?text=UrlAliasFixtures . Is it ok if I submit a patch for that module instead? It will be easier for everyone :)

14. install_profile_modules() calls install_core_entity_type_definitions() which installs the new path_alias entity type provided by core.

15. I'm not sure, maybe this can be discussed in #2480077: Using the URL alias UI to change aliases doesn't do necessary invalidations: path aliases don't have cache tags along with point 7. above.

16. Expanded the test coverage :)

Status: Needs review » Needs work

The last submitted patch, 98: 2336597-98.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -needs profiling, -Needs tests, -Needs change record updates
FileSize
96.56 KB
6.15 KB

Forgot that the testbot installs Drupal in a subfolder. Let's try this one.

Also forgot to address #97.8: added legacy tests for the deprecated hooks.

mondrake’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Path/AliasTest.php
@@ -3,39 +3,51 @@
+      $result = $connection->query('SELECT * FROM {path_alias} WHERE path = :path AND alias= :alias AND langcode = :langcode', [':path' => $alias['source'], ':alias' => $alias['alias'], ':langcode' => $alias['langcode']]);

@@ -55,7 +67,7 @@ public function testCRUD() {
+      $result = $connection->query('SELECT id FROM {path_alias} WHERE path = :path AND alias= :alias AND langcode = :langcode', [':path' => $alias['source'], ':alias' => $alias['alias'] . '_updated', ':langcode' => $alias['langcode']]);

@@ -66,21 +78,47 @@ public function testCRUD() {
+      $result = $connection->query('SELECT * FROM {path_alias} WHERE id = :id', [':id' => $pid]);

We are trying to avoid direct db calls to entity tables, see e.g. #3012599: Replace all db calls to aggregator_feed and aggregator_item tables with Entity APIs. Suppose these ones can be converted to EntityQuery instead?

Wim Leers’s picture

Currently just a spectator, just wanted to thank everyone here for the significant recent progress! 👏😃

amateescu’s picture

@mondrake, good point, I'll do that in #2233595: Deprecate the custom path alias storage since we're moving that test method to a different test class over there.

vasi1186’s picture

Hi all!
That's again some really great work! I've also tried this patch and it worked good, migration from the old aliases table worked perfect.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,114 @@
    +  public function getCacheTagsToInvalidate() {
    +    return ['route_match'];
    +  }
    

    Fascinating! Can we document why this is the case?

  2. Also: shouldn't this then also become the list cache tag? To do that, you'd just add list_cache_tags = { "route_match" } to the annotation. ✅
  3. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,147 @@
    +      $this->moduleHandler()->invokeAllDeprecated("Use hook_ENTITY_TYPE_{$hook}() for the 'path_alias' entity type instead. See https://www.drupal.org/node/3013865.", 'path_' . $hook, [$values]);
    

    Shouldn't this say that it'll be removed in Drupal 9.0.0?

  4. +++ b/core/modules/rest/tests/src/Functional/Update/EntityResourcePermissionsUpdateTest.php
    @@ -33,16 +33,17 @@ public function setDatabaseDumpFiles() {
    +    $rest_permissions_callback = \Drupal::service('controller_resolver')->getControllerFromDefinition('Drupal\rest\RestPermissions::permissions');
    

    Let's use RestPermissions::class . ':permissions' instead to simplify future refactoring. ✅

  5. +++ b/core/modules/system/tests/src/Kernel/DeprecatedPathHooksTest.php
    @@ -0,0 +1,76 @@
    +   * The deprecated hook hook_path_delete() is implemented in these functions: path_deprecated_test_path_delete(). Use hook_ENTITY_TYPE_delete() for the 'path_alias' entity type instead. See https://www.drupal.org/node/3013865.
    

    Looks like @expectedDeprecation is missing? ✅

  6. +++ b/core/tests/Drupal/FunctionalTests/Rest/PathAliasResourceTestBase.php
    @@ -0,0 +1,157 @@
    +  protected static $firstCreatedEntityId = 3;
    ...
    +  protected static $secondCreatedEntityId = 4;
    

    Hah, another entity type that needs to override these relatively obscure edge case properties :)

    Great to see you were able to write this test without changing anything in the existing base classes!

  7. +++ b/core/tests/Drupal/FunctionalTests/Rest/PathAliasResourceTestBase.php
    @@ -0,0 +1,157 @@
    +    switch ($method) {
    +      case 'GET':
    +        $this->grantPermissionsToTestedRole(['administer url aliases']);
    +        break;
    +      case 'POST':
    +        $this->grantPermissionsToTestedRole(['administer url aliases']);
    +        break;
    +      case 'PATCH':
    +        $this->grantPermissionsToTestedRole(['administer url aliases']);
    +        break;
    +      case 'DELETE':
    +        $this->grantPermissionsToTestedRole(['administer url aliases']);
    +        break;
    +    }
    ...
    +    switch ($method) {
    +      case 'GET':
    +        return "The 'administer url aliases' permission is required.";
    +      break;
    +      case 'POST':
    +        return "The 'administer url aliases' permission is required.";
    +      break;
    +      case 'PATCH':
    +        return "The 'administer url aliases' permission is required.";
    +      break;
    +      case 'DELETE':
    +        return "The 'administer url aliases' permission is required.";
    +      break;
    +    }
    

    I'd argue that switching on method is unnecessary here :)

    And in fact, in the case of getExpectedUnauthorizedAccessMessage(), this is what the base implementation already does for you. ✅

  8. +++ b/core/tests/Drupal/FunctionalTests/Rest/PathAliasXmlAnonTest.php
    @@ -0,0 +1,36 @@
    +  public function testPatchPath() {
    +    // Deserialization of the XML format is not supported.
    +    $this->markTestSkipped();
    +  }
    
    +++ b/core/tests/Drupal/FunctionalTests/Rest/PathAliasXmlBasicAuthTest.php
    @@ -0,0 +1,46 @@
    +  public function testPatchPath() {
    
    +++ b/core/tests/Drupal/FunctionalTests/Rest/PathAliasXmlCookieTest.php
    @@ -0,0 +1,41 @@
    +  public function testPatchPath() {
    

    This is copy/pasted from \Drupal\Tests\node\Functional\Rest\NodeXmlCookieTest::testPatchPath(), but does not make sense here. ✅

Items with a ✅ have been addressed in this reroll.

amateescu’s picture

Thanks, Wim!

Re #105:

1. Because that's what the current alias storage does :)

https://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Path/AliasS...
https://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Path/AliasS...

3. Sure, why not. The full deprecation message can be seen in the interdiff, in the @expectedDeprecation annotations.

The rest of the changes look good to me from #105.

Wim Leers’s picture

Sounds good! And…

+++ b/core/modules/rest/tests/src/Functional/Update/EntityResourcePermissionsUpdateTest.php
@@ -53,7 +53,7 @@ public function testBcEntityResourcePermissionSettingAdded() {
-    $rest_permissions_callback = \Drupal::service('controller_resolver')->getControllerFromDefinition(RestPermissions::class .'::permissions');
+    $rest_permissions_callback = \Drupal::service('controller_resolver')->getControllerFromDefinition(RestPermissions::class . '::permissions');

Hah, oops! 😅 Thanks for silently fixing it for me ❤️


Next steps:

  1. Whose review do we need to get this to RTBC?
  2. Also, doesn't this need profiling? I know that that happened at some point, but are those numbers still accurate?
amateescu’s picture

Whose review do we need to get this to RTBC?

I pinged @Berdir and he will try to look at this patch sometime :)

Also, doesn't this need profiling?

As reviewed and mentioned by @larowlan in #97.2, the parts of the alias system that are in the critical path are not changed in this patch, they're just moved from the old alias storage to the new entity based one, so I don't think there's anything else we can do in terms of performance here..

Wim Leers’s picture

👌 That's what I thought, thanks for confirming!

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +66,121 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +      /** @var \Drupal\Core\Path\PathAliasInterface $path_alias */
    +      $path_alias = $this->getPathAliasEntityStorage()->load($pid);
    

    I'm curious why we don't use assert($path_alias instanceof PathAliasInterface)?

  2. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +66,121 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +      $original_values = [
    +        'source' => $path_alias->getPath(),
    +        'alias' => $path_alias->getAlias(),
    +        'langcode' => $path_alias->get('langcode')->value,
    +      ];
    +
    

    There is no checking for the return value, this would cause a fatal error when something passes along a wrong PID. Is this fine?

  3. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +66,121 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
       public function load($conditions) {
    ...
    +    $query = $this->getPathAliasEntityStorage()->getQuery();
    +    $query->accessCheck(FALSE);
    

    It would be nice to document why we don't want access() here

  4. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +66,121 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +        'langcode' => $path_alias->get('langcode')->value,
    

    Is there a reason you don't use language()->getId()?

  5. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,115 @@
    +/**
    + * Defines the path_alias entity class.
    + *
    ...
    +class PathAlias extends ContentEntityBase implements PathAliasInterface {
    

    I'm wondering whether this path alias entity should be marked as internal. I would expect that the prefered interacton is still via the path alias storage class?

  6. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,115 @@
    +    $fields['path'] = BaseFieldDefinition::create('string')
    +      ->setLabel(new TranslatableMarkup('Path'))
    +      ->setDescription(new TranslatableMarkup('The path that this alias belongs to.'))
    +      ->setRequired(TRUE)
    +      ->setDefaultValue('');
    +
    +    $fields['alias'] = BaseFieldDefinition::create('string')
    +      ->setLabel(new TranslatableMarkup('Alias'))
    +      ->setDescription(new TranslatableMarkup('The alias used with this path.'))
    +      ->setRequired(TRUE)
    +      ->setRevisionable(TRUE)
    +      ->setDefaultValue('');
    

    Could/should we add constrains that validate that these paths are valid?

  7. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,115 @@
    +    // The language code of a path alias should not be able to change between
    +    // revisions.
    +    $fields['langcode']
    +      ->setRevisionable(FALSE)
    +      ->setDefaultValue(LanguageInterface::LANGCODE_NOT_SPECIFIED);
    

    This comment should explain why, not what :)

  8. +++ b/core/lib/Drupal/Core/Path/PathAliasInterface.php
    @@ -0,0 +1,48 @@
    +  /**
    +   * Gets the source path of the alias.
    +   *
    +   * @return string
    +   *   The source path.
    +   */
    +  public function getPath();
    +
    +  /**
    +   * Sets the source path of the alias.
    +   *
    +   * @param string $path
    +   *   The source path.
    +   *
    +   * @return $this
    +   */
    +  public function setPath($path);
    

    It feels like using source() would be a better name aka. more obvious of what is going on.

  9. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,147 @@
    +    if ($langcode == LanguageInterface::LANGCODE_NOT_SPECIFIED) {
    ...
    +    elseif ($langcode > LanguageInterface::LANGCODE_NOT_SPECIFIED) {
    

    I'm confused about the conditions here. == points towards a string, but > points towards a number, but the constant is 'und'?

Berdir’s picture

Better to know than to guess :)

Test scenario, disabled page and data cache bins but enabled render cache and dynamic page cache, so enough to hit the alias check but not too much more. Testing with a plain ab -n 500. A node at /node/1 and aliased as /foo.

Both HEAD and with the patch have around 60 requests/s (16ms per request), varies a bit, sometimes as low as 55requests/s but the difference between the patch is definitely within the variance between each run. At first I tested with the data cache bin still enabled but then realized I was hitting the route cache and inbound path processing didn't even run. But having that enabled or disabled did not result in a measurable difference with ab, so there's simply too much other stuff going on that .

That's not very surprising, just like @amateescu said, that code path is basically unchanged.

Still need to review it, one thing I did run into is the constructor change in AliasStorage, once when applying the patch and running drush updb, it died with a fatal error on the old constructor argument. That's a tricky situation, one thing we could try is remove the type hint and check what we get but even that would break subclasses I think.

dawehner’s picture

Still need to review it, one thing I did run into is the constructor change in AliasStorage, once when applying the patch and running drush updb, it died with a fatal error on the old constructor argument. That's a tricky situation, one thing we could try is remove the type hint and check what we get but even that would break subclasses I think.

In general this is an easy to avoid problem, so let's just avoid it.

amateescu’s picture

@dawehner, thanks for the review!

Re #110:

  1. Because we don't really need an assertion there, that comment is just for IDE type hinting :)
  2. I think it's fine, it's no different than what the current code does: https://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Path/AliasS...
  3. Sure, added a comment in both places.
  4. That's because of a bug in the entity system which doesn't return the right language object for non-translatable entity type. I'll open an issue for it and link it here :)
  5. Not at all, contrib/custom should interact with the new entity type just like with every other one that we have in core, including REST API calls.
  6. Yup, those are added in #3007832: Convert custom path alias forms to entity forms.
  7. Ok, expanded the comment a bit :)
  8. Not sure.. both @catch and I thought that path feels better. @Berdir, any opinion on this point?
  9. That's just code moved from the current storage, I'd prefer to not change anything about it that is not strictly required for this issue..

Also fixed the problem mentioned in #111 with the old AliasStorage constructor.

amateescu’s picture

Regarding #110.8, how about calling the field system_path instead of just path -> $path_alias->getSystemPath() / $path_alias->setSystemPath()? That should make it's purpose perfectly clear.

Status: Needs review » Needs work

The last submitted patch, 113: 2336597-113.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
95.65 KB

Status: Needs review » Needs work

The last submitted patch, 116: 2336597-116.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
95.65 KB
644 bytes

Fixed that test as well.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

FileSize
95.5 KB

Reroll. I'd like to get this fixed before Drupal 9.

Status: Needs review » Needs work

The last submitted patch, 120: 2336597-120.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jibran’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
98.76 KB

So entity without the title field cokes up JSON:API tests. Also, Drupal\Tests\jsonapi\Functional\PathAliasTest::testRelationships test doesn't handle 'und' languange very well. I'll ask JSON:API experts a ping in slack to help me with it.

jibran’s picture

Status: Needs review » Needs work

Here is some details about what I commented in #122:

  1. +++ b/core/modules/jsonapi/tests/src/Functional/PathAliasTest.php
    @@ -0,0 +1,107 @@
    +  public static $modules = ['user'];
    

    Even though there is no uid or revsion_uid field for PathAlias entity. I still have to mention this because \Drupal\Tests\jsonapi\Functional\ResourceTestBase::setUpFields() adds a user ref field.

  2. +++ b/core/modules/jsonapi/tests/src/Functional/PathAliasTest.php
    @@ -0,0 +1,107 @@
    +      'langcode' => 'en',
    ...
    +          'langcode' => 'en',
    ...
    +          'langcode' => 'en',
    

    PathAlias sets default value of 'langcode' to 'und' but \Drupal\Tests\jsonapi\Functional\ResourceTestBase::doTestRelationshipMutation show 405 for 403 PATCH request.

  3. As there is not 'title' field for PathAlias entity Drupal\Tests\jsonapi\Functional\PathAliasTest::testPostIndividual fails with following error.
    1) Drupal\Tests\jsonapi\Functional\PathAliasTest::testPostIndividual
    Undefined index: 
    
    modules/jsonapi/tests/src/Functional/ResourceTestBase.php:883
    modules/jsonapi/tests/src/Functional/ResourceTestBase.php:1887
Wim Leers’s picture

Go @jibran! 🥳

jibran’s picture

Status: Needs work » Needs review
FileSize
563 bytes
98.85 KB

Let's try this.

Status: Needs review » Needs work

The last submitted patch, 125: 2336597-125.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jibran’s picture

Status: Needs work » Needs review
FileSize
616 bytes
587 bytes
98.93 KB

Some more progress.

Status: Needs review » Needs work

The last submitted patch, 127: 2336597-127.patch, failed testing. View results

jibran’s picture

Status: Needs work » Needs review
FileSize
1.49 KB
100.02 KB

HT by @gabesullice worked great. This patch should be green unless I broke something in JSON:API. I'm going to credit @gabesullice for helping me out in the slack and during API first meeting.

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 132: 2336597-132.patch, failed testing. View results

jibran’s picture

amateescu’s picture

Status: Needs work » Needs review
FileSize
101.13 KB
1.06 KB

This should fix the test failure.

amateescu’s picture

jibran’s picture

I think we need to get #3012050: Prepare for the conversion of path aliases to entities in committable condition to commit this patch to the core.

amateescu’s picture

Fixed the description of the alias base field, as pointed out by @chx, and brought over a few improvements from #3007832: Convert custom path alias forms to entity forms that should be part of this initial patch.

jibran’s picture

amateescu’s picture

Status: Needs work » Needs review

It can stay at NR so people can review it.

amateescu’s picture

FileSize
107.92 KB
6.23 KB

Fixed the JSON:API thing.

amateescu’s picture

While working on #3012050: Prepare for the conversion of path aliases to entities, I found that there's a need for looking up a path alias by system path (source) in a few places, most notably \Drupal\path\Plugin\Field\FieldType\PathFieldItemList::computeValue() (core) and \Drupal\pathauto\AliasStorageHelper::loadBySource() (contrib).

The \Drupal\Core\Path\AliasStorageInterface::lookupPathAlias() from core is already doing that, but it's only returning the alias string, while those two methods above need a few more information (like id and langcode), so I changed the corresponding methods from the newly introduced \Drupal\Core\Path\PathAliasStorage entity storage class to return a full path_alias entity object, which is usable in all the places where we need to find a path alias for a given system path.

Important note for reviewers: This change means that AliasManager::getPathByAlias() and AliasManager::getAliasByPath() now have to do an additional (entity) cache get if the given system path or alias are not in the statically cached list of aliases. However, AliasManager::getAliasByPath() is very aggressive with populating that static cache via $storage->preloadPathAlias(), so I don't think that the entity cache get is actually reached in most cases (i.e. when the "preload cache" is warm).

amateescu’s picture

FWIW, #3012050-12: Prepare for the conversion of path aliases to entities passes all tests, so Pathauto is ready for this core change :)

Wim Leers’s picture

#141: that looks an awful lot like the changes being made in #3057175: Implementation of user name in JSON:API can result in overwriting data! :) That should land very soon, and will make this patch slightly simpler again 🥳

(Thanks to @jibran for pointing me to #141!)

Status: Needs review » Needs work

The last submitted patch, 142: 2336597-142.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
109.8 KB
1.99 KB

@Wim Leers, glad to hear that!

Fixed the ordering order :)

jibran’s picture

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -21,7 +19,7 @@ class AliasStorage implements AliasStorageInterface {
@@ -37,6 +35,20 @@ class AliasStorage implements AliasStorageInterface {

Do we need to deprecate this class and its methods? Maybe in a follow-up?

amateescu’s picture

jibran’s picture

Thanks!

I was thinking about the performance impact of the conversation. Just putting my thoughts down here:

The only things concerning for me are changes like these:

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -63,227 +77,127 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
-      return $select
-        ->fields(static::TABLE)
-        ->orderBy('pid', 'DESC')
-        ->range(0, 1)
-        ->execute()
-        ->fetchAssoc();
...
+    $result = $query
+      ->sort('id', 'DESC')
+      ->range(0, 1)
+      ->execute();
+    $entities = $this->getPathAliasEntityStorage()->loadMultiple($result);
...
+    $path_alias = reset($entities);
+    if ($path_alias) {
+      return [
+        'pid' => $path_alias->id(),
+        'source' => $path_alias->getPath(),
+        'alias' => $path_alias->getAlias(),
+        'langcode' => $path_alias->get('langcode')->value,
+      ];

We are replacing a simple select query with EFQ + loading of the entity using entity API but static and built-in entity cache will help here so I think this is not awfully concerning.

Recently, I worked on a client project on which we installed the redis module. Ever since we changed all the cache backends to redis, MySQL url_alias select (~1000 calls per min on ~200 requests per min) is the second most time consuming query after MySQL key_value select. After path aliases conversion to entities MySQL url_alias select will go away becuase of built in entity cache in Durpal 8. This was the most interesting thing for me which intrigued me to work on this patch back in #120.

Do we need to benchmark this patch to highlight its importance of the performance gains after path aliases conversion?

jibran’s picture

For me this issue is RTBC but it would be great to get some overall reviews from API-First team, entity subsystem maintainer other than @amateescu, @Berdir as pathauto maintainer and @catch as Path subsystem maintainer.

Wim Leers’s picture

#149: That's a super super interesting perspective rooted in real world challenges. Thank you for sharing that!

@Fabianx added the Needs profiling tag in #26, @amateescu removed it in #100. I also asked about this in #107.2. @amateescu responded in #108 that it doesn't because the parts of the alias system in the critical path are not changed, which framework manager and core committer @larowlan already pointed out in #97.2.

But … that seems to contradict what @jibran wrote in #149, where he specifically calls out that changing the critical path to using Entity API's loading + built-in caching improves performance for him. @amateescu, can you clarify?

amateescu’s picture

@Wim Leers, @Berdir profiled the impact of this patch on the critical path in #111 and the results were good, as expected, because it's not changed. Note that _saving_ a path alias is not in the critical path at all :)

Wim Leers’s picture

Aha! GREAT! I'll do a deep dive on the patch then. Meanwhile, it'd be super useful to get an updated issue summary. That will be necessary for this to be committed anyway.

(The change record at https://www.drupal.org/node/3013865 already looks 👍 Although I think a valuable addition would be to include a recommendation for how a contrib module can support both 8.7 and 8.8 at the same time: presumably by implementing both hooks and maximally reusing code? Or perhaps not? That's exactly why a recommendation would be valuable!)

Ghost of Drupal Past’s picture

My interest in this patch comes from, if I gather correctly, this will make aliases work with the workspaces module which we need. We currently coded up a drafter module to work around the problem of workspaces not being ready (I will release this shortly, I already wrote a blogpost too). Without further rambling:

Amazing, incredible work. A few nits:

  1. The table is no longer created on demand. This is OK because on demand was more necessary for simpletest to speed up installs. Its' more than OK actually because it simplifies the code.
  2. I do not see a discussion on the renaming of pid to id. Core traditionally uses nid,tid,cid,vid etc but then again there's no reason to rote following old habits. I do not want to nitpick, I just would like to see some reasoning for this rename. Maybe in the far future we want to drop entity keys and just make all of them id / rid (or vid) / uid / langcode etc?
  3. "The language code of a path alias should not be able to change between revisions" -- that sounds good ... "because path aliases are not translatable" huh that sounds strange on the other hand.

The post update function scares me... I mean, yeah, it's using the entity API is real nice but the performance implications scare me a bit. I feel this could be solved with a few INSERT ... SELECT at a much much higher speed. Was this option considered?

Wim Leers’s picture

#3057175: Implementation of user name in JSON:API can result in overwriting data just landed, the part of the patch added in #141 should become much simpler now. :)

amateescu’s picture

I was hoping for that as well, but apparently that's not the case, we still have to complement the changes that were done in that issue.

Status: Needs review » Needs work

The last submitted patch, 156: 2336597-156.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
106.55 KB
5.57 KB

I shouldn't have tried to fix JSON:API's test coverage in this patch, reverted most of the changes from #156 and kept only what's strictly needed.

jibran’s picture

Here is an interesting point about upgrade path, in pervious patch releases we have updated existing entities(block content, menu link, taxonomy terms) to revisionable/publishable and we have seen developers battling with the upgrade paths for those entities.

In this case, the upgrade path is pretty straight-forward:

  • Install the new entity type.
  • Load data from the old table.
  • Create and save new entities.

Installing a new entity type is way simpler than upgrading an existing one. It also has fewer edge cases.

I can't predict the future and it is just an observation so don't quote me on this. :)

Berdir’s picture

Starting with a code review to get back into it. I'll try to reply to some comments above, need to catch up on what exactly I did in regards to performance testing and if the logic is really still the same (see below).

Also, I've been testing the upgrade path on a site with around 700k or so aliases and yes, as #159 said, this update function is simpler than existing entity => revisionable update functions, *but* that doesn't mean is faster. Based on some rough calculations, it would take about 2h for this site (and I'm sure there are larger ones out there) and increasing the entity batch size only helped marginally.

So what we agreed on is to switch to raw INSERT queries. I don't think we can do a single INSERT INTO ... SELECT FROM, because we also need to generate UUID's, but with a few bulk insert queries, I still expect this to be massively faster than saving entities.

  1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +77,127 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +        'langcode' => $path_alias->get('langcode')->value,
    

    could use language()->getId()?, I see that came up before, but not sure if that follow-up issue you promised exists? :)

  2. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +77,127 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +    // API functions should be able to access all entities regardless of access
    +    // restrictions. Those need to happen on a higher level.
    +    $query->accessCheck(FALSE);
    

    not quite sure about the comment here and also another one above. The necessary generic statement ((all) api functions should be able to access all entities) isn't actually always true.

    Maybe just: // Ignore access restrictions for this API?

  3. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +77,127 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +
    +    $result = $query->execute();
    +    $entities = $this->getPathAliasEntityStorage()->loadMultiple($result);
    +    $this->getPathAliasEntityStorage()->delete($entities);
    

    we use the storage here 3x, maybe store in $storage, then you could possibly even do something like $storage->delete($storage->loadMultiple())?

  4. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -313,7 +227,7 @@ public function aliasExists($alias, $langcode, $source = NULL) {
       public function languageAliasExists() {
         try {
    -      return (bool) $this->connection->queryRange('SELECT 1 FROM {url_alias} WHERE langcode <> :langcode', 0, 1, [':langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED])->fetchField();
    +      return (bool) $this->connection->queryRange('SELECT 1 FROM {' . static::TABLE . '} WHERE langcode <> :langcode', 0, 1, [':langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED])->fetchField();
         }
         catch (\Exception $e) {
    

    is there a reason we're not yet updating these other functions?

  5. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,128 @@
    +    $fields['path'] = BaseFieldDefinition::create('string')
    +      ->setLabel(new TranslatableMarkup('System path'))
    +      ->setDescription(new TranslatableMarkup('The path that this alias belongs to.'))
    +      ->setRequired(TRUE)
    +      ->setDefaultValue('');
    +
    +    $fields['alias'] = BaseFieldDefinition::create('string')
    +      ->setLabel(new TranslatableMarkup('Path alias'))
    +      ->setDescription(new TranslatableMarkup('An alias used with this path.'))
    +      ->setRequired(TRUE)
    +      ->setRevisionable(TRUE)
    +      ->setDefaultValue('');
    

    does an empty default really make sense, does that just reflect the existing schema?

  6. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,128 @@
    +    // The language code of a path alias should not be able to change between
    +    // revisions because path aliases are not translatable.
    +    $fields['langcode']
    +      ->setRevisionable(FALSE)
    +      ->setDefaultValue(LanguageInterface::LANGCODE_NOT_SPECIFIED);
    

    hm. I haven't fully thought this through, but we had a few really weird problems in the past when making fields not revisionable.

    They are not translatable, but the language can still change, typically between language specific and all languages. that would mean that doing this in a workspace would also immediately affect the default revision?

  7. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,128 @@
    +   * {@inheritdoc}
    +   */
    +  public function getPath() {
    +    return $this->get('path')->value;
    

    This would likely be another case where the API from #2580551: Optimize getCommentedEntity() would come in handy.

    One of the slowest parts about loading an entity is accessing the first value through a field, as we have to load the field definitions, initialize multiple objects and so on.

  8. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,128 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function label() {
    +    return $this->getAlias();
    +  }
    +
    

    huh. if we actually have a custom implementation and don't just set the key, would it make sense to do something like "$source => $alias" instead of just alias?

  9. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,156 @@
    +   */
    +  public function preloadPathAlias($preloaded, $langcode) {
    +    $select = $this->database->select($this->baseTable)
    +      ->fields($this->baseTable, ['path', 'alias']);
    +
    +    if (!empty($preloaded)) {
    +      $conditions = new Condition('OR');
    +      foreach ($preloaded as $preloaded_item) {
    +        $conditions->condition('path', $this->database->escapeLike($preloaded_item), 'LIKE');
    +      }
    +      $select->condition($conditions);
    +    }
    +
    +    $this->addLanguageFallback($select, $langcode);
    +
    +    // We order by ID ASC so that fetchAllKeyed() returns the most recently
    +    // created alias for each source. Subsequent queries using fetchField() must
    +    // use ID DESC to have the same effect.
    +    $select->orderBy('id', 'ASC');
    +
    +    return $select->execute()->fetchAllKeyed();
    +  }
    

    Hm, wondering if we want to use a different name for this, as in context of an entity storage, it doesn't actually pre-"load" (the entity)?

    Below we have the lookupX methods, those then however do an actual full load. I'm wondering if that changed since my previous (performance) review, because I'd definitely expect that to be a visible performance issue.

  10. +++ b/core/lib/Drupal/Core/Path/PathAliasStorage.php
    @@ -0,0 +1,156 @@
    +  public function lookupBySystemPath($path, $langcode) {
    +    // See the queries above. Use LIKE for case-insensitive matching.
    +    $select = $this->database->select($this->baseTable)
    +      ->fields($this->baseTable, ['id'])
    +      ->condition('path', $this->database->escapeLike($path), 'LIKE');
    +
    +    $this->addLanguageFallback($select, $langcode);
    +
    +    $select->orderBy('id', 'DESC');
    +
    +    $result = $select->execute()->fetchCol();
    +    if ($id = reset($result)) {
    +      return $this->load($id);
    

    this. seems a bit strange now, apart from the problem that entity query is slower than a DB query, this seems doable as an entity query + load if that's really what we want to do?

  11. +++ b/core/modules/path/src/Controller/PathController.php
    @@ -85,12 +85,12 @@ public function adminOverview(Request $request) {
           // @todo Should Path module store leading slashes? See
           //   https://www.drupal.org/node/2430593.
    -      $row['data']['alias'] = Link::fromTextAndUrl(Unicode::truncate($data->alias, 50, FALSE, TRUE), Url::fromUserInput($data->source, [
    +      $row['data']['alias'] = Link::fromTextAndUrl(Unicode::truncate($data->alias, 50, FALSE, TRUE), Url::fromUserInput($data->path, [
             'attributes' => ['title' => $data->alias],
           ]))->toString();
    -      $row['data']['source'] = Link::fromTextAndUrl(Unicode::truncate($data->source, 50, FALSE, TRUE), Url::fromUserInput($data->source, [
    +      $row['data']['source'] = Link::fromTextAndUrl(Unicode::truncate($data->path, 50, FALSE, TRUE), Url::fromUserInput($data->path, [
             'alias' => TRUE,
    

    isn't this an API change?

    This uses the existing getAliasesForAdminListing() method and desn't have to change it. Should that method ensure that they properties remains the same?

    I guess as a follow-up, we'd look into converting this into an entity list builder + view?

  12. +++ b/core/modules/simpletest/src/KernelTestBase.php
    @@ -386,7 +386,7 @@ public function containerBuild(ContainerBuilder $container) {
         if ($container->hasDefinition('path_processor_alias')) {
    -      // Prevent the alias-based path processor, which requires a url_alias db
    +      // Prevent the alias-based path processor, which requires a path_alias db
           // table, from being registered to the path processor manager. We do this
           // by removing the tags that the compiler pass looks for. This means the
    

    I guess the comment could be be slightly improved to say it needs to have the entity schema installed or so, but it's a deprecated class anyway (but the same might exist in the new base class)

  13. +++ b/core/modules/system/system.module
    @@ -1416,26 +1417,37 @@ function system_block_view_system_main_block_alter(array &$build, BlockPluginInt
      */
    -function system_path_insert($path) {
    -  \Drupal::service('path.alias_manager')->cacheClear($path['source']);
    +function system_path_alias_update(EntityInterface $entity) {
    +  /** @var \Drupal\Core\Path\PathAliasInterface $entity */
    +  $alias_manager = \Drupal::service('path.alias_manager');
    +  $alias_manager->cacheClear($entity->getPath());
    +  $alias_manager->cacheClear($entity->original->getPath());
     }
    

    I was hoping we'd finally find a way to do this better (not have hooks in system.module), but the entity system doesn't have events yet, so.. :)

  14. +++ b/core/modules/views/tests/src/Functional/Update/ExposedFilterBlocksUpdateTest.php
    @@ -46,7 +46,11 @@ public function testViewsPostUpdateExposedFilterBlocksWithoutBlock() {
    -    $this->container->get('module_installer')->uninstall(['block']);
    +    // We also need to uninstall the menu_link_content module because
    +    // menu_link_content_entity_predelete() invokes alias processing and we
    +    // don't have a working path alias system until
    +    // system_post_update_convert_path_aliases_to_entities() runs.
    +    $this->container->get('module_installer')->uninstall(['menu_link_content', 'block']);
    

    the comment will be a bit confusing when commit, because the "also need to" only makes sense in the context of this diff when you know the old value. Maybe explain why both need to be uninstalled or just drop the also?

  15. +++ b/core/tests/Drupal/FunctionalTests/Hal/PathAliasHalJsonAnonTest.php
    @@ -0,0 +1,29 @@
    +
    +/**
    + * @group hal
    + */
    +class PathAliasHalJsonAnonTest extends PathAliasHalJsonTestBase {
    

    How nice, 20 new test classes :p

    I'm sure Wim will not like it, but one option could be to split out the path entity json/rest (?) test coverage to a follow-up, then we could skip all those label-related changes here.

  16. +++ b/core/tests/Drupal/FunctionalTests/Rest/PathAliasXmlAnonTest.php
    @@ -0,0 +1,28 @@
    +
    +namespace Drupal\FunctionalTests\Rest;
    +
    +use Drupal\Tests\rest\Functional\AnonResourceTestTrait;
    

    interesting location for these tests, too. Would IMHO make more sense for them to be in rest/jsonapi, now we have Tests in core/tests that need those modules.

Berdir’s picture

> @Wim Leers, @Berdir profiled the impact of this patch on the critical path in #111 and the results were good, as expected, because it's not changed.

Yes, just like I suspected :) Yes, I did test it back then, but the lookup* methods back then returned the raw value, so really nothing changed. That apparently changed now, which definitely does change the code in the critical path.

Recently, I worked on a client project on which we installed the redis module. Ever since we changed all the cache backends to redis, MySQL url_alias select (~1000 calls per min on ~200 requests per min) is the second most time consuming query after MySQL key_value select. After path aliases conversion to entities MySQL url_alias select will go away becuase of built in entity cache in Durpal 8. This was the most interesting thing for me which intrigued me to work on this patch back in #120.

Yeah, no, that's not how this works, I wish it were that simple :)

First, you still have a query to find the alias ID, that isn't going away. If the entity is then cached, then in the best-case scenario, you end up with the same amount of queries. *If* the entity is cached. If not, then you end up with two (?) additional queries against the base and the revision table to then actually load the data, instantiate the entity object, and then access the information through it, which needs to get the field definitions, instantiate field item and list objects, ....

There are two relevant scenarios for performance of the alias storage:

A) Every incoming request (excluding things that where a internal page cache hit) needs to do a lookup by alias to translate it back into the system path. That happens very early in the request and is very much in the critical path. redirect.module has a similar scenario, what we did there is add \Drupal\redirect\RedirectRepository::findMatchingRedirect(), that with minimal dependencies executes a raw query with query() to find a possibly existing redirect ID. The difference there is that actually finding a redirect and returning it isn't really in the critical path anymore, because most requests won't have that. For path, that's different, having an alias is the rule not the exception and it needs to be as fast as possible as well.

B) The other side is that every URL we *build* while creating a page needs to look up if there is an alias, so we have lots of queries against that table on sites with a lot of teasers, menu links and so on. We spent *a lot* of time optimizing that in D8 and before, for example with the whitelist (unchanged by this) and also the preloading. I didn't profile it yet ( will do so on my large site after we have an upgrade path that doesn't take 2h), but I expect that we have a considerable performance hit there when the preload cache isn't ready yet. Which will be the case for many requests as that is basically per path.

As commented above, the lookup/preload methods are currently inconsistent in what they return. My basic, not fully thought through, proposal would be to keep these functions that really are in the critical path as they are now, with direct DB queries and returning raw array results in place. I'd even suggest to keep them as a separate AliasStorage service/interface. And then we only deprecate all the stuff that isn't actually in the critical path and the entity system does better anyway (save(), load(), delete()*, that admin listing stuff). Going back to the original use case, that would allow to implement a decorator of this service that stores the data in redis/memcache and then you could really get to a point where you can avoid queries against the url_alias/path_alias table at all. Given your redis/memcache instance has enough memory to store all that information.

* For delete(), I realized that we could run into scalability issues, simple use case: Deleting all DE aliases because you just removed that language (not that we actually do that right now...). You might have 100k of them. And right now delete() is a DELETE query with conditins, so that would kinda work. But the new implementation needs to load all entities and then delete them. I don't think we can or have to do something about it, except maybe document that in the CR, that you should use batch for that? (pathauto btw already does that, as it also has to clean up some of its own data).

amateescu’s picture

Thanks @Charlie ChX Negyesi and @Berdir for the reviews!

Posting just a quick patch for now which changes the update function to do raw insert queries, and also fixes #160.6. I'll write a more detailed reply in a followup comment :)

Status: Needs review » Needs work

The last submitted patch, 162: 2336597-162.patch, failed testing. View results

amateescu’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
101.79 KB
28.7 KB

Here we go:

Re @Charlie ChX Negyesi in #154:

2. I discussed this with @catch when I started working on this patch last year, and the consensus was that we should rename the fields/columns to something that makes (more) sense, for example "source" to "path". The "pid" to "id" change was based on the fact that the entity type ID is "Path Alias", so the short version would have to be "paid", and we both though that a simple "id" would be better :)

3. That comment no longer exists because the langcode field is revisionable now.

4. The update path was changed to raw insert queries in #162, and apparently it's ~20x faster according to @Berdir's test.

Re @Berdir in #160:

  1. Right, I still need to open that issue and link it in the patch. Keeping this as TODO for now.
  2. Updated the comment with your recommended test.
  3. Fixed. The part about mentioning in the change record that large deletes should be done in a batch is still TODO.
  4. No other reason than me forgetting about it. Done!
  5. It doesn't make sense at all because they are both required fields. Removed the default value.
  6. Ouch.. very good point! Changed the langcode field to be revisionable.
  7. Right, we really should look into optimizing these parts of the Entity API, especially now that we have a ton of test coverage that will help us catch unintended side effects.
  8. As discussed, let's keep this point as a TODO investigation for one of the followup issues.
  9. preloadPathAlias is no longer part of the path alias entity storage class ;)
  10. Same for the two lookup* methods.
  11. Good point! Updated the API method to return the same data array as in HEAD. And yes, this controller is being converted to a list builder in #3011807: Convert the path alias admin overview to a list builder
  12. Re-worded the comment in all three places where it exists.
  13. *shrug* :)
  14. Dropped the "also".
  15. I spoke to Wim about it and he'd prefer to keep them in this patch. I don't really have a strong opinion..
  16. Interesting point, I'll let Wim decide what to do about the location of the tests :)

Re #161: I removed all the performance-sensitive methods from the new entity storage class and kept them in the existing path.alias_storage service. Let's decide whether we want to move them to a different repository class/service in #2233595: Deprecate the custom path alias storage.

Ghost of Drupal Past’s picture

I was thinking INSERT INTO path_alias SELECT *,uuid() FROM url_alias or some variant thereof. PostgreSQL has uuid_in(md5(random()::text || clock_timestamp()::text)::cstring); per https://stackoverflow.com/a/21327318/308851. This wouldn't be 20% faster, it'd be ~infinitely faster :)

Berdir’s picture

FWIW, it's ~20x faster (based on the estimated 2h in the previous patch) not 20% :)

If you think a db-agnostic upgrade path is possible then that could be interesting, there's also an issue to add a revision UUID and the upgrade path is one of the things it got stuck on. but my personal opinion is that this is fast enough.

A single query that would write 700k or possibly millions of records at once might run into other limits, e.g. transaction size on complex mysql setups with multiple servers?

Ghost of Drupal Past’s picture

My bad, sorry. Sure, let's keep like this. Let's explore my idea in a separate issue. And let's hope people do statement/hybrid replication :)

jibran’s picture

Excellent analysis @Berdir.

First, you still have a query to find the alias ID, that isn't going away. If the entity is then cached, then in the best-case scenario, you end up with the same amount of queries. *If* the entity is cached. If not, then you end up with two (?) additional queries against the base and the revision table to then actually load the data, instantiate the entity object, and then access the information through it, which needs to get the field definitions, instantiate field item and list objects, ....

This observation is spot on I was running some before and after scenarios and it always comes back to time spent querying url_alias or after the patch path_alias. The patch did add two extra queries to key_value table on each page load.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -44,17 +58,19 @@ class AliasStorage implements AliasStorageInterface {
    @@ -63,227 +79,186 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    
    @@ -63,227 +79,186 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
           throw new \InvalidArgumentException(sprintf('Alias path %s has to start with a slash.', $alias));
         }
     
    -    $fields = [
    -      'source' => $source,
    -      'alias' => $alias,
    -      'langcode' => $langcode,
    

    I guess the AliasStorage ands its current state with this patch is likely the trickiest part of this patch.

    With our strict deprecation rules now, it does feel a bit like cheating as what we are doing here is basically soft-deprecating a bunch of methods like save(), delete() and load() that we definitely want to get rid of. Actually deprecating them would force us to replace all usages.

    On the other side, there are good arguments to do it like this:
    * We have to stop *somewhere* with this initial patch. If we convert this, what's next? convert the alias forms to entity forms? convert the overview controller to a list builder/view?
    * Doing so would considerably increase (double?) the size of this already non-trivial patch, there are about 50 usages of the alias storage service at the moment in core, mostly in tests.

    I guess the decision will be on whether the current state of the patch is something we could live with for 8.8 (and 8.9 and 9.0) in the worst case of not bein able to land the follow-ups in time.

  2. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
    @@ -76,7 +76,7 @@ public function delete($conditions);
        * @return string[]
    -   *   Source (keys) to alias (values) mapping.
    +   *   System path (keys) to alias (values) mapping.
        */
       public function preloadPathAlias($preloaded, $langcode);
     
    @@ -103,7 +103,7 @@ public function lookupPathAlias($path, $langcode);
    
    @@ -103,7 +103,7 @@ public function lookupPathAlias($path, $langcode);
        * The default implementation performs case-insensitive matching on the
        * 'source' and 'alias' strings.
        *
    -   * @param string $path
    +   * @param string $alias
        *   The path to investigate for corresponding system URLs.
        * @param string $langcode
        *   Language code to search the path with. If there's no path defined for
    @@ -112,7 +112,7 @@ public function lookupPathAlias($path, $langcode);
    
    @@ -112,7 +112,7 @@ public function lookupPathAlias($path, $langcode);
        * @return string|false
        *   A Drupal system path, or FALSE if no path was found.
        */
    -  public function lookupPathSource($path, $langcode);
    +  public function lookupPathSource($alias, $langcode);
    

    Wondering if we want to drop these variable/documentation renames (for now anyway), they're the only changes in this interface. For the return value, we also keep the existing keys and while these might actually be the methods that stick, we can still do that in the follow-up that will deprecate all methods on this service that we no longer want to maintain.

  3. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,122 @@
    +   * {@inheritdoc}
    +   */
    +  public function label() {
    +    return $this->getAlias();
    +  }
    

    Actually defining 'alias' as the label entity key would probably have the same effect and not require quite a few changes in the rest/json base classes. But we discussed that we might want to change this implementation and e.g. include more than one field, so I'm fine with keeping it like this.

  4. +++ b/core/modules/menu_link_content/tests/src/Kernel/PathAliasMenuLinkContentTest.php
    @@ -28,6 +28,7 @@ protected function setUp() {
     
         $this->installEntitySchema('user');
         $this->installEntitySchema('menu_link_content');
    +    $this->installEntitySchema('path_alias');
    

    This will break a bunch of contrib tests, which is fine, the annoying part is that they can't just add this line and move on because that will in turn fail tests on 8.7.

    Maybe we can add a snippet to the change record that combines it with a class/entity type exists check before trying to install it?

  5. +++ b/core/modules/views/tests/src/Functional/Update/ExposedFilterBlocksUpdateTest.php
    @@ -46,7 +46,11 @@ public function testViewsPostUpdateExposedFilterBlocksWithoutBlock() {
     
    -    $this->container->get('module_installer')->uninstall(['block']);
    +    // We need to uninstall the menu_link_content module because
    +    // menu_link_content_entity_predelete() invokes alias processing and we
    +    // don't have a working path alias system until
    +    // system_post_update_convert_path_aliases_to_entities() runs.
    +    $this->container->get('module_installer')->uninstall(['menu_link_content', 'block']);
    

    Hm, last time I checked this I focused only on the comment, but now I'm wondering, aren't we cheating here?

    The call chain where this fails is pretty wild, it's actually the uninstallation of block, which in turn uninstalls first block_content, then we delete the basic default bundle, which in turn deletes its default field, and this finally calls the menu_link_content hook and there $entity->uriRelationships().

    So this specific case is self-inflicted by uninstalling a module before running updates, but couldn't this also happen in a real update function as well, before our update runs? You're not *supposed* to use the entity type, but installing/uninstalling modules in update hooks isn't uncommon and who knows what that does.

    What about adding a try/catch in that hook instead? maybe with a watchdog_exception() and explanation why it can fail? That makes the test pass too.

    PS: This also has one of the those post updates that you mentioned.

  6. +++ /dev/null
    @@ -1,33 +0,0 @@
    -/**
    - * Base class for Path/URL alias integration tests.
    - */
    -abstract class PathUnitTestBase extends KernelTestBase {
    -
    

    I suppose its fine to remove this, we basically have to remove the fixtures and http://grep.xnddx.ru/search?text=PathUnitTestBase doesn't find anything except that single core class using it. And there's no reason to do so.

Berdir’s picture

Ah, the change record needs a few updates (the part about the storage providing some methods isn't true anymore). Maybe we want to wait until we mention the AliasStorage at all until we really deprecate any of these methods?

And I'm sure this is important enough to also need a release note snippet in the issue summary. Speaking of that, maybe you can cover some of my notes above in there and specifically explain what is done here in this issue and what's done follow-ups?

amateescu’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record updates, -Needs release note, -Needs issue summary update
FileSize
101.02 KB
5.96 KB

Thanks, @Berdir! I've done all the updates to the CR and IS, added a release note snippet and fixed the points from #169 except for 5.

For that point, I started to implement your proposal and do a try/catch in menu_link_content_entity_predelete(), but then I remembered that we recently made a change to not use the alias system during updates exactly for this purpose, so the scenario for uninstalling a module in an update function should work fine. The change in testViewsPostUpdateExposedFilterBlocksWithoutBlock() is, as you said, self-inflicted because we choose to uninstalling a module _outside_ of the regular db update process :)

Berdir’s picture

  1. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -129,8 +129,11 @@ public static function create(ContainerInterface $container) {
    +      // Internal entity types should never be moderated, and the 'path_alias'
    +      // entity type needs to be excluded for now.
    +      // @todo Enable moderation for path aliases after they become publishable.
    +      //   @see https://www.drupal.org/project/drupal/issues/3007669
    +      if ($entity_type->isRevisionable() && !$entity_type->isInternal() && $entity_type_id !== 'path_alias') {
             $entity_types[$entity_type_id] = $this->addModerationToEntityType($entity_type);
    

    @see should only be used in a docblock as a standalone link, I'd just write this as "after they become publishable in URL."

  2. +++ b/core/modules/views/tests/src/Functional/Update/ExposedFilterBlocksUpdateTest.php
    @@ -48,8 +48,7 @@ public function testViewsPostUpdateExposedFilterBlocksWithoutBlock() {
     
         // We need to uninstall the menu_link_content module because
         // menu_link_content_entity_predelete() invokes alias processing and we
    -    // don't have a working path alias system until
    -    // system_post_update_convert_path_aliases_to_entities() runs.
    +    // don't have a working path alias system until system_update_8802() runs.
         $this->container->get('module_installer')->uninstall(['menu_link_content', 'block']);
    

    maybe we can explain what you said in #171 in this comment, in case someone else is going to wonder about it as well?

amateescu’s picture

Fixed both points from #172 :)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Ok, I think this is ready.

This doesn't change anything in the queries that we execute in the critical path, only saving/deleting/loading with conditions is done as path entities, and those aren't used when viewing an entity, only when e.g. editing aliases or nodes.

I've also tested the update path on a very large site, which lead to refactoring it to use direct database queries and making it much faster with that. Options using a single query were discussed but I think this is good enough.

There are quite a few follow-ups but as discussed extensively, we need to split up this patch somehow and this seems like a good compromise. The next steps are defined and the plan for AliasStorage is to basically deprecate everything except those three lookup/preload methods. But for that we need to convert the migration, forms and the overview first, or we have to to touch quite a few things there twice.

catch’s picture

@jibran #168

The patch did add two extra queries to key_value table on each page load.

Is this still the case? (I'm assuming not since we've kept the critical path the same) And if so which queries are these?

There are a lot of follow-ups here, but as with Berdir's comment in #174 this seems like the best compromise for splitting things up. This patch, and the one to make things publishable, have the biggest impact on the schema and potential data issues, and are cleaning up the technical debt of path aliases being a custom table.

The other follow-ups (entity list builder, entity form) are further clean-up of technical debt that this patch enables, but doesn't really introduce (i.e. we already have those custom forms etc., we're just not getting rid of them yet). What it does introduce is a discrepancy/duplication where we have custom forms for an entity, which could be confusing for someone learning the path alias system from scratch, but that seems like an OK issue to introduce vs. having an unreviewable patch.

One thing to note is that publishable path aliases will still need to happen in 8.8.x, or be deferred to 9.1.x, because we're trying to have a moratorium on schema changes between 8.8.x and 8.9.x/9.0.x.

Very glad that the update path has been optimized to direct queries here that will save a lot of stress for a lot of people.

I've reviewed the patch a few times before, and reviewed the last few interdiffs, and don't have anything to complain about.

catch’s picture

Issue tags: +WI critical

Also tagging WI critical.

Berdir’s picture

> Is this still the case? (I'm assuming not since we've kept the critical path the same) And if so which queries are these?

Correct, that was based on a patch from before #162 which passed the queries through to the entity storage. And the two queries are from initializing the installed field storage/entity type definitions, which are kept in key_value, so every entity storage we access on a page results in two extra key_value queries. That's a side effect of using the installed definitions in 8.7 that I think we didn't fully realize back then.

jibran’s picture

Is this still the case? (I'm assuming not since we've kept the critical path the same) And if so which queries are these?

Yeah, I can confirm before and after SQL query count is the same now.

amateescu’s picture

Issue summary: View changes
Issue tags: +8.8.0 highlights
plach’s picture

Status: Reviewed & tested by the community » Needs work

The patch looks great! Aside from a few minor issues and some totally non-blocking nits this seems good to go to me.

However, one thing that I was wondering is why we need alias management to be baked in \Drupal\Core rather than in its own module. Of course, this design decision is pre-existing but the refactoring initiated here could be a good chance to extract all the alias management logic to a new path_alias module. It seems that with the deprecation follow-ups we would already be rewriting/getting rid of most of the alias-related code in \Drupal\Core\Path and in the path module.

If we decided to go this way, it might make sense to create the new module here, but it would definitely not be a blocker as long as we do that before 8.8.0.


  1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +72,186 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    -      else {
    -        $select->condition($field, $value);
    +      if ($field === 'pid') {
    ...
    -      else {
    -        $query->condition($field, $value);
    +      if ($field === 'pid') {
    

    Nit: the second ifs could be replaced with elseifs to improve clarity :)

  2. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +72,186 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +    $result = $query->execute();
    

    To match the previous logic exactly shouldn't we add a ->range(0,1) here as we do in ::load()?

  3. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -63,227 +72,186 @@ public function save($source, $alias, $langcode = LanguageInterface::LANGCODE_NO
    +    return $select->execute()->fetchAllKeyed();
    ...
    +    return $select->execute()->fetchField();
    ...
    +    return $select->execute()->fetchField();
    
    @@ -295,30 +263,19 @@ public function aliasExists($alias, $langcode, $source = NULL) {
    +    return (bool) $query->execute()->fetchField();
    ...
    +    return (bool) $this->connection->queryRange('SELECT 1 FROM {' . static::TABLE . '} WHERE langcode <> :langcode', 0, 1, [':langcode' => LanguageInterface::LANGCODE_NOT_SPECIFIED])->fetchField();
    
    @@ -332,121 +289,42 @@ public function getAliasesForAdminListing($header, $keys = NULL) {
    +    $query->addField(static::TABLE, 'id', 'pid');
    +    $query->addField(static::TABLE, 'path', 'source');
    +    return $query
    +      ->fields(static::TABLE, ['alias', 'langcode'])
    +      ->orderByHeader($header)
    +      ->limit(50)
    +      ->execute()
    +      ->fetchAll();
    ...
    +    return (bool) $query
    +      ->condition('path', $this->connection->escapeLike($initial_substring) . '%', 'LIKE')
    +      ->range(0, 1)
    +      ->execute()
    +      ->fetchField();
    

    Why are we removing the try/catch blocks in these cases?

  4. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,122 @@
    +    // Trim the alias value of whitespace and slashes. Ensure to not trim the
    +    // slash on the left side.
    +    $alias = rtrim(trim(trim($this->getAlias()), ''), "\\/");
    

    I don't understand this: isn't trim($string, '') a noop? If not, can we update the comment?

  5. +++ b/core/modules/content_moderation/src/EntityTypeInfo.php
    @@ -129,8 +129,11 @@ public static function create(ContainerInterface $container) {
    +      if ($entity_type->isRevisionable() && !$entity_type->isInternal() && $entity_type_id !== 'path_alias') {
    

    Could we check for EntityPublishedInterface instead of hard-coding the path_alias type?

  6. +++ b/core/modules/jsonapi/tests/src/Functional/PathAliasTest.php
    @@ -0,0 +1,112 @@
    +        'type' => 'path_alias--path_alias',
    ...
    +        'type' => 'path_alias--path_alias',
    

    Nit: we could replace these with static::$resourceTypeName.

  7. +++ b/core/modules/menu_link_content/menu_link_content.module
    @@ -50,10 +50,11 @@ function menu_link_content_menu_delete(MenuInterface $menu) {
    +function menu_link_content_path_alias_insert(EntityInterface $entity) {
    
    @@ -75,23 +76,25 @@ function _menu_link_content_update_path_alias($path) {
    +function menu_link_content_path_alias_update(EntityInterface $entity) {
    ...
    +function menu_link_content_path_alias_delete(EntityInterface $entity) {
    

    Nit: we could type hint with PathAliasInterface (and $path_alias) here.

  8. +++ b/core/modules/system/system.install
    @@ -2357,3 +2356,104 @@ function system_update_8801() {
    +function system_update_8802() {
    ...
    +/**
    + * Convert path aliases to entities.
    + */
    ...
    +  $database = \Drupal::database();
    ...
    +    $sandbox['progress'] = 0;
    

    We already have system_update_8802() committed now. These need to be renumbered and all references updated accordingly.

  9. +++ b/core/modules/system/system.install
    @@ -2357,3 +2356,104 @@ function system_update_8801() {
    +/**
    + * Convert path aliases to entities.
    + */
    ...
    +  $database = \Drupal::database();
    +
    +  if (!isset($sandbox['current_id'])) {
    

    I think we need an early return when the storage is not an instance of PathAliasStorage.

  10. +++ b/core/modules/system/system.install
    @@ -2357,3 +2356,104 @@ function system_update_8801() {
    +    $last_id = end($url_aliases);
    +    $sandbox['current_id'] = $last_id->pid;
    

    Nit: this bit confused me at first sight, what about $last_url_alias?

  11. +++ b/core/modules/system/system.install
    @@ -2357,3 +2356,104 @@ function system_update_8801() {
    +    $database->schema()->dropTable('url_alias');
    

    What about using the entity_update_backup setting to drop this conditionally?

  12. +++ b/core/modules/system/system.module
    @@ -1416,26 +1417,29 @@ function system_block_view_system_main_block_alter(array &$build, BlockPluginInt
    +function system_path_alias_insert(EntityInterface $entity) {
    ...
    +function system_path_alias_update(EntityInterface $entity) {
    ...
    +function system_path_alias_delete(EntityInterface $entity) {
    

    Nit: as above, we could type hint with PathAliasInterface (and $path_alias) here.

Berdir’s picture

> However, one thing that I was wondering is why we need alias management to be baked in \Drupal\Core rather than in its own module. Of course, this design decision is pre-existing but the refactoring initiated here could be a good chance to extract all the alias management logic to a new path_alias module. It seems that with the deprecation follow-ups we would already be rewriting/getting rid of most of the alias-related code in \Drupal\Core\Path and in the path module.

Hm. I think that might have been a good idea if we'd define a new system. I'm also a bit confused by the last sentence. 80% of Drupal\Core\Path *is* alias management (AliasStorage, AliasManager, AliasWhiteList) and 100% of path.module (it's nothing but the UI for path aliases), so there would be very little left in either place.

We'd have to move all of that to a the module (actually, we would IMHO instad move it to path.module), meaning we'd have to deprecate all of these services and would need to make the module either required or would require everything that looks up aliases to explicitly check for that module. We also have dependencies to Drupal\Core\Path\Alias.. in \Drupal\Core\EventSubscriber\PathSubscriber (which shouldn't be in that namespace but that's a different topic) and \Drupal\Core\PathProcessor\PathProcessorAlias, that would need to move too. As well as code in system.module: the hooks (these would be nice ;)), conditions (moving plugins is annoying) and the site information form (this can't move)

In short, I don't think that's worth doing and alias handling is such a central part of Drupal that I think it is fine to keep it there.

plach’s picture

@Berdir:

In short, I don't think that's worth doing and alias handling is such a central part of Drupal that I think it is fine to keep it there.

Ideally decoupling all subsystems and leaving \Drupal\Core only absolutely required stuff should lead to a better system (more maintainable, more testable, blah, blah, blah), but I agree that we should definitely evaluate the effort of such a refactoring. The changes you described more or less match my expectations from my cursory look and didn't seem hugely problematic to me.

Personally I think we should try to reduce the amount of entity definitions we have in \Drupal\Core (ideally to 0) and make what's in there just glue/adapter code between \Drupal\Component and core modules. I understand though that this might not be the right time to focus on this stuff, as -alpha1 is looming, I was just pointing out that investing some time now would save (way) more time later.

plach’s picture

To clarify, I'd be fine even with temporary having backwards dependencies between \Drupal\Core\Path and the path/path_alias module (I thought a new module might make dealing with BC easier), as long as the plan is completing the refactoring.

amateescu’s picture

Status: Needs work » Needs review
FileSize
102.95 KB
14.16 KB

Addressed all the points from #180, thanks for taking a look!

  1. Fixed.
  2. Fixed.
  3. Because we were previously using those try/catch blocks for the auto-creation of the url_alias table. Now that the entity system installs it for us, they're no longer needed :)
  4. That code was copied from the path alias form, but I agree that trim($var, '') is a no-op, so fixed!
  5. We can't because path aliases will implement that interface in #3007669: Add publishing status to path aliases
  6. Fixed.
  7. Fixed.
  8. Fixed.
  9. Fixed.
  10. Fixed.
  11. Sure, but we can't keep the table name as-is because it will cause contrib or custom code to provide stale data if they're not aware of the change. So let's rename it to old_url_alias instead :)
  12. While working and testing all the changes above, I realized that the path (previously source) field is not "read -only" for an URL alias, so it needs to be revisionable as well. Fixed that and added test coverage that it's copied properly to the revision table.

  13. @Berdir mentioned that we could move these hooks to a couple of post* methods on the PathAlias entity type, and it's certainly nice to get rid of scattered code from the system module.
Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -160,14 +160,14 @@ public function delete($conditions) {
     
    -    $result = $query->execute();
    +    $result = $query->range(0, 1)->execute();
         $storage->delete($storage->loadMultiple($result));
       }
     
    

    Hm, This is actually the delete method, which currently doesn't have a range?

    With aliases for multiple languages, it is definitely possible right now that you'd want to delete multiple aliases at once when e.g. deleting a node? So I don't think this is correct, wondering if we actually have test coverage for it.

  2. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -71,10 +72,35 @@ public function preSave(EntityStorageInterface $storage) {
         // Trim the alias value of whitespace and slashes. Ensure to not trim the
         // slash on the left side.
    -    $alias = rtrim(trim(trim($this->getAlias()), ''), "\\/");
    +    $alias = rtrim(trim($this->getAlias()), "\\/");
         $this->setAlias($alias);
       }
    

    was wondering about that line, but I just assumed you copied it from somewhere and then didn't look at it more closely. Agreed that the extra trim isn't necessary and doesn't do anything.

  3. +++ b/core/modules/system/system.install
    @@ -2452,7 +2457,11 @@ function system_update_8803(&$sandbox = NULL) {
       if ($sandbox['#finished'] >= 1) {
    -    $database->schema()->dropTable('url_alias');
    +    // Keep a backup of the old 'url_alias' table if possible.
    +    $keep_backup = Settings::get('entity_update_backup', TRUE);
    +    if ($keep_backup && !$database->schema()->tableExists('old_url_alias')) {
    +      $database->schema()->renameTable('url_alias', 'old_url_alias');
    +    }
    

    nitpick: I guess "if requested" or "if configured" makes more sense, it's always "possible"?

amateescu’s picture

Status: Needs work » Needs review
FileSize
102.93 KB
1.13 KB

Discussed the range() stuff with Berdir and he mentioned that in HEAD we only load a path alias before executing the delete statement so we can pass it along to the delete hook. Fixed that and also point 3. from #185.

The last submitted patch, 184: 2336597-184.patch, failed testing. View results

amateescu’s picture

One test failure from #184 shows that we do have test coverage for the range() stuff, because it passes (locally) with the patch from #186 :)

Fixed the other fails.

The last submitted patch, 186: 2336597-186.patch, failed testing. View results

plach’s picture

Thanks for the updates!

We can't because path aliases will implement that interface in #3007669: Add publishing status to path aliases.

But won't we be able to support content moderation at that point? Or will that happen in a later iteration? It seems checking the publishable interface would be useful regardless, I guess it's not even possible to support an entity type not implementing that...


  1. +++ b/core/lib/Drupal/Core/Path/Entity/PathAlias.php
    @@ -0,0 +1,148 @@
    +  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    ...
    +  public static function postDelete(EntityStorageInterface $storage, array $entities) {
    

    Why don't we do this in PathAliasStorage itself, so we can inject the services?

  2. +++ b/core/modules/system/system.install
    @@ -2359,3 +2356,113 @@ function system_update_8802() {
    +  if ($sandbox['#finished'] >= 1) {
    +    // Keep a backup of the old 'url_alias' table if requested.
    +    $keep_backup = Settings::get('entity_update_backup', TRUE);
    +    if ($keep_backup && !$database->schema()->tableExists('old_url_alias')) {
    +      $database->schema()->renameTable('url_alias', 'old_url_alias');
    +    }
    +
    +    return t('Path aliases have been converted to entities.');
    +  }
    

    We're never dropping the table now :)
    Btw, what about 'old_' . uniqueid() . '_' as prefix to reduce the likelihood of collisions?

amateescu’s picture

But won't we be able to support content moderation at that point? Or will that happen in a later iteration?

Yup, we will be able to support CM, but that also means some more UI work, which should happen in a later iteration.

It seems checking the publishable interface would be useful regardless, I guess it's not even possible to support an entity type not implementing that...

CM actually supports non-publishable entity types, it's just not that useful for them :)

Why don't we do this in PathAliasStorage itself, so we can inject the services?

Not injecting the service was one of the goals for doing it in the entity class, and the reason is that we want to prevent (or at least lessen the chances of) a circular dependency with the alias manager service.

We're never dropping the table now :)

Oops, fixed :)

Status: Needs review » Needs work

The last submitted patch, 191: 2336597-191.patch, failed testing. View results

amateescu’s picture

Status: Needs work » Needs review
FileSize
102.72 KB
857 bytes

Sigh.. since we are now generating a unique table name, we can't reliably determine whether the table exists, so we need to drop that assertion and trust the code :)

Berdir’s picture

+++ b/core/modules/system/tests/src/Functional/Update/PathAliasToEntityUpdateTest.php
@@ -53,9 +53,6 @@ public function testConversionToEntities() {
 
-    // Check that we have a backup of the old table.
-    $this->assertTrue($schema->tableExists('old_url_alias'));

As mentioned on slack, I think "$schema->findTables('old%url_alias')" or so should work?, e.g. as an assertCount(1, ...)

For integration with Content Moderation, I think the problem basically is that there's no way to actually test/see a draft alias. So while technically possible, there's little point in doing it (Similar to problems with media entities). Unlike workspaces, where we should be able to intercept the alias lookup when being in a non-default workspace.

amateescu’s picture

As mentioned on slack, I think "$schema->findTables('old%url_alias')" or so should work?, e.g. as an assertCount(1, ...)

Yup, that works ;)

plach’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, back to RTBC!

Leaving the final word to @catch since he's reviewed this many times and is the subsystem maintainer (among the rest :)

Not injecting the service was one of the goals for doing it in the entity class, and the reason is that we want to prevent (or at least lessen the chances of) a circular dependency with the alias manager service.

Ouch, I hope we can clean this up while working on the various deprecation issues.

Berdir’s picture

RTBC +1.

On the move-to-module topic. I do think that it would be a fairly complex task that would result in having to deprecate and move ~5 services, and updating a number of services depending on them and some tricky interactions e.g. in system.module. IMHO, it would be pretty challenging to manage that all in time (not just implement, but then also review again) with the various follow-ups that we have. On the other side, if we don't do it now it will likely be even harder to move it all again later. So not really something we can push to a follow-up. My recommendation would be to not move it, but if others (specifically @catch I guess) think it is a good idea then I'm not going to argue against it.

plach’s picture

My recommendation would be to not move it, but if others (specifically @catch I guess) think it is a good idea then I'm not going to argue against it.

Same here, from the opposite side. If there were any way to make the move-to-module thing happen I'd be happier, but that's definitely not going to impact on my +1 on this :)

Thanks again!

  • catch committed 099ce08 on 8.8.x
    Issue #2336597 by amateescu, slashrsm, jibran, Wim Leers, saki007ster,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

So I really like the idea of the entity type living in a module, but it opens up other problems, not the least of which is we'd need to make either path module or the new path_alias module required and have an update to force-enable it.

Agreed that doing it later is going to be even harder but we will probably need to figure out a change of module/provider for entity types eventually, even if it's not for path aliases in the first instance.

So it's a shame none of us thought of this earlier, but feels like something we can try to tackle during the 9.x cycle and shouldn't block this patch going in. Opened an issue here #3084874: Consider moving path aliases to a new path_alias module.

As before, the patch is a massive step forward and enables us to make lots of other improvements in further issues.

So..... Committed 099ce08 and pushed to 8.8.x. Thanks!

jibran’s picture

Yay!!! Congrats everyone it was a team effort.

catch’s picture

Issue tags: +8.8.0 release notes
larowlan’s picture

Issue summary: View changes

Expanded the release notes to add reference to pathauto update #3012050: Prepare for the conversion of path aliases to entities which will be needed by people as part of core update. I suspect that this will trip people up. I wonder if we can add a conflict entry to our composer.json for versions of pathauto that don't have #3012050: Prepare for the conversion of path aliases to entities to help out those who use composer. For those who use drush upc, not sure we can do anything.

larowlan’s picture

xjm’s picture

Issue summary: View changes

Making the release note a bit more emphatic.

penyaskito’s picture

For those writing upgrade tests in contrib:
if you used to verify anything before running runUpdates(), your calls to drupalLogin or drupalGet will fail with

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'test94250581path_alias' doesn't exist...

Remove your calls to drupalLogin or drupalGet to get them back to green. See #3085889: Fix \Drupal\Tests\lingotek\Functional\Update\LingotekJobIdInvalidCharsPostUpdateTest after path aliases converted to entities as an example.

xjm’s picture

Issue summary: View changes

Thanks @penyaskito. Maybe you could add that to the CR for the issue?

Making a small wording tweak to the release note.

Status: Fixed » Closed (fixed)

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

penyaskito’s picture