New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing canonical in SEF plugin #4612

Closed
wants to merge 1 commit into
base: staging
from

Conversation

Projects
None yet
8 participants
@Hackwar
Member

Hackwar commented Oct 12, 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.

@dgrammatiko

This comment has been minimized.

Contributor

dgrammatiko commented Oct 12, 2014

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

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 13, 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.

@infograf768

This comment has been minimized.

Member

infograf768 commented Oct 13, 2014

See other canonical issue:
#4493

@infograf768

This comment has been minimized.

Member

infograf768 commented Oct 13, 2014

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

@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 13, 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.

@infograf768

This comment has been minimized.

Member

infograf768 commented Oct 13, 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
@Hackwar

This comment has been minimized.

Member

Hackwar commented Oct 13, 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.

@ghost

This comment has been minimized.

ghost commented Oct 17, 2014

Tested and it works.

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

@joomlamarco

This comment has been minimized.

joomlamarco commented Oct 17, 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.

@nicksavov nicksavov added the PBF14 label Oct 17, 2014

@nicksavov

This comment has been minimized.

Contributor

nicksavov commented Oct 24, 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.

@chivitli

This comment has been minimized.

Contributor

chivitli commented Nov 13, 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).

@ch2856

This comment has been minimized.

ch2856 commented Dec 6, 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.

@Hackwar Hackwar deleted the Hackwar:canonical branch Dec 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment