Offer a modern MySQL Adapter [MySQL]

[Incognito]

ONLY_FULL_GROUP_BY is now an optional flag for MySQL connections, and disabled by default in MySQL 5.7+. I think we should replace the adapter (deprecate the existing one, introduce a new modern one) because MySQL changed the behaviour of their software.

If this flag is not added to connections on newer versions of MySQL it will break queries.

[Incognito]

Related

Propel2/src/Propel/Runtime/Adapter/Pdo/MysqlAdapter.php

Lines 140 to 168 in 0dbf800

	public function getGroupBy(Criteria $criteria)
	{
	$groupBy = $criteria->getGroupByColumns();
	if ($groupBy) {
	// check if all selected columns are groupBy'ed.
	$selected = $this->getPlainSelectedColumns($criteria);
	$asSelects = $criteria->getAsColumns();
	foreach ($selected as $colName) {
	if (!in_array($colName, $groupBy)) {
	// is a alias there that is grouped?
	if ($alias = array_search($colName, $asSelects)) {
	if (in_array($alias, $groupBy)) {
	continue; //yes, alias is selected.
	}
	}
	$groupBy[] = $colName;
	}
	}
	if ($groupBy) {
	return ' GROUP BY ' . implode(',', $groupBy);
	}
	}
	return '';
	}

[mringler]

I though about this a bit, when I had problems getting nested group by queries to work. I think it is correct that propel fails with an error for queries with nonaggregated columns. After all, those queries really are incorrect, and best Propel can do is to tell users that they need to fix it.
A huge problem is that nested group queries at the moment cannot be fixed by the user, because select() does not work (see PR), causing all table columns to end up in the select clause of the nested query. This makes nested queries pretty much unusable, and was a big problem in my project. But I don't think the problem lies with the adapter, when MySQL rejects bad queries, but with the query generator.

Looking at above code (I think it is from the Postgres Adapter, not MySQL), it tries to secretly fix the nonaggregated column issue by adding those columns to the group by statement. However, when it changes the group, it actually changes the result, and I don't think that this is intended. Simple example:

  SELECT id, author_id, count(*) AS numberOfBooks  -- "id" is not aggregated
  FROM book
  GROUP BY author_id

becomes

  SELECT id, author_id, count(*) AS numberOfBooks
  FROM book
  GROUP BY author_id, id  -- and now we group by a unique column, numberOfBooks will always be 1

So doing BookQuery::create()->groupByAuthorId()->find() gives you the exact same result as without the group by. Instead of an error, which I think would be the expected behavior. This is also a different results than what the query gave before, without the ONLY_FULL_GROUP_BY check.
If you actually wanted to correct the query, you would have to remove the nonaggregated columns. But that would mean that BookQuery does not return valid Book objects anymore.

Long story short:
In case of a wrong query, failing with a clean error is better than guessing how to fix it, but it is necessary that users can formulate the query correctly.
I think the approach taken in the Postgres adapter is actually a hack, that rather masks the problem than fixes it.
Removing nonaggregated columns would be a nice shortcut, but it breaks the object model in specific cases, so it is a no no.