Problem/Motivation
I've been very confused with the addition of canonical links in Drupal 7. The default behaviour seems to be to assign the actual node url of the node being displayed on the front page as the canonical uri. But that must be incorrect, since going to that uri will only redirect to the front page.
Steps to reproduce
TBD
Proposed resolution
TBD (may still be the same but should be confirmed)
Remaining tasks
Tests
Review
User interface changes
TBD
API changes
Data model changes
Release notes snippet
The logic of the AliasPathProcessor has been changed to return /
when requesting the URL of the frontpage instead of an empty string.
Original Post
I've been very confused with the addition of canonical links in Drupal 7. The default behaviour seems to be to assign the actual node url of the node being displayed on the front page as the canonical uri. But that must be incorrect, since going to that uri will only redirect to the front page. So I've made a few changes to two files:
common.inc, line 2143, before:
if ($path == '<front>') {
$path = '';
}
changed to:
if ($path == '<front>' || $path == variable_get('site_frontpage', 'node')) {
$path = '';
}
Now the front page has "/" as canonical url. Also, other tags like < meta about ..> get updated.
Once I got going, I made two other changes as well in node.module, line 2592:
$uri['options']['absolute'] = TRUE;
// Set the node path as the canonical URL to prevent duplicate content.
$canonical_href = url($uri['path'], $uri['options']);
drupal_add_html_head_link(array('rel' => 'canonical', 'href' => $canonical_href), TRUE);
// Set the non-aliased path as a default shortlink.
$shortlink_href = url($uri['path'], array_merge($uri['options'], array('alias' => TRUE)));
if(strlen($shortlink_href) < strlen($canonical_href)) {
drupal_add_html_head_link(array('rel' => 'shortlink', 'href' => $shortlink_href), TRUE);
}
This does two things:
1. Sets canonical links as absolute urls, which seems better if site can be accessed from different urls. Should perhaps be an option somewhere?
2. Only adds the shortlink IF its url is actually shorter than the aliased path. It seems weird to say that the shortlink of "/" is "/node/12345", or that another shortlink of "/abc" is "/node/34567".
What do you think about these changes? I think it's crucial SEO-wise to get it right. I'm not an SEO expert but I've asked around a bit and this seems to be along the right tracks. I hope that the Drupal core can be updated some way to be more optimized for SEO and web standards.
Comment | File | Size | Author |
---|
Issue fork drupal-1255092
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
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is definitely not optimal. Triaging.
The gist of this issue is that
url()
should return/
when asked for the URL of a path that is assigned to the frontpage.There is nothing wrong with relative canonical links. Drupal's default behavior is to output relative links everywhere (this behavior can be altered using hook_url_outbound_alter(). There is also nothing wrong with the "shortlink" being potentially longer then the "canonical" either. The original specification mentions "[The shortlink] will typically be shorter than other URIs".
Comment #2
pjcdawkins CreditAttribution: pjcdawkins commentedI agree url() should return / for <front>.
#1: I agree the canonical link can be relative. But shouldn't the shortlink be an absolute URL?
Comment #3
dillix CreditAttribution: dillix commented+1
Comment #4
marcingy CreditAttribution: marcingy commentedIssue are fixed in d8 first
Comment #4.0
marcingy CreditAttribution: marcingy commentedfixed invisible tag
Comment #8
akalata CreditAttribution: akalata commentedIn D8 this is relevant when using
\Drupal::service('path.alias_manager')->getAliasByPath('/node/' . $node->id());
, returns '/node/[nid]' instead of '/' when the node is set as the front page.Comment #9
borisson_We fixed this in a client project by overwriting the UrlGenerator class's doGenerate method with this:
Comment #12
raphaeltbm CreditAttribution: raphaeltbm commented@borisson_ It seems that your solution fix automatically a lot of problems relatives to the frontpage url case issued in various modules (alternate hreflang url, canonical url, frontpage urls displayed in various places (view content, ...)). But, it's a bit too brutal, you can't access anymore to the internal path of the current object if it match as the frontpage and so it breaks a lot of things.
Stuff like:
Become ineffective for the object/entity/view/whatever defined as frontpage.
At what level should it be fixed? In
UrlGenerator
OREntity->toUrl()
OR ... ? I can't see a kind of global solution for this right now.See my comment #2987635-4: Front Page canonical is Incorrect
I guess the frontpage canonical url being not the internal path or aliased path, is the most expected behaviour by most of the people, no?
If a new kind of rel canonical ('canonical-aware') or a new option for
Entity->toUrl()
('frontpage-aware') have to be created, all core and contribs modules should update their way to retrieve the URL of an entity following the context.Or there is just no real global solution possible in the current state of the core and it should stay the responsibility of each developer/module to manage the frontpage url case when needed?
Comment #14
bpizzillo CreditAttribution: bpizzillo commentedIn case anyone is looking, I created a service implementing the OutboundPathProcessorInterface to return '/' for anyone generating a link to the front-page node ID. What I don't understand is why PathProcessorFront.php in core/lib/Drupal/Core/PathProcessor does not do this, but only replaces `/` with `/`.
my_module.services.yml
FixHomeUrlAliasOutboundPathProcessor.php
Comment #15
matthewv789 CreditAttribution: matthewv789 commentedHave you tried entering "/" as the path alias for that node?
Comment #16
kevster CreditAttribution: kevster commentedI have tried adding / as an alias to the homepage - the simple xml sitemap now generates two urls for the homepage:
https://www.example.com/
and
https://www.example.com
The second one should not be there at all?
Comment #18
extraloopingI made a patch for PathProcessorFront based on #14. I added some extra logic, as the path configured for frontpage can also be an alias. I did some testing and so far seems to works well. Besides further testing, we probably need to add unit test as well. I have no experience here and it's seems a little more complicated, maybe someone else iabel to provide ii?
Comment #20
maximpodorov CreditAttribution: maximpodorov commentedThis patch may be more reliable on older Drupal core versions.
Comment #22
extraloopingThe patch from #20 fails, because it reintroduces deprecated namespaces.
Comment #23
maximpodorov CreditAttribution: maximpodorov commentedThe path language must be considered as it can differ from the current language.
Comment #25
maximpodorov CreditAttribution: maximpodorov commentedThe code style is fixed.
Comment #28
dieppon CreditAttribution: dieppon as a volunteer commented#18 worked for me
Comment #29
JayKandari#20 worked ... :)
Comment #30
purushotam.rai CreditAttribution: purushotam.rai commentedPath Alias core module is optional and hence probably we cannot directly inject this service. We will have to find some alternative solution.
Checkout this: https://www.drupal.org/project/drupal/issues/3096092
With this patch applied on drupal/core, Drupal installation fails.
Comment #31
purushotam.rai CreditAttribution: purushotam.rai commentedTemporary Fix to use path_alias.manager conditionally, created patch.
Comment #32
purushotam.rai CreditAttribution: purushotam.rai commentedSomehow hasService is working differnetly, I've updated the temporary solution to use Drupal module handler.
Comment #34
Nilesh Chhantbar CreditAttribution: Nilesh Chhantbar commentedPatch 25 modified to support drupal 9.2.x Not working as path.alias service can't be used directly.
Comment #35
zuker2 CreditAttribution: zuker2 commentedNilesh Chhatbar Can u send to me this files after fix? something i do wrong and i have still problem after fix, site take down
or public paste surc both files - i don't use git to update- manualy edit
Comment #37
Ruuds CreditAttribution: Ruuds at Groowup Digital Agency commentedAttached is a path that applies to 9.3.x. Drupal\Core\PathProcessor\PathProcessorFront lost its OutboundPathProcessorInterface implementation in 9.3.
Comment #38
ankithashettyJust fixed the custom command failure error in #37, thanks!
Comment #39
DamienMcKennaSo how about if the logic was moved into the path_alias module?
Comment #40
DamienMcKennaI missed a bit in the last patch.
Comment #41
yogeshmpawarResolves CSpell error & added an interdiff.
Comment #43
DamienMcKennaThe tests failures are down from 97 to 15, that's an accomplishment ;-)
Comment #46
smustgrave CreditAttribution: smustgrave at Mobomo commentedJust postponed #3100350: Unable to save '/' root path alias on this one so triaging it some.
Cleanup the issue summary would help. Started with the template.
Will also need tests.
Comment #47
AnybodyThanks @smustgrave. So @yogeshmpawar could you perhaps reroll this as MR and try to fix the remaining tests?
Comment #51
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedI created an issue fork on 11.x and applied patch "1255092-41.patch" by @yogeshmpawar.
Comment #52
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedHm, Jenkins seems to be configured differently for 11.x? At least 1 PHPStan error, doesn't really make sense.
Changing to 10.1.x-dev
Comment #53
Anybody@Grevil: I think we should still target 10.1.x - if needed, we can switch later.
Comment #55
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedAlright, no chance to run the tests remotely on neither 10.1.x nor 11.x. I'll test it locally...
Comment #56
AnybodyThe patch had a breaking change in the constructor parameters, which of course isn't allowed, at least if it's not consistently changed.
So for now I reverted that change, but kept the class property:
Unsure what's the DI strategy here in core for this case, perhaps someone else could have a look?
I guess the tests should run now.
Comment #57
smustgrave CreditAttribution: smustgrave at Mobomo commentedBelieve should be targeting 11.x as that’s the latest development branch. And changes can be backported
Comment #58
Anybody@Grevil did you read #56?
Any good reasons why you broke it again later in https://git.drupalcode.org/project/drupal/-/merge_requests/4030/diffs?co...?
I don't think it's a good idea here to provide the generic config as 2nd parameter in __construct() in this class where there's no DI.
Where ever this is called outside, that call would have to be replaced to provide the 2nd parameter. Isn't that a BC here then?
Of course I might be wrong, but please leave a comment here then to explain the reasons.
You also revertet my other fixes without leaving a comment. Why?
Now we're back at the state where we were before with issues like
Comment #59
BramDriesenSince we are adding dependencies to an existing interface, I guess we need a change record as this is a breaking change.
Comment #60
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commented@Anybody, the class you manually implemented the "\Drupal::service" call does indeed not implement the "ContainerInjectionInterface". But that's because it is a service itself and the dependency injection happens through the service.yml (see "core/modules/path_alias/path_alias.services.yml" -> "path_alias.path_processor").
Comment #62
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedFrom the MR by @Anybody:
This is definitely important as "/" is quite untypical!
I don't have much time the next couple of weeks, so if anyone could add a test for this and do some general clean-up, go for it!
Comment #63
smustgrave CreditAttribution: smustgrave at Mobomo commentedDid not test
Issue summary needs updating
Change record
Release notes
Comment #64
BramDriesenChange record drafted: https://www.drupal.org/node/3362769
Release note added to the issue summary.
Still needs work for the CCF.
Comment #65
BramDriesenWill leave it at needs work for the tests and issue summary updates.
Comment #66
BerdirI think this should not be in the path_alias module, which is now technically optional although most people are using it. It should be a separate event subscriber in core.
Comment #67
DSushmita CreditAttribution: DSushmita as a volunteer and at SJ Innovation LLC commentedSolution:
1. Install and enable the "Metatag" module.
2. Configure the Metatag module by navigating to admin/config/search/metatag.
3. Define a custom token for the canonical URL by clicking on "Add custom token" in the Metatag configuration.
4. Set the custom token as [node:url:absolute] for the canonical URL on the front page.
5. Save the configuration and clear the Drupal caches.
By following these steps, the Metatag module will allow you to define a custom token for the canonical URL. You can set this custom token as [node:URL: absolute] for the canonical URL on the front page. Saving the configuration and clearing the Drupal caches will ensure that the updated canonical URL settings take effect.
Comment #68
Anybody@DSushmita thanks for the workaround, but at least for the cases discussed here, #67 isn't a real solution. Still, it might help some folks.
Comment #69
Grevil CreditAttribution: Grevil as a volunteer and at DROWL.de commentedWhat's everyone's opinion on @berdir's comment in #66? What would be a good place for this Event Subscriber, if we would move the implementation to core?
Comment #70
AnybodyAnother related one: #2421941: Determine whether not only "/" but also "<front>" should be valid input for the Link widget
Comment #71
andypostLooks like duplicate but not really #3394691: setting path alias to `/` causes PHP8.1/str_starts_with/null error
Comment #74
manuel.adanAnother approach made to this in branch 1255092-front_page_outbound_url_path_processor, based as well in URL processor but outside of URL alias as suggested by Berdir at #66. I added outbound URL processing to the existing front page path processor.
It may fail in case of a front page path with query parameters were set, but it is also not well managed by other core parts like PathMatcher, see #3442235: Front page path with query parameters makes front path condition to fail. Added as related issue since support for query parameters should be added.
Comment #76
smustgrave CreditAttribution: smustgrave at Mobomo commentedWill need test coverage and issue summary update.
Left some comments.