[2.0] DDC-336 DDC-337 - Changed @OrderBy annotation to take a DQL not SQL...

[2.0] DDC-336 DDC-337 - Changed @OrderBy annotation to take a DQL not SQL snippet, restrict it to field names and positional orderings. Changed all test-cases and added a test-case that shows the behaviour for OneToMany JoinedSubclassPersister Managed Entities.

lib/Doctrine/ORM/Mapping/ManyToManyMapping.php

@@ -61,7 +61,11 @@ class ManyToManyMapping extends AssociationMapping
     //public $keyColumn;
 
     /**
-     * Order this collection by the given SQL snippet.
+     * Order this collection by the given DQL snippet.
+     * 
+     * Only simple unqualified field names and ASC|DESC are allowed
+     *
+     * @var array
      */
     public $orderBy = null;
     
@@ -142,7 +146,18 @@ class ManyToManyMapping extends AssociationMapping
         }
 
         if (isset($mapping['orderBy'])) {
-            $this->orderBy = $mapping['orderBy'];
+            $parts = explode(",", $mapping['orderBy']);
+            $orderByGroup = array();
+            foreach ($parts AS $part) {
+                $orderByItem = explode(" ", trim($part));
+                if (count($orderByItem) == 1) {
+                    $orderByGroup[$orderByItem[0]] = "ASC";
+                } else {
+                    $orderByGroup[$orderByItem[0]] = array_pop($orderByItem);
+                }
+            }
+
+            $this->orderBy = $orderByGroup;
         }
     }
 

lib/Doctrine/ORM/Mapping/OneToManyMapping.php

@@ -86,7 +86,18 @@ class OneToManyMapping extends AssociationMapping
                 (bool) $mapping['orphanRemoval'] : false;
 
         if (isset($mapping['orderBy'])) {
-            $this->orderBy = $mapping['orderBy'];
+            $parts = explode(",", $mapping['orderBy']);
+            $orderByGroup = array();
+            foreach ($parts AS $part) {
+                $orderByItem = explode(" ", trim($part));
+                if (count($orderByItem) == 1) {
+                    $orderByGroup[$orderByItem[0]] = "ASC";
+                } else {
+                    $orderByGroup[$orderByItem[0]] = array_pop($orderByItem);
+                }
+            }
+
+            $this->orderBy = $orderByGroup;
         }
     }
     

lib/Doctrine/ORM/Persisters/JoinedSubclassPersister.php

@@ -294,10 +294,12 @@ class JoinedSubclassPersister extends StandardEntityPersister
      * Gets the SELECT SQL to select one or more entities by a set of field criteria.
      *
      * @param array $criteria
-     * @return string The SQL.
+     * @param AssociationMapping $assoc
+     * @param string $orderBy
+     * @return string
      * @override
      */
-    protected function _getSelectEntitiesSql(array &$criteria, $assoc = null)
+    protected function _getSelectEntitiesSql(array &$criteria, $assoc = null, $orderBy = null)
     {
         $tableAliases = array();
         $aliasIndex = 1;
@@ -419,10 +421,15 @@ class JoinedSubclassPersister extends StandardEntityPersister
             $conditionSql .= ' = ?';
         }
 
+        $orderBySql = '';
+        if ($orderBy !== null) {
+            $orderBySql = $this->_getCollectionOrderBySql($orderBy, $baseTableAlias, $tableAliases);
+        }
+
         return 'SELECT ' . $columnList
                 . ' FROM ' . $this->_class->getQuotedTableName($this->_platform) . ' ' . $baseTableAlias
                 . $joinSql
-                . ($conditionSql != '' ? ' WHERE ' . $conditionSql : '');
+                . ($conditionSql != '' ? ' WHERE ' . $conditionSql : '') . $orderBySql;
     }
     
     /** @override */

lib/Doctrine/ORM/Persisters/StandardEntityPersister.php

@@ -548,11 +548,7 @@ class StandardEntityPersister
     {
         $owningAssoc = $this->_class->associationMappings[$coll->getMapping()->mappedByFieldName];
         
-        $sql = $this->_getSelectEntitiesSql($criteria, $owningAssoc);
-
-        if ($assoc->orderBy !== null) {
-            $sql .= ' ORDER BY '.str_replace('%alias%', $this->_class->getTableName(), $assoc->orderBy);
-        }
+        $sql = $this->_getSelectEntitiesSql($criteria, $owningAssoc, $assoc->orderBy);
 
         $params = array_values($criteria);
         
@@ -653,9 +649,11 @@ class StandardEntityPersister
      * Gets the SELECT SQL to select one or more entities by a set of field criteria.
      *
      * @param array $criteria
-     * @return string The SQL.
+     * @param AssociationMapping $assoc
+     * @param string $orderBy
+     * @return string
      */
-    protected function _getSelectEntitiesSql(array &$criteria, $assoc = null)
+    protected function _getSelectEntitiesSql(array &$criteria, $assoc = null, $orderBy = null)
     {
         // Construct WHERE conditions
         $conditionSql = '';
@@ -676,9 +674,43 @@ class StandardEntityPersister
             $conditionSql .= ' = ?';
         }
 
+        $orderBySql = '';
+        if ($orderBy !== null) {
+            $orderBySql = $this->_getCollectionOrderBySql(
+                $orderBy, $this->_class->getQuotedTableName($this->_platform)
+            );
+        }
+
         return 'SELECT ' . $this->_getSelectColumnList() 
              . ' FROM ' . $this->_class->getQuotedTableName($this->_platform)
-             . ($conditionSql ? ' WHERE ' . $conditionSql : '');
+             . ($conditionSql ? ' WHERE ' . $conditionSql : '') . $orderBySql;
+    }
+
+    /**
+     * Generate ORDER BY Sql Snippet for ordered collections
+     * 
+     * @param array $orderBy
+     * @return string
+     */
+    protected function _getCollectionOrderBySql(array $orderBy, $baseTableAlias, $tableAliases = array())
+    {
+        $orderBySql = '';
+        foreach ($orderBy AS $fieldName => $orientation) {
+            if (!isset($this->_class->fieldMappings[$fieldName])) {
+                ORMException::unrecognizedField($fieldName);
+            }
+
+            $tableAlias = isset($this->_class->fieldMappings['inherited']) ?
+                $tableAliases[$this->_class->fieldMappings['inherited']] : $baseTableAlias;
+            $columnName = $this->_class->getQuotedColumnName($fieldName, $this->_platform);
+            if ($orderBySql != '') {
+                $orderBySql .= ', ';
+            } else {
+                $orderBySql = ' ORDER BY ';
+            }
+            $orderBySql .= $tableAlias . '.' . $columnName . ' '.$orientation;
+        }
+        return $orderBySql;
     }
     
     /**
@@ -724,7 +756,7 @@ class StandardEntityPersister
     /**
      * Gets the SQL to select a collection of entities in a many-many association.
      *
-     * @param ManyToManyMapping $assoc
+     * @param ManyToManyMapping $manyToMany
      * @param array $criteria
      * @return string
      */
@@ -761,7 +793,9 @@ class StandardEntityPersister
 
         $orderBySql = '';
         if ($manyToMany->orderBy !== null) {
-            $orderBySql = ' ORDER BY '.str_replace('%alias%', $this->_class->getTableName(), $manyToMany->orderBy);
+            $orderBySql = $this->_getCollectionOrderBySql(
+                $manyToMany->orderBy, $this->_class->getQuotedTableName($this->_platform)
+            );
         }
         
         return 'SELECT ' . $this->_getSelectColumnList() 

tests/Doctrine/Tests/ORM/Functional/AllTests.php

@@ -41,6 +41,8 @@ class AllTests
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\OneToOneSelfReferentialAssociationTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\OneToManySelfReferentialAssociationTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\ManyToManySelfReferentialAssociationTest');
+        $suite->addTestSuite('Doctrine\Tests\ORM\Functional\OrderedCollectionTest');
+        $suite->addTestSuite('Doctrine\Tests\ORM\Functional\OrderedJoinedTableInheritanceCollectionTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\ReferenceProxyTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\LifecycleCallbackTest');
         $suite->addTestSuite('Doctrine\Tests\ORM\Functional\StandardEntityPersisterTest');

tests/Doctrine/Tests/ORM/Functional/OrderedCollectionTest.php

@@ -9,7 +9,7 @@ use Doctrine\Tests\Models\Routing\RoutingRouteBooking;
 
 require_once __DIR__ . '/../../TestInit.php';
 
