Problem/Motivation
PathMatcher::isFrontPage
does not work with aliased paths, causing the front page path to not redirect to the root path if an aliased path is used. Internal paths like /node/1
work fine.
- With /homepage (path alias) as front page, visiting / redirects to /homepage
- With /node/1 as front page, visiting / does not redirect (this is the correct behavior)
The site configuration form makes it look like the front path is the path alias, but in reality the /node/1
path is stored in system.site
config. This creates a hard dependency between site content and site configuration, something that's not desirable in modern workflows.
Steps to reproduce
- Create a page node with alias /homepage
- Configure the front page path to be /homepage
- Visiting the front page will result in the /homepage path, instead of the root path
Proposed resolution
Change PathMatcher::isFrontPage
to check both internal and aliased paths.
Remaining tasks
Comment | File | Size | Author |
---|---|---|---|
#72 | drupal-n1503146-72.patch | 8.45 KB | DamienMcKenna |
#72 | drupal-n1503146-72.interdiff.txt | 2.02 KB | DamienMcKenna |
#69 | drupal-n1503146-69.patch | 6.43 KB | DamienMcKenna |
#60 | 1503146-60.patch | 6.28 KB | DieterHolvoet |
#46 | 1503146-46--test-only.patch | 2.11 KB | gambry |
Issue fork drupal-1503146
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:
Comments
Comment #1
tim.plunkettSee
drupal_path_initialize()
, it updates$_GET['q']
first.Comment #2
pendashteh CreditAttribution: pendashteh commentedIt indeed does not and here is the bug.
Take a look at what happens:
As you see, the output is 'node/2' not 'home'. first it set
$_GET['q']
tovariable_get('site_frontpage', 'node');
that's exactly what is checked bydrupal_is_front()
, but then it reverts back to the source by callingdrupal_get_normal_path()
.It's apparent the following code simply fails:
Comment #3
AaronBaumanThe only case I can think of that you might run into this is if the variable
site_frontpage
is manually set to a url alias, e.g. via "drush vset" or calling variable_set() directly.When you submit the Site Info config page, any alias gets normalized, so that Drupal need not compare normal paths AND the aliases.
Please verify that your site_frontpage variable is a system path, and not an alias, by re-submitting the Site Info config page (admin/config/system/site-information). And please verify that you are not calling variable_set() to programatically set this variable to an alias.
Comment #5
pendashteh CreditAttribution: pendashteh commentedThat's exactly the problem. The config page.
I have 'node/2' aliased as 'home'.
In Site Config page I set the homepage as 'home' and it accepts it. As you say it should normalize it (change it to 'node/2') but it does not.
It behaves exactly reverse; When I try to set homepage as 'node/2' it changes it back to 'home'!
The only way to get around this is to clear alias from 'node/2' and then set 'node/2' as homepage. So as you see this is a bug. I haven't set any variable manually, there is no custom module and I haven't use drush commands you told. It's a fairly fresh install and is happening to me in each project.
I think it worth to mention that I have two languages defined and the default language is not English in my projects.
And a list of contrib modules in two of my projects that have this problem:
Comment #6
tim.plunkettCan you reproduce on a clean install?
Comment #7
mahtoranjeet CreditAttribution: mahtoranjeet commentedI could not able to reproduce this on fresh installation
I have installed a fresh drupal 8.
I have 'node/2' aliased as 'home'
In Site Config page I set the homepage as 'node/2'.
then I print $variables in preprocess function. it print
[is_front] => 1
After that
In Site Config page I set homepage as 'home' and save it.
Then I print $variables it print
[is_front] => 1
Either you set homepage as node/nid (node/2) or its aliased (home) it returns true.
Comment #8
amaisano CreditAttribution: amaisano commentedI noticed this glitchy behavior too, in D7.
I have a custom Context plugin reaction changing the site_frontpage in this manner:
When I set the contextual front page through this to 'node/7' in the UI, visiting the root of the website (clicking the logo) renders as 'front'
But visiting that page directly does NOT - it renders as 'not-front'
However when I save the contextual front page in the UI as 'my-custom-path/my-page' (the alias for that same node), neither the clicking the logo nor visiting the page directly by it's alias or system path result in the page rendering as 'front' (it's rendered as not-front).
In both cases visiting the page set as front page globally in Site Information also renders as 'front' - which is more likely a problem with this custom plugin.
So something's up with how D7 is reacting to system paths vs. aliases in the front_page value.
Comment #9
dawehnerAre you still this is in the case in an up to date Drupal 8?
Comment #10
amaisano CreditAttribution: amaisano commentedCan't speak to Drupal 8, sorry.
Comment #11
dawehnerLet's move it to d7 then.
Comment #12
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI can't reproduce this in either Drupal 7 or 8.
If you look at:
https://api.drupal.org/api/drupal/modules!system!system.admin.inc/functi...
https://api.drupal.org/api/drupal/modules!system!system.admin.inc/functi...
(Drupal 7)
or:
https://api.drupal.org/api/drupal/core!modules!system!src!Form!SiteInfor...
https://api.drupal.org/api/drupal/core!modules!system!src!Form!SiteInfor...
(Drupal 8)
You can see that when the form is displayed to the administrator, the path alias (e.g. "home") with always be shown as the default value of the field, but when it is saved, the actual system path (e.g. "node/2") will be saved. Thus what is noted in #5:
is expected behavior and does not indicate a bug. Since the system path is always saved, everything should work as intended.
Can anyone reproduce this without custom code, or otherwise explain what the bug is (in either Drupal 7 or 8)?
Comment #13
gambryI don't think it is a bug. The problem is the expected behaviour must be updated as frontpage can be set in several ways directly to the variable
site_frontpage
(Features, drush, settings.php, etc.) and not passing from the form.The solution above is suggesting to move the alias-to-internal translation step from the administrative form to the
drupal_is_frontpage()
function, and so from a mapping approach on saving the form to an universally checking one on loading the page.Then whatever is the
site_frontpage
value - alias or internal - thedrupal_is_front_page
will always return correctly.EDIT/UPDATE: I've carefully read all the comments and I can see someone is tripping over some unclear bugs. I can't reproduce them on 7.x, haven't tried on 8.x but linked code is clear. The only issue I see here is still the one on the description of the issue related to drupal_is_frontpage_code(). Other core modules check both internal and alias paths on evaluating a page (i.e. block module on block_block_list_alter() when matching request path against
$block-pages
).Comment #14
chrisgross CreditAttribution: chrisgross commentedI've run into this issue, too. For whatever reason, some of my sites have the site_frontpage variable stored as the alias to the front page node, in my case 'home'. I don't know if this is from an older version of drupal that allowed this or something. Either way, wouldn't it be safe to change the code in drupal_is_front_page to
since drupal_get_normal_path() will return the node path regardless of which path is passed to it?
Comment #15
amit0212 CreditAttribution: amit0212 as a volunteer and at Iksula commentedWhen looking at the drupal_is_front_page function you see it performs a check on
($_GET['q'] == variable_get('site_frontpage', 'node'));
So it only returns true if there is an exact match.
You can recreate the function and use something this instead, to check if the frontpage url is a part of the request url:
(strpos($_GET['q'], variable_get('site_frontpage', 'node')) !== false)
All other code in drupal_is_front_page is for static caching. You want to copy that if you intend to call this function often. If it is for one check you can do with this simple function:
function drupal_is_front_page2() {
$is_front_page = (strpos($_GET['q'], variable_get('site_frontpage', 'node')) !== false);
return $is_front_page;
}
Comment #16
gambry#15 I don't understand your solution.
An URL beginning (or ending) with the frontpage path is totally unrelated to this issue.
The issue is about drupal returning FALSE on either D7 and D8 on checking if path is frontpage, when the site frontpage setting (system.site.page.front/site_frontpage) is an alias.
To reproduce in D8:
drush cset syste.site page.front /foo/bar
, use same alias as step 1Step to reproduce in D7 are similar, just use
drush vset site_frontpage
and paths don't start with "/".Proposed solution:
\Drupal::service('path.alias_manager')->getPathByAlias($path);
on PathMatcher::isFrontPage() for matching with system.site.page.frontdrupal_get_normal_path(variable_get('site_frontpage', 'node')));
on drupal_is_front_page()EDIT/UPDATE: I've removed the "Bug" type as I don't think is a bug, but it's not clear if system.site.page.front MUST be an internal path or it can be anything. The form itself is misleading (you add an alias, it stores its internal path but still show the alias on the form field after submission) and even if we make it clear it's easy for the developer to forget it must be internal, store an alias and break the website.
Feel free to restore "Bug".
Besides I'm thinking internal path can change - for instance between different environments - while alias can stay the same - you create a node with the same title/alias -, so I can see why a developer want to store the front page url using alias and not internal path. I'm more keen to restore the "bug" type... thoughts?
Comment #19
altrugon CreditAttribution: altrugon at Affinity Bridge commentedI was able to reproduce this bug on D8.4.5, here is how:
I hope this helps to find out what is wrong.
Comment #21
raphaeltbm CreditAttribution: raphaeltbm commentedI guess that the basic site settings form should not alias the url, it brings a lot of confusion as it is now.
See closed duplicate issue: #2958253: When exporting configuration site frontpage converts to node/id
In SiteInformationForm.php, see:
At the minimum the aliasing part should be removed to stop confusion with this screen. I don't see the point to alias the internal path. Was there a reason?
If it's really wanted to show the language-aware aliased url of the targeted content/page, it could be added below the field in the description area. Knowing than a frontpage is mostly pointing directly to
<front>
(plus when a module like redirect is enabled with the option "Enforce clean and canonical URLs." checked).If the possibility to use an alias for the frontpage settings is done, it will brings some problematics to be aware with the multilingual case too:
- make the frontpage field translatable (to be able to manage a different alias for each language) :
- how to manage a case where the frontpage will be set on different kind of entities/object ?
>> maybe on the saving, just forbidding it when an alias is filled but don't target the same internal path than on the default frontpage setting? Or should it be just ok to allow that?
And the
\Drupal::service('path.matcher')->getFrontPagePath()
should then returned the internal path it the path is aliased via agetPathByAlias()
.By the way, currently, if you really need to set a different frontpage following your environments, you should take a look to config_split and/or config_ignore modules.
Comment #22
mlncn CreditAttribution: mlncn at Agaric for Drutopia, National Institute for Children's Health Quality commentedOne way the page.front variable can be set to the alias rather than the internal path is when the configuration is being set in concert with default content, as in an installation profile or a feature (ran into this with the Drutopia distribution).
It's such a bizarre bug i think Drupal core does need to do more to process incoming front page configuration, no matter what its source, or to flag an alias as an issue on the status report dashboard or something.
Comment #25
Vj CreditAttribution: Vj as a volunteer commentedMy issue is related to redirect from domain.com to domain.com/home. Which is happening due to $this->pathMatcher->isFrontPage() in src/EventSubscriber/RouteNormalizerRequestSubscriber.php returns false on frontpage.
Steps to reproduce.
Its more of core bug which fails to detect frontpage with above steps hence redirect module fails for me.
Comment #27
aiphesHello.
I face off this issue on one of my d8 websites. Usually I don't set a specific as FP, and I use a FP template to display content.
On 894 then on 895, FP don't detect varaible and display login page instead.
Details are there: https://www.drupal.org/project/drupal/issues/3170184
Comment #28
gambryLong time has passed and this issue now requires IS updates either to reflect changes required and properly scope out solutions based on 9.x OR focusing this issue to D7 and moving D9 work somewhere else (#3170184: Homepage replaced by login page, why ? maybe?).
Comment #34
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedChecking both the internal path and the normal path in
PathMatcher::isFrontPage
seems to do the trick. Haven't added any tests yet.Comment #35
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedI added a patch of the current state of the MR for easy inclusion in projects.
Comment #36
quietone CreditAttribution: quietone at PreviousNext commentedSetting to NW for, at least, the issue summary update. A title change is needed as well because drupal_is_front_page was removed in #2388629: remove drupal_is_front_page()..
Comment #37
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedI changed the title and issue summary. Feedback on my proposed fix is welcome.
Comment #38
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedI updated the form to stop automatically resolving aliases to paths. I added a patch of the current state of the MR for easy inclusion in projects.
Comment #39
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedI noticed this approach doesn't work when using language path prefixes. I experimented a little bit, but I don't think we can do this without a dependency on the path_alias module. A couple ideas:
Comment #40
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedI went with the PathMatcher service override in the path_alias module, seems to work great.
Comment #41
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedComment #42
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedFixed CS errors.
Comment #43
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commented@ranjith_kumar_k_u development is happening in the MR, only posting the patches for use with composer-patches.
Comment #45
gambryIn this commit:
public
. So changing it toprotected
. If needs to be public, then we better add it to PathMatcherInterface too.This still needs test.
Comment #46
gambryBefore checking those failures, here a test-only patch. setting it for Needs Review just to trigger the testbot. This is Needs Work.
Comment #47
gambryOops. Needs Review, for the testbot.
Comment #48
gambryI'm not sure I understand correctly this bit of the IS:
With Standard profile, if I visit a /node/1 path configured as front page there is no redirect. Neither with the system path /node/1, nor with an alias. Anyone has got different behaviors? If I'm right, do we need an IS update?
Comment #49
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedNo, that's not what I meant:
Comment #50
gambry@DieterHolvoet it doesn't on Standard profile?
With standard:
Comment #51
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedThe site settings form automatically transforms the alias to the node path, can you check the form again to make sure that didn't happen? Because now the node ID is stored in config, and that's exactly what I'm trying to prevent from happening.
Comment #52
gambryIf I understand correctly the issue you are describing, then no. The alias is not changed into the node path in the Basic site settings form.
Comment #53
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedNot in the form because it's automatically being converted to the path alias, but did you check the actual
system.site
config?Comment #54
gambryComment #55
gambryPutting in the form
/node/1
converts it to its alias/ciao-mamma
.Comment #56
gambryI'm tempted to close this as "cannot reproduce". The issue doesn't seem to happen anymore, at least in 9.4.x.
The steps to reproduce don't trigger the issue.
The behavior either you use the system path or an alias is exactly the same, as exception of the scenario where you set a system path through the form which is replaced with its alias if one exists.
Feel free to re-open, showing exactly what the issue is and how to reproduce it.
Thanks everyone!
Comment #57
DieterHolvoet CreditAttribution: DieterHolvoet as a volunteer commentedThe steps to reproduce do trigger the issue. I set up a site for demonstration purposes: https://master-ypsdica8tvhdqv2vcikdh68sjrl2mniq.tugboat.qa/ (login admin, password admin)
I changed the front page to a node alias: https://master-ypsdica8tvhdqv2vcikdh68sjrl2mniq.tugboat.qa/en/admin/conf...
When trying to export the system.site config, you'll see
page.front
is set to/node/11
: https://master-ypsdica8tvhdqv2vcikdh68sjrl2mniq.tugboat.qa/en/admin/conf...Comment #58
gambry@DieterHolvoet yeah, you are right. Probably I was running on top of the
1503146-drupalisfrontpage-does-not
branch.So, on Basic Settings form:
system.site page.front
is/node/ID
system.site page.front
is/page-alias
This patch address this issue, which mainly focus on decoupling the config from the content. Using an alias rather than an internal content path in config is preferable. 👍
RE url redirects:
So my understanding is yes, there is an issue with the form, but not with URL redirects? In this case we still need to update the IS and remove all mentions to "redirect" since are misleading.
Comment #59
gambryAdding Needs Issue Summary Updates due notes on #58, still pending confirmation if anyone can test my assumptions?
Also keeping Needs Tests since the tests I posted #46 don't focus on
system.site page.front
changes we are trying to achieve.Comment #60
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedFixed an issue when there's no active route match.
Comment #62
DamienMcKennaComment #63
smustgrave CreditAttribution: smustgrave at Mobomo commentedSending back to NW. Patch #60 appears to be failing tests. ANd it needs tests of its own.
Comment #66
rodrigoaguileraI took the code from the merge request, rebased it against 10.1.x and added a fix for a unnecesary named argument but now I can't edit the MR to be against 10.1.x. Should a new MR be opened?
Comment #67
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedOnly the person who opened the MR can edit it. I just did.
Comment #69
DamienMcKennaThe merge request rerolled as SiteInformationForm.php had a conflict.
Comment #70
DamienMcKennaGoing to work on the test failures.
Comment #71
DamienMcKennaLooking at the test failures, it seems like the failures in both TaxonomyDefaultArgumentTest and DisplayPageTest could be fixed by adding path_alias to the test's modules list. I'll test this locally first.
Comment #72
DamienMcKennaI made a few additional changes and was able to get the tests to pass locally. Whether we want to make these changes to the tests is another question.
Comment #73
DamienMcKennaComment #76
DamienMcKennaThat's weird - InstallerExistingConfigSyncDirectoryMultilingualTest passes locally, and didn't fail on previous test runs, e.g. https://www.drupal.org/pift-ci-job/2673595 from #69.
Anyways, next off we need test coverage.
Comment #77
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedComment #82
neclimdulsorry for the noise. Forcing a 11.x branch to the issue fork to work around a bug in the core gitlab config.
The patch seems to be working great. Note about how we can make it better in the review.