@test this fixes the fatal error, thanks @Hackwar

Agree with the PR. That check doesn't make much sense there.

@test - thank you!

RTC

Sorry but I am removing the RTC for now.

This code was introduced in #5416 to fix an issue. Does the change proposed here break that again?

Please provide a link to an extension that is effected by #5416 so that others can check

restarted travis checks

@brianteeman every extension which wants to deactivate this counter ($options['countItems'] == 0) is affected, when the page is multilingual.

Was my comment so hard to understand?

Does the change proposed here break that again?

Please provide a link to an extension that is effected by #5416 so that others can check

Is the code so complicated to read, @brianteeman ?
No, it doesn't break that.

@brianteeman Looking at the code, it should not break the original issue. But feel free to validate.
There is no real need for the 3rd party extension, the current code is indeed wrong and needs fixing.

Strongly disagree with this PR: content flagged for the * language (All languages) will not be returned.
This was an old long-standing bug solved in #5416

Perhaps the best idea is to revert #5416 and implement it correctly?
(correctly == the extension itself have to set $options['countItems'] = 1, when it needs this option for multilingual, no fuzzy check in general)

@smz That check shouldn't have anything to do with that.

The problem with current code is that the counting is done when either multilanguage is enabled OR "countItems" is enabled. Which obviously is wrong.

@bakual: I'll have to check, but according to my memories it had...
Just test this PR in a multilingual environment with some article flagged for specific languages and some flagged for "All"

@smz then the bug is somewhere else, but THAT line is definitely wrong.

Before this get's dirty, breath deeply before posting comments, keeps the blood pressure lower.

@smz Just tested the original issue. Having a subcategory with 2 german, one english and one "all" article shows with a count of 2 (1 english + 1 all) when in english language. Looks correct.

@Bakual
You should also test "List categories" with some categories being flagged for "German" and others for "All"

Same behaviour with or without patch. Actually it's a unpexpected behaviour as it shows the german subcategory in my english page. But that's unrelated to this PR.

without your component to test I can not test as you wont provide it then I wont test
good luck

@Bakual

Actually it's a unpexpected behaviour as it shows the german subcategory in my english page. But that's unrelated to this PR.

It wouldn't do that without this PR...

It wouldn't do that without this PR...

that error is there with and without this PR. So it's unrelated.

OK: time for real testing from my part. Will do. Of course I cannot test the issue this solves as we don't have access to the affected component. Only regressions will be tested.

One more consideration: in https://github.com/joomla/joomla-cms/pull/5416/files#r33668622 @Hackwar describes the issue at hand as:

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.

I wondering (not affirming the contrary) if this is a correct implementation.

I wondering (not affirming the contrary) if this is a correct implementation.

It doesn't really matter. The counting (which needs to join over the content table) should only be performed when needed. Currently it's also done when multilanguage is enabled. Which makes no sense at all.

I'm sure there was a reason for that: will come back later when I'll have finished my regressions testing.

@smz since I wrote the code and implemented this option for exactly such a situation: Yes, it is a correct implementation

hmmm... after testing I have to agree that apparently there is no regression.

I still object on how this PR has been proposed, without any indication of what should have been tested: correct information should have called for testing with a "List All Categories" menu item, in a multilingual environment, with several categories flagged for specific languages and the "All" language, a mix of articles (again, flagged for specific languages and the "All" language), assigned to different categories also in unusual ways (i.e. languages flagged as "All" in specific languages categories and the inverse too).

Of course a real test of the solved issue can be performed only by the few fortunate who have access to the affected component (I'm assuming @bembelimen and @chmst had such access)

@Bakual
I cannot duplicate your issue with the "german subcategory in my english page.":

• Was that with "List All Categories"?
• Did you had the "Empty Categories" options set to "Hide"?
• Didn't you have any English or "All" content assigned to the German category?

Edit: Oh yes, and testing instruction should have called for the "# Articles in Category" option to be turned off for real testing, of course!

@smz yes, it's a commercial one

@bembelimen did you test this PR, just to make sure we have someone to blame if it goes south ;-)

@smz as if big testing instructions in this project make a difference. I've written PRs with pages of instructions on how to test and short instructions on how to test and most of them waited month and years to be processed, while other PRs without any testing instructions and without any testing at all, have been merged in a matter of half an hour. And more than once failed miserably afterwards.

@rdeutz I tested the patch for the issues @Hackwar described:
1. before path: in a multilingual page, the count-clause will be called every time, regardless of the parameter "countItems", after patch the check take care for it
2. before patch: there was a error, when there is no catid + multilinguale page with no way to prevent it, after the patch the error disappeared, if "countItems" is not 1/true

Tested with success.
This patch solves also following issue with Phoca Guestbook:
PhocaGuestbook not working after Joomla update to 3.4.2

P.S.: From my point of view this PR solves a fatal error and so should be 1. merged ASAP, and 2. result in a hotfix (i.e. a 3.4.3).
It is not acceptable that with 3.4.2 (i.e. without this patch here) components stop to work.

@smz - yes, I have access

FYI:
This will be merged into 3.4.3 which will be a hotfix.
Today some other regression was found, so we want to have them fixed and included as well.

I want to express my deepest and most sincere apologies to all those who were affected by this issue that I introduced in #5416

Furthermore, I want apology for having being initially stubborn about that piece of code being necessary: I should had checked before talking.

I really don't know what I had in my mind when I coded that line... Murphy should had been standing at my shoulders , or maybe I was just too for coding...

@smz Well, looking at your profile picture, I think you maybe just were diving too deep into it, so maybe you just had forgotten to use an oxigen bottle ;-)

BTW, nice to see you around, Richard!

you just had forgotten to use an oxigen bottle

/start smart ass offtopic comment
Being a diver myself, I really hope he didn't use an oxigen bottle to dive. That gets deadly quite fast We usually dive with regular air.
/end smart ass offtopic comment

Oh yeah! Never at more than 6m on oxygen!

@Bakual
/begin ugly bad joke
What you say is true for water ... but not for some of the Joomla code. There you need pure oxigen ... or better laughing gas (nitrous oxide).
/end ugly bad joke

@smz No need to apologise. This can happen to everyone of us. However, it shows that our reliance on 2 tests to merge something, is not enough and we need a code review for every PR.

We usually review things before merge. But even reviewing can fail. Especially if you're not familiar with the code in question.
And getting familiar with every code before reviewing is to time consuming for an unpaid position.

So in the end, there is always a small risk in introducing bad code.

Thanks for your words, Hannes: they are encouraging...

... we need a code review for every PR

Yes, I agree with that, and since sometime I'm thinking about proposing the formation of a "Test Squad" and/or "Code Review Squad"...

Another (probably not very much shared) opinion I have, is that we should merge PRs as soon as possible and issue patch releases very frequently (once a week or once a fortnight...).

At the same time we could modify the Joomla Update component with an option to ignore such patch releases (maybe even by default)

Minor releases too should be more frequent: even if not significant new features have been released, when a significant corpus of fixes have been collected (say once every three months, or something like that), they could be incorporated in a minor release.

With the above scheduling 3rd party developers would be more motivated at checking eventual regressions with their extensions.

And of course RC for minor releases should be more long-lived and iterated, to give the time to check and fix regressions.

At the end of the day - that is why beta and RC releases are made so that
people will test - especially when an issue occurs with an extension or
template that is not part of the core.

On 2 July 2015 at 16:52, Thomas Hunziker notifications@github.com wrote:

We usually review things before merge. But even reviewing can fail.
Especially if you're not familiar with the code in question.
And getting familiar with every code before reviewing is to time consuming
for an unpaid position.

So in the end, there is always a small risk in introducing bad code.

—
Reply to this email directly or view it on GitHub
#7303 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

RC was about a week or even more. Still, regressions are only found after release. I don't think making more RCs or longer changes that.

To "speak with straight tongue", what will change things is if 3rd party would actually test RCs with more stringent tests (which will require quite more than a week IMHO), but we can't point guns at nobody's head...

So, releasing patch versions more frequently will help at least at not having a lot of people (different 3rd parties and their customers) angry for different reasons (different fuck-ups) at the same time...

So, releasing patch versions more frequently will help at least at not having a lot of people (different 3rd parties and their customers) angry for different reasons (different fuck-ups) at the same time...

I think we all agree with more frequently releasing :)

I got "1054 Unknown column 'i.language' in 'on clause'" for hwd Mediashare in Joomla 373. I checked lines in categories.php. Same of patch. I didn't understand.

Looks like your database is faulty. If you think there is a bug, please open a new issue. If you need help, go to the Joomla forum. But you wont get much help in a closed pull request.