[#7057] - Deprecate getCurrentLanguage() methods
- Closed
- 13 Jul 2015
- Medium
- Build: staging
- Easy Test
- # 7057
- Diff
- smz:getCurrentLanguage
- Success The Travis CI build passed Details
User tests: Successful: Unsuccessful:
Description
There is no apparent issue involved, but two getCurrentLanguage() methods were using questionable code.
This PR greatly simplify that code (by making the two methods proxies to the official way to get the currently active language) and make it more robust
The methods are deprecated too, and any reference to them has been substituted with the correct (already existing) method.
Steps to test
To test that there is no regression:
- have a multilingual site
- create some tags and assign them to the different languages and to the "All" languages
- assign those tags to some different articles in different languages (consistently and also inconsistently to the article's language)
- publish a "Tags - Popular" module on "All pages" and for all languages. Set the "Display Number of Items" option to "Yes"
- in Tags component options, under "Item Selection Options" select "Language filter: Current"
The reason to use "Tags" to test this PR is that the two involved methods are used by com_tags only (AFAIK...)
Expected result
With or without this patch the displayed tags, the number of items for each tag, and the available items under each tag should be the same
Additional comments
If you assigned tags in an inconsistent way (i.e. a "French" tag to an "English" article) and also if you assigned tags flagged as "All languages" to specific language articles, the displayed number of items for each tag will not match the number of items available under that tag for a particular language, but this is a separate issue and that behavior too will not be affected by this patch.
Thanks @bakual for suggesting the usage of JFactory::getLanguage()->getTag() to get the currently active language.
Votes
I wonder since you say this is only used by com_tags, if we could additionally deprecate the helper methods and use JFactory::getLanguage()->getTag() directly in the places where those helper methods are called. It doesn't make sense to have to call a helper method which only has one line of code 
OK! Will do later on today...
Deprecate should be @ 3.4.2 or @ 4.0 ?
Please use @deprecated 4.0 Use JFactory::getLanguage()->getTag() instead
Hmm, this was the original tracker/PR
See: http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=30231&start=0
#784
@infograf768 First, as I have just turned 60, a beg you to share with me your secret recipe for maintaining a good memory!
Yes, #784 introduced the language filter for tagged items, and the associated original tracker is the one you cited.
In that PR the getCurrentLanguage() was used in several instances, but that method itself must had been introduced sometime earlier as I don't see it defined there.
This PR doesn't seems to affect the functionality of #784 in any way (and it shouldn't from code analysis) as my test methodology, if not exactly equal to, is a very close relative to it of the one described in the tracker, and thus IMHO it is OK to have this accepted...
| Title |
|
||||||
| Title |
|
||||||
joomla-cms-bot
- | Labels |
Added:
?
|
||
| Labels |
Added:
?
|
||
Thanks, looks great from code review.
In that PR the getCurrentLanguage() was used in several instances, but that method itself must had been introduced sometime earlier as I don't see it defined there.
it was in that PR in cms/helper/content.php, ( e1993e1#diff-70a9f5171aac5ecc71b5c63f03bdfee1R258 ) and as you can read, I was already questioning some aspects of it, check browser language for example.
For cms/helper/helper.php this was introduced a bit later in 251410c#diff-0163ec31770d206724d25d3946742aeeR30
Which makes me wonder why we should keep ($detectBrowser = true) if we do not use the browser check anymore.
Is this really B/C?
zero-24
- | Category | ⇒ | Libraries |
| Status | New | ⇒ | Pending |
| Easy | No | ⇒ | Yes |
Which makes me wonder why we should keep ($detectBrowser = true) if we do not use the browser check anymore.
Is this really B/C?
nope... I just forgot to remove the = true... oops!
I meant in public static function getCurrentLanguage($detectBrowser = true)
Fixed: I left the two now ignored parameters there (but un-initialized) as they are public methods and thus the API doc is calling for those parameters. Correct?
@infograf768, @dgt41, can you please test this?
@infograf768, @dgt41, @Denitz: Can you please test this?
No issue here. Behaviour is same.
As we have a "Go!" by @Bakual and @infograf768, can we have this set to RTC and merged for 3.4.2?
| Labels |
Added:
?
|
||
| Labels |
Added:
?
|
||
| Milestone |
Added: |
||
| Milestone |
Added: |
||
| Milestone |
Removed: |
||
According to SemVer this has to go into 3.5.0 since we deprecate something.
We can remove the deprecation, have good code merged and add the deprecation back @3.5.0. Or not?
Since it doesn't really fix anything but is code refactoring, it should go into 3.5.0 anyway. There is no reason to split the PR.
@Bakual
Actually, looking at the code mods, I have the feeling that there is an issue fixed by this PR: probably mod_tags_popular doesn't show the correct tags if the current language is not the one of your browser.
Unhappily while trying to see if this is indeed the case I discovered that probably we have a regression in the setting of categories associations. I'm thus concentrated on this at this time... will let you know and in case open an urgent issue....
| Labels |
Removed:
?
|
||
| Labels |
Removed:
?
|
||
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-07-13 08:10:34 |
| Closed_By | ⇒ | smz |
| Milestone |
Removed: |
||
@infograf768