? Pending

User tests: Successful: Unsuccessful:

avatar andrepereiradasilva
andrepereiradasilva
10 Dec 2016

Summary of Changes

Deprecates JLanguageMultilang::isEnabled() in favour of JPluginHelper::isEnabled('system', 'languagefilter') (like all other plugins) and replaces the deprected method in all code base.

Testing Instructions

  1. Code review
  2. Use latest staging, apply patch and joomla language works as before.

Documentation Changes Required

None.

9f68ad4 10 Dec 2016 avatar andrepereiradasilva ups
avatar andrepereiradasilva andrepereiradasilva - open - 10 Dec 2016
avatar andrepereiradasilva andrepereiradasilva - change - 10 Dec 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 10 Dec 2016
Category Administration com_admin com_categories com_contact com_content com_fields com_finder com_languages com_menus com_modules com_newsfeeds com_users Templates (admin) Front End com_config
avatar andrepereiradasilva andrepereiradasilva - change - 11 Dec 2016
Labels Added: ?
avatar infograf768
infograf768 - comment - 11 Dec 2016

TBH, I see no use for this. We created JLanguageMultilang::isEnabled() to simplify code.

avatar andrepereiradasilva
andrepereiradasilva - comment - 11 Dec 2016

I don't agree. In that Logic we could use more method for other check that components, plugins are enabled. JTagsHelper::isEnabled, JContentHistoryHelper::isEnabled and so on.
I don't think we should add "special" cases.

Also this removes one db query is the cases the plugins are already fetched from the db.

avatar chrisdavenport
chrisdavenport - comment - 11 Dec 2016

I would argue that the meaning of JLanguageMultilang::isEnabled() is more explicit to the reader than what you are proposing to replace it with. This makes the code more readable and understandable.

A better "fix", in my opinion, would be to simply rewrite JLanguageMultilang::isEnabled() as

public static function isEnabled()
{
	return JPluginHelper::isEnabled('system', 'languagefilter');
}
avatar andrepereiradasilva
andrepereiradasilva - comment - 11 Dec 2016

well we can do just that, but still i think we should not make special cases with that argument.

Having more methods that do the same thing is IMHO somewhat confusing if you look at the code as a whole.

my point is: as it is now you have

  • JPluginHelper::isEnabled for plugins.
  • JComponentHelper::isEnabled for component.
  • JLibraryHelper::isEnabled for libraries.
  • and this "special" case.

(side note: i actually thinking of having an JExtensionHelper to reduce this 3 db queries in all pages to just one)

anyway, i thing as a whole is more easy to understand if we always use the same methods, not making special cases with the argument that is more readble. Yes, is more readble if you just look at this case, but if you look at code as a whole, IMHO is not.

And what differenciates this case from all other plugins/components/libraries? Are we going to make "special cases" for all of then? At which point we decide: "yes, this needs a special method to check if enabled because is used x times?"

Of course as always, this is only my opinion.

avatar infograf768
infograf768 - comment - 12 Dec 2016

We use JPluginHelper::isEnabled(), all plugins included, 32 times in core. 12 of them for content vote, 4 for editors, the others being unique or twice.

We use JLanguageMultilang::isEnabled() 70 times in core. It touches almost all aspects of Joomla, contrary to the ones above.
I guess these are the figures that count. I maintain my position about this PR.

avatar andrepereiradasilva
andrepereiradasilva - comment - 12 Dec 2016

sure @infograf768, still IMHO there should not be special cases, expect if not possible otherwise.

anyway, i have no feelings about code, i propose what i think is more correct, if the community does not accept it, fine by me, if accepts it, fine by me also.

Since the two people that comment in this PR (which i appreciate) seem to be against it, if in some days no one agree/comment or a mantainer rejects it: Fine, i will make a PR to go the alternative way described above.

avatar andrepereiradasilva
andrepereiradasilva - comment - 14 Dec 2016

closed in favour of #13210

avatar andrepereiradasilva andrepereiradasilva - change - 14 Dec 2016
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-12-14 22:33:32
Closed_By andrepereiradasilva
avatar andrepereiradasilva andrepereiradasilva - close - 14 Dec 2016

Add a Comment

Login with GitHub to post a comment