Updated: Comment #0

Problem/Motivation

path_load() is procedural wrapper around \Drupal::service('path.alias_storage')->load($conditions), which is used on one single place in the entire core.

Proposed resolution

Let's kill path_load() and just use service directly instead.

Remaining tasks

- code it
- test it

User interface changes

N/A

API changes

- path_load() is gone and \Drupal::service('path.alias_storage')->load($conditions) should be used instead.

CommentFileSizeAuthor
#16 node-2233607-16.patch1.53 KBvisabhishek
#13 node-2233607-13.patch1.59 KBvisabhishek
#13 interdiff-2233607-11-13.txt672 bytesvisabhishek
#11 node-2233607-11.patch1.74 KBvisabhishek
#11 interdiff-2233607-08-11.txt1.33 KBvisabhishek
#8 node-2233607-08.patch2.31 KBvisabhishek
#7 node-2233607-07.patch801 bytesvisabhishek
#7 interdiff-2233607-04-07.txt1.89 KBvisabhishek
#4 node-2233607-04.patch1.52 KBvisabhishek
#2 node-2233607-02.patch645 bytesvisabhishek
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
Assigned: Unassigned » visabhishek
visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
Status: Active » Needs review
FileSize
node-2233607-02.patch645 bytes

i have created a patch please review

slashrsm’s picture

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

path_load() needs to be removed.

visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
Status: Needs work » Needs review
FileSize
node-2233607-04.patch1.52 KB

patch updated as per #3.

Status: Needs review » Needs work

The last submitted patch, 4: node-2233607-04.patch, failed testing.

The last submitted patch, 2: node-2233607-02.patch, failed testing.

visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
Status: Needs work » Needs review
FileSize
interdiff-2233607-04-07.txt1.89 KB
node-2233607-07.patch801 bytes

i think we have to add follwing code from path_load() to avoid exceptions.

if (is_numeric($conditions)) {
    $conditions = array('pid' => $conditions);
  }
  elseif (is_string($conditions)) {
    $conditions = array('source' => $conditions);
  }
  elseif (!is_array($conditions)) {
    return FALSE;
  }
visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
FileSize
node-2233607-08.patch2.31 KB

Sorry i have uploaded wrong patch. Now i am uploading correct one

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Path/AliasStorage.php
@@ -82,6 +82,15 @@ public function save($source, $alias, $langcode = Language::LANGCODE_NOT_SPECIFI
+     if (is_numeric($conditions)) {
+       $conditions = array('pid' => $conditions);
+     }
+     elseif (is_string($conditions)) {
+       $conditions = array('source' => $conditions);
+     }
+     elseif (!is_array($conditions)) {
+     return FALSE;
+    }

I'd rather see this being fixed when called (PathController::adminEdit()) instead of here.

marcingy’s picture

marcingy CreditAttribution: marcingy commented

When the function in question calls path_load it always provides a pid. So if you set the condition you pass in to be a pid that should solve the issue. The else statements are from a time when path load was called by more areas in the code.

visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
Status: Needs work » Needs review
FileSize
interdiff-2233607-08-11.txt1.33 KB
node-2233607-11.patch1.74 KB

i have updated patch as per #9.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs review » Needs work 
+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
@@ -24,7 +24,16 @@ public function adminOverview($keys = NULL) {
-    $path = path_load($path);
+    if (is_numeric($path)) {
+       $path = array('pid' => $path);
+     }
+     elseif (is_string($path)) {
+       $path = array('source' => $path);
+     }
+     elseif (!is_array($path)) {
+     return FALSE;
+    }  ¶
+    $path = \Drupal::service('path.alias_storage')->load($path);
     module_load_include('admin.inc', 'path');

As @marcingy already mentioned in #10 you don't need to support all 3 conditions. Just the first if should do just fine in here...

visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
Status: Needs work » Needs review
FileSize
interdiff-2233607-11-13.txt672 bytes
node-2233607-13.patch1.59 KB

i have updated patch as per #12.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs review » Needs work 
+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
@@ -24,7 +24,10 @@ public function adminOverview($keys = NULL) {
-    $path = path_load($path);
+    if (is_numeric($path)) {
+       $path = array('pid' => $path);
+     }
+    $path = \Drupal::service('path.alias_storage')->load($path);

Why not simply:

$path = \Drupal::service('path.alias_storage')->load(array('pid' => $path));

The last submitted patch, 13: node-2233607-13.patch, failed testing.

visabhishek’s picture

visabhishek CreditAttribution: visabhishek commented
Status: Needs work » Needs review
FileSize
node-2233607-16.patch1.53 KB

@slashrsm: ok , i am updating the patch again.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Status: Needs review » Reviewed & tested by the community

Looks good. Note that #2233595: Deprecate the custom path alias storage kind of depends on this and it will need a re-roll when this gets in.

alexpott’s picture

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

We need update the https://drupal.org/node/1853148 change notice.

Committed 2ccfe55 and pushed to 8.x. Thanks!

  • Commit 2ccfe55 on 8.x by alexpott: 
    Issue #2233607 by visabhishek: Kill path_load() and use \Drupal::service...

Status: Fixed » Closed (fixed)

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