Articles count in "List all categories" #5416

smanzi · 2014-12-13T01:22:40Z

Fixes articles count in "List all categories": articles for non active language were erroneously accounted for (see: #5412)

Fixes articles count in "List all categories": articles for non active language were erroneously accounted for (see: 5412)

smanzi · 2014-12-13T01:27:31Z

I also took the occasion for cleaning up the code in JCategories::_load() for the 'countItems' option...

smanzi · 2014-12-13T01:44:00Z

Travis not very clear on this...

FILE: ...avis/build/joomla/joomla-cms/libraries/legacy/categories/categories.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
270 | ERROR | Expected 1 space after "%s"; %s found
275 | ERROR | Expected 1 space after "%s"; %s found
--------------------------------------------------------------------------------

I think it is the double spaces after the .= operators... let's try!

smanzi · 2014-12-13T02:08:43Z

There is conceptual error: the active language must be taken into account when computing the instance hash...
Going to fix this

smanzi · 2014-12-13T02:25:58Z

I have a fix ready but I cannot sync my local repo with upstream... I hope it is GitHub momentary issue...

smanzi · 2014-12-13T02:28:49Z

Fixed!

smanzi · 2014-12-14T04:49:34Z

Don't panic: "smz" is my new GitHub account

smz · 2014-12-14T04:50:12Z

Confirmed!

AlexRed · 2014-12-17T18:02:41Z

Successfull test.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5416.}

smz · 2014-12-17T19:30:23Z

@AlexRed Thank-you for testing, Alessandro!

What I forgot to underline in the presentation of this PR is that this issue is particularly nasty when the returned count should be zero and instead it is non-zero: there you have an empty category displayed while it should not.

I know that mixing articles tagged for different languages inside a common category is not considered a best practice and common wisdom is against this, but I think instead that this gives you a further degree of freedom that may be leveraged in different scenarios. Sometimes best practices and common wisdom derive from the necessity to work around existing issues...

stellainformatica · 2014-12-18T09:48:21Z

@test:
Patch tested, it solves the issue.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5416.}

smz · 2014-12-18T13:28:44Z

@stellainformatica Thanks for testing, ste, appreciated!

C'mon guys/girls, some more tests and comments, so we have the opportunity of this bug being fixed!

infograf768 · 2014-12-18T15:06:00Z

@smz
Please kindly look at
#4937

I had already found that issue while testing your patch when I saw that report.
Looks like in config.xml as well as categories/default.xml, the field
<field name="maxLevelcat
is wrong.
I.e. it does not include "None" with value "0"
Therefor the code to diisplay the subcategories should also include "None" and the display conditions should take into account "All" and "None"

Looks also that the issue exists for other core components.

smz · 2014-12-18T15:25:00Z

@infograf768 JM, I may be, as they say, "thick as brick", but I fail to understand if you're telling me that what I'm trying to solve here is a consequence of #4937 (and hence once fixed that, this issue will be automatically fixed as well) or if you are asking me to try to fix #4937 too while I'm trying to fix this...

Sorry, sometimes is not so easy to understand between each other, especially when both are not using their native language!

infograf768 · 2014-12-18T15:27:58Z

Just asking if you have time to also fix #4937 as you have been working on categories. :)
This would be another PR, as I see it, as it concerns quite a few files and tests and the issue is different

smz · 2014-12-18T15:31:25Z

Ah, OK! Will look into it and let you know if I'm able to... Right now I'm preparing a new commit to #5463 as I discovered that the dates conditions ("before", "after" and "exact") are not translated in the query description and so I'm adding 3 new strings for i18n...

infograf768 · 2014-12-21T09:24:12Z

@smz
Don't we also have to port this PR here to the other core components as you did in #5469 ?

smz · 2014-12-21T10:57:16Z

@infograf768 JM, if we are convinced this is the correct behavior for all components, I think we can just get rid of the new option and modify the library with the following:

        $options['currentlang'] = JLanguageMultilang::isEnabled() ? 1 : 0;
        if ($options['currentlang'] == 1)
        {
            $options['currentlang'] = JFactory::getLanguage()->getTag();
        }

doing that we will have it automatically applied to all current and future components. What do you think?

smz · 2014-12-21T10:57:55Z

P.S.: I already checked that it does not have any detrimental effect in backend...

smz · 2014-12-21T11:03:45Z

or better:

$options['currentlang'] = JLanguageMultilang::isEnabled() ? JFactory::getLanguage()->getTag() : 0;

infograf768 · 2014-12-24T10:19:28Z

Have you tested?

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5416.}

smz · 2014-12-24T10:49:31Z

Not yet, TBH... If all is well I'll push a commit, OK?

smz · 2014-12-24T10:50:46Z

I mean: I already tested in com_content, but not with other components.

wilsonge · 2014-12-24T23:53:21Z

All fixed with smz#2

Add test dependencies

smz · 2014-12-25T00:10:44Z

@wilsonge Thanks, George, merged!

smz · 2014-12-25T00:57:39Z

com_contact items count issue is fixed in #5515 (I think this is so trivial it may jump to merge...)

Small typo: double semicolon

infograf768 · 2014-12-25T08:43:01Z

OK here. Need one more tester?

smz · 2014-12-25T10:35:10Z

