Problem/Motivation

Since path aliases have been converted to entities and their forms to entity forms, we should deprecate the existing path.alias_storage service because some of its methods are no longer used anywhere (save(), load(), delete(), aliasExists(), languageAliasExists(), getAliasesForAdminListing()).

Proposed resolution

Deprecate \Drupal\Core\Path\AliasStorage and its service (path.alias_storage), and introduce a new path.alias_repository service containing only the methods that are used in the critical path of each request :

- preloadPathAlias()
- lookupBySystemPath()
- lookupByAlias()
- pathHasMatchingAlias()

The main advantage of having a separate service is that instantiating the entity storage is heavier, because it needs two queries on the last installed definitions key value store (besides requiring to load more and larger class files). On top of that, every looked up path alias would imply an entity load (even a load from cache has its own overhead) with all the related field item (list) objects needing initialization. Hence the need for a more lightweight system.

Full deprecation list for AliasStorageInterface:

Existing method Replacement
save() \Drupal::entityTypeManager()->getStorage('path_alias')->save()
load() \Drupal::entityTypeManager()->getStorage('path_alias')->load()
delete() \Drupal::entityTypeManager()->getStorage('path_alias')->delete()
preloadPathAlias() AliasRepositoryInterface::preloadPathAlias()
lookupPathAlias() AliasRepositoryInterface::lookupBySystemPath()
lookupPathSource() AliasRepositoryInterface::lookupByAlias()
pathHasMatchingAlias() AliasRepositoryInterface::pathHasMatchingAlias()
aliasExists() No replacement.
languageAliasExists() No replacement.
getAliasesForAdminListing() No replacement.

Remaining tasks

Review the patch.

User interface changes

Nope.

API changes

The CRUD related methods from \Drupal\Core\Path\AliasStorageInterface have been deprecated in favor of the CRUD methods provided by the entity storage.

A few methods from \Drupal\Core\Path\AliasStorageInterface have been moved to \Drupal\Core\Path\AliasRepositoryInterface.

A few methods from \Drupal\Core\Path\AliasStorageInterface have been deprecated without any replacement: aliasExists(), languageAliasExists() and getAliasesForAdminListing().

CommentFileSizeAuthor
#106 interdiff-105.txt6.78 KBamateescu
#106 2233595-105.patch111.74 KBamateescu
#96 interdiff-96.txt20.88 KBamateescu
#96 2233595-96.patch110.6 KBamateescu
#91 interdiff-91.txt8.89 KBamateescu
#91 2233595-91-combined.patch128.19 KBamateescu
#91 2233595-91.patch110.79 KBamateescu
#90 interdiff-90.txt16.73 KBamateescu
#90 2233595-90-combined.patch120.65 KBamateescu
#90 2233595-90.patch105.19 KBamateescu
#86 interdiff-86.txt34.91 KBamateescu
#86 2233595-86-combined.patch220.85 KBamateescu
#86 2233595-86.patch92.64 KBamateescu
#83 interdiff-83.txt8.61 KBamateescu
#83 2233595-83-combined.patch243.41 KBamateescu
#83 2233595-83.patch84.08 KBamateescu
#82 interdiff-82.txt1.72 KBamateescu
#82 2233595-82-combined.patch237.98 KBamateescu
#82 2233595-82-for-review.patch80.19 KBamateescu
#81 2233595-81-combined.patch230.97 KBamateescu
#81 2233595-81-for-review.patch80.04 KBamateescu
#79 interdiff-79.txt14.16 KBamateescu
#79 2233595-79-combined.patch263.11 KBamateescu
#79 2233595-79-for-review.patch78.74 KBamateescu
#77 interdiff-77.txt2.01 KBamateescu
#77 2233595-77-combined.patch208.72 KBamateescu
#77 2233595-77-for-review.patch67.79 KBamateescu
#76 2233595-76-combined.patch201.5 KBamateescu
#76 2233595-76-for-review.patch67.76 KBamateescu
#74 2233595-74-mega-combined.patch201.75 KBamateescu
#73 interdiff-73.txt19.72 KBamateescu
#73 2233595-73-combined.patch195.24 KBamateescu
#73 2233595-73-for-review.patch69.12 KBamateescu
#72 2233595-72-combined.patch191.83 KBamateescu
#72 2233595-72-for-review.patch65.93 KBamateescu
#71 2233595-71-combined.patch191.14 KBamateescu
#71 2233595-71-for-review.patch68.19 KBamateescu
#59 refactor-2233595-59.patch14.26 KBSharique
#57 refactor-2233595-57.patch14.18 KBSharique
#49 refactor-2233595-49.patch15.92 KBcarletex
#38 refactor-2233595-38.patch18.23 KBSpartyDan
#38 interdiff.txt747 bytesSpartyDan
#35 refactor-2233595-35.patch18.18 KBSpartyDan
#30 issue-2233595-30.patch16.1 KBmarcingy
#26 issue-2233595-26.patch16.1 KBmarcingy
#24 issue-2233595-24.patch13.9 KBvisabhishek
#18 issue-2233595-18.patch16.09 KBmarcingy
#7 issue-2233595-07.patch14.1 KBvisabhishek
#2 issue-2233595.patch14.82 KBmarcingy
#7 interdiff-2233595-04-07.txt617 bytesvisabhishek
#4 issue-2233595-04.patch14.81 KBvisabhishek
#40 interdiff_38_40.txt1.15 KBSpartyDan
#40 refactor-2233595-40.patch18.59 KBSpartyDan
#4 interdiff-2233595-02-04.txt1.89 KBvisabhishek
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcingy’s picture

Assigned: Unassigned » marcingy
marcingy’s picture

Status: Active » Needs review
FileSize
14.82 KB

Alright, we have a bit of a hack in path_load() for now but that is dying else where, depending on how that other issue progress I may well merge that into here.

slashrsm’s picture

Status: Needs review » Needs work

Looks great! Thanks! I basically have just one terminology related comment....

  1. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -95,12 +92,79 @@ public function load($conditions) {
    +  public function loadByPath($source, $langcode = NULL) {
    +    $query = $this->connection->select('url_alias')
    +      ->fields('url_alias')
    +      ->condition('source', $source);
    +
    +    if (isset($langcode)) {
    +      $query->condition('langcode', $langcode);
    +    }
    +
    

    I'd use $path instead of $source. "Source path" is one of the names that are currently used for non-aliased path, but we agreed that we should simply use "Path" for that. Using correct variable names will help us to enforce this terminology.

  2. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -95,12 +92,79 @@ public function load($conditions) {
    +  public function deleteByPath($source, $langcode = NULL) {
    +    $path = $this->loadByPath($source, $langcode);
    +    $query = $this->connection->delete('url_alias')
    +      ->condition('source', $source);
    +
    

    Same as above.

  3. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
    @@ -37,28 +37,71 @@
    -  public function load($conditions);
    +  public function loadByPath($source, $langcode = NULL);
     
    

    As above.

  4. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
    @@ -37,28 +37,71 @@
    +   * @param string $source
    +   *   The internal system path.
    +   * @param string|null $langcode
    +   *   The language code of the alias.
    +   */
    +  public function deleteByPath($source, $langcode = NULL);
    +
    

    As above.

visabhishek’s picture

Status: Needs work » Needs review
FileSize
1.89 KB
14.81 KB

i am updating the patch as per #3

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Looks OK.

Note that this conflicts with #2233607: Kill path_load() and use \Drupal::service('path.alias_storage')->load($conditions) instead. We either have to merge the two patches or re roll when first of the two gets in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: issue-2233595-04.patch, failed testing.

visabhishek’s picture

Status: Needs work » Needs review
FileSize
617 bytes
14.1 KB

Now patch is updated as per #5.

slashrsm’s picture

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

7: issue-2233595-07.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: issue-2233595-07.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
marcingy’s picture

7: issue-2233595-07.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: issue-2233595-07.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

7: issue-2233595-07.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: issue-2233595-07.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

7: issue-2233595-07.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: issue-2233595-07.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
16.09 KB

Reroll and a couple of tweaks for migrations having an issue with the interdiff generation.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: issue-2233595-18.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review

18: issue-2233595-18.patch queued for re-testing.

slashrsm’s picture

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

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
git ac https://drupal.org/files/issues/issue-2233595-18.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 16477  100 16477    0     0  10816      0  0:00:01  0:00:01 --:--:-- 16969
error: patch failed: core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php:114
error: core/modules/locale/lib/Drupal/locale/Tests/LocalePathTest.php: patch does not apply
error: patch failed: core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php:142
error: core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php: patch does not apply

Needs a reroll

visabhishek’s picture

Status: Needs work » Needs review
FileSize
13.9 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 24: issue-2233595-24.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
16.1 KB

Status: Needs review » Needs work

The last submitted patch, 26: issue-2233595-26.patch, failed testing.

shumer’s picture

Status: Needs work » Needs review

26: issue-2233595-26.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 26: issue-2233595-26.patch, failed testing.

marcingy’s picture

Status: Needs work » Needs review
FileSize
16.1 KB

Or not as the case was this should be green, I missed a method in the test that had changed.

slashrsm’s picture

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

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
    @@ -37,28 +37,71 @@
    +  /**
    +   * Fetches a specific URL alias from the database based on path.
    +   *
    

    These all say 'fetches/deletes a specific alias' but what happens if there are two different aliases pointing to the same path?

  2. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
    @@ -37,28 +37,71 @@
        *   FALSE if no alias was found or an associative array containing the
        *   following keys:
    -   *   - source (string): The internal system path.
    -   *   - alias (string): The URL alias.
    -   *   - pid (int): Unique path alias identifier.
    -   *   - langcode (string): The language code of the alias.
        */
    

    Why are the keys removed here? Should remove the reference to them if they really don't need to be here.

  3. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
    @@ -37,28 +37,71 @@
    +   *   following keys:
    

    The keys should be listed here.

benjifisher’s picture

Issue summary: View changes

I updated the issue summary. I also tested locally (did not want to bother the busy testbot) and found that the patch does not apply. I think it should be easy to re-roll it: the directory structure has changed.

SpartyDan’s picture

I am re-rolling the patch.

SpartyDan’s picture

FileSize
18.18 KB

Re-rolled patch from comment #30. Does not address issues raised in comment #32.

I'm not sure that I understand the issues raised in #32. Please help me (novice) by elaborating on comment #32.

marcingy’s picture

Status: Needs work » Needs review

Sending too bot.

hanoii’s picture

I will be reviewing this patch now, please let me know if anybody else is reviewing it.

SpartyDan’s picture

FileSize
18.23 KB
747 bytes

I added documentation to patch #35 that addresses questions in comment #32. Patch and interdiff are attached.

  /**
   * Fetches a specific URL alias from the database using a pid.
   *
   * @param int $pid
   *   Unique path alias identifier.
   *
   * @return mixed[]|bool
   *   FALSE if no alias was found or an associative array containing the
   *   the following keys:
   *   - source (string): The internal system path.
   *   - alias (string): The URL alias.
   *   - pid (int): Unique path alias identifier.
   *   - langcode (string): The language code of the alias.
   */
  public function load($pid);

The method will only return 0 or 1 records because it is querying the database for one unique identifier.

hanoii’s picture

  1. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
    @@ -37,28 +37,76 @@
    +   * @return mixed[]|bool
    +   *   FALSE if no alias was found or an associative array containing the
    +   *   following keys:
    +   */
    +  public function loadByPath($path, $langcode = NULL);
    

    Keys missing here in the documentation

  2. +++ b/core/lib/Drupal/Core/Path/AliasStorageInterface.php
    @@ -37,28 +37,76 @@
    +   * @return mixed[]|bool
    +   *   FALSE if no alias was found or an associative array containing the
    +   *   following keys:
    +   */
    +  public function loadByAlias($alias, $langcode = NULL);
    

    And here as well

EDIT: Dreditor code properly updated

I will get to review the patch in more depth now, the concern on what happens with multiple aliases to the same path applies to the *ByPath() alternatives of the method, as there can be different aliases to the same path, I guess that should be somehow working and only needs documentation, but will try to get to review that as well.

SpartyDan’s picture

add documentation as noted in #39

Patch and interdiff attached.

hanoii’s picture

Issue summary: View changes

We reviewed the concerns of @catch about the possibility of having multiple aliases of the same path and although that's perfectly possible, the code doesn't currently account for that (and it never did on previous versions), the reason is on the code below:

+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -95,12 +92,79 @@ public function load($conditions) {
+    return $query->execute()
+      ->fetchAssoc();
+  }
...
+    return $query->execute()
+      ->fetchAssoc();
+  }

Because the previous ->load() or the new loadByPath() and loadByAlias() rely on fetchAssoc(), it's OK for the code that uses these methods assume that only one record will be return and act accordingly. There is the possibility of having multiple alias of the same node, however, the possibility is never accounted on the code. I am unsure of what way to go from here.

The only place that loadByPath() is used by this refactor (and this still applies to the code without it) is on core/modules/path/src/Plugin/Field/FieldWidget/PathWidget.php, which is probably the widget that edits the path alias on the node.

In the case of multiple aliases for the same node (on the same language) one of the aliases will be likely pulled randomly from the db and used there.

I can think of different approaches here. The best one I think is to enforce some kind of uniqueness for a combination of language/paths, which would actually make a lot of sense. I would then remove the possibility to have a NULL language code be passed to the loadByPath() so that always a combination of path/language is returned, even if it's 'None'. This will prevent the function to be used to pull all aliases of a path for every language but that's certainly not being happening currently in the code, and if that's necessary, probably a new function can be created. If this is the way to go, new testcases will need to be added, which I can offer myself to do is this approach is preferred.

hanoii’s picture

hanoii’s picture

Assigned: marcingy » Unassigned

Un-assigning from what I understand from DrupalCon Austin on how assignments should work.

hanoii’s picture

a_thakur’s picture

Patch in comment #40 does not apply to latest 8.x codebase.

a_thakur’s picture

Status: Needs review » Needs work

The last submitted patch, 40: refactor-2233595-40.patch, failed testing.

carletex’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +#DrupalZaragozaSprintDay
FileSize
15.92 KB

Reroll done.

We have deleted the .orig file because I think it was a mistake, let me know if it was correct.

Status: Needs review » Needs work

The last submitted patch, 49: refactor-2233595-49.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 49: refactor-2233595-49.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 49: refactor-2233595-49.patch, failed testing.

jsobiecki’s picture

Assigned: Unassigned » jsobiecki
Issue tags: +Needs reroll

I'm working on re-roll

jgeryk’s picture

going to work on reroll

Sharique’s picture

Status: Needs work » Needs review
FileSize
14.18 KB

Re-rolling patch.

Status: Needs review » Needs work

The last submitted patch, 57: refactor-2233595-57.patch, failed testing.

Sharique’s picture

Status: Needs work » Needs review
FileSize
14.26 KB

Fixed syntax error from previous patch.

Status: Needs review » Needs work

The last submitted patch, 59: refactor-2233595-59.patch, failed testing.

Crell’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Postponed

This isn't an 8.0.x-safe issue as it's an API change.

What we could do in 8.1 is leave load() as is, but add the other methods including a loadById(). That would keep the current API intact. That has to wait for 8.1 though.

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.

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.

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.

jibran’s picture

The reson mentioned in #61 to postpone is not valid anymore but we can postpone this on #2336597: Convert path aliases to full featured entities.

jibran’s picture

Title: Refactor AliasStorage::load() and AliasStorage::delete() » [PP-1] Refactor AliasStorage::load() and AliasStorage::delete()
Related issues: +#2336597: Convert path aliases to full featured entities
amateescu’s picture

Title: [PP-1] Refactor AliasStorage::load() and AliasStorage::delete() » [PP-1] Deprecate the custom path alias storage
Component: base system » path.module
Assigned: jsobiecki » Unassigned
Issue tags: -API clean-up, -Novice, -#DrupalZaragozaSprintDay, -Needs reroll +@deprecated
Parent issue: #2002126: [meta] Refactor the alias subsystem (classes in \Drupal\Core\Path) » #3007661: Modernize the path alias system

In light of #3007661: Modernize the path alias system, I'd like to re-purpose this issue to deprecate the existing path alias storage. Still blocked on #2336597: Convert path aliases to full featured entities.

amateescu’s picture

This should be everything needed to deprecate the existing path alias storage. This patch applies on top of #3011807: Convert the path alias admin overview to a list builder because there are a lot of usages of the deprecated storage that have already been converted in #3007832: Convert custom path alias forms to entity forms and #3011807: Convert the path alias admin overview to a list builder.

amateescu’s picture

All the dependent patches were updated, so this one needed a reroll too.

amateescu’s picture

Fixed most of the fails and brought back some tests because the deprecated alias storage still needs to be tested until we remove it.

amateescu’s picture

Nice, the only fails left are the migration ones, so I posted a patch over at #3009014-2: Convert path alias migrations to use entity:path_alias destination. Let's try to include that one in the combined patch.

amateescu’s picture

Title: [PP-1] Deprecate the custom path alias storage » Deprecate the custom path alias storage

PP-1 is wishful thinking, this is PP-6 at the moment but it's easier to not try and keep track of that number anymore :)

amateescu’s picture

amateescu’s picture

Updated the patch to point to the change record.

amateescu’s picture

Issue summary: View changes

Also rewrote the issue summary.

amateescu’s picture

Fixed the remaining deprecated usages in migrate tests and updated the queries in AliasStorageTest to use an entity query instead.

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.

amateescu’s picture

Title: Deprecate the custom path alias storage » [PP-4] Deprecate the custom path alias storage
Issue tags: +Workflow Initiative
FileSize
80.04 KB
230.97 KB

Rerolled on top of the latest patches from #2336597-136: Convert path aliases to full featured entities and all of its followups.

amateescu’s picture

Rerolled again and fixed the last relevant test fail.

amateescu’s picture

Rerolled for all the recent changes in the parent issues.

jibran’s picture

Title: [PP-4] Deprecate the custom path alias storage » [PP-3] Deprecate the custom path alias storage
Chris Matthews’s picture

Status: Postponed » Needs work

Status: Needs review » Needs work

The last submitted patch, 86: 2233595-86-combined.patch, failed testing. View results

Berdir’s picture

  1. +++ b/core/lib/Drupal/Core/Path/AliasRepository.php
    @@ -0,0 +1,142 @@
    +/**
    + * Provides the default path alias lookup operations.
    + */
    +class AliasRepository implements AliasRepositoryInterface {
    +
    

    What are your thoughts on making this a new service vs. just deprecating the things we no longer need on AliasStorage?

    I would have assumed that to be the easier change?

    Either way, we should probably document here a bit why this is a separate service that is accessing the table directly and not using the entity API?

    Not 100% sure about deprecation messages and how to handle that exactly here. If we AliasStorage, then we need to add @trigger_error() to each actually deprecated method including legacy tests. With a new service, you might be able to skip @trigger_error() on the methods, but we will still need legacy test for the code, *including* for the methods that are really just moved over here.

  2. +++ b/core/lib/Drupal/Core/Path/AliasRepository.php
    @@ -0,0 +1,142 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function pathHasMatchingAlias($initial_substring) {
    +    $query = $this->getBaseQuery();
    +    $query->addExpression(1);
    +
    +    return (bool) $query
    +      ->condition('base_table.path', $this->connection->escapeLike($initial_substring) . '%', 'LIKE')
    +      ->range(0, 1)
    +      ->execute()
    +      ->fetchField();
    +  }
    

    Hm. This should be possible as an entity query too with a STARTS_WITH condition?

    It's used only by AliasWhitelist which in turn is basically a private service for AliasManager. (Literally the *only* call, there not even a test using this method)

    And while it may be called during the critical-path alias-lookup, it's only called once for the root parts of known routes and as well cached as technically feasible ;)

    DI is a bit tricky, as the alias services need the entity type manager then, but we only need to initialize the storage with the extra queries on the installed definition if we actually have a miss.

  3. +++ b/core/lib/Drupal/Core/Path/AliasRepositoryInterface.php
    @@ -0,0 +1,71 @@
    +  public function lookupBySystemPath($path, $langcode);
    

    *If* we go with a separate service then maybe we should then align the arguments and method names with the new field names of the PathAlias entity now?

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

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

amateescu’s picture

Title: [PP-3] Deprecate the custom path alias storage » [PP-1] Deprecate the custom path alias storage
Status: Needs work » Needs review
FileSize
105.19 KB
120.65 KB
16.73 KB

Fixed the tests for now.

amateescu’s picture

Re @Berdir in #88:

1. As discussed, the reason for introducing a new service is to improve the method names and return values for lookupPathAlias() and lookupPathSource().

2. I would prefer to keep the existing query as-is. We can always deprecate this method later if we are comfortable with the slight performance penalty.

3. The new methods are already improved :)

Added full test coverage for all the methods of the deprecated AliasStorage service, and updated the IS as well.

plach’s picture

Title: [PP-1] Deprecate the custom path alias storage » Deprecate the custom path alias storage

The blocker is in!

Status: Needs review » Needs work

The last submitted patch, 91: 2233595-91-combined.patch, failed testing. View results

Berdir’s picture

First chunk of review.

  1. +++ b/core/core.services.yml
    @@ -960,11 +960,17 @@ services:
       path.alias_storage:
         class: Drupal\Core\Path\AliasStorage
         arguments: ['@database', '@module_handler', '@entity_type.manager']
         tags:
           - { name: backend_overridable }
    +    deprecated: The "%service_id%" service is deprecated. Use the "path.alias_repository" service instead, or the entity storage handler for the "path_alias" entity type for CRUD methods. See https://www.drupal.org/node/3013865
    

    Found two subclasses of this: http://grep.xnddx.ru/search?text=extends%20AliasStorage

    By deprecating and no longer using this, we break those modules, but that's always the possibility when deprecating a service/method. It's easier with functions, you can't subclass/replace a function ;)

  2. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
    @@ -104,8 +104,8 @@ class AliasManager implements AliasManagerInterface, CacheDecoratorInterface {
        *   Cache backend.
        */
    -  public function __construct(AliasStorageInterface $storage, AliasWhitelistInterface $whitelist, LanguageManagerInterface $language_manager, CacheBackendInterface $cache) {
    -    $this->storage = $storage;
    +  public function __construct(AliasRepositoryInterface $alias_repository, AliasWhitelistInterface $whitelist, LanguageManagerInterface $language_manager, CacheBackendInterface $cache) {
    +    $this->pathAliasRepository = $alias_repository;
         $this->languageManager = $language_manager;
         $this->whitelist = $whitelist;
         $this->cache = $cache;
    

    This needs BC, probably by removing the type and then checking what it is and if we don't like it, do a @trigger_error(). And probably use the DeprecatedServicePropertyTrait for the storage property.

    Didn't find a subclass of this, but I did find a decorator/duplicate: http://grep.xnddx.ru/search?text=implements+AliasManager&filename=

  3. +++ b/core/lib/Drupal/Core/Path/AliasStorage.php
    @@ -53,11 +67,14 @@ class AliasStorage implements AliasStorageInterface {
    +    $this->aliasRepository = $alias_repository ?: \Drupal::service('path.alias_repository');
    

    This might be a problem.

    I applied the patch locally, then tried to do a drush cr. Problem is that apparently drupal_flush_all_caches() does the twig clearing stuff before rebuilding the container, and the twig service has a nice dependency chain through the url_generator, through route provider then all the path processors down into this.

    Suggestion: If you happen to be in that really weird state, having working aliases is probably not a priority, so we could not fall back here and return nothing in these methodss if we don't have a service? Or we keep the old implementation as fallback. Is there a reason you replace it (the workspace replacement?) or just to avoid duplicated code?

  4. +++ b/core/lib/Drupal/Core/Path/AliasWhitelist.php
    @@ -37,13 +37,13 @@ class AliasWhitelist extends CacheCollector implements AliasWhitelistInterface {
        *   The state keyvalue store.
    -   * @param \Drupal\Core\Path\AliasStorageInterface $alias_storage
    -   *   The alias storage service.
    +   * @param \Drupal\Core\Path\AliasRepositoryInterface $alias_repository
    +   *   The path alias repository.
        */
    -  public function __construct($cid, CacheBackendInterface $cache, LockBackendInterface $lock, StateInterface $state, AliasStorageInterface $alias_storage) {
    +  public function __construct($cid, CacheBackendInterface $cache, LockBackendInterface $lock, StateInterface $state, AliasRepositoryInterface $alias_repository) {
         parent::__construct($cid, $cache, $lock);
         $this->state = $state;
    -    $this->aliasStorage = $alias_storage;
    +    $this->pathAliasRepository = $alias_repository;
       }
     
    

    Same here, BC..

Berdir’s picture

  1. +++ b/core/modules/shortcut/tests/src/Functional/ShortcutLinksTest.php
    @@ -42,11 +44,7 @@ public function testShortcutLinkAdd() {
     
         // Create an alias for the node so we can test aliases.
    -    $path = [
    -      'source' => '/node/' . $this->node->id(),
    -      'alias' => '/' . $this->randomMachineName(8),
    -    ];
    -    $this->container->get('path.alias_storage')->save($path['source'], $path['alias']);
    +    $path_alias = $this->createPathAlias('/node/' . $this->node->id(), '/' . $this->randomMachineName(8));
     
    

    Really like the trait, makes these tests much nicer.

  2. +++ b/core/modules/workspaces/src/AliasRepository.php
    @@ -3,14 +3,12 @@
     /**
      * Provides workspace-specific path alias lookup queries.
      */
    -class AliasStorage extends CoreAliasStorage {
    +class AliasRepository extends CoreAliasRepository {
     
    

    Wondering if prefixing the class with Workspace would make sense, thought about that before too. Would avoe the use X as Y easier to find the actual implementation in an IDE.

  3. +++ b/core/modules/workspaces/src/AliasRepository.php
    @@ -20,19 +18,15 @@ class AliasStorage extends CoreAliasStorage {
        */
    -  public function __construct(Connection $connection, ModuleHandlerInterface $module_handler, EntityTypeManagerInterface $entity_type_manager, WorkspaceManagerInterface $workspace_manager) {
    -    parent::__construct($connection, $module_handler, $entity_type_manager);
    +  public function __construct(Connection $connection, WorkspaceManagerInterface $workspace_manager) {
    +    parent::__construct($connection);
         $this->workspaceManager = $workspace_manager;
    

    This could be the place to switch to setter injection to not having to touch the constructor, per my comment in the published status issue. But still not very important ;)

  4. +++ b/core/tests/Drupal/KernelTests/Core/Path/DeprecatedAliasStorageTest.php
    @@ -0,0 +1,229 @@
    +
    +/**
    + * @coversDefaultClass \Drupal\Core\Path\AliasStorage
    + * @group path
    + * @group legacy
    + */
    +class DeprecatedAliasStorageTest extends KernelTestBase {
    

    I think it's more common to name these classes LegacyXy, that's already filtered out then be our tools look for deprecated method calls.

    Also, we should have some @expectedDeprecation things here.

    I'm not quite sure how that will work as we just have the one on the service and you already get that in the constructor. Maybe that will still work if we just pot that on every test method? Even less sure about the one in the class file as that will only be loaded once I suppose.

  5. +++ b/core/tests/Drupal/Tests/Core/Path/AliasManagerTest.php
    @@ -69,11 +77,12 @@ protected function setUp() {
     
         $this->aliasStorage = $this->createMock('Drupal\Core\Path\AliasStorageInterface');
    +    $this->aliasRepository = $this->createMock(AliasRepositoryInterface::class);
    

    we can remove $this->aliasStorage?

  6. +++ b/core/tests/Drupal/Tests/Traits/Core/PathAliasTestTrait.php
    @@ -0,0 +1,111 @@
    +   * @param string|null $langcode
    +   *   (optional) A language code for the path alias. Defaults to NULL.
    

    null isn't the actual default though, that would be undefined as the implicit default or not? so maybe document as that? Or just make that the explicit default and remove the conditional?

amateescu’s picture

Priority: Normal » Major
Status: Needs work » Needs review
FileSize
110.6 KB
20.88 KB

Thanks for the review!! Fixed all of #94 and #95.

For #94.3: I wanted to avoid duplicated code, but since the deprecated service has full test coverage in LegacyAliasStorageTest, I guess it's better to keep the old code in place so we can keep the upgrade process as smooth as possible.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

It's a big patch but I I think this is ready. Reviewed it in depth before and the interdiff now looks good.

As commented in #94, fully deprecating the service will "break" a handful modules that subclassed/replaced this service. But that's partially already the case as we already no longer call these methods in the places that matter (This mostly updates tests). And we also missed the alpha deadline and this does technically add more deprecations. However, it is the last patch in a series of steps that got in, and not committing this would leave us in a pretty weird state for Drupal 9, with two different, not-officially deprecated API's to do the same things.

There would theoretically be a version of this that would be smaller, by only deprecating the removed methods and not removing the alias storage service completely. The new service allows us to improve DX with better named methods and also improved performance, specifically in PathFieldItemList, we can save an additional query because all information we need is already returned. It would also require additional work/time to go back to that.

So, I guess it is up to the RM/FM's now (that sounds like a radio station, with @xjm as the DJ)

jibran’s picture

catch’s picture

I am pro backporting this during alpha (assuming it's ready, I've not reviewed the whole patch yet), since it is part of a set of changes that otherwise would have required a 500kb patch, rather than a new deprecation that missed the deadline. The disruption is already pretty much there in practice, and it will help to reduce confusion in 9.x. However making sure xjm gets a chance to weigh in here so tagging.

If we don't backport it to 8.8.x and only commit it to 8.9.x, jibran is right it will end up blocked on #3088246: [policy, no patch] How to handle Drupal 8.9.x deprecations until we have a plan there.

xjm’s picture

Welcome back to RM FM radio, streaming to you live on this brisk fall morning. Expect unfrozen commits, with a chance of upgrade path regression in the path alias storage. Now, some mellow tunes to help you unwind after a long week of late hours.

xjm’s picture

(...I think this in theory is better to do sooner rather than later given the risk of having both around, but would like to give it a read first since it's a big patch.)

xjm’s picture

Yeah this seems like something we should do between alpha and beta for the reasons described in #99. Let's make sure to document it clearly for the beta release notes, though.

+++ b/core/modules/path/tests/src/Kernel/Migrate/d6/MigrateUrlAliasTest.php
@@ -100,54 +104,53 @@ public function testUrlAlias() {
-    $this->assertPath('2', $conditions, $path);
+    $this->assertPath(8, $conditions, $path_alias);

Why is this one changing from 2 to 8? And why didn't the test fail for that?

amateescu’s picture

@xjm, the test is not failing in HEAD because \Drupal\Tests\path\Kernel\Migrate\d6\MigrateUrlAliasTest::assertPath() does not assert the path alias ID. However, this patch adds an assertion for the ID in that method, and the correct value (8) can be seen in core/modules/migrate_drupal/tests/fixtures/drupal6.php - search for source-noslash :)

plach’s picture

Issue tags: -Needs release manager review +Needs follow-up

Aside from the nits below, my main concern about this is that AliasRepositoryInterface and AliasManagerInterface have very similar methods with very similar descriptions and it’s not easy to figure out at first sight which ones to use. Looking at the code AMI implementations depend on ARI ones, so it’s clear AMI should be used. However, at very least we should mark the repository as @internal, but probably a follow-up providing additional docs would help.

  1. +++ b/core/lib/Drupal/Core/Path/AliasRepositoryInterface.php
    @@ -0,0 +1,71 @@
    +   * Searches a path alias entity for a given Drupal system path.
    ...
    +   * Searches a path alias entity for a given alias.
    

    This is mentioning entities: we should either change it or ditch the ArrayPI ;)

  2. +++ b/core/modules/system/tests/src/Kernel/PathHooksTest.php
    @@ -30,35 +30,35 @@ protected function setUp() {
         // Check system_path_alias_insert();
    ...
         // Check system_path_alias_update();
    ...
         // Check system_path_alias_delete();
    

    Not introduced by this patch but we should probably update these comments.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Path/AliasTest.php
    @@ -29,166 +33,84 @@ protected function setUp() {
         // Hook that clears cache is not executed with unit tests.
         \Drupal::service('path.alias_manager')->cacheClear();
    

    Same here.

Removing the RM tag, since both @xjm and @catch agreed on this being ok to do ASAP.

Edit: typos

plach’s picture

Issue summary: View changes

Documented a few details about the proposed solution in the IS.

amateescu’s picture

Issue tags: -Needs follow-up
FileSize
111.74 KB
6.78 KB

Thanks for reviewing! Fixed all the points from #104 and added (hopefully) very clear documentation for the new service.

plach’s picture

Issue summary: View changes

Patch looks good to me, thanks!

catch’s picture

@plach mentioned this in slack:

AliasManagerInterface still mentions the alias storage in the PHP docblock

Fixable on commit - putting it here so it's not forgotten.

  • catch committed bdf4817 on 9.0.x
    Issue #2233595 by amateescu, marcingy, SpartyDan, visabhishek, Sharique...

  • catch committed 70a3496 on 8.9.x
    Issue #2233595 by amateescu, marcingy, SpartyDan, visabhishek, Sharique...

  • catch committed 696296b on 8.8.x
    Issue #2233595 by amateescu, marcingy, SpartyDan, visabhishek, Sharique...
catch’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 9d58bed and pushed to 9.0.x, then cherry-picked to 8.9.x and 8.8.x. Thanks!

Had forgotten this issue was five years old until going through issue credit, nice to close it out finally!

Status: Fixed » Closed (fixed)

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