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

Hackwar · 2015-07-01T10:35:01Z

With the changes in #5416, the option to disable the count-join in JCategories is permanently enabled, making extensions that don't use this feature and for example don't have a catid field in their table, fail. Just because you have a multilang-site, the option to disable the counting should NOT be overriden.

bembelimen · 2015-07-01T10:43:30Z

@test this fixes the fatal error, thanks @Hackwar

Bakual · 2015-07-01T10:48:17Z

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

chmst · 2015-07-01T10:48:49Z

@test - thank you!

zero-24 · 2015-07-01T10:54:36Z

RTC

brianteeman · 2015-07-01T10:59:45Z

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

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

rdeutz · 2015-07-01T11:02:21Z

restarted travis checks

bembelimen · 2015-07-01T11:10:53Z

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

brianteeman · 2015-07-01T11:14:12Z

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

Hackwar · 2015-07-01T11:21:11Z

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

Bakual · 2015-07-01T11:21:13Z

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

smz · 2015-07-01T11:23:07Z

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

bembelimen · 2015-07-01T11:26:11Z

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)

Bakual · 2015-07-01T11:26:33Z

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

Bakual · 2015-07-01T11:27:52Z

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

smz · 2015-07-01T11:28:09Z

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

Hackwar · 2015-07-01T11:29:20Z

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

rdeutz · 2015-07-01T11:29:33Z

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

Bakual · 2015-07-01T11:33:50Z

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

smz · 2015-07-01T11:36:58Z

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

Bakual · 2015-07-01T11:44:30Z

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.

brianteeman · 2015-07-01T11:49:38Z

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

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

smz · 2015-07-01T11:52:51Z

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

Bakual · 2015-07-01T11:55:43Z

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

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

smz · 2015-07-01T12:00:39Z

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.

Bakual · 2015-07-01T12:05:50Z

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.

smz · 2015-07-01T12:06:59Z

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

Hackwar · 2015-07-01T12:13:31Z

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

smz · 2015-07-01T13:09:37Z

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!

bembelimen · 2015-07-01T13:11:07Z

@smz yes, it's a commercial one

rdeutz · 2015-07-01T13:12:44Z

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

Hackwar · 2015-07-01T13:14:51Z

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

bembelimen · 2015-07-01T13:17:46Z

@rdeutz I tested the patch for the issues @Hackwar described:

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

richard67 · 2015-07-01T15:42:02Z

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

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

richard67 · 2015-07-01T20:43:32Z

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.

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

chmst · 2015-07-01T21:37:54Z

@smz - yes, I have access

Bakual · 2015-07-02T08:44:20Z

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.

smz · 2015-07-02T13:35:01Z

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

richard67 · 2015-07-02T13:42:32Z

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

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

smz · 2015-07-02T13:43:42Z

😄

BTW, nice to see you around, Richard!

Bakual · 2015-07-02T13:57:09Z

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

smz · 2015-07-02T13:58:46Z

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

richard67 · 2015-07-02T14:05:50Z

@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

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

Hackwar · 2015-07-02T15:40:34Z

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

Bakual · 2015-07-02T15:52:04Z

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.

smz · 2015-07-02T15:56:52Z

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.

brianteeman · 2015-07-02T15:58:47Z

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 [email protected] 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/

Bakual · 2015-07-02T16:12:38Z

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.

smz · 2015-07-02T16:20:58Z

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

Bakual · 2015-07-02T17:53:22Z

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

burgutex · 2017-11-23T13:31:46Z

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.

Hackwar · 2017-11-23T14:00:33Z

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.

Having a multilang-site should not override the option of JCategories

ca0c2f9

joomla-cms-bot added the PR-staging label Jul 1, 2015

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

brianteeman removed the RTC This Pull Request is Ready To Commit label Jul 1, 2015

Bakual mentioned this pull request Jul 1, 2015

Articles count in "List all categories" #5416

Closed

Bakual added the RTC This Pull Request is Ready To Commit label Jul 1, 2015

Bakual added this to the Joomla! 3.4.3 milestone Jul 2, 2015

Hackwar deleted the patch-59 branch July 3, 2015 12:30

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

Uh oh!

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

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

Uh oh!

Conversation

Hackwar commented Jul 1, 2015

Uh oh!

bembelimen commented Jul 1, 2015

Uh oh!

Bakual commented Jul 1, 2015

Uh oh!

chmst commented Jul 1, 2015

Uh oh!

zero-24 commented Jul 1, 2015

Uh oh!

brianteeman commented Jul 1, 2015

Uh oh!

rdeutz commented Jul 1, 2015

Uh oh!

bembelimen commented Jul 1, 2015

Uh oh!

brianteeman commented Jul 1, 2015

Uh oh!

Hackwar commented Jul 1, 2015

Uh oh!

Bakual commented Jul 1, 2015

Uh oh!

smz commented Jul 1, 2015

Uh oh!

bembelimen commented Jul 1, 2015

Uh oh!

Bakual commented Jul 1, 2015

Uh oh!

Bakual commented Jul 1, 2015

Uh oh!

smz commented Jul 1, 2015

Uh oh!

Hackwar commented Jul 1, 2015

Uh oh!

rdeutz commented Jul 1, 2015

Uh oh!

Bakual commented Jul 1, 2015

Uh oh!

smz commented Jul 1, 2015

Uh oh!

Bakual commented Jul 1, 2015

Uh oh!

brianteeman commented Jul 1, 2015

Uh oh!

smz commented Jul 1, 2015

Uh oh!

Bakual commented Jul 1, 2015

Uh oh!

smz commented Jul 1, 2015

Uh oh!

Bakual commented Jul 1, 2015

Uh oh!

smz commented Jul 1, 2015

Uh oh!

Hackwar commented Jul 1, 2015

Uh oh!

smz commented Jul 1, 2015

Uh oh!

bembelimen commented Jul 1, 2015

Uh oh!

rdeutz commented Jul 1, 2015

Uh oh!

Hackwar commented Jul 1, 2015

Uh oh!

bembelimen commented Jul 1, 2015

Uh oh!

richard67 commented Jul 1, 2015

Uh oh!

richard67 commented Jul 1, 2015

Uh oh!

chmst commented Jul 1, 2015

Uh oh!

Bakual commented Jul 2, 2015

Uh oh!

smz commented Jul 2, 2015