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.

Sign up for GitHub

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

Optimization and fix of multilingual associations and add layouts to com_content links #20229

Merged
merged 19 commits into from
May 5, 2018
Merged

Optimization and fix of multilingual associations and add layouts to com_content links #20229

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
0d9b502
Revert #19681
Septdir Apr 24, 2018
4e3ae20
Revert #19683
Septdir Apr 24, 2018
01e6786
Remove addition query and check after #19314
Septdir Apr 24, 2018
35f8ac0
Add layout to com_content links
Septdir Apr 24, 2018
9183852
Add layout to com_content article associations
Septdir Apr 24, 2018
1686ab3
Add layout to category associations
Septdir Apr 24, 2018
d99dca5
add advanced where clause param
Septdir Apr 25, 2018
79ed099
add advanced where clause for com_content article associations
Septdir Apr 25, 2018
aa7c21e
drone code formatting fix
Septdir Apr 25, 2018
0aac63a
drone code formatting fix
Septdir Apr 25, 2018
85af069
drone code formatting fix
Septdir Apr 25, 2018
cb41a47
Line exceeds 150 characters
Septdir Apr 25, 2018
a3406a2
PHPCS rules
Septdir Apr 25, 2018
425345e
Remove parenthesis
Septdir Apr 25, 2018
bb21207
Change queryKey
Septdir Apr 25, 2018
f4ca53e
Fix typo
Apr 25, 2018
9a40a4c
Merge pull request #1 from Quy/patch-1
Septdir Apr 25, 2018
2b14e47
Improve description
Apr 25, 2018
f5d85ff
Merge pull request #2 from Quy/patch-1
Septdir Apr 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions administrator/components/com_categories/helpers/association.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@ abstract class CategoryHelperAssociation
*
* @param integer $id Id of the item
* @param string $extension Name of the component
* @param string $layout Category layout
*
* @return array Array of associations for the component categories
*
* @since 3.0
*/
public static function getCategoryAssociations($id = 0, $extension = 'com_content')
public static function getCategoryAssociations($id = 0, $extension = 'com_content', $layout = null)
{
$return = array();

Expand All @@ -46,11 +47,13 @@ public static function getCategoryAssociations($id = 0, $extension = 'com_conten
{
if (class_exists($helperClassname) && is_callable(array($helperClassname, 'getCategoryRoute')))
{
$return[$tag] = $helperClassname::getCategoryRoute($item, $tag);
$return[$tag] = $helperClassname::getCategoryRoute($item, $tag, $layout);
}
else
{
$return[$tag] = 'index.php?option=' . $extension . '&view=category&id=' . $item;
$viewLayout = ($layout) ? '&layout=' . $layout : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove parenthesis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


$return[$tag] = 'index.php?option=' . $extension . '&view=category&id=' . $item . $viewLayout;
}
}
}
Expand Down
102 changes: 48 additions & 54 deletions components/com_content/helpers/association.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,81 +20,75 @@
*/
abstract class ContentHelperAssociation extends CategoryHelperAssociation
{
/**
* Cached array of the content item id.
*
* @var array
* @since __DEPLOY_VERSION__
*/
protected static $filters = array();

/**
* Method to get the associations for a given item
*
* @param integer $id Id of the item
* @param string $view Name of the view
* @param integer $id Id of the item
* @param string $view Name of the view
* @param string $layout View layout
*
* @return array Array of associations for the item
*
* @since 3.0
*/
public static function getAssociations($id = 0, $view = null)
public static function getAssociations($id = 0, $view = null, $layout = null)
{
$jinput = JFactory::getApplication()->input;
$view = $view === null ? $jinput->get('view') : $view;
$id = empty($id) ? $jinput->getInt('id') : $id;
$user = JFactory::getUser();
$groups = implode(',', $user->getAuthorisedViewLevels());
$jinput = JFactory::getApplication()->input;
$view = $view === null ? $jinput->get('view') : $view;
$component = $jinput->getCmd('option');
$id = empty($id) ? $jinput->getInt('id') : $id;

if ($layout === null && $jinput->get('view') == $view && $component == 'com_content')
{
$layout = $jinput->get('layout', '', 'string');
}

if ($view === 'article')
{
if ($id)
{
if (!isset(static::$filters[$id]))
$user = JFactory::getUser();
$groups = implode(',', $user->getAuthorisedViewLevels());
$db = JFactory::getDbo();
$advClause = array();

// Filter by user groups
$advClause[] = 'c2.access IN (' . $groups . ')';

// Filter by current language
$advClause[] = 'c2.language != ' . $db->quote(JFactory::getLanguage()->getTag());

if ((!$user->authorise('core.edit.state', 'com_content')) && (!$user->authorise('core.edit', 'com_content')))
Copy link
Member

Choose a reason for hiding this comment

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

No need imho for double parenthesis line 61

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@infograf768
I agree. I just copied this line from models/article.php
https://github.com/joomla/joomla-cms/blob/staging/components/com_content/models/article.php#L55

At work it does not affect. Perhaps double parentheses were made for better code performance ?

Remove double parenthesis in line 61 or leave?

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

{
// Filter by start and end dates.
$nullDate = $db->quote($db->getNullDate());
$date = JFactory::getDate();

$nowDate = $db->quote($date->toSql());

$advClause[] = '(c2.publish_up = ' . $nullDate . ' OR c2.publish_up <= ' . $nowDate . ')';
$advClause[] = '(c2.publish_down = ' . $nullDate . ' OR c2.publish_down >= ' . $nowDate . ')';

// Filter by published
$advClause[] = 'c2.state = 1';
}

$associations = JLanguageAssociations::getAssociations('com_content', '#__content', 'com_content.item', $id, 'id', 'alias', 'catid', $advClause);

$return = array();

foreach ($associations as $tag => $item)
{
$associations = JLanguageAssociations::getAssociations('com_content', '#__content', 'com_content.item', $id);

$return = array();

foreach ($associations as $tag => $item)
{
if ($item->language != JFactory::getLanguage()->getTag())
{
$arrId = explode(':', $item->id);
$assocId = $arrId[0];

$db = JFactory::getDbo();
$query = $db->getQuery(true)
->select($db->qn('state'))
->from($db->qn('#__content'))
->where($db->qn('id') . ' = ' . (int) $assocId)
->where($db->qn('access') . ' IN (' . $groups . ')');
$db->setQuery($query);

$result = (int) $db->loadResult();

if ($result > 0)
{
$return[$tag] = ContentHelperRoute::getArticleRoute($item->id, (int) $item->catid, $item->language);
}
}

static::$filters[$id] = $return;
}

if (count($associations) === 0)
{
static::$filters[$id] = array();
}
$return[$tag] = ContentHelperRoute::getArticleRoute($item->id, (int) $item->catid, $item->language, $layout);
}

return static::$filters[$id];
return $return;
}
}

if ($view === 'category' || $view === 'categories')
{
return self::getCategoryAssociations($id, 'com_content');
return self::getCategoryAssociations($id, 'com_content', $layout);
}

return array();
Expand All @@ -105,7 +99,7 @@ public static function getAssociations($id = 0, $view = null)
*
* @param integer $id Id of the article
*
* @return array An array containing the association URL and the related language object
* @return array An array containing the association URL and the related language object
*
* @since 3.7.0
*/
Expand Down
34 changes: 18 additions & 16 deletions components/com_content/helpers/route.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,13 @@ abstract class ContentHelperRoute
* @param integer $id The route of the content item.
* @param integer $catid The category ID.
* @param integer $language The language code.
* @param string $layout The layout value.
*
* @return string The article route.
*
* @since 1.5
*/
public static function getArticleRoute($id, $catid = 0, $language = 0)
public static function getArticleRoute($id, $catid = 0, $language = 0, $layout = null)
{
// Create the link
$link = 'index.php?option=com_content&view=article&id=' . $id;
Expand All @@ -42,6 +43,11 @@ public static function getArticleRoute($id, $catid = 0, $language = 0)
$link .= '&lang=' . $language;
}

if ($layout)
{
$link .= '&layout=' . $layout;
}

return $link;
}

Expand All @@ -50,12 +56,13 @@ public static function getArticleRoute($id, $catid = 0, $language = 0)
*
* @param integer $catid The category ID.
* @param integer $language The language code.
* @param string $layout TThe layout value.
*
* @return string The article route.
*
* @since 1.5
*/
public static function getCategoryRoute($catid, $language = 0)
public static function getCategoryRoute($catid, $language = 0, $layout = null)
{
if ($catid instanceof JCategoryNode)
{
Expand All @@ -68,24 +75,19 @@ public static function getCategoryRoute($catid, $language = 0)

if ($id < 1)
{
$link = '';
return '';
}
else
{
$link = 'index.php?option=com_content&view=category&id=' . $id;

if ($language && $language !== '*' && JLanguageMultilang::isEnabled())
{
$link .= '&lang=' . $language;
}
$link = 'index.php?option=com_content&view=category&id=' . $id;

$jinput = JFactory::getApplication()->input;
$layout = $jinput->get('layout');
if ($language && $language !== '*' && JLanguageMultilang::isEnabled())
{
$link .= '&lang=' . $language;
}

if ($layout !== '')
{
$link .= '&layout=' . $layout;
}
if ($layout)
{
$link .= '&layout=' . $layout;
}

return $link;
Expand Down
15 changes: 13 additions & 2 deletions libraries/src/Language/Associations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,22 @@ class Associations
* @param string $pk The name of the primary key in the given $table.
* @param string $aliasField If the table has an alias field set it here. Null to not use it
* @param string $catField If the table has a catid field set it here. Null to not use it
* @param array $advClause Array with advanced where clause use c as parent column key, c2 as associations column key
*
* @return array The associated items
*
* @since 3.1
*
* @throws \Exception
*/
public static function getAssociations($extension, $tablename, $context, $id, $pk = 'id', $aliasField = 'alias', $catField = 'catid')
public static function getAssociations($extension, $tablename, $context, $id, $pk = 'id', $aliasField = 'alias', $catField = 'catid',
$advClause = array())
{
// To avoid doing duplicate database queries.
static $multilanguageAssociations = array();

// Multilanguage association array key. If the key is already in the array we don't need to run the query again, just return it.
$queryKey = implode('|', func_get_args());
$queryKey = implode('|', array($extension, $tablename, $context, $id));
Copy link
Contributor

Choose a reason for hiding this comment

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

The $advClause param should probably be part of the key here as that will alter the query result, so what's cached can't really be re-used here.

Copy link
Contributor Author

@Septdir Septdir Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@mbabker I thought about that. But that key can turn out too long

Current key

com_content|#__content|com_content.item|1

Key with $advClause

com_content|#__content|com_content.item|1|c2.access IN (1,1,5)|c2.language != 'en-GB'|(c2.publish_up = '0000-00-00 00:00:00' OR c2.publish_up <= '2018-04-25 11:39:07')|(c2.publish_down = '0000-00-00 00:00:00' OR c2.publish_down >= '2018-04-25 11:39:07')|c2.state = 1

Key with all arguments

com_content|#__content|com_content.item|1|id|alias|catid|c2.access IN (1,1,5)|c2.language != 'en-GB'|(c2.publish_up = '0000-00-00 00:00:00' OR c2.publish_up <= '2018-04-25 11:40:39')|(c2.publish_down = '0000-00-00 00:00:00' OR c2.publish_down >= '2018-04-25 11:40:39')|c2.state = 1

Choose, in this issue I do not have a principled position

The key with all the parameters is the most accurate if we talk about caching, but I'm not sure that such precision is needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I'm not concerned with key length, I'm more concerned with ensuring accuracy in the code. I do think having the id, alias, and catid column names in the key is redundant because it's not like the table schema is going to change in the middle of the request (and the table is already part of the key) so dropping those are a good idea.

Make this the key:

$queryKey = md5(serialize(array_merge(array($extension, $tablename, $context, $id), $advClause)));

The key length is now consistent and has enough entropy that if you run this method with two different $advClause parameters that your result won't be affected because that param isn't considered as part of the cache and inherently the second or later calls ignore it completely.

Copy link
Contributor Author

@Septdir Septdir Apr 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the code. Great idea. Done


if (!isset($multilanguageAssociations[$queryKey]))
{
Expand Down Expand Up @@ -97,6 +99,15 @@ public static function getAssociations($extension, $tablename, $context, $id, $p
$query->where('c.extension = ' . $db->quote($extension));
}

// Advanced where clause
if (!empty($advClause))
{
foreach ($advClause as $clause)
{
$query->where($clause);
}
}

$db->setQuery($query);

try
Expand Down