Framework: 5.3 Query Builder count() Sql error - PR proposal

[davidrushton]

This might be related to #14997.

After upgrading to Laravel 5.3, I get the following Sql error.

Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1 (SQL: select count(*) as aggregate from `items` where `type` = product and `items`.`deleted_at` is null order by)

I think i've traced it down to the fact that in my repository, I call count() on an already created Builder instance which has an order by (to create a manual paginator for DataTables).

$query = $this->buildSearchQuery($type, $filters, $sort);
$total = $query->count();
...

The Query\Builder class has the following method:

public function aggregate($function, $columns = ['*'])
{
    $this->aggregate = compact('function', 'columns');

    $previousColumns = $this->columns;

    // We will also back up the select bindings since the select clause will be
    // removed when performing the aggregate function. Once the query is run
    // we will add the bindings back onto this query so they can get used.
    $previousSelectBindings = $this->bindings['select'];

    $this->bindings['select'] = [];
    $this->orders = [];

    $results = $this->get($columns);

    // Once we have executed the query, we will reset the aggregate property so
    // that more select queries can be executed against the database without
    // the aggregate value getting in the way when the grammar builds it.
    $this->aggregate = null;

    $this->columns = $previousColumns;

    $this->bindings['select'] = $previousSelectBindings;

    if (! $results->isEmpty()) {
        return array_change_key_case((array) $results[0])['aggregate'];
    }
}

I propose changing to the following, which works, using the existing paginator helper methods.

public function aggregate($function, $columns = ['*'])
{
    $this->aggregate = compact('function', 'columns');

    $this->backupFieldsForCount();

    $results = $this->get($columns);

    // Once we have executed the query, we will reset the aggregate property so
    // that more select queries can be executed against the database without
    // the aggregate value getting in the way when the grammar builds it.
    $this->aggregate = null;

    $this->restoreFieldsForCount();

    if (! $results->isEmpty()) {
        return array_change_key_case((array) $results[0])['aggregate'];
    }
}

Does anyone know if that will cause any problems with the count(), min(), max() or average() methods / queries?

David.

[themsaid]

That's not how the aggregate() method of the Query Builder looks like in 5.3! Are you extending it or something?

[themsaid]

This $this->orders = []; is the reason of your problem, not sure how it got there!

[davidrushton]

@themsaid Thanks for pointing that out. I don't know how that got in there either, but if I install 5.3 from composer from scratch I still get the issue

SQLSTATE[42000]: Syntax error or access violation: 1140 Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause (SQL: select count(*) as aggregate from `items` where `type` = product and `items`.`deleted_at` is null order by `created_at` desc)

The two fixes either seem to be:

1. In config/database.php

'strict' => false,

1. In Illuminate\Database\Query\Builder, updating the method to:

public function aggregate($function, $columns = ['*'])
{
    $this->aggregate = compact('function', 'columns');

    $this->backupFieldsForCount();

    $results = $this->get($columns);

    // Once we have executed the query, we will reset the aggregate property so
    // that more select queries can be executed against the database without
    // the aggregate value getting in the way when the grammar builds it.
    $this->aggregate = null;

    $this->restoreFieldsForCount();

    if (! $results->isEmpty()) {
        return array_change_key_case((array) $results[0])['aggregate'];
    }
}

Number 1 will work, but I wonder if number 2 would be suitable without breaking the other aggregates?

[themsaid]

This query

select count(*) as aggregate from `items` where `type` = product and `items`.`deleted_at` is null order by `created_at` desc

shouldn't have any problems in strict mode, I tested on MySQL though, not sure about MariaDB.

Can you please test using MySQL?

[davidrushton]

Curiously, if I run the query above manually (Sequel Pro) on MariaDB it works fine, it's just when it's being executed through Laravel it is failing.

Failing on MySQL 5.5.5-10.1.14-MariaDB (Valet)

Works fine through Laravel on MySQL 5.7.12 (Homestead)

[themsaid]

That's really confusing, it should never fail, can't think of a sql mode where this would fail :)

[themsaid]

The query generated by laravel is valid (MySQL 5.5.5 & 5.7.12) in strict mode, I believe there's some sort of misconfiguration in your database setup. I think you should just disable strict mode or check your engine configuration.

[helmut]

FYI - I hit this issue too with Laravel 5.3.26 using 10.1.14-MariaDB on PHP 7.0.7.

select count(*) as aggregate from `users` order by `email` asc

If I run that exact query in a DB::statement I get the exception below. If I run that in SequelPro I do not get the error. Disabling strict mode in database config fixes the issue. Perhaps SequelPro doesn't run in strict mode by default. Removing the order by fixes the issue.

QueryException in Connection.php line 769:
SQLSTATE[42000]: Syntax error or access violation: 1140 Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause (SQL: select count(*) as aggregate from `users` order by `email` asc)

Perhaps you are not supposed to order by when counting in strict mode? I guess it makes sense not to sort as it has no bearing on the count. My use case is that I am building a query in one part of the application and then a separate system uses the query builder instance twice - performing a count for total and then performing a query returning limited results.

[narek-king]

'strict' => false, worked for me.
Thanks

[dle-sweethearts]

'strict' => false worked for me too

[arielmorry]

'strict' => false, worked for me as well.
thanks.

[craighooghiem]

Same issue, same solution.
Had a few issues with the default strict = true.

[Remo]

Sorry for commenting on such an old issue, but I think I can add some more information. It might be useful as it still applies today. The problem is caused by the sql mode ONLY_FULL_GROUP_BY.

The surprising thing is that it only happens with MariaDB, not MySQL. I'm currently running MariaDB 10.2.24 and get the above error, when running this query:

SET sql_mode = 'ONLY_FULL_GROUP_BY';

select count(*) as aggregate from `forum_messages` where `forum_messages`.`topic_id` = 20 and `forum_messages`.`topic_id` is not null order by `created_at` asc

The same query on MySQL 8.0.2 runs just fine!