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

Fix count_all_results when ordering #4212

Closed
wants to merge 2 commits into from
Closed

Fix count_all_results when ordering #4212

wants to merge 2 commits into from

Conversation

addbee
Copy link

@addbee addbee commented Nov 4, 2015

Fixes error introduced in v3 when there is a pending qr_order_by (used typically in pagination to get a count before results)

Method adapted from v2 DB_active_rec line 911

Fixes error introduced in v3 when there is a pending qr_order_by (used typically in pagination to get a count before results)
@narfbg
Copy link
Contributor

narfbg commented Nov 4, 2015

You're saying it fixes an error, but you're not saying what error that is ...

@addbee
Copy link
Author

addbee commented Nov 4, 2015

Using the sqlsrv driver, query builder generates the following query for count_all_results

SELECT COUNT(*) AS "numrows" FROM "tenants" LEFT JOIN "properties" "p" ON "p"."id" = "tenants"."property_id" WHERE "tenants"."deleted_date" =0 AND "tenants"."company_id" = '72' ORDER BY "tenants"."property_id" ASC

In SQL server, this generates the error

Column "tenants.property_id" is invalid in the ORDER BY clause because it is not contained in either an aggregate function or the GROUP BY clause

In the previous version of CodeIgniter, the order by clause was omitted during compilation of the count_all_results query, to avoid this error.

MySQL seems more forgiving of this issue, but omitting the ORDER BY for count_all_results will not change the output for other database drivers

narfbg added a commit that referenced this pull request Nov 4, 2015
@narfbg narfbg added the Bug label Nov 4, 2015
@narfbg narfbg added this to the 3.0.4 milestone Nov 4, 2015
@narfbg narfbg closed this Nov 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants