Unable to save '/' root path alias

Just ran into the same thing. Having "/" as URL alias for the front page is super important for SEO and the current situation is a real issue.

@mrshowerman I guess this should be "Needs review"? And I don't think we need this to work with 9.5.x as it will be against 10.1.x?

Reviewed #43 patch applied successfully and working fine.
Attaching screenshot for reference.

@rajneeshb did you save and re-open (edit) the node? That will be important, as currently, afterwards the "/" is gone.
The same should also be tried at /admin/config/search/path (create and edit).

Could you also provide screenshots from that process, please?

If both are fine, we're VERY close to RTBC. I'll also mark this for review in our company. Thanks!

@Anybody, I needed this for D9.5 so I tried to provide a fully working patch for it. But I agree that it's not vital for the tests to be passing against 9.5 since it's gonna be in 10.1 only.
So NR is the correct status, thanks.

Verified and tested patch #43 on 10.1.x-dev. Patch applied cleanly and resolves the issue.

Test Result:
1. Able to create a node with the path alias '/'
2. Able to set front page URL in basic site settings with "/"
3. Able to add and edit URL alias path with "/"
4. After save and re-open (edit) the node, "/" is not gone.
Please refer attached before patch screenshots and after patch video

RTBC +1

Reviewing the code as manual testing was completed in #49 thanks!

trimAlias($alias)

1. Should be typehinted both for parameter and return type

Rest seems good

May be an issue though

I'm able to save "/" as the default front page even without a node who's alias is "/" seems it should stop me right? Think that should be answered before advancing.

Updated attributions

As I said in #22,

Given the subtlety of the bug, why is there not even one comment in the code about the new regular expression? Someone is going to see that a few years from now and "simplify" it. Without comments, they will have no way of knowing why it was written that way.

The new regular expression and trimAlias() method absolutely need comments. "Helper method to trim an alias consistently." is totally insufficient. What is it trimming? From the beginning or the end or both? Describe what the alias should look like after trimming. Do properly formatted aliases start with a slash? End with a slash? Contain spaces? etc. A complete description should appear in the method documentation comment as this will become part of the core Drupal API. In-line comments should also describe the intent of the parts of the regexp.

The description of what is considered valid input on the form should also be checked and expanded. Right now the form element looks like this:

    $element['alias'] = [
      '#type' => 'textfield',
      '#title' => $element['#title'],
      '#default_value' => $items[$delta]->alias,
      '#required' => $element['#required'],
      '#maxlength' => 255,
      '#description' => $this->t('Specify an alternative path by which this data can be accessed. For example, type "/about" when writing an about page.'),
    ];

This implies to the user that an alias must start with a /, but it doesn't say anything about allowed characters (are spaces or other characters that might have to be HTML encoded allowed?), can you use slashes inside an alias (e.g. /about/this/site), etc.

It's very frustrating to have a form validation function which doesn't explain these things up front to the user.

So I'm asking three things:

1. Documentation comments on the trimAlias() method.
2. In-line comments for the regular expression.
3. Form element #description which tells the user what valid input is.

Re #50

I'm able to save "/" as the default front page even without a node who's alias is "/" seems it should stop me right? Think that should be answered before advancing.

No, I don't think so. At least it doesn't have to be a node, just an existing path. I'd personally be fine to not do further checks on "/" and trust something is existing there. We *could* also say, that's the user's job to ensure.
If there's a validation, it should be against all existing paths, not just nodes.

What's up with the issue fork's in this issue? One is for 9.2 specifically (containing old code) and the other is a complete different implementation, compared to patch #43 by @mrshowerman!

Going to clean up a bit here.

I reset the "3100350-unable-save-to-slash" branch to 10.1.x and applied patch #43 by @mrshowerman on top of it.

Since only core maintainers can change the target branch of a merge request, I created a THIRD MR for 10.1.x.

No more patches please, let's get this done through "3100350-unable-save-to-slash"!

Grevil → closed merge request !3667

Grevil → reopened merge request !3667

Tried to close the wrong MR, sry.

I agree with @TR's comment in #52. At least, the helper method should specify how it trims a given string. Maybe, whoever implemented the helper function, could add an appropriate description? Would be appreciated!

The trimAlias implementation is still not complete. The method should be "idempotent" (running the method on the same string multiple times should lead to the same result as running it once), which it currently isn't. The tests show this perfectly:

