- 🇺🇸United States neclimdul Houston, TX
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.
- 🇮🇳India rinku jacob 13 Kerala
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
- Status changed to Needs work
almost 2 years ago 2:30pm 13 February 2023 - Status changed to Needs review
almost 2 years ago 3:17pm 15 February 2023 - Status changed to Needs work
almost 2 years ago 6:21pm 15 February 2023 - 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
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. - Issue was unassigned.
- Status changed to Needs review
almost 2 years ago 7:07pm 1 May 2023 - last update
almost 2 years ago Patch Failed to Apply - 🇺🇸United States neclimdul Houston, TX
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.
- last update
almost 2 years ago 21 pass - 🇺🇸United States neclimdul Houston, TX
woops... diff'd the wrong commit. This should be correct.
Also noticed there was a dead property on the path processor. @see interdiff.
- 🇬🇧United Kingdom mistergroove
Nice one @neclimdul. Will test this today.
- 🇧🇪Belgium lobsterr
Maybe we should use a generic solution ✨ Ignore specific paths Needs work
Like this we can ignore not only admin, but any path we want. It would be more flexible
- 🇨🇦Canada nickdickinsonwilde Victoria, BC (T'So-uke lands)
MR shouldn't be introducing any code standards issues.
see https://git.drupalcode.org/issue/subpathauto-2830425/-/pipelines/316044 - First commit to issue fork.
- 🇺🇸United States DamienMcKenna NH, USA
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.
- 🇺🇸United States DamienMcKenna NH, USA
Marking the existing work as RTBC as it works really well.
- 🇳🇱Netherlands batigolix Utrecht
Can the maintainer give his opinion on this generic solution for ignoring paths?:
✨ Ignore specific paths Needs work
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"