Skip to content
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.

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

Issue #135: CActiveRecord::count() respects group and having #2167

Conversation

klimov-paul
Copy link
Member

Fixes #135
Now "CActiveRecord::count()" respects group and having.
Unit tests for "CActiveRecord" has been updated. Tests have been passed successfully.

@samdark
Copy link
Member

samdark commented Mar 4, 2013

Was it tested with non-MySQL? Especially SQLite.

@ghost ghost assigned samdark Mar 4, 2013
@klimov-paul
Copy link
Member Author

Test case "CActiveRecordTest" uses SQLite memory connection (line 19).

@klimov-paul
Copy link
Member Author

This pull request adds logic from "CDbCommandBuilder::createCountCommand()" to "CActiveFinder::count()".

@samdark
Copy link
Member

samdark commented Mar 4, 2013

Looks OK for me.

@yiisoft/core-developers any comments?

@qiangxue
Copy link
Member

qiangxue commented Mar 4, 2013

This could potentially break BC.
Also, it is not semantically right if you do count with GROUP BY because you end up with returning the first count.

@klimov-paul
Copy link
Member Author

In this fix I perform the count via sub query, which means count of the result of other query.
Group by will not affect the count result anyhow.
Also I can not see how this can break a BC, @qiangxue please provide an example of what exaclty you aware.

Actually the solution I suggest just copies the approach of "CDbCommandBuilder::createCountCommand()". Back there none has any complains.

@qiangxue
Copy link
Member

qiangxue commented Mar 4, 2013

By BC, I mean users' existing code may rely on the fact that group-by/having is ignored in the old code.
Since you are copying the code from CDbCommandBuilder::createCountCommand(), this probably isn't a big concern then.

@klimov-paul
Copy link
Member Author

As for BC: may be you should tell @zhongjq and @alexandernst the issue they request actually breaks a BC and just close the issue #135.

@samdark
Copy link
Member

samdark commented Mar 4, 2013

I think we should try merging it for the next RC.

@acorncom
Copy link
Contributor

acorncom commented Mar 5, 2013

It seems like most people who want group by/having to work with their code show up here to report bugs :-) There are regular reports of problems. Having it actually fixed and then clearly documented as one of the more major fixes would eliminate the BC concern, wouldn't it?

@samdark
Copy link
Member

samdark commented Mar 5, 2013

Should be OK but we definitely need to mention it in the UPGRADE.

samdark added a commit that referenced this pull request Mar 5, 2013
…ect-group-and-having

Issue #135: CActiveRecord::count() respects group and having
@samdark samdark merged commit 1279019 into yiisoft:master Mar 5, 2013
@samdark
Copy link
Member

samdark commented Mar 5, 2013

0eec059

@samdark
Copy link
Member

samdark commented Mar 5, 2013

Thanks for working on this one.

@simontong
Copy link

You forgot to add the params (bindParams()) to the criteria

@klimov-paul
Copy link
Member Author

@simontong, it is already done by #2230

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Difficulties with HAVING clauses in AR
5 participants