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

This should fix MySQL 5.7 issues caused by ONLY_FULL_GROUP_BY #5973

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 32 additions & 9 deletions lib/Doctrine/ORM/Tools/Pagination/LimitSubqueryOutputWalker.php
Original file line number Diff line number Diff line change
Expand Up @@ -348,33 +348,56 @@ private function addMissingItemsFromOrderByToSelect(SelectStatement $AST)
/**
* Generates new SQL for statements with an order by clause
*
* @param array $sqlIdentifier
* @param string $innerSql
* @param string $sql
* @param OrderByClause $orderByClause
* @param array $sqlIdentifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert

* @param string $innerSql
* @param string $sql
* @param OrderByClause $orderByClause
*
* @return string
*/
private function preserveSqlOrdering(array $sqlIdentifier, $innerSql, $sql, $orderByClause)
{
private function preserveSqlOrdering(array $sqlIdentifier, $innerSql, $sql, $orderByClause) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

// If the sql statement has an order by clause, we need to wrap it in a new select distinct
// statement
if (! $orderByClause instanceof OrderByClause) {
if (!$orderByClause instanceof OrderByClause) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert

return $sql;
}

// Rebuild the order by clause to work in the scope of the new select statement
/* @var array $orderBy an array of rebuilt order by items */
$orderBy = $this->rebuildOrderByClauseForOuterScope($orderByClause);
$orderByFields = str_replace([' DESC', ' ASC'], ['', ''], $orderBy);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may have something like ORDER BY ASCII ASC - that would be replaced with ORDER BYII here - no-go.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calls for additional test scenarios for both descending and ascending sorting clauses

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to match a series of ((\S+)\s+(ASC|DESC)\s*,?)* here (or similar)


$innerSqlIdentifier = [];

foreach ($orderByFields as $k => $v) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$k and $v are not good names

// remove fields that are selected by identifiers,
// if those are ordered by in the query
if (in_array($v, $sqlIdentifier)) {
unset($orderByFields[$k]);
}
}

foreach ($sqlIdentifier as $k => $v) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

$innerSqlIdentifier[$k] = 'dctrn_result_inner.' . $v;
$sqlIdentifier[$k] = 'dctrn_result.' . $v;
}

// add the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment incomplete?

foreach ($orderByFields as $k => $v) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: $k, $v are not good names

$innerSqlIdentifier[$k] = 'dctrn_result_inner.' . $v;
}

// Build the select distinct statement
$sql = sprintf(
'SELECT DISTINCT %s FROM (%s) dctrn_result ORDER BY %s',
implode(', ', $sqlIdentifier),
'SELECT DISTINCT %s FROM (%s) dctrn_result_inner ORDER BY %s',
implode(', ', $innerSqlIdentifier),
$innerSql,
implode(', ', $orderBy)
);

// now only select distinct identifier
$sql = sprintf('SELECT DISTINCT %s FROM (%s) dctrn_result', implode(', ', $sqlIdentifier), $sql);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return directly


return $sql;
}

Expand Down