Doctrine 1
  1. Doctrine 1
  2. DC-897

Pager ignores default model hasMany ORDER BY statements, caused by getLimitSubquery ignoring same

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.2.3
    • Fix Version/s: None
    • Component/s: Pager
    • Labels:
      None

      Description

      Our model configuration includes several hasMany statements, for example:

      $this->hasMany('Subcategory as Subcategories', array(
      'refClass' => 'SubcategoryTone',
      'local' => 'tone_id',
      'foreign' => 'subcategory_id',
      'cascade' => array('delete'),
      'orderBy' => 'order_id',
      ));

      We noticed that the ORDER BY directive worked just fine with a normal query, but the order by was being ignored when we fed it into the Pager.

      For example:
      $aa = $t->execute(array(), Doctrine_Core::HYDRATE_ARRAY);
      var_dump($aa[4]);

      $pager = new Doctrine_Pager($t, $currentPage, $resultsPerPage);
      $bb = $pager->execute(array(), Doctrine_Core::HYDRATE_ARRAY);

      var_dump($bb[4]);

      These two var_dumps would give different results because the ORDER BY is ignored by the limit subquery in the pager.

        Activity

        Hide
        Andrew Eross added a comment -

        I've also found a fix for the issue (thanks to George over here for finding the location of the problem) ... we found that simply moving the ORDER BY generation code inside of buildSqlQuery() to be ABOVE the if block containing getLimitSubquery() resolves the issue.

        We're not super familiar with the Doctrine code-base, so everything looks to work fine after moving the code block, and it fixes the issue, but would love to hear if this is a real fix.

        diff from 1.2.3 via our SVN:

        Index: Query.php
        ===================================================================
        — Query.php (revision 1120)
        +++ Query.php (working copy)
        @@ -1256,7 +1256,46 @@
        $this->_sqlParts['where'][] = '(' . $string . ')';
        }
        }
        +
        + // Fix the orderbys so we only have one orderby per value
        + foreach ($this->_sqlParts['orderby'] as $k => $orderBy) {
        + $e = explode(', ', $orderBy);
        + unset($this->_sqlParts['orderby'][$k]);
        + foreach ($e as $v)

        { + $this->_sqlParts['orderby'][] = $v; + }

        + }

        + // Add the default orderBy statements defined in the relationships and table classes
        + // Only do this for SELECT queries
        + if ($this->_type === self::SELECT) {
        + foreach ($this->_queryComponents as $alias => $map) {
        + $sqlAlias = $this->getSqlTableAlias($alias);
        + if (isset($map['relation'])) {
        + $orderBy = $map['relation']->getOrderByStatement($sqlAlias, true);
        + if ($orderBy == $map['relation']['orderBy']) {
        + if (isset($map['ref']))

        { + $orderBy = $map['relation']['refTable']->processOrderBy($sqlAlias, $map['relation']['orderBy'], true); + }

        else

        { + $orderBy = null; + }

        + }
        + } else

        { + $orderBy = $map['table']->getOrderByStatement($sqlAlias, true); + }

        +
        + if ($orderBy) {
        + $e = explode(',', $orderBy);
        + $e = array_map('trim', $e);
        + foreach ($e as $v) {
        + if ( ! in_array($v, $this->_sqlParts['orderby']))

        { + $this->_sqlParts['orderby'][] = $v; + }

        + }
        + }
        + }
        + }
        +
        $modifyLimit = true;
        $limitSubquerySql = '';

        @@ -1307,47 +1346,8 @@

        $q .= ' WHERE ' . $limitSubquerySql . $where;
        // . (($limitSubquerySql == '' && count($this->_sqlParts['where']) == 1) ? substr($where, 1, -1) : $where);

        • }
          + }
        • // Fix the orderbys so we only have one orderby per value
        • foreach ($this->_sqlParts['orderby'] as $k => $orderBy) {
        • $e = explode(', ', $orderBy);
        • unset($this->_sqlParts['orderby'][$k]);
        • foreach ($e as $v) { - $this->_sqlParts['orderby'][] = $v; - }
        • }
          -
        • // Add the default orderBy statements defined in the relationships and table classes
        • // Only do this for SELECT queries
        • if ($this->_type === self::SELECT) {
        • foreach ($this->_queryComponents as $alias => $map) {
        • $sqlAlias = $this->getSqlTableAlias($alias);
        • if (isset($map['relation'])) {
        • $orderBy = $map['relation']->getOrderByStatement($sqlAlias, true);
        • if ($orderBy == $map['relation']['orderBy']) {
        • if (isset($map['ref'])) { - $orderBy = $map['relation']['refTable']->processOrderBy($sqlAlias, $map['relation']['orderBy'], true); - }

          else

          { - $orderBy = null; - }
        • }
        • } else { - $orderBy = $map['table']->getOrderByStatement($sqlAlias, true); - }

          -

        • if ($orderBy) {
        • $e = explode(',', $orderBy);
        • $e = array_map('trim', $e);
        • foreach ($e as $v) {
        • if ( ! in_array($v, $this->_sqlParts['orderby'])) { - $this->_sqlParts['orderby'][] = $v; - }
        • }
        • }
        • }
        • }
          -
          $q .= ( ! empty($this->_sqlParts['groupby'])) ? ' GROUP BY ' . implode(', ', $this->_sqlParts['groupby']) : '';
          $q .= ( ! empty($this->_sqlParts['having'])) ? ' HAVING ' . implode(' AND ', $this->_sqlParts['having']): '';
          $q .= ( ! empty($this->_sqlParts['orderby'])) ? ' ORDER BY ' . implode(', ', $this->_sqlParts['orderby']) : '';
          @@ -1396,7 +1396,7 @@
          $subquery = 'SELECT DISTINCT ';
          }
          $subquery .= $this->_conn->quoteIdentifier($primaryKey);
          -
          +
          // pgsql & oracle need the order by fields to be preserved in select clause
          if ($driverName == 'pgsql' || $driverName == 'oracle' || $driverName == 'oci' || $driverName == 'mssql' || $driverName == 'odbc') {
          foreach ($this->_sqlParts['orderby'] as $part)
          Unknown macro: {@@ -1420,7 +1420,7 @@ // don't add primarykey column (its already in the select clause) if ($part !== $primaryKey) { $subquery .= ', ' . $partOriginal; - }+ }


          }
          }
          }

        Property changes on: Query.php
        ___________________________________________________________________
        Deleted: svn:keywords

        • Id Revision
          Deleted: svn:eol-style
        • LF
        Show
        Andrew Eross added a comment - I've also found a fix for the issue (thanks to George over here for finding the location of the problem) ... we found that simply moving the ORDER BY generation code inside of buildSqlQuery() to be ABOVE the if block containing getLimitSubquery() resolves the issue. We're not super familiar with the Doctrine code-base, so everything looks to work fine after moving the code block, and it fixes the issue, but would love to hear if this is a real fix. diff from 1.2.3 via our SVN: Index: Query.php =================================================================== — Query.php (revision 1120) +++ Query.php (working copy) @@ -1256,7 +1256,46 @@ $this->_sqlParts ['where'] [] = '(' . $string . ')'; } } + + // Fix the orderbys so we only have one orderby per value + foreach ($this->_sqlParts ['orderby'] as $k => $orderBy) { + $e = explode(', ', $orderBy); + unset($this->_sqlParts ['orderby'] [$k] ); + foreach ($e as $v) { + $this->_sqlParts['orderby'][] = $v; + } + } + // Add the default orderBy statements defined in the relationships and table classes + // Only do this for SELECT queries + if ($this->_type === self::SELECT) { + foreach ($this->_queryComponents as $alias => $map) { + $sqlAlias = $this->getSqlTableAlias($alias); + if (isset($map ['relation'] )) { + $orderBy = $map ['relation'] ->getOrderByStatement($sqlAlias, true); + if ($orderBy == $map ['relation'] ['orderBy'] ) { + if (isset($map ['ref'] )) { + $orderBy = $map['relation']['refTable']->processOrderBy($sqlAlias, $map['relation']['orderBy'], true); + } else { + $orderBy = null; + } + } + } else { + $orderBy = $map['table']->getOrderByStatement($sqlAlias, true); + } + + if ($orderBy) { + $e = explode(',', $orderBy); + $e = array_map('trim', $e); + foreach ($e as $v) { + if ( ! in_array($v, $this->_sqlParts ['orderby'] )) { + $this->_sqlParts['orderby'][] = $v; + } + } + } + } + } + $modifyLimit = true; $limitSubquerySql = ''; @@ -1307,47 +1346,8 @@ $q .= ' WHERE ' . $limitSubquerySql . $where; // . (($limitSubquerySql == '' && count($this->_sqlParts ['where'] ) == 1) ? substr($where, 1, -1) : $where); } + } // Fix the orderbys so we only have one orderby per value foreach ($this->_sqlParts ['orderby'] as $k => $orderBy) { $e = explode(', ', $orderBy); unset($this->_sqlParts ['orderby'] [$k] ); foreach ($e as $v) { - $this->_sqlParts['orderby'][] = $v; - } } - // Add the default orderBy statements defined in the relationships and table classes // Only do this for SELECT queries if ($this->_type === self::SELECT) { foreach ($this->_queryComponents as $alias => $map) { $sqlAlias = $this->getSqlTableAlias($alias); if (isset($map ['relation'] )) { $orderBy = $map ['relation'] ->getOrderByStatement($sqlAlias, true); if ($orderBy == $map ['relation'] ['orderBy'] ) { if (isset($map ['ref'] )) { - $orderBy = $map['relation']['refTable']->processOrderBy($sqlAlias, $map['relation']['orderBy'], true); - } else { - $orderBy = null; - } } } else { - $orderBy = $map['table']->getOrderByStatement($sqlAlias, true); - } - if ($orderBy) { $e = explode(',', $orderBy); $e = array_map('trim', $e); foreach ($e as $v) { if ( ! in_array($v, $this->_sqlParts ['orderby'] )) { - $this->_sqlParts['orderby'][] = $v; - } } } } } - $q .= ( ! empty($this->_sqlParts ['groupby'] )) ? ' GROUP BY ' . implode(', ', $this->_sqlParts ['groupby'] ) : ''; $q .= ( ! empty($this->_sqlParts ['having'] )) ? ' HAVING ' . implode(' AND ', $this->_sqlParts ['having'] ): ''; $q .= ( ! empty($this->_sqlParts ['orderby'] )) ? ' ORDER BY ' . implode(', ', $this->_sqlParts ['orderby'] ) : ''; @@ -1396,7 +1396,7 @@ $subquery = 'SELECT DISTINCT '; } $subquery .= $this->_conn->quoteIdentifier($primaryKey); - + // pgsql & oracle need the order by fields to be preserved in select clause if ($driverName == 'pgsql' || $driverName == 'oracle' || $driverName == 'oci' || $driverName == 'mssql' || $driverName == 'odbc') { foreach ($this->_sqlParts ['orderby'] as $part) Unknown macro: {@@ -1420,7 +1420,7 @@ // don't add primarykey column (its already in the select clause) if ($part !== $primaryKey) { $subquery .= ', ' . $partOriginal; - }+ } } } } Property changes on: Query.php ___________________________________________________________________ Deleted: svn:keywords Id Revision Deleted: svn:eol-style LF
        Hide
        Andrew Eross added a comment -

        Diff file

        Show
        Andrew Eross added a comment - Diff file
        Hide
        Andrew Eross added a comment -

        patch -p0 ./libs/doctrine/Doctrine/Query.php ./Doctrine_Query.php.ORDERBY.patch

        Show
        Andrew Eross added a comment - patch -p0 ./libs/doctrine/Doctrine/Query.php ./Doctrine_Query.php.ORDERBY.patch

          People

          • Assignee:
            Guilherme Blanco
            Reporter:
            Andrew Eross
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated: