Unsure if I should have used $current_link instead of $active_link...

@test OK with a minor thing that needs some attention

Try some article that has "*" all languages as selected language, the produced code is invalid (star is not a language!) Maybe a simple check if...

Should be OK now, but I'm still unsure I'm doing the "right stuff"...

@tesk OK here! Thanks Sergio! Damn coffee doesn’t work today...

... I'd love if @Hackwar could give a look at this too...

Also, but this is unrelated, I'm seeing a bunch of en-GB in here, and I'm not sure at all it is correct assuming en-GB being the default language or even it being installed at all (but with the latter I'm probably wrong).

These are fallbacks, I guess

... will check...

@joomdonation, but... why associations are set-up only when $active_link == $current_link?

@dgt41

These are fallbacks, I guess

Correct: they are all fallbacks! Thanks for the head-up!

@joomdonation, but... why associations are set-up only when $active_link == $current_link?

That's menu item association, so we only find the association if the link is a direct link from a menu item, not a sub-link (in my example in previous comment, sub-link is a link to article detail)

So for example, if we are in article page of English language (and the menu item point to Category Page in English language), if we try to find association, we might get category page of French language and it is wrong alternate link(because we are now in article detail page, not in the category page).

That's what I see from the code (I don't have much experience with multilingual). Hope my explanation is clear.

@joomdonation, exactly! And this is why I'm still unconvinced I'm doing the right thing:

Apparently (from code analysis) the rel="alternate" links are only created for the home page and when menu items associations exists ($assocs array) and this happens only when $active_link == $current_link, i.e. not for sub-links.

To make things more complicated, an association may exist also through an external callable component-related class.

My code blindly issue a rel="alternate" link for the active language, even when such links may not exist for the remaining (not active) languages, and thus I think it is wrong and need review.

Ahhhhh, I think the solution is easy: just remove unset($associations[$active->language]); (and something else for the home page...)

Let me try...

nope: OK for the home page but missing for other menu items.
Probably MenusHelper::getAssociations() doesn't return the association to the active language in first place... will check tomorrow...

@dgt41, Dimitris, please retract your positive test...

... now it might work ... (but review and thorough testing is needed)

This works here for associated menu items and Home pages, not for component associations.

Not sure we need the alternate for the current language btw. Google is a bit confusing.

Could be solved by adding
$doc->addHeadLink($server . $current_link, 'alternate', 'rel', array('hreflang' => $active->language));

in

                    if (isset($cassociations[$language]))
                    {
                        $link = JRoute::_($cassociations[$language] . '&lang=' . $lang->sef);

                        // Check if language is the default site language and remove url language code is on
                        if ($lang->sef == $this->lang_codes[JComponentHelper::getParams('com_languages')->get('site', 'en-GB')]->sef
                            && $this->params->get('remove_default_prefix', 0))
                        {
                            $link = preg_replace('|/' . $lang->sef . '/|', '/', $link, 1);
                        }

                        $doc->addHeadLink($server . $link, 'alternate', 'rel', array('hreflang' => $language));
+                       $doc->addHeadLink($server . $current_link, 'alternate', 'rel', array('hreflang' => $active->language));
                    }

As it is impossible to associate an item to another item tagged to "ALL"

Corrected above the typo: it is $current_link and not $active_link :)

@infograf768 I suspected something like that...

wouldn't just remove this block of code make the trick?

                $lang_code = $this->app->input->cookie->getString(JApplicationHelper::getHash('language'));

                // No cookie - let's try to detect browser language or use site default.
                if (!$lang_code)
                {
                    if ($this->params->get('detect_browser', 1))
                    {
                        $lang_code = JLanguageHelper::detectLanguage();
                    }
                    else
                    {
                        $lang_code = JComponentHelper::getParams('com_languages')->get('site', 'en-GB');
                    }
                }

                unset($cassociations[$lang_code]);

But now I have a bigger problem while trying to test the above solution:

1. make a clean installation from the staging code installing two additional languages with their example categories/articles (so there are 3 of each in total)
2. set-up association between the 3 language specific categories (no problem)
3. set-up association between the 3 articles (oh, oh... )

... I've opened a separate issue (#6965) for the above problem.

Now, back to the issue:
removing the (apparently useless) block of code does not solve the issue at hand because the JLanguageAssociations::getAssociations() strips out from the returned array the ID for which associations are seeked (and hence my previous assertion about the block of code being probably useless)

Now the question is: is this a correct behavior, or should JLanguageAssociations::getAssociations() return ALL associated IDs (also the "passed" one)?

I made a quick patch to JLanguageAssociations::getAssociations() in that sense and I can't see any detrimental effect (at least at first sight...), and, together with the removal of the above block of code, it solves the issue at hand, but:

• will this break B/C? (i.e: is it documented that getAssociations() doesn't returns the passed ID itself?)
• will this break something else?

If someone want to review/test the above, it is at: https://github.com/smz/joomla-cms/tree/RelAlternateForActiveLanguageEXTREME
Branch deleted...

The more I think of it the more I think I'm on a wrong route...
After all associated links generation seems to perfectly work in the Language Switcher: is there any reason for not going the DRY way and use that code?

#6965 being solved by #6971
(needs one more tester)
let's go back to this issue (please do not mix issues ☺ ).

After all associated links generation seems to perfectly work in the Language Switcher: is there any reason for not going the DRY way and use that code?

The point is that $cassociations never returns the current item (just do a var_dump($cassociations) in the module helper or in languagefilter. The patch you did in your EXTREME branch for JLanguage::getAssociations (and only that part) works fine here.

FYI, the code taking off the current item from the current list of associations was introduced here
https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/language/associations.php#L99
came from this patch
#1178 by @phproberto

Are there any reason Roberto for doing that? taking it off from MenusHelper::getAssociations() and JLanguageAssociations::getAssociations() does not seem to break anything here.

Found out the reason: it was to display only the associated items in admin managers in the Association column.
Therefore I think the solution is this PR here + the line of code I proposed above.

As there were more things I didn't like that much in the current code (e.g.: it was not working for the home pages if "Use URL Rewriting" wasn't set), I took the bold way and proceeded to an almost complete rewrite of PlgSystemLanguageFilter::onAfterDispatch(), roughly based on the Language Switcher code

As far as I can see it works flawlessly and I honestly find it more "lean" and readable.

@dgt41, @infograf768, @joomdonation and @Simonkloostra: can you please test/review this? Thanks!

@smz The same test case implies or we have to look for more?

Well... as I changed a lot of stuff it would probably be better to perform a more thorough test.
Maybe something in the lines of what @phproberto suggested in #1178...
Sorry, I know it's a pain in the back...

@smz that will take a while… But I will do it tonight!

@dgt41 Thanks!!

This does not work at all here. take a menu item displaying a single article associated to a menu item also displaying a single article in another language.

here is what we get

<base href="http://localhost:8888/testwindows/trunkgitnew/guillaume-apollinaire.html" />
  <meta http-equiv="content-type" content="text/html; charset=utf-8" />
  <meta name="author" content="Super User" />
  <meta name="generator" content="Joomla! - Open Source Content Management" />
  <title>Nom de site français - Guillaume Apollinaire</title>
  <link href="/testwindows/trunkgitnew/en/lewis-caroll.html" rel="alternate" hreflang="en-GB" />
  <link href=":8888/testwindows/trunkgitnew/guillaume-apollinaire.html" rel="alternate" hreflang="fr-FR" />
  <link href="/testwindows/trunkgitnew/de/" rel="alternate" hreflang="de-DE" />
  <link href="/testwindows/trunkgitnew/it/" rel="alternate" hreflang="it-IT" />
  <link href="/testwindows/trunkgitnew/es/" rel="alternate" hreflang="es-ES" />
  <link href="/testwindows/trunkgitnew/mk/" rel="alternate" hreflang="mk-MK" />
  <link href="/testwindows/trunkgitnew/ta/" rel="alternate" hreflang="ta-IN" />

the href must contain the domain.
the home page alternates should not display.

TBH, @smz, I suggest we just do as I suggested above. That worked as should and was real simple.

We though have indeed to solve

it was not working for the home pages if "Use URL Rewriting" wasn't set

Note: that issue is only for the default site language Home when URL Language Code is set to No and we switch homes from another language.

@infograf768

take a menu item displaying a single article associated to a menu item also displaying a single article in another language.

damn... that is a case I didn't tested... will check and see if I can solve...

the href must contain the domain.

Are you sure this is required? In any case this can be easly solved.

the home page alternates should not display.

Why?

TBH, @smz, I suggest we just do as I suggested above. That worked as should and was real simple.

TBH me too, I was seeing things I didn't like, like e.g. the fact that in some cases the cookie was involved: rel='alternate' links are created essentialy for the sake of search engines (which AFAIK do not store cookies), so I was strongly tempted to re-implement the whole thing...

Let me give it a try... if I fail, no problem, we have a "plan B"

ALL THE OTHERS: please stand-by testing as we still have some major issue (but code review and advices are warmheratedly accepeted!)

Thanks!

ah, sorry, now I get what you mean with:

the home page alternates should not display.

You mean in the case of a menu item not having association with a corresponding one in a different language, and yes, you're totally right!

That for sure comes from the fact that this is the correct behavior in the Language Switcher, from which I borrowed most of my code...

@smz ping me when it’s ready!

@dgt41 working hard on it... tnx! Will let you know... (the home page detection logic is broken, IMHO)

It should be OK now...
There are still some debug statements commented out that I'll later remove.
Also, as it is, it does not set the domain in the href, but for a first rough test it should work (unsure in case you use non-standard port...)

Yep! removing them right now, but PR is not ready yet for prime time: It is still TBD if the domain must be part of the href or not

I finally decided to add the 'scheme, host, port' pattern to the generated URLs, so that they are more conformant to the previous implementation.

In case we decide we dont need that, it will be easy to strip it out...

Travis happy, ready for testing...

Travis is happy but the patch still needs care:
One empty line before a conditional
Example: add an empty line between these $home = false and if ($active)

// Load menu associations
            $home = false;
            if ($active)

No need to condition to && JLanguageMultilang::isEnabled() as the method IS IN the languagefilter plugin.

We do get better results (default home page solved when no url rewriting) except that we can't mockup all the time the langswitcher behavior.

Example:
We display a category list with non associated items, the menu displaying this category is itself associated to a menu displaying a similar category in another language.
We click on one of the items in the category list to display that item.

We get:

  <base href="http://localhost:8888/testwindows/trunkgitnew/index.php/po%C3%A8tes-fran%C3%A7ais/4-pierre-de-ronsard.html" />
  <meta http-equiv="content-type" content="text/html; charset=utf-8" />
  <meta name="author" content="Super User" />
  <meta name="generator" content="Joomla! - Open Source Content Management" />
  <title>Nom de site français - Pierre de Ronsard</title>
  <link href="http://localhost:8888/testwindows/trunkgitnew/index.php/en/english-poets.html" rel="alternate" hreflang="en-GB" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/index.php/po%C3%A8tes-fran%C3%A7ais/4-pierre-de-ronsard.html" rel="alternate" hreflang="fr-FR" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/index.php/de/deutsche-dichter.html" rel="alternate" hreflang="de-DE" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/index.php/it/poeti-italiani.html" rel="alternate" hreflang="it-IT" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/index.php/es/poetas-españoles.html" rel="alternate" hreflang="es-ES" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/index.php/mk/македонски-поети.html" rel="alternate" hreflang="mk-MK" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/index.php/ta/தமிழ்-புலவர்கள்.html" rel="alternate" hreflang="ta-IN" />

where all alternates except the current page are associated PARENT menu items displaying categories.

It should NOT have an alternate as it is not itself associated, but we get alternate for the category in the other language, which is only a convenience offered by the languageswitcher.

I do not think that proposing a category as alternate to an article is good at all for SEO.

It works for me but reading the comment of @infograf768 I am a little bit confused. Also another thing that came up as I was reading the code can we drop those else, else if

@dgt41
maybe I was not clear enough.
As, in the case I describe above, the items themselves are not associated, we should not have any alternate, even if the language switcher nicely displays the parent associated menu item.

@infograf768
It should be now OK with your test case...
Sorry, I forgot to fix the code style: will do later...

@dgt41
Potentially (but I didn't thorougly analized that...) more than one condition may hold true, so the order and the exclusivity of the tests is important. Hence all those elseif that you don't like that much (and neither I do!)

A possible alternative, since we are inside a foreach loop, could be to structure the code this way:

// testing condition 1
if (condition 1)
{
    action 1;
    continue;
}

// testing condition 2
if (condition 2)
{
    action 2;
    continue;
}

etc...

Do you think this will be more efficient/readable?

@smz is better (performance wise) to drop the else’s. Yes something like that was what I had in my mind

@dgt41 performance wise I'm unsure as I think (but I'm unsure) that once the interpreter finds a TRUE condition it will skip all susequent else, same as a continue;

... and in terms of "lines of code" the solution without the elseif has one more line for each condition tested, but I'll make a test program (a loop with a zillion of iterations) and see which one is better.

For the time being I leave it as its...

@smz all i am saying is that 4 cases return unset($languages[$i]);
2 return $language->link = JRoute::_($item->link . '&Itemid=' . $item->id . '&lang=' . $language->sef);
and another returns $language->link = JRoute::_('index.php?lang=' . $language->sef . '&Itemid=' . $homes[$language->lang_code]->id);
These can be 3 if or something like that (theoretically speaking)

@dgt41 yes, the first three if can be combined into one with || operator between the conditions, I agree (and I'll probably do that once we're sure there isn't anything wrong in the logic...)

For the others I fail to see the similarity as the only common part is the use of the JRoute_. method, but with very different arguments. Then we should do something like that:

if (condion 1 || condition 2 || condition 3)
{
   if (conditon 1)
   {
     $argument = 1;
   }
   elseif (conditon 2)
   {
     $argument = 2;
   }
   elseif (conditon 3)
   {
     $argument = 3;
   }
   $result = JRoute::_($argument, ....);
}

... and I don't think this is any better...

@smz Ah yeah speed reading failed me! I thought the other cases had the same returning value. Just ignore all these comments. Let’s get some tests to move forward…

No problem and thanks for testing!

BTW: another possible construct that I like but it seems many don't is:

switch(true)
{
   case (condition 1):
   case (condition 2):
      do_something();
      break;
   case (condition 3):
      do_something_else();
      break;
   default:
      do_something_completely_different();
}

@smz
Sorry, the problem is not solved. Results:

<base href="http://localhost:8888/testwindows/trunkgitnew/en/english-poets/6-john-keats.html" />
  <meta http-equiv="content-type" content="text/html; charset=utf-8" />
  <meta name="author" content="Super User" />
  <meta name="generator" content="Joomla! - Open Source Content Management" />
  <title>multiassoc - John Keats</title>
  <link href="http://localhost:8888/testwindows/trunkgitnew/en/english-poets/6-john-keats.html" rel="alternate" hreflang="en-GB" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/poètes-français.html" rel="alternate" hreflang="fr-FR" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/de/deutsche-dichter.html" rel="alternate" hreflang="de-DE" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/it/poeti-italiani.html" rel="alternate" hreflang="it-IT" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/es/poetas-españoles.html" rel="alternate" hreflang="es-ES" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/mk/македонски-поети.html" rel="alternate" hreflang="mk-MK" />
  <link href="http://localhost:8888/testwindows/trunkgitnew/ta/தமிழ்-புலவர்கள்.html" rel="alternate" hreflang="ta-IN" />

although we should NOT get any alternate in that case.

@infograf768
My apologies, JM: I really thought I tested that case... (but of course I didn't).
With latest commit (which should work) I also "switched" my implementation.

ehmm... sorry, one more modification:
I changed the source of $languages from JLanguageHelper::getLanguages(); to $this->lang_codes; so that the indexes are the language codes themselves and less indirection is required (and also an if could be easly moved outside of a loop).

@zero-24 Travis stuck?

@infograf768
JM, have you considered the consequences of the following code in the current implementation?

if (class_exists($cName) && is_callable(array($cName, 'getAssociations')))
{
    $cassociations = call_user_func(array($cName, 'getAssociations'));

    $lang_code = $this->app->input->cookie->getString(JApplicationHelper::getHash('language'));

    // No cookie - let's try to detect browser language or use site default.
    if (!$lang_code)
    {
        if ($this->params->get('detect_browser', 1))
        {
            $lang_code = JLanguageHelper::detectLanguage();
        }
        else
        {
            $lang_code = JComponentHelper::getParams('com_languages')->get('site', 'en-GB');
        }
    }

    unset($cassociations[$lang_code]);
    $assocs = array_merge(array_keys($cassociations), $assocs);
}

Even if this may seems to work while testing (i.e. when you see the results through a browser), I think the behavior will be broken when spiders are hitting:

• No cookie set
• No browser language detected
• $lang_code set to the default language for site (or en-GB as a fallback)
• component association for the default language deleted even when visiting different languages pages

I think this is very wrong...

Travis has issues - https://www.traviscistatus.com/

Thanks Michael!

@smz
@test OK now here

Concerning

I think this is very wrong...

This was committed in eda46f5
was part of @bembelimen implementation of component associations.
Indeed not sure why it was introduced.

@infograf768 thanks for testing and guidance!

BTW, the reason why I pointed you to that code segment was not for pointing fingers, but to explain why I've been so "pushy in my will to reimplement this method. I think that with this new implemention and of course thanks to @Simonkloostra who raised the initial issue, SEO for multilingual J! sites will be a lot better.

I'm fighting a war with Travis who wants me to indent a multi-line assignment a lot more than I'm willing (after all, if you need multi-line assignments, it means you have a long assignment and indenting it so much will almost cancel the benefit of splitting it in more lines, and I sincerly think Travis is not considering the fact that we are indenting with tabs an not spaces), so...

I decided to shorten the name of the $default_language variable to $default_lang, but that lead me to the find that there is already a protected $default_lang; in class PlgSystemLanguageFilter which is nor initialized, nor used anywhere. JM, what do you think we should do with that? Remove it? Initialize it in the class construct?

@dgt41 Dimitris, can you please give this a try so that it can go RTC?

I will probably change something to make Travis happy, but it will not be any modification in the method logic, so I think it is safe to test it as it is now...

Thanks everybody!

Sorry... Due to wrong automatic login I sent the above from my old GitHub account...

@test all good here! Thanks Sergio.

$this->default_lang is really used in quite a few places.
It is in fact the current language lang_code since Hannes patch was merged.
I had already to correct a few errors because of that mistake.
See #6704

This means you should be able to use it instead of
$current_language = JFactory::getLanguage()->getTag();

@infograf768

insted of:
$current_language = JFactory::getLanguage()->getTag();
or:
$default_language = JComponentHelper::getParams('com_languages')->get('site', 'en-GB');
?

Sorry... I've now read better and you already stated that $this->default_lang is actually the current language (quite misleading, IMHO...)

That was the common error...

Hannes coded $this->default_lang as being the current language... i.e it equals JFactory::getLanguage()->getTag();
JComponentHelper::getParams('com_languages')->get('site', 'en-GB') is the real default site language as defined in the language manager.

before Hannes patch, we had (correctly)
self::$default_lang = JComponentHelper::getParams('com_languages')->get('site', 'en-GB');
and that was the site default language....

can't we fix this, maybe by introducing a $this->current_lang or $this->active_lang and changing in every place where Hannes has used $this->default_lang? Will it be a breach of B/C?

... in any case, and for the time being, I think I'll leave my code as it is, for sake of readability...

Do you agree?

In case, can we have RTC/Milestone?

doh... I didn't realised we also had #6809 about this issue... thanks to @gijs007 too!

I tested changing
protected $default_lang;
to
protected $current_lang;
and then changed accordingly in the rest of the file all occurences.

Looks like there are no issue here.

Do you want me to introduce that in this PR or should we go with a new one?

Let's separate issues. I am now testing another part to be able to also use $this->default_lang as it should be.

@infograf768 perfect, agreed!

... and if you have that mods merged before this PR, I can rebase it and further simplify...

2 good test. Can go RTC imho.

RTC based on testing.

Thanks @zero-24! Milestone?

Normaly the committer add the milestone bevor or after merge

@zero-24 Got it, thanks! Sorry, I'm not that familiar with the process...

Merged into staging - thanks!

@smz @dgt41

Please test
#7032

I've just patched up to Joomla 3.4.2 and tested this.
It appears that the default language is not making use of x-default hreflang tag for the default language.

@gijs007

according to http://googlewebmastercentral.blogspot.it/2013/04/x-default-hreflang-for-international-pages.html:

The new x-default hreflang attribute value signals to our algorithms that this page doesn’t target any specific language or locale and is the default page when no other page is better suited.

... not our case, IMHO

To further elaborate, this would be the case for when:

The homepage shows users a country selector and is the default page for users worldwide

But this is, AFAIK, a functionality currently not offered by Joomla...

Great to see this issue in 3.4.2. Thanks to all working so hard on this, while I only reported this issue and sat back watching the developers work so hard...
As far as the x-default question, I don't think it is needed.

@smz
I see, no issue there then :)

I did however notice that the country iso code is grabbed from the Language pack instead of out of the System - Language Code plugin.

I've explained why this is an issue in more detail here: http://forum.joomla.org/viewtopic.php?f=711&t=891482&sid=6e86c7f2835309f77c9bfa7635e74f3c&p=3316604#p3316604

As explained there, it will be grabbed from the Language Code plugin if set such.
<link href="http://mysite.com/index.php/" rel="alternate" hreflang="en" />
will be displayed if the en-GB lang code is replaced by en in the system language code.