'/first/second// / ' leads to '/first/second// ', now if we run the output through the function again, the output will be '/first/second" and therefore the function isn't idempotent. Furthermore, the comment stating "Trim the alias value of whitespace and slashes" is incorrect, as running it once won't completely remove all whitespaces and slashes.

Fixed the regex, adjusted the function description and adjusted the tests. (Note, that an array of strings can not be returned by trimAlias, as we are not allowing arrays as parameters).

Please review!

Great work @Grevil! Let's push this forward to take it into 10.1.x asap!

I added my review comments, just some polishing, then this is RTBC from my side.

@catch, @smustgrave, @TR could you also have a final look to get this fixed and safe us all from further rebases?
For site builders, this is an important fix, as it again and again leads to issues for setting the frontpage path to "/". I'd even tend to set priority to "Major" as it's definitely broken functionality currently, removing the "/" from the field when saving. What do you think?

@Maintainers:
MR !972 should please be closed outdated. If this should be backported to 9.x it should happen based on the latest MR.

Seems there are some failures in the MR.

Let's see if this works now. Had some final discussions with @Grevil.

FILE: /var/www/html/core/modules/path_alias/src/Entity/PathAlias.php
----------------------------------------------------------------------
FOUND 12 ERRORS AFFECTING 12 LINES
----------------------------------------------------------------------
51 | ERROR | [x] Line indented incorrectly; expected 2 spaces, found
| | 3
| | (Drupal.WhiteSpace.ScopeIndent.IncorrectExact)
52 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
53 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
54 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
55 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
56 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
57 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
58 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
59 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
60 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
61 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
62 | ERROR | [x] Expected 4 space(s) before asterisk; 3 found
| | (Drupal.Commenting.DocCommentAlignment.SpaceBeforeStar)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 12 MARKED SNIFF VIOLATIONS AUTOMATICALLY

Change looks good to me.

Only thing left I can see is a change record for the new public static function on PathAlias.

@smustgrave, great to hear! I added a change record. See https://www.drupal.org/node/3348648 → .

Thanks!

larowlan → closed merge request !972

Left some comments on the MR, looks close though

/var/www/html/core/modules/path_alias/src/Entity/PathAlias.php:68:39 - Unknown word (occured)

Finally, all tests green now! Please review once again! :)

Think this is ready for committer review.

Removing credits for screenshots that were not the first set

Left some comments in the MR

Updated issue credits

Resolved comments from #82.

Never did this before, hope it works. Please review the regex and explanation carefully, didn't write any for a while... sorry.

MR has a few test failures but seems close!

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@smustgrave seems like MR !1162 (9.3.x) is causing the trouble? Do you have permissions to close it? Tests for the 10.1.x seem to be green? Or do I get the bot wrong?

Furthermore I don't seem to have permission to set the points mentioned above as "resolved"? No checkbox for me below the comments in GitLab.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

nod_ → closed merge request !1162

closed it sorry for the noise

Sorry @Anybody just saw your comment. Don't know how to grant ability to close threads myself usually I just mark them with a comment, like you did.

But changes look good.

Are we sure about this? I think we should prevent the path alias being set to /. If you sent the path alias on node save it takes you to the front page even before / has been set as the frontpage. I think what we'd prefer people to do in this situation is not set / as the path alias for the name. But to set the frontpage to whatever you've set the node's alias to or to node/ID.

Yes this is a regression from ages ago but I think we should question the UX of setting a path alias to / - it does not work till you set the front page to /.

But to set the frontpage to whatever you've set the node's alias to or to node/ID.

I'm not a fan of either option. If you set it to the node's alias, e.g. /homepage, the homepage will be served on both / and /homepage, which is something you don't want. At least there should be a redirect from /homepage to /. If you set it to node/ID you create a direct coupling between content and config, which is something I like to avoid. Config is committed to VCS and deployed across environments, while content ID's can differ between environments.

Also this is inconsistent with views where you cannot set a path to empty or just /

@alexpott:

Are we sure about this? I think we should prevent the path alias being set to /.

Thanks for looking into this and taking the time. I totally understand your concerns. "/" is something special and the frontpage setting also is.
But yes, we're sure. This isn't a theoretical issue, instead it's a real world problem we're running into again and again
#95 points out parts of this. Another one is the fact that currently the frontpage needs to have a fake ("/homepage" sth.) path alias, while what we want is just a stable "/" (frontpage) path alias that is clear, unique, works for SEO and is 100% clear..

