Skip to content

Articles count in "List all categories" #5416

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

Closed
wants to merge 10 commits into from
Closed

Articles count in "List all categories" #5416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b678307
Articles count in "List all categories"
smz Dec 13, 2014
d9e2c0b
For Travis...
smz Dec 13, 2014
f432d36
Fixed instance hashing
smz Dec 13, 2014
010fbcd
Stupid mistake
smz Dec 13, 2014
0f57ab4
Added clarifying comment
smz Dec 14, 2014
a3fcd40
Made this the default behaviour
smz Dec 24, 2014
352ca75
Add test dependencies
wilsonge Dec 24, 2014
dd69bb2
Merge pull request #2 from wilsonge/patch-19
smz Dec 25, 2014
bfdced4
Update JCategoriesTest.php
smz Dec 25, 2014
e8a544a
Use $db->quote('*') instead of '*'
smz Dec 25, 2014
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
17 changes: 10 additions & 7 deletions libraries/legacy/categories/categories.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ public function __construct($options)
$this->_statefield = (isset($options['statefield'])) ? $options['statefield'] : 'state';
$options['access'] = (isset($options['access'])) ? $options['access'] : 'true';
$options['published'] = (isset($options['published'])) ? $options['published'] : 1;
$options['countItems'] = (isset($options['countItems'])) ? $options['countItems'] : 0;
$options['currentlang'] = JLanguageMultilang::isEnabled() ? JFactory::getLanguage()->getTag() : 0;
$this->_options = $options;

return true;
Expand Down Expand Up @@ -259,20 +261,21 @@ protected function _load($id)
->where('badcats.id is null');

// Note: i for item
if (isset($this->_options['countItems']) && $this->_options['countItems'] == 1)
if ($this->_options['currentlang'] !== 0 || $this->_options['countItems'] == 1)
Copy link
Member

@Hackwar Hackwar Jul 1, 2015

Choose a reason for hiding this comment

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

This one makes third party extensions fail on multilang sites, which don't want this part activated. The whole option of "currentlang" has no business here. It counters "countItems". The first part of this if needs to be removed.

Copy link
Contributor

@smz smz Jul 1, 2015

Choose a reason for hiding this comment

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

Can you please make an example?

That condition is essential for returning the correct items in multilingual environments: you want to get items that are flagged for either the current language or *

Copy link
Member

@Hackwar Hackwar Jul 1, 2015

Choose a reason for hiding this comment

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

Before your change, you were able to disable the whole counting stuff. Your change forces this to be enabled when you are in a multi-language site, regardless of the component even using any language stuff at all. With your change, I can't switch this off.

I have a component that uses the category system, but does not have a catid field in the data table. So, since I don't need the whole counting thing and it is also not applicable to my situation, I disabled it for my JCategories implementation. This change forces this option to ON and since I don't have a DB table field for the category ID, I get a nice big error page with a broken query.

Copy link
Contributor

@smz smz Jul 1, 2015

Choose a reason for hiding this comment

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

How would you solve the original issue this PR has solved without breaking your component?

Copy link
Contributor

@Bakual Bakual Jul 1, 2015

Choose a reason for hiding this comment

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

There is already a PR #7303 by Hannes. I think it should fix both the original issue from here and the regression introduced.

{
$queryjoin = $db->quoteName($this->_table) . ' AS i ON i.' . $db->quoteName($this->_field) . ' = c.id';

if ($this->_options['published'] == 1)
{
$query->join(
'LEFT',
$db->quoteName($this->_table) . ' AS i ON i.' . $db->quoteName($this->_field) . ' = c.id AND i.' . $this->_statefield . ' = 1'
);
$queryjoin .= ' AND i.' . $this->_statefield . ' = 1';
}
else

if ($this->_options['currentlang'] !== 0)
{
$query->join('LEFT', $db->quoteName($this->_table) . ' AS i ON i.' . $db->quoteName($this->_field) . ' = c.id');
$queryjoin .= ' AND (i.language = ' . $db->quote('*') . ' OR i.language = ' . $db->quote($this->_options['currentlang']) . ')';
}

$query->join('LEFT', $queryjoin);
$query->select('COUNT(i.' . $db->quoteName($this->_key) . ') AS numitems');
}

Expand Down
19 changes: 16 additions & 3 deletions tests/unit/suites/libraries/legacy/categories/JCategoriesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,29 @@ class JCategoriesTest extends TestCaseDatabase
*
* @since 3.2
*/
protected function setUp()
public function setUp()
{
parent::setUp();

// Add JApplication and JLanguage dependencies
$this->saveFactoryState();
JFactory::$language = $this->getMockLanguage();
JFactory::$application = $this->getMockCmsApp();
}

/**
* Tears down the fixture, for example, closes a network connection.
* This method is called after a test is executed.
* Overrides the parent tearDown method.
*
* @return void
*
* @see PHPUnit_Framework_TestCase::tearDown()
* @since 3.2
*/
protected function tearDown()
{
$this->restoreFactoryState();

parent::tearDown();
}

/**
Expand Down