Updated: Comment #0

Problem/Motivation

Some classes in Path subsystem have strange naming conventions. We should re-think this and make naming more clear and logic.

This issue is dependent on #2199381: Add \Drupal\Core\Path\PathInterface.

Proposed resolution

Remaining tasks

- identify classes that need to be fixed
- discuss new naming conventions
- implement renaming

User interface changes

N/A

API changes

Some classes will be renamed.

CommentFileSizeAuthor
#27 2208631_27.patch540 bytesslashrsm
#21 2208631_21.patch28.24 KBslashrsm
#18 interdiff.txt12.84 KBslashrsm
#18 2208631_18.patch28.14 KBslashrsm
#12 2208631_12.patch15.9 KBslashrsm
#7 2208631_7.patch14.82 KBslashrsm
#7 interdiff.txt15.65 KBslashrsm
#3 2208631_3.patch31.77 KBslashrsm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Component: php.module » path.module
slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Related issues: +#2209145: Move all path alias SQL queries to a single storage controller

I would suggest "Path" and "PathInterface" to be renamed to "AliasStorageController" and "AliasStorageControllerInterface".

I believe this is already becoming a storage controller and it will become even more if #2209145: Move all path alias SQL queries to a single storage controller moves forward. Also, I think this service is not so much about path as it is about aliases (it takes care about storing aliases into storage back-end).

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Active » Needs review
FileSize
2208631_3.patch31.77 KB

Status: Needs review » Needs work

The last submitted patch, 3: 2208631_3.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review

Sounds good to me.

@slashrsm, can you configure your git to use renames=copies, please, so we can properly review the changes in the renamed class file? See https://drupal.org/node/1542048

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

I would suggest to *not* name them StorageController. See #2188613: Rename EntityStorageController to EntityStorage. Just leave away the Controller part.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
FileSize
interdiff.txt15.65 KB
2208631_7.patch14.82 KB

Should install now. Renamed as suggested in #6 and configured my .gitconfig as suggested in #5.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Title: Improve naming of Path-related classes » Rename \Drupal\Core\Path\Path to \Drupal\Core\Path\AliasStorage
Parent issue: » #2002126: [meta] Refactor the alias subsystem (classes in \Drupal\Core\Path)
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Awesome!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
  1. +++ b/core/core.services.yml
@@ -333,7 +333,7 @@ services:
   path.crud:
-    class: Drupal\Core\Path\Path
+    class: Drupal\Core\Path\AliasStorage

    Why not rename the service too then? 

  2. +++ b/core/lib/Drupal/Core/Path/AliasManager.php
@@ -13,11 +13,11 @@
-   * @var \Drupal\Core\Path\Path
+   * @var \Drupal\Core\Path\AliasStorage

@@ -74,15 +74,15 @@ class AliasManager implements AliasManagerInterface {
+   * @param \Drupal\Core\Path\AliasStorage $storage
+   *   The alias storage service.
...
+  public function __construct(AliasStorage $storage, AliasWhitelistInterface $whitelist, LanguageManager $language_manager) {

    Let's create the promised AliasStorageInterface from the issue summary and use it :)

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs work » Needs review

Why not rename the service too then?

Agree. I think it would make sense to rename all alias related services to go along the naming conventions we're planning with #2002126: [meta] Refactor the alias subsystem (classes in \Drupal\Core\Path) (use "path" for non-aliased URL and "alias" part). It makes sense to do this in one patch (possibly together with renaming in the other places).

Let's create the promised AliasStorageInterface from the issue summary and use it :)

Interface is added in #2199381: Add \Drupal\Core\Path\PathInterface. Should we merge them?

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
FileSize
2208631_12.patch15.9 KB

#2199381: Add \Drupal\Core\Path\PathInterface has landed. Reroll uses interface instead of the class directly.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

Code in #12 looks great. Still any remaining action items from #11?

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

There is a renaming question left. Do we rename path service here or in a patch that will consolidate all naming related to aliases (see parent issue summary for more info).

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

In case the service is cleanly injected everywhere, that should only affect some .services.yml files, right? If so, let's do right in here? :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work

Well ... sadly we do have plugins and controllers.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

dawehner, please be more specific about what needs fixing or what is wrong, explicitly.
You just provided a statement.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs work » Needs review
FileSize
2208631_18.patch28.14 KB
interdiff.txt12.84 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Ehem, the neeeds work was not intended. My comment was just for sun.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/core.services.yml
@@ -149,7 +149,7 @@ services:
+    arguments: ['@alias.storage', '@path.alias_whitelist', '@language_manager']

Hmmm... but the alias storage can not be used for storing other types of aliases. How about path.alias_storage which matches up nicely with path.alias_manager

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs work » Needs review
FileSize
2208631_21.patch28.24 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks for the quick turnaround, @slashrsm!

I agree with @alexpott's suggestion, that's a much more sane naming :)

alexpott’s picture

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

Committed 719b36a and pushed to 8.x. Thanks!

+++ b/core/core.services.yml
@@ -348,8 +348,8 @@ services:
diff --git a/core/core.services.yml b/core/core.services.yml.orig

diff --git a/core/core.services.yml b/core/core.services.yml.orig
similarity index 100%
copy from core/core.services.yml
copy to core/core.services.yml.orig

Huh? I excluded core/core.services.yml.orig when I applied the patch.

  • Commit 719b36a on 8.x by alexpott: 
    Issue #2208631 by slashrsm: Rename \Drupal\Core\Path\Path to \Drupal\...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Please update the https://drupal.org/node/1853148 change record.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

Done: https://drupal.org/node/1853148/revisions/view/2842887/7101305

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Fixed » Needs review
FileSize
2208631_27.patch540 bytes

One service name change sneaked out of the patch...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Fixed

Yeah this lead me to #2232391: Revert drupal 8 changes made to generate-d7-content.sh and generate-d6-content.sh

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

Ah... forget this! I guess it is to early for my brain....

Status: Fixed » Closed (fixed)

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