? Success

User tests: Successful: Unsuccessful:

avatar infograf768
infograf768
9 Jul 2015

In Protostar, the link used for the logo header is always a direct link to the home page.
Code is

<a class="brand pull-left" href="<?php echo $this->baseurl; ?>/">

This means that we get an url of the type http://mysite.com/

In a multilanguage site it means the url will display the site default language or browser default language home page (depending on "Language Selection for new visitors") when sef is off

This patch, depending if sef is on or off, creates the good url.

  1. Test sef on, "Remove URL Language code" set to Yes and with "Use URL Rewriting" set to Yes or No
  2. Test sef on, "Remove URL Language code" set to No and with "Use URL Rewriting" set to Yes or No
  3. Test with sef off

In each case, clicking on the logo will display the site home page in the language in use.

avatar infograf768 infograf768 - open - 9 Jul 2015
avatar infograf768 infograf768 - change - 9 Jul 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 9 Jul 2015
Labels Added: ?
avatar infograf768
infograf768 - comment - 9 Jul 2015

Thanks to Mig Manickam for pointing this issue

avatar mbabker
mbabker - comment - 9 Jul 2015

There has got to be a way to do this with JRoute::_(). Our routing API cannot be so bad that we manually piece together URLs.

avatar imanickam
imanickam - comment - 9 Jul 2015

Tested the new file index.php of the template Protostar template in a multilingual site (that had three languages en-GB, fr-FR, and ta-IN) with the following Global Configuration Options combinations:

(a) Search Engine Friendly URLs - No
(b) Search Engine Friendly URLs - Yes & Use URL Rewriting - No
(c) Search Engine Friendly URLs - Yes & Use URL Rewriting - Yes

In all the above combinations the link in the logo pointed to the URL of the appropriate language's Default menu item (i.e., Home Page of that language) in the proper way.

The URLs created for the language fr-FR are:

Case (a) => http://localhost/joomla_34_3_ml/index.php?lang=fr
Case (b) => http://localhost/joomla_34_3_ml/index.php/fr/
Case (c) => http://localhost/joomla_34_3_ml/fr/

The result was consistent across other languages (en-GB and ta-IN) as well.

avatar infograf768
infograf768 - comment - 9 Jul 2015

@mbabker
I first indeed tested with JRoute
$logo_link = JRoute::_('index.php?lang=' . $this->sef . '&Itemid=' . $homemenu->id);
it works fine but the issue is that we get full long links when sef is off
http://mysite.com/index.php?option=com_content&view=featured&Itemid=153&lang=it
instead of the clean:
http://mysite.com/index.php?lang=it

avatar mbabker
mbabker - comment - 9 Jul 2015

Sometimes I think the only way to love our router is after the consumption of :beers:

avatar Bakual
Bakual - comment - 9 Jul 2015

Haven't tested, but shouldn't JRoute::_('index.php') work? It should apply the language and itemid automatically. Or am I wrong?
As for the non-SEF URL, technically http://mysite.com/index.php?option=com_content&view=featured&Itemid=153&lang=it is the correct link to the homepage. it's the same link that will be generated for the "home" menuitem.

avatar Kubik-Rubik
Kubik-Rubik - comment - 9 Jul 2015

@infograf768 I had some time to check your PR. As @Bakual said, JRoute is completely fine to solve this issue.

Just change the line 138

<a class="brand pull-left" href="<?php echo $this->baseurl; ?>/">

to

<a class="brand pull-left" href="<?php echo JRoute::_('index.php'); ?>">

Thanks!

avatar brianteeman
brianteeman - comment - 9 Jul 2015

Tested the code above from @Kubik-Rubik and it works correctly with all 3 sef settings on a multilingual site. Also tested on a monolingual site and it works there as well


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

avatar imanickam
imanickam - comment - 10 Jul 2015

The code suggested by @Kubik-Rubik brings/shows the URL of the current page viewed (in the browser). It does not show the Default Menu Item's (i.e., Home Page's) URL.

The logo's link is suppose to take the user to the Home Page - not the current page being viewed (unless it is the home page that is being viewed :-))

The original code change suggested by @infograf768 shows the URL of the Home Page as the Logo's link irrespective of the page being viewed.

Hope this test helps.

avatar zero-24 zero-24 - change - 10 Jul 2015
Category Multilanguage
avatar infograf768
infograf768 - comment - 10 Jul 2015

@Bakual @Kubik-Rubik @brianteeman
Indeed, I should have added to the instructions that the test should also be done on a page which is not the home page.
<?php echo JRoute::_('index.php'); ?> just reloads the current page.

Therefore, it is a matter of preference:
Do you prefer the long link or the short one?

avatar Hackwar
Hackwar - comment - 10 Jul 2015

whatever you do, the proposed code so far is definitely wrong. It will not work for sites that drop the default language SEF. We might indeed have to add the default menu itemid, but you don't need to add the language tag and especially not the SEF to the URL. Everything other than JRoute::_('index.php?Itemid='); is unacceptable.

avatar infograf768
infograf768 - comment - 10 Jul 2015

It will not work for sites that drop the default language SEF.

Wrong. It works fine. I suggest you test for yourself.
Will test though JRoute::_('index.php?Itemid=');

avatar Hackwar
Hackwar - comment - 10 Jul 2015

It will work, because the language switcher jumps through hoops to get this stuff right, the router parses about every URL twice, etc. We had to introduce all that code in the SEF plugin and the language switcher to account for exactly such wrong code.

avatar infograf768
infograf768 - comment - 10 Jul 2015

Tested with