I'm fine if someone still wants to use the complex and error-prune "/homepage" way, but for us it causes trouble technical, SEO and translation trouble again and again insted of having the reliable solution outlined here, which is totally clear and works safe.

For example we don't want certain tokens / metatags to expose the fake path ("/homepage") as that fake path simply shouldn't exist and is never needed. Links pointing to this node built from its route should also never point to "/homepage" which would / should at least cause a redirect, but the correct way would also be to point to "/". So the fake path causes a lot of follow-up issues and duplicate content / duplicate URL trouble and is dangerous for people who don't know about the technical details.

The solution from this issue works safe, is easy to understand and is what you'd expect.

Also this is inconsistent with views where you cannot set a path to empty or just /

I think you're right, setting "/" to make something the true & safe homepage root, shouldn't only work for nodes.
So we should create a separate issue for views to also allow setting "/" if you wish to use the view as frontpage?

If you sent the path alias on node save it takes you to the front page even before "/" has been set as the frontpage.

I agree we should further discuss how to improve that, to have a consistent, intuitive and bulletproof behavior for both without the downsides mentioned. Not yet sure how to solve that perfectly, also considering #95.

Finally once again, this is not a theoretical problem, it's a real one and I think perhaps even due to too much complexity and restrictions, not too less. So let's try to simplify this as much as possible.

@Anybody good points. So I think we can't go forward with the patch as it currently is. The UX makes no sense. If I apply this patch and change a node's path alias to / when I save I get taken to some completely other page. Until I've set the system.site:front.page to '/'. I also think once you have a path alias that's '/' the front page setting is a bit strange. I'm not sure about the best way for any of this to work. But yeah if we want to allow this for nodes it feels as though we should allow this for views.

Also, for what it is worth, when alias a node to '/' and then go and set the front page to '/' in the system information the system.site:front.page value will be set to '/node/NID' - that's how that stuff works. So this patch isn't really changing how frontpage resolution is working it's really only changing how path aliasing is working. I do agree with @Anybody's comments in #97 that having a node only aliased to '/' is a good thing. Maybe the best way is if the user changes the path to '/' to also update the system.site:front.page value as this is what is intended BUT I think that this should be behind the same permission that allows you to change system.site. And if you do change the front page by saving a node there should be an additional message on the screen that informs the user this has occurred.

I didn't fully read everything, but I feel like 🐛 url() should return / when asked for the URL of the frontpage Needs work , which is already a related issue, would solve at least part of this in a cleaner way, that we would just consistently render a link to to the frontpage node/path as /. Not quite sure if that would also work with the frontpage setting set to an alias. Then there technically would be a path/alias, but you'd never see it.

Using aliases for this feels like a workaround.

@berdir I think you might be right but to reiterate Dieters point more explicitly, how would I set the configuration for `/node/X` in say an installation profile where some default content will populate the node? The config used for the installation has to have the path but the X doesn't exist until after the installation. Using the alias / on the content and the config resolves this by just agreeing on the special path.

@ As was the focus of the original report and the above example, I'm _most_ concerned about getting the validation fixed so this is technically possible. As far as the UI quirk described by Alex, pretty sure that's how Drupal's worked since there was a front page which is well before my time. Just confirmed that's pretty much exactly the behavior in D7 using simpletest.me. If that's a hard requirement to fix I guess we can try to tackle it but it seems like a dedicated usability issue probably makes more sense.

@neclimdul - unfortunately it doesn't quite work like that. If you try and set system.site:page.front to '/' via the UI it'll resolve the alias and set it to node/NID. Maybe you can hack the config to have '/' but still if you go to site information form and press save it'll change.

@berdir I think you might be right but to reiterate Dieters point more explicitly, how would I set the configuration for `/node/X` in say an installation profile where some default content will populate the node?

We solve that by implementing the default_content import event and then based on the UUID set the front/404/403 setting, which all need this as well. We also set some other stuff there, for example simple_sitemap excludes of those.

Maybe lets try this.

Postpone this issue for 🐛 url() should return / when asked for the URL of the frontpage Needs work

Once that lands we see if anything here needs/can be done.