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

[com_associations] - use cache for getContentLanguages() #18544

Merged
merged 17 commits into from Nov 18, 2017

Conversation

Projects
None yet
9 participants
@alikon
Contributor

alikon commented Nov 9, 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)

@csthomas

This comment has been minimized.

Show comment
Hide comment
@csthomas

csthomas Nov 9, 2017

Contributor

I would suggest this for getContentLanguages().

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

	return static::$languages;
}
Contributor

csthomas commented Nov 9, 2017

I would suggest this for getContentLanguages().

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

	return static::$languages;
}
clean code
thanks @csthomas
@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 9, 2017

Contributor

thanks @csthomas 👍 more clean now

Contributor

alikon commented Nov 9, 2017

thanks @csthomas 👍 more clean now

cs
@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 10, 2017

Member

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.

Member

infograf768 commented Nov 10, 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.

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 10, 2017

Contributor

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

Contributor

alikon commented Nov 10, 2017

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

alikon added some commits Nov 10, 2017

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 10, 2017

Contributor

hope cs fixed now

Contributor

alikon commented Nov 10, 2017

hope cs fixed now

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 10, 2017

Contributor

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

Contributor

andrepereiradasilva commented Nov 10, 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

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 10, 2017

Contributor

wellcome back @andrepereiradasilva , we are missing you ,

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

Contributor

alikon commented Nov 10, 2017

wellcome back @andrepereiradasilva , we are missing you ,

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

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 11, 2017

Contributor

applyed the tips from @andrepereiradasilva
please re-test

Contributor

alikon commented Nov 11, 2017

applyed the tips from @andrepereiradasilva
please re-test

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 11, 2017

Member

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.

Member

infograf768 commented Nov 11, 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.

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 11, 2017

Member

Changed my test to unsuccessful.

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

Member

infograf768 commented Nov 11, 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

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 11, 2017

Contributor
Contributor

alikon commented Nov 11, 2017

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 11, 2017

Contributor

re-re-test please

Contributor

alikon commented Nov 11, 2017

re-re-test please

cs
ops
@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 11, 2017

Member

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.

Member

infograf768 commented Nov 11, 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.

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 11, 2017

Contributor

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

Contributor

alikon commented Nov 11, 2017

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

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 12, 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.

franz-wohlkoenig commented Nov 12, 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.

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 12, 2017

Member

@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.

Member

infograf768 commented Nov 12, 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.

@infograf768 infograf768 removed the RTC label Nov 12, 2017

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 12, 2017

Member

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

Member

infograf768 commented Nov 12, 2017

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

alikon added some commits Nov 13, 2017

include unpublished languages
and remove proxy
@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 13, 2017

Contributor

applyed the tips from @joomdonation

Contributor

alikon commented Nov 13, 2017

applyed the tips from @joomdonation

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 13, 2017

Member

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.

Member

infograf768 commented Nov 13, 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.

@andrepereiradasilva

This comment has been minimized.

Show comment
Hide comment
@andrepereiradasilva

andrepereiradasilva Nov 13, 2017

Contributor

@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.

Contributor

andrepereiradasilva commented Nov 13, 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.

@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 14, 2017

Contributor

managed the trashed lanugages

Contributor

alikon commented Nov 14, 2017

managed the trashed lanugages

proxy for B/C
added deprecation
@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 14, 2017

Member

Works here

Member

infograf768 commented Nov 14, 2017

Works here

joomdonation added some commits Nov 15, 2017

@joomdonation

This comment has been minimized.

Show comment
Hide comment
@joomdonation

joomdonation Nov 15, 2017

Contributor

@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.

Contributor

joomdonation commented Nov 15, 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.

Merge pull request #38 from joomdonation/patch-7
Improve getContentLanguages
@alikon

This comment has been minimized.

Show comment
Hide comment
@alikon

alikon Nov 15, 2017

Contributor

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

Contributor

alikon commented Nov 15, 2017

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

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 15, 2017

Member

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.

Member

infograf768 commented Nov 15, 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.

@joomdonation

This comment has been minimized.

Show comment
Hide comment
@joomdonation

joomdonation Nov 15, 2017

Contributor

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.

Contributor

joomdonation commented Nov 15, 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.

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig Nov 15, 2017

@infograf768 can you please retest?

franz-wohlkoenig commented Nov 15, 2017

@infograf768 can you please retest?

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 15, 2017

Member

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.

Member

infograf768 commented Nov 15, 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.

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 Nov 15, 2017

Member

RTC. Thanks to all.


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

Member

infograf768 commented Nov 15, 2017

RTC. Thanks to all.


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

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 15, 2017

@infograf768 infograf768 added this to the Joomla 3.8.3 milestone Nov 15, 2017

@mbabker mbabker merged commit 8d0347f into joomla:staging Nov 18, 2017

5 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!

@joomla-cms-bot joomla-cms-bot removed the RTC label Nov 18, 2017

@alikon alikon deleted the alikon:patch-85 branch Nov 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment