Posted by demon326 on December 3, 2009 at 4:29pm
| Project: | Nodewords: D6 Meta Tags |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | v6.x-1.12 blocker |
Issue Summary
Hello,
to start, thanks for maintaining such a great module for drupal, but I've found, what i think to be an issue...
lets say you made a node with the title "Hello World", when using the path auto you get: "mydrupalwebsite.com/hello-world" but the canonical url uses the default clean drupal path as:<link rel="canonical" href="/node/2616" />
and i think that the most path auto users block the /node/** path in there robots.txt to avoid double content.
Also i think its best to use the default url, and not the relative part, so it becomes:<link rel="canonical" href="http://mydrupalwebsite.com/hello-world" />
this because not everybody has there base url defined..
i hope it's clear enough :)
Comments
#1
It does look for the path alias, but it does it in the wrong place.
The attached patch changes the development snapshot code to fix this problem.
#2
The code has been changed, and committed in CVS for the development snapshot.
#3
Hi,
I think that the fix is not complete with regard to the Original Poster requests (the absolute/relative URL issue).
If a base_url is not defined we should still use the global one to get an absolute URL; currently, passing an empty string in
$options['base_url']tourl()makes it ignore the 'absolute' setting.See this patch (also attached), it shows the problem, and the difference between a NULL value and an empty string on a check based on
isset()(seeurl()implementation).diff -u -r1.1.2.47 nodewords_basic.module
--- nodewords_basic/nodewords_basic.module 7 Dec 2009 22:35:02 -0000 1.1.2.47
+++ nodewords_basic/nodewords_basic.module 12 Dec 2009 21:04:28 -0000
@@ -242,7 +242,7 @@
$base_url = rtrim(variable_get('nodewords_base_url', ''), '/');
$options = array(
'absolute' => TRUE,
- 'base_url' => $base_url,
+ 'base_url' => empty($base_url) ? NULL : $base_url,
);
$tags['canonical'] = !empty($content['value']) ? check_url(url(drupal_get_path_alias($content['value']), $options)) : '';
An alternative could be to use a snippet like the following, functionally equivalent to the one-liner.
<?php
$options = array(
'absolute' => TRUE,
);
$base_url = rtrim(variable_get('nodewords_base_url', ''), '/');
if (!empty($base_url)) {
$options['base_url'] = $base_url;
}
?>
Regards,
Antonio
#4
The code has been changed, and committed in CVS.
Thanks for the help, and the patch.
#5
Automatically closed -- issue fixed for 2 weeks with no activity.
#6
#7
Hello,
i reopend the issue because in the actual developer snapshot (2010-Jan-27) and also in the 6.x-1.12-beta2 the canonical url doesnt work for me.
thats what it produces:
<link rel="canonical" href="http://www.example.com/en/node/15864" />
thats what i expected :
<link rel="canonical" href="http://www.example.com/en/very-nice-content-here" />
i think this how it should work, or I am wrong ?
[Edited by kiamlaluno] to show the tags that were not show by the current input format
#8
I explained in #658630: Generated canonical URL is not correct why the code is not using the path alias as canonical URL.
#9
@kiamlaluno so works as designed, do you think it will be helpfull to have a feature request for this ? and thanks for the advise
#10
@Sputnik: I would reply in the same way I replied here.
The canonical URL is a URL that doesn't change with the time, as it needs to uniquely identify a resource. This means that in Drupal a canonical URL is http//example.com/node/<nid> for nodes, http//example.com/user/<uid> for user profiles, etc.
#11
@kiamlaluno From the technical point off view I agree with you.
On the other hand , from the SEO point off view, I prefer a URL like
http//example.com/<titel>the alias of the Node URL as canonical URL, because the CTR will be significant higher than with the "Node" Version.That's one of the reasons because we use modules like Pathauto , Global Redirect ... . And we try to suppress the access to duplicate content, like the author of this issue demon326 explained.
I think to have an option like "use alias URL instead of node URL for canonical URL" will be a great advantage for this module.
Or is there an error in my reasoning ?
TIA
#12
@kiamlaluno fully agree with Sputnik on this. The canonical tag should be controlled by the webmaster and not by drupal core's default url structure, as this is a basic SEO principle and this tag is supported by search engines for this exact purpose.
#13
In fact, it is possible to change the Canonical URL to the one you prefer. As there is nothing wrong using the Drupal canonical URL as canonical URL (considering also the purpose of that meta tag), I am changing back the status of this issue.
#14
+1
I also think that the webmaster should be able to decide about the Canonical URL.
#15
See my previous comment.
#16
I have marked #707876: Canonical URLs get not Pathauto as duplicate of this report.
#17
@kiamlaluno my problem is that I have generated vocabularies using taxonomy_csv module and I need it to automatically enter the canonical url using the path for the term and not with paths like taxonomy/term/xx
Need the SEO-friendly one which is /category-name/sub-category-name/
#18
hello
i switch today from 6.x-1.12-beta2 to Nodewords 6.x-1.x-dev (2010-Feb-09)
the problem with the Canonical URL seems not fixed for me...all my canonicals are ./node/** or / and ./taxonomy/**
cache cleaned!
#19
@eule: The status of this report is fixed because of comment #3; the code has been changed to solve what reported about the canonical URL not being an absolute URL when the base URL is an empty string.
As already explained, in Drupal the canonical URL has the form node/<nid> for nodes; if you want to use a path alias as canonical URL (which I would not suggest), then you set the canonical URL to the one you prefer. Branch 6.x-3 will allow you to use a token to globally set the canonical URL to the path alias, if you really want to do so.
#20
subscribing
+1 comment #11
@kiamlaluno - yes it's true that we can manually control the canonical url. but now this means i have to go though and touch hundreds of nodes.. i'm sure for many this is thousands of nodes.
#21
subscribing
+1 comment #11
#22
How about a simple option (e.g. variable_get('nodewords_canonical_alias', FALSE)) that decides if nodewords_basic_canonical_prepare() runs the path via drupal_get_path_alias() first?
#23
Here's a patch that adds a new option "Canonical URL use alias" to the main settings form that will control whether internal paths or url aliases are used. By default the variable is FALSE so it will use the internal path until the variable is set.
#24
The proposed patch doesn't actually change the way the canonical URL is generated. Using the path alias will be possible once that the code is changed as per #508496: Support for tokens, and #706630: Implement the hooks necessary to add more tokens.
<?php+ if (variable_get('nodewords_canonical_alias', FALSE)) {
+ $content['value'] = $path;
+ }
+ else {
+ $content['value'] = $path;
+ }
?>
#25
Apologies, I hadn't fully merged the full change. Please check this new attachment.
While I laud the concern to provide a proper fix for this, Drupal via the core Path module (with PathAuto, Path_Redirect and GlobalRedirect) already has an existing structure to cleanly handle paths so it doesn't IMHO make sense to provide a redundant method of configuring the same thing.
#26
@DamienMcKenna: thanks.
@kiamlalauno: tokens are indeed more flexible, but maybe you can accept this change for the time being. It is fairly trivial and most users will be happy NOW :)
In the meantime we can discuss (in another issue if you like) if using tokens for paths _directly_ in nodewords is sane, or if relying on tokens already used in Pathauto (which this patch should cover already) is better.
Just my 2 cents.
Regards,
Antonio
#27
Another issue that came to my mind is that if I enable canonical URL's and I only specify one on a certain node, I see that all the other nodes that doesn't have a custom canonical URL's are nevertheless, all nodes have it's canonical URL pointed to themselves.
Is there any way to output the canonical URL's only on the nodes that indeed have a canonical URL and not on the other ones?
#28
This is simple and gets the job done. +1 for being off by default, but configurable. I tested this locally and its working great.
#29
For anyone who wants to try, I've made a version of v6.x-1.11 available with the patch applied, please let us know here how it works for you.
#30
As Nodewords now supports tokens, and it implements two tokens though to be used for the canonical URL, I am marking this report as fixed.
As usual, read what reported in the project page, before to update the installed modules.
#31
Thanks, looking into the code, I see the token is [metatags-url-alias], right?
However it is not listed on q=admin/content/nodewords/default. The parameter is not saved as reported in another issue (1.13beta4). So I reroll the patch for a temporary fix (sorry, I don't know about the token thing, so I can't help).
#32
The issue hs been already fixed. The hook to list the tokens were not returning the result as expected from token.module, and that caused the custom tokens to not appear.
#33
Automatically closed -- issue fixed for 2 weeks with no activity.
#34
afaict (and maybe i'm missing something)
1. the patch in #25 no longer applies to 6.x-1.x-dev, nor does it apply to 6.x-1.11
it seems to affect a non-existent nodewords_basic/includes/nodewords_basic.nodewords.tags.inc file.
maybe it's meant to apply to nodewords_basic.module, or i'm doing something really dumb?
2. the token feature referred to by kiam is only in the 6.x-2.x branch.
i'm not sure if i should update to 6.x-2.x or not - would it be worth rerolling #25 to apply to the current 6.x-1.x-dev?
#35
@firebus, I applied #25 to nodewords_basic.module for 6.x-1.11 and it is working for me.
#36
I've re-rolled & slightly simplified the patch from #25 to work on 6.x-1.x-dev.
#37
I've committed the patch from #36 to 6.x-1.x-dev, it'll be available in the next release.
#38
This should have been tagged.
#39
Per feedback from jwilson3, I added an extra comment to help explain what appears as backwards logic in nodewords_basic_canonical_prepare() for deciding whether to use an alias or not.
#40
Automatically closed -- issue fixed for 2 weeks with no activity.