@infograf768 JM, we also have @AlexRed and @stellainformatica tests (if you still consider those as valid), but what do you think about my concern about the need to use $db->quote with string constants inside a query [see: https://github.com//pull/5416#issuecomment-68076921]?

infograf768 · 2014-12-25T10:47:05Z

We do not use db->quote in
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_languages/helpers/multilangstatus.php#L138
but . $db->quote('*') is used in
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_languages/models/languages.php#L126
and quite a few other places.

If test OK, I guess it would be better.

smz · 2014-12-25T10:55:43Z

Seems to be OK here...

smz · 2014-12-28T04:25:32Z

@infograf768 JM, what do we do with this? In the meanwhile I discovered that it solves the same issue (wrong articles count) also for mod_articles_categories...

smz · 2015-05-04T10:06:58Z

Sorry, guys, may I ask why this PR which fixes a bug and has been extensively discussed/tested isn't apparently considered for merging?

brianteeman · 2015-05-04T10:12:10Z

OK here. Need one more tester?

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/5416.}

smz · 2015-05-04T10:15:37Z

That looks more as a question than a statement and http://issues.joomla.org/tracker/joomla-cms/5416 says we already have 2 successfull test, but, please, feel free to test yourself.

dgrammatiko · 2015-05-08T23:50:37Z

@test OK
Sorry Sergio for the very late response here, I thought this has been merged already

smz · 2015-05-08T23:59:07Z

@dgt41
No problem, Dimitris! Actually I think this PR already had 2 positive tests (three counting JM)

wilsonge · 2015-05-09T00:15:48Z

Merged - thanks for bringing to my attention. For some reason I thought I'd merged this earlier in the week. Totally my bad! Sorry!

smz · 2015-05-09T00:17:44Z

It's OK, George! Thanks!

Hackwar · 2015-07-01T09:21:07Z

libraries/legacy/categories/categories.php

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


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.

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 *

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.

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

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

Articles count in "List all categories"

b678307

Fixes articles count in "List all categories": articles for non active language were erroneously accounted for (see: 5412)

jissues-bot added the PR-staging label Dec 13, 2014

smanzi mentioned this pull request Dec 13, 2014

Multilingual: possible error in articles count with "List all categories" #5412

Closed

For Travis...

d9e2c0b

Fixed instance hashing

f432d36

smz added 2 commits December 13, 2014 03:51

Stupid mistake

010fbcd

Added clarifying comment

0f57ab4

brianteeman added the RTC This Pull Request is Ready To Commit label Dec 20, 2014

Add test dependencies

352ca75

Merge pull request #2 from wilsonge/patch-19

dd69bb2

Add test dependencies

Update JCategoriesTest.php

bfdced4

Small typo: double semicolon

Use $db->quote('*') instead of '*'

e8a544a

smz mentioned this pull request May 3, 2015

Multilanguage: do not use a language if it has been deleted/disabled #6746

Closed

smz mentioned this pull request May 8, 2015

JCategories Fatal error: Call to a member function getChildren() on a non-object fixed #6064 #6440

Merged

wilsonge closed this in 497e2da May 9, 2015

zero-24 added this to the Joomla! 3.4.2 milestone May 9, 2015

zero-24 added the RTC This Pull Request is Ready To Commit label May 9, 2015

smz deleted the ListCategoriesItemsCount branch May 29, 2015 01:31

johanjanssens pushed a commit to joomlatools/joomla-fork that referenced this pull request Jun 19, 2015

Articles count in "List all categories". Fixes joomla#5416

495670c

Hackwar reviewed Jul 1, 2015
View reviewed changes

Hackwar mentioned this pull request Jul 1, 2015

Having a multilang-site should not override the options of JCategories #7303

Merged

zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015

joomla-cms-bot added the Unit/System Tests label Oct 14, 2015

Articles count in "List all categories" #5416

Articles count in "List all categories" #5416

Conversation

smanzi commented Dec 13, 2014

smanzi commented Dec 13, 2014

smanzi commented Dec 13, 2014

smanzi commented Dec 13, 2014

smanzi commented Dec 13, 2014

smanzi commented Dec 13, 2014

smanzi commented Dec 14, 2014

smz commented Dec 14, 2014

AlexRed commented Dec 17, 2014

smz commented Dec 17, 2014

stellainformatica commented Dec 18, 2014

smz commented Dec 18, 2014

infograf768 commented Dec 18, 2014

smz commented Dec 18, 2014

infograf768 commented Dec 18, 2014

smz commented Dec 18, 2014

infograf768 commented Dec 21, 2014

smz commented Dec 21, 2014

smz commented Dec 21, 2014

smz commented Dec 21, 2014

infograf768 commented Dec 24, 2014

smz commented Dec 24, 2014

smz commented Dec 24, 2014

wilsonge commented Dec 24, 2014

smz commented Dec 25, 2014

smz commented Dec 25, 2014

infograf768 commented Dec 25, 2014

smz commented Dec 25, 2014

infograf768 commented Dec 25, 2014

smz commented Dec 25, 2014

smz commented Dec 28, 2014

smz commented May 4, 2015

brianteeman commented May 4, 2015

smz commented May 4, 2015

dgrammatiko commented May 8, 2015

smz commented May 8, 2015

wilsonge commented May 9, 2015

smz commented May 9, 2015

Hackwar Jul 1, 2015

Choose a reason for hiding this comment

smz Jul 1, 2015

Choose a reason for hiding this comment

Hackwar Jul 1, 2015

Choose a reason for hiding this comment

smz Jul 1, 2015

Choose a reason for hiding this comment

Bakual Jul 1, 2015

Choose a reason for hiding this comment