Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
There is a very subtle bug in the alias storage from #2336597: Convert path aliases to full featured entities.
Steps to recreate
- Create a site with the standard install profile.
- disable the /node view
- Create a node with the path alias '/'
- Got to site settings and set the frontpage path to '/'
In 8.7 and prior you will see all of these succeed and the frontpage and '/' path of your site will be a node.
In 8.8 you will see all of these steps complete without errors but trying to view the node will be broken and the frontpage of your site will be a 404.
Description of the bug
In 8.7 the path validation looks like this which will be important to reference.
// Trim the submitted value of whitespace and slashes.
$alias = rtrim(trim($element['alias']['#value']), " \\/");
if (!empty($alias)) {
$form_state->setValueForElement($element['alias'], $alias);
// Validate that the submitted alias does not exist yet.
$is_exists = \Drupal::service('path.alias_storage')->aliasExists($alias, $element['langcode']['#value'], $element['source']['#value']);
if ($is_exists) {
$form_state->setError($element['alias'], t('The alias is already in use.'));
}
}
if ($alias && $alias[0] !== '/') {
$form_state->setError($element['alias'], t('The alias needs to start with a slash.'));
}
In 8.8/8.9 the entity has a regex constraint of /^\//i and a preSave that does this:
// Trim the alias value of whitespace and slashes. Ensure to not trim the
// slash on the left side.
$alias = rtrim(trim($this->getAlias()), "\\/");
$this->setAlias($alias);
The patch converting to entities translated the trim logic and slash enforcement and a cursory comparison might look like the two doing the same thing. There's a subtle difference though when you pass '/' as the value.
In 8.6 we do the trims, and the rtrim() removes the right slash which in this case is also the left slash. But then we do this empty comparison and skip the rest of the validation because it passed form api's empty validation and a empty value means its "/" and everything is good. It doesn't also does not touch the form state, and the '/' gets saved. Inside the empty check for other paths we save the trimmed path to form state and everything is happy. This kind of complicated interaction between the validation and processing is why we like moving away from doing it but it worked.
In 8.8 '/' passes through all the validation but then when get to preSave we always trim the right '/' which leaves and empty string, which then saves as null in the database and... whoops the node's path is broken.
Proposed resolution
Fix the trim logic in presave to correctly handle '/'
Remaining tasks
Core-Maintainers: Close MR !972 (outdated)
User interface changes
N/A
API changes
N/A, this fixes a regression.
Data model changes
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #90 | 3100350-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #88 | 3100350-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #86 | 3100350-nr-bot.txt | 90 bytes | needs-review-queue-bot |
| #49 | bef_pat43_aliaspath.png | 150.08 KB | sonam.chaturvedi |
| #49 | after_pat43.mov | 12.33 MB | sonam.chaturvedi |
Issue fork drupal-3100350
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:
changes
Comments
Comment #2
neclimdulHere's a simple path. I'm not sure if its the best approach since it will be a bit inefficient trimming from both sides and re-add the preceding slash but it works. Maybe someone has a more clever and less brute force approach.
I'm also not sure where tests for this entity live so I didn't create a test just yet.
Comment #4
neclimdulHm yeah I guess I didn't quite recreate the same logic either on the other side. Here's a try with a regex. Its kinda hard to read but it does the same thing as the trims but excludes a starting slash.
I put the method in a static so I could unit test it and so we could use it in the widget if we wanted... not sure about it though it was just a quick way to get it tested.
Comment #5
neclimdul@group
Comment #6
berdirI was wondering why that old code was so complicated.
Other than BC, what's the point of a / alias though? I'd rather properly solve the old issue about generating an URL for the front node or something resulting in a path instead of /. This seems like a workaround for that?
Comment #7
neclimdulThe site I ran into this on used '/' as the alias as a convention for some various install profile stuff that was written like 3 years ago. I think at the time it made some things work better. The fact that it results in a broken node has to be dealt with regardless though because any user can type '/' in currently and break the routing of a node. I'm not sure changing the validation to block it is correct.
Comment #9
tanubansal commentedTested the following steps on 9.1 :
Create a site with the standard install profile.
disable the /node view
Create a node with the path alias '/'
Got to site settings and set the frontpage path to '/'
Same issue is there on 9.1. Its showing 'Page not found' when trying to view the node having path alias '/'. Also, /node view is also showing 'Page not found'
Comment #10
Oscaner commentedComment #12
ntsekov commentedUpdated patch for Drupal 8.9.13
Comment #13
ranjith_kumar_k_u commentedUpdated for 9.2
Comment #14
ranjith_kumar_k_u commentedRemoved the white space
Comment #15
gauravvvv commented#14 working for me. Patch applied cleanly. Adding screenshot for reference. Moving to RTBC.
Comment #16
gauravvvv commentedComment #17
catchThis needs test coverage.
I also have the same question as Berdir here. But if this previously did work (even in an undefined way), it might make sense to restore that behaviour here, then decide whether we should change the validation to prevent it in a follow-up.
Comment #22
tr commented1) You have to remove the unused use statements, otherwise you're going to keep seeing "Custom Commands Failed". I don't know why you put the use statements in there in the first place as they have nothing to do with the patch.
2) 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.
3) As said by others, needs a test. Test that the '/' alias works. Show that the test fails without the patch and passes with the patch. That way if someone does try to rewrite the regular expression in the future the tests will let them know if they did it right.
Comment #23
gauravvvv commentedComment #24
vikashsoni commentedpatch #14 applied successfully
for ref sharing screenshots ...
Comment #25
tr commented@vikashsoni Please do not post just to say the patch applied successfully. That is what the automated tests are for, and they demonstrate the patch does apply without cluttering this issue with irrelevant comments and screenshots. If the patch is old, you can always trigger a re-test to show if it still applies or not.
If you want to help contribute, apply the patch on your site and verify that it works as describe in the issue summary, not just that it applies. Post your experience with the patch. Test if the above concerns have been addressed by the patch, and point out problems with the patch, if any. Then change the issue status based on your review.
This issue is at "Needs work" because a number of problems have been pointed out and not yet addressed. You could help fix the problems. But your current comment adds nothing to this issue, and it appears that you just attached screenshots so that you could get credit for this issue. That does not help move this issue forward to completion.
Comment #27
danflanagan8Here's a test-only fail patch. I was able to add a few quick assertions to existing tests. I'm going to post and run it here first. If all goes well (meaning if it fails!), then we can add it to the MR.
Locally, the code from the MR fixed these tests, which is cool.
Comment #29
danflanagan8The fail test in #27 failed correctly. It won't apply to the issue fork branch though since that is so far behind. I'm not confident working with MRs so I'm going to have to leave it to someone else to get the MR up to date and then add the patch from #27.
I'm removing the Needs tests tag.
Comment #31
ankithashettyAs requested in #29, rebased the existing MR and applied the patch provided in #27, thanks!
Comment #32
superbiche commentedSeems that
\Drupal::service('path.matcher')->isFrontPage()returns false even when I'm actually on the / frontpage.Comment #33
tr commented@superbiche: Are you saying it returns false with the patch and returns true without the patch? Do you have your frontpage set the "/" ?
Please provide instructions for reproducing this problem if you think there's a problem with the patch. And a test case to demonstrate the problem would be great, so we could use that test case to test the proposed solution.
Comment #34
superbiche commented@TR: it returns false with the patch. Without the patch, I cannot save the "/" alias, so I can't say what it returns.
I tried to force the "/" in the URL as I thought this could be an issue, but the result is the same.
Steps to reproduce on 9.3.9:
\Drupal::service('path.matcher')->isFrontPage()returns falseI'll write a testcase this evening after my day job.
Comment #36
smustgrave commentedMoving back to needs work for the test case mentioned in #34
Comment #37
neclimdulI don't know, this never had any connection to the matcher. Sounds like a different kinda bug. Probably this one #1503146: Aliased paths cannot be set as front page.
Rerolled fixing a bunch of updates to testing methods and re-adding the original unit tests from the original patch which where lost for some reason.
Comment #38
akashkumar07 commentedFixing the CS issues of #37.
Comment #39
akashkumar07 commentedI missed one more cs issue to fix in the last patch.
Comment #40
smustgrave commentedComment #42
mrshowermanTrying to make this work on 9.5.x
Comment #43
mrshowermanComment #44
anybodyJust 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?
Comment #45
anybodyComment #46
rajneeshb commentedReviewed #43 patch applied successfully and working fine.
Attaching screenshot for reference.
Comment #47
anybody@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!
Comment #48
mrshowerman@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.
Comment #49
sonam.chaturvedi commentedVerified 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
Comment #50
smustgrave commentedReviewing the code as manual testing was completed in #49 thanks!
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.
Comment #51
gauravvvv commentedUpdated attributions
Comment #52
tr commentedAs I said in #22,
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:
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:
trimAlias()method.Comment #53
anybodyRe #50
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. "/" is really special as root. 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. And what we really need to make sore is, that there's no chance for a redirect loop.
Comment #54
grevil commentedWhat'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.
Comment #56
grevil commentedI 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, see https://git.drupalcode.org/project/drupal/-/merge_requests/3667.
Comment #57
grevil commentedNo more patches please, let's get this done through "3100350-unable-save-to-slash"!
Comment #60
grevil commentedTried to close the wrong MR, sry.
Comment #61
grevil commentedI 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!
Comment #62
grevil commentedThe 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.Comment #63
grevil commentedFixed 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!
Comment #64
anybodyGreat work @Grevil! Let's push this forward to take it into 10.1.x asap!
I've 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.
Comment #65
smustgrave commentedSeems there are some failures in the MR.
Comment #66
anybodyLet's see if this works now. Had some final discussions with @Grevil.
Comment #67
smustgrave commentedFILE: /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
Comment #68
anybodyComment #69
smustgrave commentedChange looks good to me.
Only thing left I can see is a change record for the new public static function on PathAlias.
Comment #70
grevil commented@smustgrave, great to hear! I added a change record. See https://www.drupal.org/node/3348648.
Comment #71
smustgrave commentedThanks!
Comment #73
larowlanComment #74
larowlanLeft some comments on the MR, looks close though
Comment #75
grevil commentedComment #76
grevil commentedComment #77
grevil commentedComment #78
smustgrave commented/var/www/html/core/modules/path_alias/src/Entity/PathAlias.php:68:39 - Unknown word (occured)
Comment #79
grevil commentedComment #80
grevil commentedFinally, all tests green now! Please review once again! :)
Comment #81
smustgrave commentedThink this is ready for committer review.
Comment #82
larowlanRemoving credits for screenshots that were not the first set
Left some comments in the MR
Updated issue credits
Comment #83
anybodyResolved comments from #82.
Never did this (regex commenting) before, hope it works. Please review the regex and explanation carefully, didn't write any for a while... sorry.
Comment #84
smustgrave commentedMR has a few test failures but seems close!
Comment #85
anybodyComment #86
needs-review-queue-bot commentedThe 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.
Comment #87
anybodyComment #88
needs-review-queue-bot commentedThe 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.
Comment #89
anybody@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.
Comment #90
needs-review-queue-bot commentedThe 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.
Comment #92
nod_closed it sorry for the noise
Comment #93
smustgrave commentedSorry @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.
Comment #94
alexpottAre 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 /.
Comment #95
dieterholvoet commentedI'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/homepageto/. If you set it tonode/IDyou 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.Comment #96
alexpottAlso this is inconsistent with views where you cannot set a path to empty or just /
Comment #97
anybody@alexpott:
Thanks for looking into this and taking the time. I totally understand your concerns. And we should care. "/" 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% safe..
I'm fine if someone still wants to use the complex and error-prune "/homepage" way, which will still work.
But for us it causes technical, SEO and translations trouble again and again insted of having the reliable solution outlined here, which is safe-explaining, expected and works safe.
For example
So the fake path causes a lot of follow-up problems, duplicate content / duplicate URL SEO trouble and is much more 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.
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?
Leaving the path empty should still be disallowed, I think! "/" should be something different then empty.
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.
Comment #98
alexpott@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.
Comment #99
berdirI didn't fully read everything, but I feel like #1255092: url() should return / when asked for the URL of the frontpage, 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.
Comment #100
neclimdul@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.
Comment #101
alexpott@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.
Comment #102
berdirWe 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.
Comment #103
smustgrave commentedMaybe lets try this.
Postpone this issue for #1255092: url() should return / when asked for the URL of the frontpage
Once that lands we see if anything here needs/can be done.
Comment #105
andypost