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

Solve SEF canonical issues #9565

Merged
merged 9 commits into from Mar 26, 2016

Conversation

Projects
None yet
7 participants
@andrepereiradasilva
Contributor

andrepereiradasilva commented Mar 24, 2016

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

  1. Apply this patch. With SEF system plugin active
  2. 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

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

@scence @SharkyKZ @infograf768 @ggppdk @chivitli

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

Contributor

andrepereiradasilva commented Mar 24, 2016

@scence @SharkyKZ @infograf768 @ggppdk @chivitli

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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

@wilsonge if tested ok, IMHO should go to 3.5.1

Contributor

andrepereiradasilva commented Mar 24, 2016

@wilsonge if tested ok, IMHO should go to 3.5.1

Show outdated Hide outdated plugins/system/sef/sef.php
{
$doc = $this->app->getDocument();
$doc = JFactory::getDocument();

This comment has been minimized.

@wilsonge

wilsonge Mar 24, 2016

Contributor

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

@wilsonge

wilsonge Mar 24, 2016

Contributor

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

This comment has been minimized.

@chivitli

chivitli Mar 24, 2016

Contributor

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

@chivitli

chivitli Mar 24, 2016

Contributor

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

This comment has been minimized.

@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

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.

@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 24, 2016

Contributor

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.

Contributor

SharkyKZ commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 24, 2016

Contributor

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

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

SharkyKZ commented Mar 24, 2016

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

  1. If canonical is found and canonical domain is set, replace current domain with canonical domain.
  2. If canonical is found and canonical domain is not set, leave it as is.
  3. If canonical is not found and canonical domain is set, add a canonical.
  4. If canonical is not found and canonical domain is not set, leave it as is or add a possibly incorrect canonical.
@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

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

Contributor

andrepereiradasilva commented Mar 24, 2016

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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

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.

Contributor

andrepereiradasilva commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ggppdk

ggppdk Mar 24, 2016

Contributor

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

  • will test this
Contributor

ggppdk commented Mar 24, 2016

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

  • will test this
@SharkyKZ

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 24, 2016

Contributor

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.

Contributor

SharkyKZ commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@chivitli

chivitli Mar 24, 2016

Contributor

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

Contributor

chivitli commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@chivitli

chivitli Mar 24, 2016

Contributor

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.

Contributor

chivitli commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

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.

Contributor

andrepereiradasilva commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

please check now.

Contributor

andrepereiradasilva commented Mar 24, 2016

please check now.

@chivitli

This comment has been minimized.

Show comment
Hide comment
@chivitli

chivitli Mar 24, 2016

Contributor

Test successful, works as described.

Contributor

chivitli commented Mar 24, 2016

Test successful, works as described.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 24, 2016

Contributor

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

Contributor

andrepereiradasilva commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@chivitli

chivitli Mar 24, 2016

Contributor

I have tested this item successfully on d00b99c


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

Contributor

chivitli commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 24, 2016

Contributor

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.

Contributor

SharkyKZ commented Mar 24, 2016

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

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Mar 25, 2016

Member

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?

Member

infograf768 commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Mar 25, 2016

Member

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.

Member

infograf768 commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@chivitli

chivitli Mar 25, 2016

Contributor

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.

Contributor

chivitli commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Mar 25, 2016

Member

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.

Member

infograf768 commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ggppdk

ggppdk Mar 25, 2016

Contributor

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 !
Contributor

ggppdk commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@ggppdk

ggppdk Mar 25, 2016

Contributor

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
Contributor

ggppdk commented Mar 25, 2016

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
@SharkyKZ

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 25, 2016

Contributor

I do think that in scenario 4 canonical URL should not be added at all since it's going be incorrect most of the time anyways. Better no canonical than wrong canonical.

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

As ggppdk said before, correct canonicals can only be provided by components. Unless route helpers can be used in the plugin dynamically using URL vars, I don't see how this can be fixed at the moment.

Contributor

SharkyKZ commented Mar 25, 2016

I do think that in scenario 4 canonical URL should not be added at all since it's going be incorrect most of the time anyways. Better no canonical than wrong canonical.

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

As ggppdk said before, correct canonicals can only be provided by components. Unless route helpers can be used in the plugin dynamically using URL vars, I don't see how this can be fixed at the moment.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 25, 2016

Contributor

wait i'm the one confused now!
will respond to each one of your comments.

@infograf768

After this patch, eventhough I have left the Domain field empty in the SEF plugin, I now get a canonica

It shouldn't. Are you at http://localhost:8888/trunkgitnew/contact-component.html URI?

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.

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.

No, we will only get canonical on the pages in which the canoncial is different from the current url. Example:

  • https://domain.tlg/menu-item/?aaaaawill get canonical
  • https://domain.tlg/menu-item/will not get canonical

See https://github.com/andrepereiradasilva/joomla-cms/blob/patch-6/plugins/system/sef/sef.php#L79-L82

@chivitli

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.

Actually that's true.
I can remove the check in https://github.com/andrepereiradasilva/joomla-cms/blob/patch-6/plugins/system/sef/sef.php#L79-L82 if you guys think it's better.

@ggppdk

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

Yes, before this PR. This is fixed by this PR.

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 !

Don't understand.

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

That code is already in this PR. See https://github.com/andrepereiradasilva/joomla-cms/blob/patch-6/plugins/system/sef/sef.php#L79-L82.

@SharkyKZ

I do think that in scenario 4 canonical URL should not be added at all since it's going be incorrect most of the time anyways. Better no canonical than wrong canonical.

What scenario 4? Don't understand. Do you mean If canonical is not found and canonical domain is not set, leave it as is or add a possibly incorrect canonical.
If so, that code is already in this PR. See https://github.com/andrepereiradasilva/joomla-cms/blob/patch-6/plugins/system/sef/sef.php#L79-L82.

As ggppdk said before, correct canonicals can only be provided by components. Unless route helpers can be used in the plugin dynamically using URL vars, I don't see how this can be fixed at the moment.

This PR does not overwrite the component added canonical (only if you set the domain in the domain field, and even then only changes the domain).

Contributor

andrepereiradasilva commented Mar 25, 2016

wait i'm the one confused now!
will respond to each one of your comments.

@infograf768

After this patch, eventhough I have left the Domain field empty in the SEF plugin, I now get a canonica

It shouldn't. Are you at http://localhost:8888/trunkgitnew/contact-component.html URI?

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.

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.

No, we will only get canonical on the pages in which the canoncial is different from the current url. Example:

  • https://domain.tlg/menu-item/?aaaaawill get canonical
  • https://domain.tlg/menu-item/will not get canonical

See https://github.com/andrepereiradasilva/joomla-cms/blob/patch-6/plugins/system/sef/sef.php#L79-L82

@chivitli

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.

Actually that's true.
I can remove the check in https://github.com/andrepereiradasilva/joomla-cms/blob/patch-6/plugins/system/sef/sef.php#L79-L82 if you guys think it's better.

@ggppdk

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

Yes, before this PR. This is fixed by this PR.

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 !

Don't understand.

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

That code is already in this PR. See https://github.com/andrepereiradasilva/joomla-cms/blob/patch-6/plugins/system/sef/sef.php#L79-L82.

@SharkyKZ

I do think that in scenario 4 canonical URL should not be added at all since it's going be incorrect most of the time anyways. Better no canonical than wrong canonical.

What scenario 4? Don't understand. Do you mean If canonical is not found and canonical domain is not set, leave it as is or add a possibly incorrect canonical.
If so, that code is already in this PR. See https://github.com/andrepereiradasilva/joomla-cms/blob/patch-6/plugins/system/sef/sef.php#L79-L82.

As ggppdk said before, correct canonicals can only be provided by components. Unless route helpers can be used in the plugin dynamically using URL vars, I don't see how this can be fixed at the moment.

This PR does not overwrite the component added canonical (only if you set the domain in the domain field, and even then only changes the domain).

@SharkyKZ

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 25, 2016

Contributor

Yes, currently it adds incorrect canonical. Perhaps should just leave it as is (i.e. don't add canonical at all). Wrong canonical URL beats the purpose of canonical tag, to be honest.

To clarify, this is because JRoute::_('index.php?' . http_build_query($this->app->getRouter()->getVars()), false); builds non-canonical URL. So the "canonical" URL isn't really canonical, which makes it pointless and maybe even harmful (in sense of search engines).

Contributor

SharkyKZ commented Mar 25, 2016

Yes, currently it adds incorrect canonical. Perhaps should just leave it as is (i.e. don't add canonical at all). Wrong canonical URL beats the purpose of canonical tag, to be honest.

To clarify, this is because JRoute::_('index.php?' . http_build_query($this->app->getRouter()->getVars()), false); builds non-canonical URL. So the "canonical" URL isn't really canonical, which makes it pointless and maybe even harmful (in sense of search engines).

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 25, 2016

Contributor

To clarify, this is because JRoute::_('index.php?' . http_build_query($this->app->getRouter()->getVars()), false); builds non-canonical URL. So the "canonical" URL isn't really canonical, which makes it pointless and maybe even harmful (in sense of search engines).

To understand better, can you give examples where the canonical is wrongly built with that code?

Is this case, at least is good
Example:

BTW an easy way to check if the canonical is added in thsi PR is add in this line https://github.com/joomla/joomla-cms/pull/9565/files#diff-78cd3849e61af8fc9737ca40ef91ba05R84

echo 'Current: ' . rawurldecode($uri->toString()) . ' | Canonical: ' . htmlspecialchars($canonical);

Then you can navigate trough the site, when that message appears, a canonical exists.

Contributor

andrepereiradasilva commented Mar 25, 2016

To clarify, this is because JRoute::_('index.php?' . http_build_query($this->app->getRouter()->getVars()), false); builds non-canonical URL. So the "canonical" URL isn't really canonical, which makes it pointless and maybe even harmful (in sense of search engines).

To understand better, can you give examples where the canonical is wrongly built with that code?

Is this case, at least is good
Example:

BTW an easy way to check if the canonical is added in thsi PR is add in this line https://github.com/joomla/joomla-cms/pull/9565/files#diff-78cd3849e61af8fc9737ca40ef91ba05R84

echo 'Current: ' . rawurldecode($uri->toString()) . ' | Canonical: ' . htmlspecialchars($canonical);

Then you can navigate trough the site, when that message appears, a canonical exists.

@SharkyKZ

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 25, 2016

Contributor

But the canonical added on https://domain.tlg/menu-item/?aaaaa is https://domain.tlg/menu-item/?aaaaa=, which is clearly incorrect.That is the problem. It should be https://domain.tlg/menu-item, but we cannot simply set this in the system-wide SEF plugin. It could be possible by checking for component and view vars and loading respective route helpers to build correct URLs but I don't think this change would be approved by the team.

Contributor

SharkyKZ commented Mar 25, 2016

But the canonical added on https://domain.tlg/menu-item/?aaaaa is https://domain.tlg/menu-item/?aaaaa=, which is clearly incorrect.That is the problem. It should be https://domain.tlg/menu-item, but we cannot simply set this in the system-wide SEF plugin. It could be possible by checking for component and view vars and loading respective route helpers to build correct URLs but I don't think this change would be approved by the team.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 25, 2016

Contributor

But the canonical added on https://domain.tlg/menu-item/?aaaaa is https://domain.tlg/menu-item/?aaaaa, which is clearly incorrect.That is the problem.

Ok, right i see what you mean now.
Actually the canonical will be https://domain.tlg/menu-item/?aaaaa=
I notice a lot of cases where that happens.
So i agree, it should be disabled them.

It should be https://domain.tlg/menu-item, but we cannot simply set this in the system-wide SEF plugin. It could be possible by checking for component and view vars and loading respective route helpers to build correct URLs but I don't think this change would be approved by the team.

That seems to be a router issue, not the canonical generation. So this will not come in this PR.

Contributor

andrepereiradasilva commented Mar 25, 2016

But the canonical added on https://domain.tlg/menu-item/?aaaaa is https://domain.tlg/menu-item/?aaaaa, which is clearly incorrect.That is the problem.

Ok, right i see what you mean now.
Actually the canonical will be https://domain.tlg/menu-item/?aaaaa=
I notice a lot of cases where that happens.
So i agree, it should be disabled them.

It should be https://domain.tlg/menu-item, but we cannot simply set this in the system-wide SEF plugin. It could be possible by checking for component and view vars and loading respective route helpers to build correct URLs but I don't think this change would be approved by the team.

That seems to be a router issue, not the canonical generation. So this will not come in this PR.

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Mar 25, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 25, 2016

Contributor

Yeah, I understand that. This is why canonical should not be added in the plugin like this. But this needs to be discussed with the team, I think.

Contributor

SharkyKZ commented Mar 25, 2016

Yeah, I understand that. This is why canonical should not be added in the plugin like this. But this needs to be discussed with the team, I think.

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Mar 25, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 25, 2016

Contributor

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.

Contributor

andrepereiradasilva commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Mar 25, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 25, 2016

Contributor

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.

Contributor

SharkyKZ commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 25, 2016

Contributor

right, you're right will change it

Contributor

andrepereiradasilva commented Mar 25, 2016

right, you're right will change it

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Mar 25, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 25, 2016

Contributor

done please retest and mark the test result again.

Contributor

andrepereiradasilva commented Mar 25, 2016

done please retest and mark the test result again.

@SharkyKZ

This comment has been minimized.

Show comment
Hide comment
@SharkyKZ

SharkyKZ Mar 25, 2016

Contributor

I have tested this item successfully on 86f63a6


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

Contributor

SharkyKZ commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@chivitli

chivitli Mar 25, 2016

Contributor

I have tested this item successfully on 86f63a6


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

Contributor

chivitli commented Mar 25, 2016

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

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 25, 2016

Contributor

@infograf768 ok for you now too?

Contributor

andrepereiradasilva commented Mar 25, 2016

@infograf768 ok for you now too?

Show outdated Hide outdated 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"

This comment has been minimized.

@wilsonge

wilsonge Mar 25, 2016

Contributor

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

@wilsonge

wilsonge Mar 25, 2016

Contributor

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

This comment has been minimized.

@joomla-cms-bot

This comment has been minimized.

Show comment
Hide comment
@joomla-cms-bot

joomla-cms-bot Mar 25, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 25, 2016

Contributor

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

Contributor

andrepereiradasilva commented Mar 25, 2016

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

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Mar 26, 2016

Member

I have tested this item successfully on 99b82a7


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

Member

infograf768 commented Mar 26, 2016

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

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Mar 26, 2016

Member

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.

Member

infograf768 commented Mar 26, 2016

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.

@joomla-cms-bot joomla-cms-bot added the RTC label Mar 26, 2016

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

1 of 2 checks passed

JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Mar 26, 2016

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge Mar 26, 2016

Contributor

Merged - thanks :)

Contributor

wilsonge commented Mar 26, 2016

Merged - thanks :)

@andrepereiradasilva andrepereiradasilva deleted the andrepereiradasilva:patch-6 branch Mar 26, 2016

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

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Mar 26, 2016

Contributor

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

Contributor

andrepereiradasilva commented Mar 26, 2016

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

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