Skip to content
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

Regression: Multilingual com_tags getting wrong language cookie #17084

Merged
merged 2 commits into from Jul 17, 2017

Conversation

infograf768
Copy link
Member

@infograf768 infograf768 commented Jul 12, 2017

Pull Request for Issue #17053

Summary of Changes

JHelper and JHelperContent do not take into account both ways of setting a lang cookie by the languagefilter plugin.
If the setting is to a year, the cookie is stored separately.
If the setting is set to Session, it is set in the session cookie (this was introduced in #12306 )

com_tags (and possibly some 3rd party extensions) use these classes to display the tags when the parameter is set to use current_language

Testing Instructions

See details in #17053

Set the languagefilter Cookie lifetime to Session:

screen shot 2017-07-12 at 09 45 49

for the following demo, I have created a single tag Mystag.
I have assigned it to the article Another article french tagged to French (fr-FR)
Also to to the article newarticle english tagged to English (en-GB)
And to the article myarticle tagged to ALL content languages.
Create a Tagged Items menu item to display that single tag and make sure Language Filter is set to Current

screen shot 2017-07-12 at 09 53 15

When switching languages, that menu item will now correctly display 2 articles, one set to the Content Language in use, the other set to ALL content languages.
cookies_tags

@mino182
@tonypartridge

@infograf768
Copy link
Member Author

Another solution (which would not touch at the helpers) is to use specifically for com_tags, every time we have a current_language conditional (therefore including the tags modules), JFactory: :getApplication()->getLanguage()->getTag(); instead of JHelperContent::getCurrentLanguage()

@mbabker
@rdeutz

What do you think?

if (JLanguageMultilang::isEnabled())
{
$plugin = JPluginHelper::getPlugin('system', 'languagefilter');
$pluginParams = new JRegistry($plugin->params);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New Registry + namespace load here please :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this pr is in the 3.7.x branch do we need to namespace in 3.7.x too ???

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registry has been namespaced since 3.4.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

me not know how to, but if I am asked to use the other solution, this would anyway be obsolete.
@mbabker please let me know your thought

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the namespacing thing, look at any other file which has a use Joomla\Registry\Registry; statement, copy that into the same place, and change JRegistry to Registry.

if (JLanguageMultilang::isEnabled())
{
$plugin = JPluginHelper::getPlugin('system', 'languagefilter');
$pluginParams = new JRegistry($plugin->params);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@mbabker
Copy link
Contributor

mbabker commented Jul 12, 2017

I honestly don't know what the purpose of JHelperContent::getCurrentLanguage() is (remember I really don't do multilingual stuff and the sites that I have it on it's all managed by others). The one thing I think seems a little off though is https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/helper/content.php#L159 using the default configured language for the site versus using the active language from JFactory.

@infograf768
Copy link
Member Author

Namespacing done.

@infograf768
Copy link
Member Author

The one thing I think seems a little off though is https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/helper/content.php#L159 using the default configured language for the site versus using the active language from JFactory.

As this is an urgent PR for 3.7.4, I guess it is better, in case we keep modifying the helpers, to leave the rest of the code alone.

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.7.4 milestone Jul 14, 2017
@AlexRed
Copy link
Contributor

AlexRed commented Jul 14, 2017

I have tested this item ✅ successfully on 3a4ad0a

Patch ok for me


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

@infograf768 infograf768 added this to the Joomla 3.7.4 milestone Jul 14, 2017
@zero-24
Copy link
Member

zero-24 commented Jul 14, 2017

I have tested this item ✅ successfully on 3a4ad0a

😄


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

@zero-24
Copy link
Member

zero-24 commented Jul 14, 2017

Thanks JM


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

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 3.7.4 milestone Jul 14, 2017
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 14, 2017
@infograf768 infograf768 added this to the Joomla 3.7.4 milestone Jul 15, 2017
@rdeutz rdeutz merged commit 087ac1f into joomla:staging Jul 17, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants