? ? Pending

User tests: Successful: Unsuccessful:

avatar alikon
alikon
9 Nov 2017

Pull Request for Issue #15494.

Summary of Changes

use cache instead of query everytime the available languages

Testing Instructions

see #15494

Expected result

reduce the number of duplicate query

Actual result

unnecessary duplicate query for get languages

Additional Comments

reduced this one only
#15494 (comment)

avatar alikon alikon - open - 9 Nov 2017
avatar alikon alikon - change - 9 Nov 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 9 Nov 2017
Category Administration com_associations
avatar csthomas
csthomas - comment - 9 Nov 2017

I would suggest this for getContentLanguages().

public static function getContentLanguages()
 {
	if (empty(static::$languages))
	{
		static::load();
	}

	return static::$languages;
}
avatar alikon alikon - change - 9 Nov 2017
Labels Added: ?
avatar alikon
alikon - comment - 9 Nov 2017

thanks @csthomas 👍 more clean now

48ef394 9 Nov 2017 avatar alikon cs
avatar infograf768
infograf768 - comment - 10 Nov 2017

Queries number has reduced a lot: 186 instead of 235.
as you said, some more could be done. Maybe for another PR?

screen shot 2017-11-10 at 08 23 24

I have seen some cs.

avatar alikon
alikon - comment - 10 Nov 2017

yes matter for another PR.....
i'll fix the cs glitch....

avatar alikon
alikon - comment - 10 Nov 2017

hope cs fixed now

avatar andrepereiradasilva
andrepereiradasilva - comment - 10 Nov 2017

hi guys, just stoped by to say: why not reuse the already available method in the library

Joomla\CMS\Language\LanguageHelper::getContentLanguages($checkPublished = true, $checkInstalled = true, $pivot = 'lang_code', $orderField = null, $orderDirection = null)

See https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Language/LanguageHelper.php#L336

avatar alikon
alikon - comment - 10 Nov 2017

wellcome back @andrepereiradasilva , we are missing you ,

i'll investigate further your suggestion, i hope in the week-end....

avatar alikon
alikon - comment - 11 Nov 2017

applyed the tips from @andrepereiradasilva
please re-test

avatar infograf768 infograf768 - test_item - 11 Nov 2017 - Tested successfully
avatar infograf768 infograf768 - alter_testresult - 11 Nov 2017 - infograf768: Tested unsuccessfully
avatar infograf768 infograf768 - test_item - 11 Nov 2017 - Tested unsuccessfully
avatar infograf768
infograf768 - comment - 11 Nov 2017

I have tested this item 🔴 unsuccessfully on 6f6c1d3


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

avatar infograf768
infograf768 - comment - 11 Nov 2017

Changed my test to unsuccessful.

I get an error
Call to undefined method AssociationsHelper::getContentLanguages()
when trying to load the side to side page

avatar alikon
alikon - comment - 11 Nov 2017

Silly me, I'll do the needed change later sorry

Il 11 nov 2017 9:45 AM, "infograf768" notifications@github.com ha scritto:

Changed my test to unsuccessful.

I get an error
Call to undefined method AssociationsHelper::getContentLanguages()
when trying to load the face to face page


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#18544 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AALFsfJnFDGi1WH53fV_Sn6pPveR-M7bks5s1V6PgaJpZM4QYkba
.

avatar alikon
alikon - comment - 11 Nov 2017

re-re-test please

e935a9d 11 Nov 2017 avatar alikon cs
avatar infograf768 infograf768 - test_item - 11 Nov 2017 - Tested successfully
avatar infograf768
infograf768 - comment - 11 Nov 2017

I have tested this item successfully on e935a9d

Perfect this time. 😄


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

avatar alikon
alikon - comment - 11 Nov 2017

perfection is my current challenge, unfortunately i already know that i cant win 😄

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 12 Nov 2017

@alikon can you please give Test Instructions?


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

avatar alikon
alikon - comment - 12 Nov 2017
  1. start with a multilanguage site with > 5 langauges
  2. add some content in en-GB for example
  3. enable debug plugin
  4. com_associations select articles and language en-gb
  5. look at database tab of debug plugin and take note of much query are executed and how much are duplicate
  6. apply PR
    7 redo 4., 5. you should notice less query and less duplicate
avatar franz-wohlkoenig franz-wohlkoenig - test_item - 12 Nov 2017 - Tested successfully
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 12 Nov 2017

I have tested this item successfully on e935a9d

Without PR

1

With PR

2
Thanks for Instructions @alikon


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18544.
avatar franz-wohlkoenig franz-wohlkoenig - change - 12 Nov 2017
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 12 Nov 2017

RTC after two successful tests.

avatar joomdonation
joomdonation - comment - 12 Nov 2017

Just want to point out that there is difference between the data returned by old code and new code. With the original code, all content languages (including unpublished) will be returned, with new code, only published content languages will be returned

Also, if we decide to use new code, I think it is better to call the new method directly instead of still calling AssociationsHelper::getContentLanguages (because this method is now basically, an alias of LanguageHelper::getContentLanguages() )

