Skip to content

[WIP] DDC-2622 - SQLServer expects the `OVER` clause to only include columns from the `FROM` clause #371

Closed
wants to merge 1 commit into from

2 participants

@Ocramius
Doctrine member

DDC-2622 - SQLServer expects the OVER clause to only include columns from the FROM clause

As of http://technet.microsoft.com/en-us/library/ms189461.aspx :

`order_by_expression:

Specifies a column or expression on which to sort. order_by_expression can only refer to columns made available by the FROM clause. An integer cannot be specified to represent a column name or alias.

That basically means that table aliases used to fetch the selected columns should NOT be included in the OVER clause.

This PR is required for doctrine/doctrine2#789

@Ocramius Ocramius DDC-2662 - SQLServer expects the `OVER` clause to only include column…
…s from the `FROM` clause

As of http://technet.microsoft.com/en-us/library/ms189461.aspx :

`order_by_expression:

 > Specifies a column or expression on which to sort. order_by_expression can only refer to columns made available by the FROM clause. An integer cannot be specified to represent a column name or alias.

That basically means that table aliases used to fetch the selected columns should NOT be included in the `OVER` clause.
1bc6576
@Ocramius
Doctrine member

@flip111 I'd need your input/feedback on this

@flip111
flip111 commented Sep 16, 2013

The hotfix and commit has DDC-2662, the bug report has DDC-2622.

@Ocramius
Doctrine member

@flip111 thanks for noticing, I'll get that fixed!

@flip111

The code tries to always iterate over $orderbyColumns, which can be a problem if ORDER BY is not used (though i didn't test if this is a problem. Also the statement now outputs the column name; it should output the column alias. I have wrote a little snippet, please have a look if this meets Doctrine coding standards and is generally a good strategy to attempt to fix this bug.

//Find alias for each colum used in ORDER BY
if ( ! empty($orderbyColumns)) {
        foreach ($orderbyColumns as $column) {
                $pattern    = sprintf('/%s\.(?:%s)\s*(?:AS)?\s*(?<alias>[^,\s\)]*)/i', $column['table'], $column['column']);

                // Is the column selected ?
                if (preg_match($pattern, $query, $matches)) {
                    // yes
                    $overColumn = $matches['alias'];
                } else {
                    // no
                    $overColumn = $column['column'];
                }

                if (isset($column['sort'])) {
                        $overColumn .= ' ' . $column['sort'];
                }

                $overColumns[] = $overColumn;
        }
}
@Ocramius
Doctrine member

Closing since the branch name is messed up. See #374

@Ocramius Ocramius closed this Sep 16, 2013
@Ocramius Ocramius deleted the hotfix/DDC-2662 branch Sep 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.