? Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
12 Oct 2014

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:

  1. 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.)
  2. 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.
  3. 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.

avatar Hackwar Hackwar - open - 12 Oct 2014
avatar jissues-bot jissues-bot - change - 12 Oct 2014
Labels Added: ?
avatar brianteeman brianteeman - change - 12 Oct 2014
Category SEF
avatar dgt41
dgt41 - comment - 12 Oct 2014

Tested and it works. Though, most rel canonical vanish in my set up

avatar Hackwar
Hackwar - comment - 13 Oct 2014

That is the way it is supposed to be. If you are already on the canonical URL, you don't need to display it.

avatar infograf768
infograf768 - comment - 13 Oct 2014

See other canonical issue:
#4493

avatar infograf768
infograf768 - comment - 13 Oct 2014

Please, on a default Joomla install with test sample data, can you explain when we should get a canonical and when not?

avatar Hackwar
Hackwar - comment - 13 Oct 2014

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.

avatar infograf768
infograf768 - comment - 13 Oct 2014

hmm, with or without your patch, I get this on a multilang site:

<link href="/testwindows/trunkgitnew/fr/instructions-multilangue.html" rel="canonical" />
  1. No http:://domain
  2. The canonical is the same page: i.e.
http://localhost:8888/testwindows/trunkgitnew/fr/instructions-multilangue.html
avatar Hackwar
Hackwar - comment - 13 Oct 2014

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.

avatar seakson seakson - test_item - 17 Oct 2014 - Tested successfully
avatar seakson
seakson - comment - 17 Oct 2014

Tested and it works.

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4612.

avatar joomlamarco joomlamarco - test_item - 17 Oct 2014 - Tested successfully
avatar joomlamarco
joomlamarco - comment - 17 Oct 2014

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.

avatar nicksavov nicksavov - change - 17 Oct 2014
Labels Added: ?
avatar losedk losedk - test_item - 19 Oct 2014 - Tested successfully
avatar nicksavov nicksavov - change - 24 Oct 2014
Status Pending Ready to Commit
avatar nicksavov
nicksavov - comment - 24 Oct 2014

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.

avatar phproberto phproberto - close - 27 Oct 2014
avatar phproberto phproberto - close - 27 Oct 2014
avatar phproberto phproberto - change - 27 Oct 2014
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2014-10-27 11:45:57
avatar chivitli
chivitli - comment - 13 Nov 2014

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).

avatar ch2856
ch2856 - comment - 6 Dec 2014

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.

avatar Hackwar Hackwar - head_ref_deleted - 10 Dec 2014

Add a Comment

Login with GitHub to post a comment