avatar infograf768 infograf768 - change - 12 Nov 2017
Status Ready to Commit Pending
avatar infograf768
infograf768 - comment - 12 Nov 2017

@joomdonation

You are totally correct. I take off RTC.
We should be able to prepare a new Content Language but not put it online until Published.

With this patch, it is now impossible to do anything for such a content language in com_associations.


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

avatar infograf768
infograf768 - comment - 12 Nov 2017

In fact I guess the solution is easy
return LanguageHelper::getContentLanguages(false, true); is enough.

avatar alikon
alikon - comment - 13 Nov 2017

applyed the tips from @joomdonation

avatar infograf768 infograf768 - test_item - 13 Nov 2017 - Tested successfully
avatar infograf768
infograf768 - comment - 13 Nov 2017

I have tested this item successfully on 69d811d

Works fine now even if the Content Language is unpublished.

Only drawback is that it also proposes those which are in the Trash.
That case is not taken care of in LanguageHelper::getContentLanguages()


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

avatar andrepereiradasilva
andrepereiradasilva - comment - 13 Nov 2017

@infograf768 reggarding the trashed language, true. the language helper method could be improved (while preserving B/C).

Something like this. Not tested. Replaced $checkPublished (boolean) argument for $publishedStates (array) and using a B/C layer for that parameter.

/**
 * Get a list of content languages.
 *
 * @param   array    $publishedStates  Array with the content language published states. Null or empty array for all.
 * @param   boolean  $checkInstalled   Check if the content language is installed.
 * @param   string   $pivot            The pivot of the returning array.
 * @param   string   $orderField       Field to order the results.
 * @param   string   $orderDirection   Direction to order the results.
 *
 * @return  array  Array of the content languages.
 *
 * @since   3.7.0
 */
public static function getContentLanguages($publishedStates = array(1), $checkInstalled = true, $pivot = 'lang_code', $orderField = null, $orderDirection = null)
{
	// B/C layer. Before __DEPLOY_VERSION__.
	if ($publishedStates === true)
	{
		$publishedStates = array(1);
	}
	elseif ($publishedStates === false)
	{
		$publishedStates = null;
	}

	// [code removed for brevity...]

	// Check if the language is published, if needed.
	if ($publishedStates !== null && $publishedStates !== array())
	{
		foreach ($languages as $key => $language)
		{
			if (in_array((int) $language->published, $publishedStates, true) === false)
			{
				unset($languages[$key]);
			}
		}
	}

	// [code removed for brevity...]
}

then, for published/unpublished, you could call it like this

LanguageHelper::getContentLanguages(array(0, 1), true);

BTW @alikon unless things changed recently i don't think it's B/C to completly remove a public method in 3.x, you should proxy it to languagehelper method and them put a deprecated 4.0 message.

avatar joomla-cms-bot joomla-cms-bot - change - 14 Nov 2017
Category Administration com_associations Administration com_associations Libraries
avatar alikon
alikon - comment - 14 Nov 2017

managed the trashed lanugages

avatar infograf768
infograf768 - comment - 14 Nov 2017

Works here

avatar joomdonation
joomdonation - comment - 15 Nov 2017

@alikon If I am not mistaken, the changes you made to \Joomla\CMS\Language\LanguageHelper::getContentLanguages() will cause B/C issue. Before the change, if someone calls \Joomla\CMS\Language\LanguageHelper::getContentLanguages() , the result will include trashed content languages. After your changes, it won't include trashed content languages anymore.

I think the propose change from @andrepereiradasilva is better. It extends the method to allow us to get content languages we want based on passed publish states. I made a PR to your branch alikon#38, please review it and if you agree, merge it so that we can have this issue sorted.

avatar alikon
alikon - comment - 15 Nov 2017

thanks folks for teamworking, hope now issues are solved in a B/C way

avatar infograf768 infograf768 - test_item - 15 Nov 2017 - Tested successfully
avatar infograf768
infograf768 - comment - 15 Nov 2017

I have tested this item successfully on e3d1b46

TBH, folks, I see absolutely no reason why anyone would list Trashed Content Languages.
In this case, but also everywhere else in J where we use the method.


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

avatar joomdonation joomdonation - test_item - 15 Nov 2017 - Tested successfully
avatar joomdonation
joomdonation - comment - 15 Nov 2017

I have tested this item successfully on f438af0


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

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 15 Nov 2017

@infograf768 can you please retest?

avatar infograf768 infograf768 - test_item - 15 Nov 2017 - Tested successfully
avatar infograf768
infograf768 - comment - 15 Nov 2017

I have tested this item successfully on f438af0


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

avatar infograf768 infograf768 - change - 15 Nov 2017
Status Pending Ready to Commit
avatar infograf768
infograf768 - comment - 15 Nov 2017

RTC. Thanks to all.


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

avatar mbabker mbabker - change - 18 Nov 2017
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2017-11-18 15:49:11
Closed_By mbabker
Labels Added: ?
avatar mbabker mbabker - close - 18 Nov 2017
avatar mbabker mbabker - merge - 18 Nov 2017

Add a Comment

Login with GitHub to post a comment