Add option to ignore admin paths

Thanks for the patch and the issue! I'm +1 for adding this feature.

When creating your next patch, could you take a look how to create patches for Drupal projects here: https://www.drupal.org/node/707484 .

1. +++ modules/contrib/subpathauto/subpathauto.install	(revision )
   @@ -0,0 +1,10 @@
   +  $config->set('admin_route_validation', true);
   

   We will most likely have to set this for the existing installation to FALSE since we don't want to change the behaviour on them.

2. +++ modules/contrib/subpathauto/src/PathProcessor.php	(revision )
   @@ -181,6 +182,31 @@
   +    if($this->configFactory->get('subpathauto.settings')->get('admin_route_validation')) {
   

   Missing space after if

3. +++ modules/contrib/subpathauto/config/install/subpathauto.settings.yml	(revision )
   @@ -1,1 +1,2 @@
   \ No newline at end of file
   

   Missing newline from the end of the file

4. +++ modules/contrib/subpathauto/src/Form/SettingsForm.php	(revision )
   @@ -25,6 +25,13 @@
   +      '#title' => $this->t('If validation of admin routes is wanted'),
   

   I think this would be clearer if this would be "Skip administration paths from sub-path processing.".

Beside this, we should add test coverage for this feature. We probably also should consider extending this for the inbound processor (especially if that could be done easily).

Hello, patch #4 breaking /admin/content when "Content Translation" module installed.

Hi there,

Attaching a modified version of the patch from @jefuri. It does the same, but changes a bit the wording of things to be closer to the behavior. It can also be applied to current latest version of the module, which has moved since the last time the patch was re-rolled.

One note, from the module description:

In combination with the Pathauto module it is possible to get rid of all remaining exposed internal non-administrative URLs.

I'd argue this is not a feature request, but actually a bug (even if minor), since according to the module description, the default behavior would be to *not* apply it to admin routes.

NOTE: I haven't been able to test with content translation enabled.

+++ b/src/PathProcessor.php
@@ -177,6 +182,32 @@ class PathProcessor implements InboundPathProcessorInterface, OutboundPathProces
+        ->matchRequest($request);

Doesn't this throw an exception when a route does not exist?

Here is a patch that fixes it.

ah, great catch! I re-rolled the existing patch and didn't notice that could happen.

Hello!

I added a small optimization to the previous patch to fix autotest warning.

And second fix to the similar warning in autotests.

Sorry, on #15 comment I made a mistake. Real Drupal config gives "0" instead "FALSE" and comparsion with "===" does not work.
Fixed.

After changes autotests generate a notice about method, but I don't know how to fix it.
> Method was expected to be called 2 times, actually called 1 times.

I have added another config "process_layout_builder_route" for layout builder route (not admin route), I have issue when layout builder enabled on taxonomy term page on multilaingual site

Fixed the tests - one was a code issue and one was a test code issue.

Last patch was garbage.

Fixed tests and a default configuration issue with max depth that was being set to 1 instead of 0 (no max depth)

Sorry, there was a mistake in the last patch.

I've had to remove a (I think) false passing test. It was passing because the function getMaxDepth was returning NULL instead of 0.

I don't understand very well that test purpose, I think it should be revised.

Now, with this patch, everything seems to work well for me.

Patch is against subpathauto version 1.1, it would need a reroll for dev!!

Re-roll against the latest dev.

Patch #28 worked well on current dev branch (1.x-dev). Looks good - thank you!!!!

Has anyone rerolled this patch for 8.x-1.2 yet? Would love this functionality to make it into the release.

@mistergroove The patch in #28 applied cleanly over v1.2

However, in large forms (like a node edit form with a lot of fields), I have detected a low performance in checking for admin or layout builder routes, so I've added a caching layer.

Sorry I'm only just getting back to you. Drupal 9 updates seemed to have consumed me for an eternity. This is great @vidorado. I'll test it this week and get back to you.

+++ b/src/PathProcessor.php
@@ -218,4 +293,60 @@ protected function getMaxDepth() {
+    $request = Request::create($path);
...
+    $request = Request::create($path);

This has a quirky bug that can be triggered in some edge cases. Don't think its affecting the functionality of the patch in real use cases but it does have the ability to cause deprecation warnings in PHP 8.1 and be broken for really weird paths.

The problem is that $path here is the raw unescaped path from processOutbound. This is unclear from the documentation but its the case and it makes sense that it wouldn't be encoded until needed after processing. The reason this is a problem for this code gets pretty technical but has to do with the fact Request::create wants a URI and passes it parse_url. Technically the path isn't a full uri but a valid one is good enough to get parsed and work however a _raw_ path as passed to process can include invalid path characters which have not yet been encoded.

To understand, here's a real example:

Take this path created by memcache_admin module:
/admin/reports/memcache/default/cache2%3A11211

If you had code that did (new Url('<current>')->toString() on that page it would trigger the out bound processing and it gets sent the raw path /admin/reports/memcache/default/cache:11211. Now that gets sent into the ::create method and through parse_url. parse_url is like "it looks like there's a port but everything before it is scuffed so this isn't a valid url" returns false. This actually kinda sends Symfony down undefined code path that triggers this deprecation https://github.com/symfony/symfony/issues/47084 and just generally doesn't work. The current behavior actually then processes the / path and gets route which gets the wrong route and admin/lb values.

As noted, I don't expect this generally affects any real world uses but it does cause a deprecation warning and in some future PHP version an error.

The maybe obvious solution of using rawurlencode doesn't work because it encodes '/' so this probably has to manually escape some things which feels icky but is probably the right solution.

Bonus review:

1. This pattern:

   $request = Request::create($path);
   $match = $router->matchRequest($request);
   

   is the same as the much easier to read

   $match = $router->match($path);
   

2. The router services should be injected into the constructor.

Here's an updated patch. It addresses my review but also makes this patch D10 compatible.

Also this restores the test that was removed in #28

Probably still needs real tests though.

Hi @neclimdul ,Reviewed the patch on drupal version 9.5.3-dev. I think Sub-pathauto (Sub-path URL Aliases) Version: 8.x-1.x-dev Works with druapl versions 8 and 9. it is not compatible with D10. So i have tested the patch with drupal9. Patch works perfectly for me. Need RTBC+1.
Before applying the patch

After applying the patch

Needs re-roll

Reroll

Testing concern:
- The default value of `process_admin_routes` is TRUE. However, in the `/tests/src/Kernel/SubPathautoKernelTest.php b/tests/src/Kernel/SubPathautoKernelTest.php` we have `\Drupal::configFactory()->getEditable('subpathauto.settings')->set('process_admin_routes', TRUE)->save();` Instead of replicating the default setting, in the test it should start with default settings and then toggle the setting and confirm it is working as expected.

Great work anyone involved in this patch so far. Does anyone else consider this essential functionality for subpathauto? Would be great to get this into the official release.

Improved testing. Does what Nick suggested plus a bit more because it doesn't look like there was any _actual_ testing of the new feature provided here.

woops... diff'd the wrong commit. This should be correct.

Also noticed there was a dead property on the path processor. @see interdiff.

Nice one @neclimdul. Will test this today.

Maybe we should use a generic solution #2964786: Ignore specific paths

Like this we can ignore not only admin, but any path we want. It would be more flexible

Re-rolled patch for 8.x-1.4

MR shouldn't be introducing any code standards issues.
see https://git.drupalcode.org/issue/subpathauto-2830425/-/pipelines/316044

Fixed

While testing the latest MR it gave an error that dynamic properties was deprecated, so adding the $adminCache property resolves the error.

I would suggest creating a follow-on issue to rework some of the logic to properly use DI for \Drupal::cache(), and some of the other things that are loaded via services.

Marking the existing work as RTBC as it works really well.

Can the maintainer give his opinion on this generic solution for ignoring paths?:

#2964786: Ignore specific paths

I think that approach will clash with the one proposed in this issue above.

Also the UI is not very clear:

+      '#title' => $this->t('Apply sub-paths aliases to admin routes.'),

For the user it is not clear what "admin routes" mean. Does this also apply to taxonomy and user edit paths, for example?

I would avoid the word "route" because it is quite technical and because elsewhere in the UI it says "paths".

My proposal would be: "Provide sub-path aliases for admin paths"

+      '#description' => $this->t('This will enable routes like node/[id]/edit to also be altered.'),

This is a bit hard to understand.

My proposal would be "Creates aliases for admin paths like node/[id]/edit and user/[id]/edit"

+      '#title' => $this->t('Apply sub-paths aliases to layout builder route.'),
+      '#description' => $this->t('This will enable routes like node/[id]/layout to also be altered.'),

Same issues.

My proposal would be:
"Provide sub-path aliases for layout builder paths"
"Creates aliases for layout builder paths like node/[id]/layout"

Added a fix to handle empty path, to replicate this issue you need to create a URL alias with just "/", screenshot attached.

I propose to postpone this, and go for the more generic solution of #2964786: Ignore specific paths

Port of https://git.drupalcode.org/project/subpathauto/-/merge_requests/8 for 1.14

Port of https://git.drupalcode.org/project/subpathauto/-/merge_requests/8 for 1.14