// Get the correct logo link in multilingual
if (JLanguageMultilang::isEnabled())
{
    $homemenu           = $app->getMenu()->getDefault(JFactory::getLanguage()->getTag());
    $logo_link          = JRoute::_('index.php?Itemid=' . $homemenu->id);
}
else
{
    $logo_link = $this->baseurl;
}

and further

<a class="brand pull-left" href="<?php echo $logo_link; ?>">

It works indeed. We get the long link when sef is off.

avatar infograf768
infograf768 - comment - 10 Jul 2015

Would that be satisfactory to all here?

avatar Kubik-Rubik
Kubik-Rubik - comment - 10 Jul 2015

Oh, JRoute, how can you deceive me like this?! :-)

@Hackwar's suggestion works but then we have the ugly query (?Itemid=) in the URL... If we don't set the query, the option variable (=component) is set as var which will be also processed in the URL creation process.

@infograf768 Your last proposal is good and the long link if SEF is off is not a problem, this is the same behavior like we have in the language switcher module. Go for it!:-)

avatar Hackwar
Hackwar - comment - 10 Jul 2015

@Kubik-Rubik I have no idea what you are talking about...

avatar infograf768
infograf768 - comment - 10 Jul 2015

@Kubik-Rubik
Not sure I got you.
My last proposal would be #7394 (comment)

Is that OK?

avatar Kubik-Rubik
Kubik-Rubik - comment - 10 Jul 2015

@infograf768 Yes.

@Hackwar I meant the check for an Itemid in the function createURI (libraries/cms/router/site.php) :-)

avatar Kubik-Rubik Kubik-Rubik - change - 10 Jul 2015
Milestone Added:
avatar infograf768
infograf768 - comment - 10 Jul 2015

Modified PR. Needs new tests. Thanks.

avatar imanickam
imanickam - comment - 10 Jul 2015

The newly proposed code works fine in both monolingual and multilingual sites.

avatar Kubik-Rubik Kubik-Rubik - change - 10 Jul 2015
Labels Added: ?
avatar Kubik-Rubik
Kubik-Rubik - comment - 10 Jul 2015

Thank you @infograf768! Merged.

avatar zero-24 zero-24 - close - 10 Jul 2015
avatar Kubik-Rubik Kubik-Rubik - change - 10 Jul 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-07-10 10:20:19
Closed_By Kubik-Rubik
avatar Kubik-Rubik Kubik-Rubik - close - 10 Jul 2015
avatar Kubik-Rubik Kubik-Rubik - close - 10 Jul 2015
avatar brianteeman
brianteeman - comment - 10 Jul 2015

@Kubik-Rubik where are the tests.


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

avatar Kubik-Rubik Kubik-Rubik - test_item - 10 Jul 2015 - Tested successfully
avatar Kubik-Rubik Kubik-Rubik - alter_testresult - 10 Jul 2015 - imanickam: Not tested
avatar Kubik-Rubik Kubik-Rubik - alter_testresult - 10 Jul 2015 - imanickam: Tested successfully
avatar Kubik-Rubik
Kubik-Rubik - comment - 10 Jul 2015

@brianteeman Set...


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

avatar brianteeman
brianteeman - comment - 10 Jul 2015

NO this is not right - the committer should not be a tester. You are the final check not part of the testing


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

avatar Kubik-Rubik
Kubik-Rubik - comment - 10 Jul 2015

@brianteeman Okay, then could you please also check this PR?

avatar Kubik-Rubik
Kubik-Rubik - comment - 10 Jul 2015

By the way, we also accept merging by "code review"... maybe we should also change this rule? :-)

avatar infograf768
infograf768 - comment - 10 Jul 2015

Committer can be tester. It is the PR author that should (normally) not commit...

avatar mbabker
mbabker - comment - 10 Jul 2015

It's helpful if the person who commits wasn't one of the two testers; the committer should serve as a final layer of review and testing on a patch and not just merge because it has a label. This is why I typically don't commit things I was a tester for.

avatar Kubik-Rubik
Kubik-Rubik - comment - 10 Jul 2015

@mbabker Yes, got it. :-) I also always test before I commit.

avatar smz
smz - comment - 10 Jul 2015

I'm sorry but I object to this PR and having it been merged

Same problem as in my language switcher code review: now, with SEF disabled, the logo link can become something like that:

/index.php?option=com_content&view=article&id=8&Itemid=102&lang=en-us

This is very bad for SEO: the home page could change in time and the indexed link will be no more valid.

avatar Hackwar
Hackwar - comment - 11 Jul 2015

@smz How is that possible? The home-link should be changed to /index.php?lang=en-GB and never show such a URL for the homepage. If that is what is currently happening, the routing code is broken.

avatar infograf768
infograf768 - comment - 11 Jul 2015

The link obtained (what I called the long link) is the one we get now for the Home menu item.
It is true that it is equivalent to index.php?lang=xx-XX
But I remind you that this is exactly the same behavior when sef is off for a monolanguage site... i.e. without the lang code...

avatar smz
smz - comment - 11 Jul 2015

@Hackwar

... If that is what is currently happening, the routing code is broken.

For crying out loud, Hannes, try it...

@infograf768

But I remind you that this is exactly the same behavior when sef is off for a monolanguage site...

Exactly: mono/multi-lingual doesn't matter here...
Actually not, and mono-lingual has his issues too, here (missing trailing slash)

Guys, with this patch having been merged, our (non-SEF) sites will not have a back-link to home page (/), not even in the frigging logo... I'm afraid Google will have a bit of hard time figuring out which is the home page...

avatar Bakual
Bakual - comment - 11 Jul 2015

@smz Please open a new issue then with steps to reproduce the issue. Commenting on a closed PR gets nothing fixed.

avatar smz
smz - comment - 12 Jul 2015
avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment