[#4612] - Fixing canonical in SEF plugin
- Closed
- 27 Oct 2014
- Medium
- Build: staging
- # 4612
- Diff
- Hackwar:canonical
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
The SEF plugin right now ALWAYS adds the canonical tag to the header. At the same time it is written extremely inefficient. Going through this step by step:
- In line 39, the JUri object is cloned, even though it is not necessary. We are doing nothing with this and we are not modifying it. This wastes memory. (Albeit its only a little bit.)
- In line 47 to 49, we take the current URI from JUri, parse that one again to retrieve the current data. $router->getVars() does the same and is a simple lookup instead of a few dozen function calls, a few thousand lines of code and lots of wasted CPU. $router->getVars() is the result of the first run of parse() over the current URI.
- Now that we've wasted lots of time and memory already, we then come to line 51, which compares an object ($uri) against a string ($link). Since we are using !==, the object also is not casted to string, so the result is always true and the canonical tag is always added, making this feature completely useless.
This PR fixes all these issues.
jissues-bot
- | Labels |
Added:
?
|
||
brianteeman
- | Category | ⇒ | SEF |
That is the way it is supposed to be. If you are already on the canonical URL, you don't need to display it.
Please, on a default Joomla install with test sample data, can you explain when we should get a canonical and when not?
You should get a canonical if for example you are on a non-SEFed URL and SEF is enabled. In that case you should get the SEF URL as canonical. In theory, you should also get a canonical URL when you access /component/content/article/42-something, which should then point to the right URL (/answers-to-the-universe/42-something) but our routing system does not work in such a way right now.
Canonical URLs should definitely not show up ALL the time and they should not show up when the current URL is what Joomla thinks should be the canonical URL.
All that said: On a default Joomla install with test sample data, you should never get a canonical URL.
hmm, with or without your patch, I get this on a multilang site:
<link href="/testwindows/trunkgitnew/fr/instructions-multilangue.html" rel="canonical" />
- No http:://domain
- The canonical is the same page: i.e.
http://localhost:8888/testwindows/trunkgitnew/fr/instructions-multilangue.html
That is rather another issue, but doesn't really matter that much, since Google will interpret that relative to the current domain. And even if it shows up as the same URL and all the time, that wouldn't be a real issue either. My main concern with this PR is, that we remove the additional JRouter::parse() run here, since JRouter::parse() should only be called once per page load.
Tested and it works.
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4612.
it works as expected - great.
php 5.4.19, mysql 5.5.32, j 3.3.6
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4612.
| Labels |
Added:
?
|
||
losedk
- | Status | Pending | ⇒ | Ready to Commit |
Looks like there are several good tests. I'm moving this to RTC to be reviewed by a committer.
By the way, a useful article on rel=canonical links:
http://googlewebmastercentral.blogspot.com/2013/04/5-common-mistakes-with-relcanonical.html
This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4612.
phproberto
- | Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2014-10-27 11:45:57 |
Google says not to use relative, but absolute path: https://support.google.com/webmasters/answer/139066?hl=en#2
Avoid errors: use absolute paths rather than relative paths with the rel="canonical" link element.
Use this structure: http://www.example.com/dresses/green/greendresss.html
Not this structure: /dresses/green/greendress.html).
It didn't worked for me (j3.3.6):
site.com/menu/333 gets canonical:site.com/menu?id=333 or site.com/menu/333-alias gets canonical: site.com/menu/333-alias.
Tested and it works. Though, most rel canonical vanish in my set up