Problem/Motivation

The path admin UI lives at admin/config/search/path, but the filtering uses admin/config/search/path/list/%.
That got lost in the local task conversions, but was not caught by tests, because we don't have any.

Proposed resolution

Fix the routing
Write tests

Remaining tasks

N/A

User interface changes

N/A

API changes

N/A

CommentFileSizeAuthor
#4 path-2244757-4.patch4.63 KBtim.plunkett
#1 path-2244757-1-PASS.patch4.6 KBtim.plunkett
#1 path-2244757-1-FAIL.patch2.52 KBtim.plunkett

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Active » Needs review
StatusFileSize
new path-2244757-1-FAIL.patch2.52 KB
new path-2244757-1-PASS.patch4.6 KB

#1987802: Convert path_admin_overview() to a new style controller will port this to OO code, but we need to have a working baseline first.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
+++ b/core/modules/path/lib/Drupal/path/Controller/PathController.php
@@ -15,7 +15,15 @@ class PathController {
+  public function adminOverview() {
+    module_load_include('admin.inc', 'path');
+    return path_admin_overview();
...
+  public function adminOverviewFiltered($keys) {

This just splits the filtered and unfiltered routes to separate methods, these will be utilized further in the conversion issue (right now they call the same function with and without $keys).

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/path/lib/Drupal/path/Tests/PathAdminTest.php
@@ -0,0 +1,92 @@
+   * Tests the path cache.
...
+  public function testPathCache() {
...
+    $node1 = $this->drupalCreateNode();

    That test naming is a c&p 

  2. +++ b/core/modules/path/path.routing.yml
@@ -7,11 +7,18 @@ path.delete:
 path.admin_overview:
-  path: '/admin/config/search/path/{keys}'
+  path: '/admin/config/search/path'
   defaults:
     _title: 'URL aliases'
     _content: '\Drupal\path\Controller\PathController::adminOverview'
-    keys: NULL
+  requirements:
+    _permission: 'administer url aliases'
+
+path.admin_overview_filter:
+  path: '/admin/config/search/path/list/{keys}'
+  defaults:
+    _title: 'URL aliases'
+    _content: '\Drupal\path\Controller\PathController::adminOverviewFiltered'
   requirements:
     _permission: 'administer url aliases'

    Keys: NULL ensures an optional part of the path.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new path-2244757-4.patch4.63 KB

1. Heh, laziness--

2. This is purposeful.

As discussed on IRC

in D7 it is admin/config/search/path and admin/config/search/path/list/%
someone messed up and ported it to admin/config/search/path/%
which doesn't work.
they forgot the /list/
otherwise you couldn't have an alias called 'add' or 'delete'

dawehner’s picture

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

Coool

The last submitted patch, 1: path-2244757-1-FAIL.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: +Quick fix

Last fail was the tests-only patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit e6ac9ec on 8.x by webchick: 
    Issue #2244757 by tim.plunkett: Path Admin UI is broken and has no test...

Status: Fixed » Closed (fixed)

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