? Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
1 Jul 2015

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.

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
5.00

avatar Hackwar Hackwar - open - 1 Jul 2015
avatar Hackwar Hackwar - change - 1 Jul 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 1 Jul 2015
Labels Added: ?
avatar bembelimen
bembelimen - comment - 1 Jul 2015

@test this fixes the fatal error, thanks @Hackwar

avatar Bakual
Bakual - comment - 1 Jul 2015

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

avatar chmst
chmst - comment - 1 Jul 2015

@test - thank you!

avatar zero-24 zero-24 - alter_testresult - 1 Jul 2015 - bembelimen: Tested successfully
avatar zero-24 zero-24 - alter_testresult - 1 Jul 2015 - chmst: Tested successfully
avatar zero-24 zero-24 - change - 1 Jul 2015
Category Multilanguage
avatar zero-24 zero-24 - change - 1 Jul 2015
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 1 Jul 2015

RTC

avatar zero-24 zero-24 - change - 1 Jul 2015
Labels Added: ?
avatar brianteeman
brianteeman - comment - 1 Jul 2015

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.

avatar brianteeman brianteeman - change - 1 Jul 2015
Status Ready to Commit Pending
avatar rdeutz
rdeutz - comment - 1 Jul 2015

restarted travis checks

avatar brianteeman brianteeman - change - 1 Jul 2015
Labels Removed: ?
avatar bembelimen
bembelimen - comment - 1 Jul 2015

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

avatar brianteeman
brianteeman - comment - 1 Jul 2015

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

avatar Hackwar
Hackwar - comment - 1 Jul 2015

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

avatar Bakual
Bakual - comment - 1 Jul 2015

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

avatar smz
smz - comment - 1 Jul 2015

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

avatar bembelimen
bembelimen - comment - 1 Jul 2015

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)

avatar Bakual
Bakual - comment - 1 Jul 2015

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

avatar Bakual
Bakual - comment - 1 Jul 2015

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

avatar smz
smz - comment - 1 Jul 2015

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

avatar Hackwar
Hackwar - comment - 1 Jul 2015

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

avatar rdeutz
rdeutz - comment - 1 Jul 2015

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

avatar Bakual
Bakual - comment - 1 Jul 2015

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

avatar Bakual Bakual - change - 1 Jul 2015
Labels Added: ?
avatar smz
smz - comment - 1 Jul 2015

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

avatar Bakual
Bakual - comment - 1 Jul 2015

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.

avatar brianteeman
brianteeman - comment - 1 Jul 2015

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.

avatar smz
smz - comment - 1 Jul 2015

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

avatar Bakual
Bakual - comment - 1 Jul 2015

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

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

avatar smz
smz - comment - 1 Jul 2015

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.

avatar Bakual
Bakual - comment - 1 Jul 2015

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.

avatar smz
smz - comment - 1 Jul 2015

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

avatar Hackwar
Hackwar - comment - 1 Jul 2015

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

avatar zero-24 zero-24 - change - 1 Jul 2015
Status Pending Ready to Commit
avatar smz
smz - comment - 1 Jul 2015

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!

avatar bembelimen
bembelimen - comment - 1 Jul 2015

@smz yes, it's a commercial one

avatar rdeutz
rdeutz - comment - 1 Jul 2015

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

avatar Hackwar
Hackwar - comment - 1 Jul 2015

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

avatar bembelimen
bembelimen - comment - 1 Jul 2015

@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

avatar richard67
richard67 - comment - 1 Jul 2015

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.

avatar richard67 richard67 - test_item - 1 Jul 2015 - Tested successfully
avatar richard67
richard67 - comment - 1 Jul 2015

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.

avatar chmst
chmst - comment - 1 Jul 2015

@smz - yes, I have access

avatar Bakual Bakual - change - 2 Jul 2015
Milestone Added:
avatar Bakual
Bakual - comment - 2 Jul 2015

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.

avatar zero-24 zero-24 - close - 2 Jul 2015
avatar mbabker mbabker - reference | 01c3102 - 2 Jul 15
avatar mbabker mbabker - merge - 2 Jul 2015
avatar mbabker mbabker - close - 2 Jul 2015
avatar mbabker mbabker - change - 2 Jul 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-07-02 13:02:07
Closed_By mbabker
avatar mbabker mbabker - close - 2 Jul 2015
avatar smz
smz - comment - 2 Jul 2015

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

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 :smiling_imp:, or maybe I was just too :sleeping: for coding...

avatar richard67
richard67 - comment - 2 Jul 2015

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

avatar smz
smz - comment - 2 Jul 2015

:smile:

BTW, nice to see you around, Richard!

avatar Bakual
Bakual - comment - 2 Jul 2015

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 :smile: We usually dive with regular air.
/end smart ass offtopic comment

avatar smz
smz - comment - 2 Jul 2015

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

avatar richard67
richard67 - comment - 2 Jul 2015

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

avatar Hackwar
Hackwar - comment - 2 Jul 2015

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

avatar Bakual
Bakual - comment - 2 Jul 2015

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.

avatar smz
smz - comment - 2 Jul 2015

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.

avatar brianteeman
brianteeman - comment - 2 Jul 2015

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/

avatar Bakual
Bakual - comment - 2 Jul 2015

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.

avatar smz
smz - comment - 2 Jul 2015

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

avatar Bakual
Bakual - comment - 2 Jul 2015

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

avatar Hackwar Hackwar - head_ref_deleted - 3 Jul 2015
avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?
avatar burgutex
burgutex - comment - 23 Nov 2017

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.

avatar Hackwar
Hackwar - comment - 23 Nov 2017

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.

Add a Comment

Login with GitHub to post a comment