Problem/Motivation

Drupal Core since Drupal 8 uses a leading slash in path fields, e.g. Node path alias, custom menu items, ...

Some modules, especially coming from Drupal 7 break with the new standard.
Sadly Pathauto as a very widely used module also breaks with it in the path pattern.

This should be fixed to validate the path pattern to start with a leadings slash.

Steps to reproduce

Enter a path pattern without a leading slash, e.g. faq/[node:title].
The form can be saved without an error and the value is also saved and displayed without a leading slash.

Adding a leading slash also works, which makes it even unclear what's expected and which side-effects this might have!

Proposed resolution

  1. Add the core description to the "Path pattern" input field
  2. Add a validation for the leading slash, like for other path fields in core. If the leading slash is missing, reject the submission. The core error message is: "The alias path has to start with a slash."
  3. Add an update hook to transform all path patterns to use the leading slash. Ensure that this won't result in double leading slashes, by ltrim('/')'ing the value first
  4. Ensure the migration also adds a leading slash when upgrading from earlier Drupal versions

See the implementations here: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/path_...

As a common module, pathauto should act as role model for other modules here.

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork pathauto-3466282

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Anybody created an issue. See original summary.

sourav_paul’s picture

sourav_paul
Location Kolkata
commented
Assigned: Unassigned » sourav_paul
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue summary: View changes

Did some initial implementation, feel free to proceed! :)

Anybody opened merge request !85

sourav_paul’s picture

sourav_paul
Location Kolkata
commented
Assigned: sourav_paul » Unassigned
Status: Active » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

@Sourav_Paul tests still fail and there's a lot of work to do. Changing a word isn't a big deal here, sorry. But thank you anyway.

lrwebks’s picture

lrwebks commented
Assigned: Unassigned » lrwebks
lrwebks’s picture

lrwebks commented
Status: Needs work » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

@lrwebks thanks! Some points:
1. Pipeline is red, while it's green on the project page, that looks wrong?
2. Shouldn't the value be validated in the form?

lrwebks’s picture

lrwebks commented

@anybody regarding #9

  1. I'm looking into it right now
  2. The function pathauto_pattern_validate() from the pathauto.module file is called from the form via the following line in PatternEditForm.php:
    '#element_validate' => ['token_element_validate', 'pathauto_pattern_validate'],

    The other checks for missing tokens and whitespace at the end of the path were already placed in the pathauto_pattern_validate() function, which is why I placed the new check there as well. I suppose that we could try to move this whole functionality over to the form if it is not used by any other part of the module, but the current implementation works fine right now.

  3. Also, are you able to resolve the two threads that I opened? Resolving your own threads seems to be disabled in the GitLab UI for this module or something, at least I am unable to do so.
lrwebks’s picture

lrwebks commented

Well, the failing test was green locally, so I decided to try to run another pipeline, and: No more fails! Seems weird to me though, as I did not update the fork or anything similar in between pipelines, so this feels like a weird PHPUnit hiccup… It could be related to the fact that the failed FunctionalJavascript test seems to use the waitForElementVisible() function a lot, which can fail randomly if the page loading takes too long.
Anyway, back to review!

lrwebks’s picture

lrwebks commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Adding a leading slash also works, which makes it even unclear what's expected and which side-effects this might have!

The answer should be "no side effects", generated aliases will always have a leading /.

Instead of failing with an error, we could just ensure the / is always there and add it on save?

Also not sure this is a bug, this just makes things a bit more consistent in the pathauto UI.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks for the feedback @berdir. :)

Also not sure this is a bug, this just makes things a bit more consistent in the pathauto UI.

Yes, I remember Drupal 8 made this switch in several cases in core, forcing to start with a leading slash, which was unclear in Drupal 7- And I think it's a good idea for consistency = usability. So I'd clearly vote to do the same in all Drupal 8+ modules, even if it's not a clear bug.

grevil’s picture

grevil commented
Status: Needs review » Needs work

I agree with @berdir here. Instead of throwing a validation error, we should simply add a leading slash to the pathauto pattern on submit, if none exists, instead of throwing a validation error here!

dalemoore’s picture

dalemoore commented

I'm in favor of consistently including the leading slash, I just noticed that in this new site I am working on I've included it in some patterns and excluded it in others. As long as it silently adds it on save but continues to work for existing patterns without (or maybe updates existing ones on sav of the Patterns page, or in a module update hook) it sounds like a nice feature to improve consistency.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Yes, I also agree with #13!

lrwebks’s picture

lrwebks commented
Status: Needs work » Needs review

Works fine now, pipeline currently fails because of some server error when composer pulls packages (cloudflare outage, probably temporary).

lrwebks’s picture

lrwebks commented
Assigned: lrwebks » Unassigned
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Great @lrwebks! All green now with Cloudflare back at work :)

grevil’s picture

grevil commented
Status: Needs review » Needs work

LGTM, and works as expected, just added a few comments regarding the tests.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Nice, thanks for the feedback! @lrwebks please finish :)

lrwebks’s picture

lrwebks commented
Status: Needs work » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

I still see a lot of yellow and red in the pipeline, please fix the related things. And especially the test(s)

lrwebks’s picture

lrwebks commented
Status: Needs work » Needs review

Fork update seems to have resolved the pipeline again, so the failure was unrelated.

lrwebks’s picture

lrwebks commented

@berdir, since I can't resolve the MR threads myself, could you do that (if what we talked about is actually resolved of course)? Pipeline is green again.

mably’s picture

mably commented
Related issues: +#2702373: Use Entity Validation API for pathauto patterns?
mably’s picture

mably commented
Related issues: +#2728725: Special characters like tab or spaces in pattern can break alias generation

Added the leading slash fix to this issue's MR: #2728725: Special characters like tab or spaces in pattern can break alias generation

mably’s picture

mably commented
Category: Bug report » Feature request
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thx @mably let's wait for @berdir's feedback on how to proceed here and finish this.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Agreed with no update function needed here, and yeah, the other MR now covers this and this is essentially a duplicate. Don't forget to transfer contributions over (I'd close as duplicate instead of marking as fixed, for posterity and generated release notes)

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks for pushing things forward here with us @mably and @berdir! I'm very happy to see #2728725: Special characters like tab or spaces in pattern can break alias generation is close to the finish line! 🎉

THANK YOU!

mably’s picture

mably commented
Status: Needs review » Closed (duplicate)

Closed as duplicate of #2728725.

Thanks everybody!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.