-class OrderedAssociationTest extends \Doctrine\Tests\OrmFunctionalTestCase
+class OrderedCollectionTest extends \Doctrine\Tests\OrmFunctionalTestCase
 {
     protected $locations = array();
 

tests/Doctrine/Tests/ORM/Functional/OrderedJoinedTableInheritanceCollectionTest.php

+<?php
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\ORM\Query;
+
+require_once __DIR__ . '/../../TestInit.php';
+
+/**
+ * Functional tests for the Single Table Inheritance mapping strategy.
+ *
+ * @author Benjamin Eberlei <kontakt@beberlei.de>
+ */
+class OrderedJoinedTableInheritanceCollectionTest extends \Doctrine\Tests\OrmFunctionalTestCase
+{
+    protected function setUp() {
+        parent::setUp();
+        try {
+            $this->_schemaTool->createSchema(array(
+                $this->_em->getClassMetadata('Doctrine\Tests\ORM\Functional\OJTIC_Pet'),
+                $this->_em->getClassMetadata('Doctrine\Tests\ORM\Functional\OJTIC_Cat'),
+                $this->_em->getClassMetadata('Doctrine\Tests\ORM\Functional\OJTIC_Dog'),
+            ));
+        } catch (\Exception $e) {
+            // Swallow all exceptions. We do not test the schema tool here.
+        }
+    }
+
+    public function testOrderdOneToManyCollection()
+    {
+        $dog = new OJTIC_Dog();
+        $dog->name = "Poofy";
+
+        $dog1 = new OJTIC_Dog();
+        $dog1->name = "Zampa";
+        $dog2 = new OJTIC_Dog();
+        $dog2->name = "Aari";
+
+        $dog1->mother = $dog;
+        $dog2->mother = $dog;
+
+        $dog->children[] = $dog1;
+        $dog->children[] = $dog2;
+
+        $this->_em->persist($dog);
+        $this->_em->persist($dog1);
+        $this->_em->persist($dog2);
+        $this->_em->flush();
+        $this->_em->clear();
+
+        $poofy = $this->_em->createQuery("SELECT p FROM Doctrine\Tests\ORM\Functional\OJTIC_Pet p WHERE p.name = 'Poofy'")->getSingleResult();
+
+        $this->assertEquals('Aari', $poofy->children[0]->getName());
+        $this->assertEquals('Zampa', $poofy->children[1]->getName());
+    }
+}
+
+/**
+ * @Entity
+ * @InheritanceType("JOINED")
+ * @DiscriminatorColumn(name="discr", type="string")
+ * @DiscriminatorMap({
+ *      "cat" = "OJTIC_Cat",
+ *      "dog" = "OJTIC_Dog"})
+ */
+abstract class OJTIC_Pet
+{
+    /**
+     * @Id
+     * @column(type="integer")
+     * @generatedValue(strategy="AUTO")
+     */
+    public $id;
+
+    /**
+     *
+     * @Column
+     */
+    public $name;
+
+    /**
+     * @ManyToOne(targetEntity="OJTIC_PET")
+     */
+    public $mother;
+
+    /**
+     * @OneToMany(targetEntity="OJTIC_Pet", mappedBy="mother")
+     * @OrderBy("name ASC")
+     */
+    public $children;
+
+    /**
+     * @ManyToMany(targetEntity="OJTIC_Pet")
+     * @JoinTable(name="OTJIC_Pet_Friends",
+     *     joinColumns={@JoinColumn(name="pet_id", referencedColumnName="id")},
+     *     inverseJoinColumns={@JoinColumn(name="friend_id", referencedColumnName="id")})
+     * @OrderBy("name ASC")
+     */
+    public $friends;
+
+    public function getName()
+    {
+        return $this->name;
+    }
+}
+
+/**
+ * @Entity
+ */
+class OJTIC_Cat extends OJTIC_Pet
+{
+    
+}
+
+/**
+ * @Entity
+ */
+class OJTIC_Dog extends OJTIC_Pet
+{
+    
+}
\ No newline at end of file

tests/Doctrine/Tests/ORM/Mapping/AbstractMappingDriverTest.php

@@ -114,7 +114,7 @@ abstract class AbstractMappingDriverTest extends \Doctrine\Tests\OrmTestCase
         $this->assertFalse($class->associationMappings['phonenumbers']->isCascadeMerge);
 
         // Test Order By
-        $this->assertEquals('%alias%.number ASC', $class->associationMappings['phonenumbers']->orderBy);
+        $this->assertEquals(array('number' => 'ASC'), $class->associationMappings['phonenumbers']->orderBy);
 
         return $class;
     }
@@ -207,7 +207,7 @@ class User
     /**
      *
      * @OneToMany(targetEntity="Phonenumber", mappedBy="user", cascade={"persist"})
-     * @OrderBy("%alias%.number ASC")
+     * @OrderBy("number ASC")
      */
     public $phonenumbers;
 

tests/Doctrine/Tests/ORM/Mapping/xml/Doctrine.Tests.ORM.Mapping.User.dcm.xml

@@ -24,7 +24,7 @@
             <join-column name="address_id" referenced-column-name="id"/>
         </one-to-one>
         
-        <one-to-many field="phonenumbers" target-entity="Phonenumber" mapped-by="user" order-by="%alias%.number ASC">
+        <one-to-many field="phonenumbers" target-entity="Phonenumber" mapped-by="user" order-by="number ASC">
             <cascade>
                 <cascade-persist/>
             </cascade>

tests/Doctrine/Tests/ORM/Mapping/yaml/Doctrine.Tests.ORM.Mapping.User.dcm.yml

@@ -27,7 +27,7 @@ Doctrine\Tests\ORM\Mapping\User:
     phonenumbers:
       targetEntity: Phonenumber
       mappedBy: user
-      orderBy: %alias%.number ASC
+      orderBy: number ASC
       cascade: [ persist ]
   manyToMany:
     groups: