Solve SEF canonical issues #9565

andrepereiradasilva · 2016-03-24T11:22:20Z

This Pull Request is related to issues #9333 and #9556.

Also to PR #9559

Summary of Changes

Add a better check for adding the canonical html tag.

Testing Instructions

Apply this patch. With SEF system plugin active
Do the following test scenarios:

Domain in the SEF plugin field	Canonical added by a component	Result
(empty)	No	The canonical html tag will NOT be added
(empty)	Yes	No change (the component added canonical html tag stays the same)
Yes	No	The canonical html tag will be added with the new domain typed in the SEF plugin domain field (ex: https://www.example.com/menu-item/).
Yes	Yes	The canonical html tag will be the same as added by the component but now with the new domain typed in the SEF plugin domain field.

andrepereiradasilva · 2016-03-24T11:25:08Z

@scence @SharkyKZ @infograf768 @ggppdk @chivitli

Can you test this and check if all issues are resolved?

andrepereiradasilva · 2016-03-24T11:27:25Z

@wilsonge if tested ok, IMHO should go to 3.5.1

wilsonge · 2016-03-24T11:55:23Z

plugins/system/sef/sef.php

 	{
-		$doc = $this->app->getDocument();
+		$doc = JFactory::getDocument();


Is there any reason this can't be $this->app->getDocument() still?

You get a fatal error if you try to access it like that in onAfterRoute event

Sorry, clarification, you get a fatal error after, because it returns null

yes ... try it and you'll see ... php errors.

it seems if you use the $this->app->getDocument(); in onAfterRoute event you get problems. Don't know why. Maybe @mbabker knows better why this happens.

BTW that has the whole reason i change it to onAfterDispatch earlier. After component rendering you can use it.

SharkyKZ · 2016-03-24T11:59:05Z

If canonical is added only when domain is set, there is no need to revert other changes since in this case SEF plugin should override components anyway. Otherwise components would not use the canonical domain.

In such a case, canonical should simply be the specified domain + current path.

SharkyKZ · 2016-03-24T12:01:57Z

By my logic, it would be best to check for existing canonicals before doing anything.

If canonical is found and canonical domain is set, replace current domain with canonical domain.
If canonical is found and canonical domain is not set, leave it as is.
If canonical is not found and canonical domain is set, add a canonical.
If canonical is not found and canonical domain is not set, leave it as is or add a possibly incorrect canonical.

andrepereiradasilva · 2016-03-24T12:21:13Z

ok so let me try to code something for that.
don't test yet.

andrepereiradasilva · 2016-03-24T12:51:35Z

ok i changed the code.

How it works now:

Back to using onAfterDispatch.
If a canonical EXISTS in the document when the onAfterDispatch is run, the plugin doesn't add a canonical.
This should solve issues with components and duplicate canonicals.
If a canonical DOESN'T EXIST in the document when the onAfterDispatch is run and a domain is ADDED in the SEF plugin domain field, it adds the canonical with that domain.
If a canonical DOESN'T EXIST in the document when the onAfterDispatch is run and a domain is NOT ADDED in the SEF plugin domain field, it only adds the canonical if the canonical URI is different than the current uri.

Please check code and test.

Open to any suggestions/improvements regarding this.

ggppdk · 2016-03-24T13:00:50Z

Nice and simple to allow components to add rel canonical without reverse engineering anything

will test this

SharkyKZ · 2016-03-24T13:28:57Z

It doesn't override component canonicals when canonical domain is set.
Can add this: foreach ($doc->_links as $canonical => $link) to get existing canonical link as $canonical. Then use it with JUri::getInstance() to get only the path. Then prepend the canonical $domain to it.
EDIT: in this case the original canonical also needs to be unset unset($doc->_links[$canonical]) since we're adding a modified.

chivitli · 2016-03-24T13:45:20Z

For me this is good, I would never override canonical from a custom component. The rest works as described. So, test ok from me.

It still remains an issue though that canonicals generated this way are not real canonicals, so I am not sure if this is useful at all (or that it doesn't have a negative impact).

chivitli · 2016-03-24T13:47:04Z

I just realized you would like the plugin to replace the domain part of the custom component - that sounds like a good idea. At present, this is indeed not the case.

andrepereiradasilva · 2016-03-24T14:21:00Z

It doesn't override component canonicals when canonical domain is set.
Can add this: foreach ($doc->_links as $canonical => $link) to get existing canonical link as $canonical. Then use it with JUri::getInstance() to get only the path. Then prepend the canonical $domain to it.
EDIT: in this case the original canonical also needs to be unset unset($doc->_links[$canonical]) since we're adding a modified.

I just realized you would like the plugin to replace the domain part of the custom component - that sounds like a good idea. At present, this is indeed not the case.

I will add this them.

andrepereiradasilva · 2016-03-24T14:38:12Z

please check now.

chivitli · 2016-03-24T14:43:18Z

Test successful, works as described.

andrepereiradasilva · 2016-03-24T14:56:20Z

Test instructions updated.

@chivitli if it's ok for you. please mark as "Tested successfully" in "Joomla! Issue Tracker" https://issues.joomla.org/tracker/joomla-cms/9565 (after login the "Test this" button appears).
For more information see https://docs.joomla.org/Testing_Joomla!_patches#Recording_test_results

chivitli · 2016-03-24T15:32:31Z

I have tested this item ✅ successfully on d00b99c

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

SharkyKZ · 2016-03-24T15:33:02Z

I have tested this item ✅ successfully on d00b99c

Works as expected.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

infograf768 · 2016-03-25T06:42:44Z

Sorry folks, but I am totally lost...
I am using basic staging.
After this patch, eventhough I have left the Domain field empty in the SEF plugin, I now get a canonical.

 <base href="http://localhost:8888/trunkgitnew/contact-component.html" />
  <meta name="description" content="UNE DESCRIPTION" />
  <meta name="generator" content="Joomla! - Open Source Content Management" />
  <title>Contact Component - My very long test site name</title>
  <link href="http://localhost:8888/trunkgitnew/contact-component.html" rel="canonical" />
  <link href="/trunkgitnew/templates/protostar/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />
[...]

As far as I know, a canonical link is only used to show Search Engine the preferred url when multiple URLs reach the same page
A canonical link element is an HTML element that helps webmasters prevent duplicate content issues by specifying the "canonical" or "preferred" version of a web page[1][2] as part of search engine optimization. It is described in RFC 6596, which went live in April 2012.

Why then create a canonical when only one URL exists?

infograf768 · 2016-03-25T06:58:24Z

I do now understand though that we get also a canonical before this patch when loading another page than the home as I have indeed overseen that in my former tests...
I just don't understand the use of it in that case.

chivitli · 2016-03-25T07:31:49Z

There are always multiple urls in Joomla. Not only multiple, but an infinite number of them... As mentioned, Google says: Mark up the canonical page and any other variants with a rel="canonical" link element.

There is no need for this optimization to not show canonical on the canonical page.

infograf768 · 2016-03-25T07:38:26Z

Are'nt we going to get a canonical for ANY page loaded this way. I mean when we have another url to get to the same page, we would have a different canonical.

ggppdk · 2016-03-25T08:09:58Z

Are'nt we going to get a canonical for ANY page loaded this way. I mean when we have another url to get to the same page, we would have a different canonical.

Ok the plugin, previously had code:

not to add rel canonical if canonical URL is same as current URL

Now it WILL happen that it will add 2 (different) rel canonical for 2 pages that are same

But

the effect will be the same as before, goggle will consider the 2 pages different
because if you do not add rel canonical for 2 urls then the rel canonical is supposed to be the URL itself !

ggppdk · 2016-03-25T08:14:35Z

I don't know google having special code to try to detect if 2 pages are the same if rel canonical is missing, furthermore which 2 pages would it compare and why it try to compare them ?

anyway the code to check if current URL is same as calculated canonical is not directly relevant to this PR, so it can be re-added

joomla-cms-bot · 2016-03-25T11:44:03Z

This PR has received new commits.

CC: @chivitli, @SharkyKZ

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

andrepereiradasilva · 2016-03-25T11:46:34Z

ok. updated the code, now it ONLY adds (or changes in the case a component already added) the canonical if the SEF plugin domain field has some value.
Also added a placeholder (hint) to the domain field.

Test instructions updated in the first post, please test now.

joomla-cms-bot · 2016-03-25T11:48:01Z

This PR has received new commits.

CC: @chivitli, @SharkyKZ

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

SharkyKZ · 2016-03-25T12:18:24Z

Since canonical URL is now used only to canonize domains, it can be left as current path with canonical domain prepended. Routed URL is incorrect anyways, so no point in leaving it there.

andrepereiradasilva · 2016-03-25T12:19:26Z

right, you're right will change it

joomla-cms-bot · 2016-03-25T12:21:23Z

This PR has received new commits.

CC: @chivitli, @SharkyKZ

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

andrepereiradasilva · 2016-03-25T12:22:17Z

done please retest and mark the test result again.

SharkyKZ · 2016-03-25T12:24:59Z

I have tested this item ✅ successfully on 86f63a6

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

chivitli · 2016-03-25T17:09:05Z

I have tested this item ✅ successfully on 86f63a6

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

andrepereiradasilva · 2016-03-25T22:16:49Z

@infograf768 ok for you now too?

wilsonge · 2016-03-25T22:25:17Z

plugins/system/sef/sef.xml

@@ -22,6 +22,7 @@
 				<field name="domain" type="url"
 					description="PLG_SEF_DOMAIN_DESCRIPTION"
 					label="PLG_SEF_DOMAIN_LABEL"
+					hint="https://www.domain.tld"


If this is an example link can you make sure it uses example.com please :) thanks!

joomla-cms-bot · 2016-03-25T22:26:31Z

This PR has received new commits.

CC: @chivitli, @SharkyKZ

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

andrepereiradasilva · 2016-03-25T22:28:09Z

Just a little change at @wilsonge request, no need to retest.

infograf768 · 2016-03-26T15:01:28Z

I have tested this item ✅ successfully on 99b82a7

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

infograf768 · 2016-03-26T15:02:35Z

It is now behaving as should. Thanks!

RTC

@wilsonge

You can merge into 3.5.1

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9565.}

wilsonge · 2016-03-26T15:12:06Z

Merged - thanks :)

andrepereiradasilva · 2016-03-26T15:24:21Z

@SharkyKZ @chivitli @infograf768 @ggppdk @wilsonge thanks for tests and/or contributions.

solve sef canocical issues

958f7c6

joomla-cms-bot added the PR-staging label Mar 24, 2016

This was referenced Mar 24, 2016

Canonical link should be absolute even if no domain is specified in SEF plugin #9559

Closed

Duplicate (one more) rel="canonical" after updating to 3.5.0 #9556

Closed

wilsonge reviewed Mar 24, 2016
View reviewed changes

Update sef.php

20c9d0e

minor change

9312a21

better sef canonical

d00b99c

just better comments

1c34b7b

use juri instead of router to avoid wrong canonicals

86f63a6

wilsonge reviewed Mar 25, 2016
View reviewed changes

make george happy

99b82a7

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 26, 2016

wilsonge merged commit d8a127f into joomla:staging Mar 26, 2016

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 26, 2016

andrepereiradasilva deleted the patch-6 branch March 26, 2016 15:12

wilsonge added this to the Joomla 3.5.1 milestone Mar 26, 2016

This was referenced Mar 28, 2016

SEF plugin, wrong canonical generation #8568

Merged

Wrong canonical URL after changing article alias #9643

Closed

This was referenced Apr 2, 2016

J3.5: SEF-URL in Article to Contact only contains ID, but not Alias of the contact #9698

Closed

Canonical url #9785

Closed

ggppdk mentioned this pull request May 19, 2016

canonical does not show appropriate URL #6069

Closed

Solve SEF canonical issues #9565

Solve SEF canonical issues #9565

Conversation

andrepereiradasilva commented Mar 24, 2016

Summary of Changes

Testing Instructions

andrepereiradasilva commented Mar 24, 2016

andrepereiradasilva commented Mar 24, 2016

wilsonge Mar 24, 2016

Choose a reason for hiding this comment

chivitli Mar 24, 2016

Choose a reason for hiding this comment

andrepereiradasilva Mar 24, 2016

Choose a reason for hiding this comment

SharkyKZ commented Mar 24, 2016

SharkyKZ commented Mar 24, 2016

andrepereiradasilva commented Mar 24, 2016

andrepereiradasilva commented Mar 24, 2016

ggppdk commented Mar 24, 2016

SharkyKZ commented Mar 24, 2016

chivitli commented Mar 24, 2016

chivitli commented Mar 24, 2016

andrepereiradasilva commented Mar 24, 2016

andrepereiradasilva commented Mar 24, 2016

chivitli commented Mar 24, 2016

andrepereiradasilva commented Mar 24, 2016

chivitli commented Mar 24, 2016

SharkyKZ commented Mar 24, 2016

infograf768 commented Mar 25, 2016

infograf768 commented Mar 25, 2016

chivitli commented Mar 25, 2016

infograf768 commented Mar 25, 2016

ggppdk commented Mar 25, 2016

ggppdk commented Mar 25, 2016

joomla-cms-bot commented Mar 25, 2016

andrepereiradasilva commented Mar 25, 2016

joomla-cms-bot commented Mar 25, 2016

SharkyKZ commented Mar 25, 2016

andrepereiradasilva commented Mar 25, 2016

joomla-cms-bot commented Mar 25, 2016

andrepereiradasilva commented Mar 25, 2016

SharkyKZ commented Mar 25, 2016

chivitli commented Mar 25, 2016

andrepereiradasilva commented Mar 25, 2016

wilsonge Mar 25, 2016

Choose a reason for hiding this comment

andrepereiradasilva Mar 25, 2016

Choose a reason for hiding this comment

joomla-cms-bot commented Mar 25, 2016

andrepereiradasilva commented Mar 25, 2016

infograf768 commented Mar 26, 2016

infograf768 commented Mar 26, 2016

wilsonge commented Mar 26, 2016

andrepereiradasilva commented Mar 26, 2016