refactor(Record/Filter): Improve the code readability

Filters had been touched during the work on the support for PHP 8. I had cleaned this part, but the code was not ready to merge. * Add proof that every element of the code of Record Filter works as it should. It keeps the current behaviour, even if it seems a bug. * Fix the code structure of the test and the code.
FriendsOfSymfony1 · Nov 17, 2023 · 05540b4 · 05540b4
1 parent aae4b07
commit 05540b4
Show file tree

Hide file tree

Showing 7 changed files with 536 additions and 107 deletions.
diff --git a/lib/Doctrine/Record/Filter.php b/lib/Doctrine/Record/Filter.php
@@ -33,13 +33,19 @@
  */
 abstract class Doctrine_Record_Filter
 {
+    /**
+     * @var Doctrine_Table
+     */
     protected $_table;
 
     public function setTable(Doctrine_Table $table)
     {
         $this->_table = $table;
     }
 
+    /**
+     * @return Doctrine_Table
+     */
     public function getTable()
     {
         return $this->_table;

diff --git a/lib/Doctrine/Record/Filter/Compound.php b/lib/Doctrine/Record/Filter/Compound.php
@@ -50,10 +50,7 @@ public function __construct(array $aliases)
      */
     public function init()
     {
-        // check that all aliases exist
-        foreach ($this->_aliases as $alias) {
-            $this->_table->getRelation($alias);
-        }
+        $this->validateAliases();
     }
 
     /**
@@ -67,54 +64,114 @@ public function init()
      */
     public function filterSet(Doctrine_Record $record, $propertyOrRelation, $value)
     {
-        foreach ($this->_aliases as $alias) {
-            // The relationship must be fetched in order to check the field existence.
-            // Related to PHP-7.0 compatibility so an explicit call to method get is required.
-            $record[$alias];
-
-            if ( ! $record->exists()) {
-                if (isset($record[$alias][$propertyOrRelation])) {
-                    $record[$alias][$propertyOrRelation] = $value;
-
-                    return $record;
-                }
-            } else {
-                if (isset($record[$alias][$propertyOrRelation])) {
-                    $record[$alias][$propertyOrRelation] = $value;
-                }
+        try {
+            $this->setAliasedRecordPropertyOrRelation($record, $propertyOrRelation, $value);
 
+            return $record;
+        } catch (Doctrine_Record_UnknownPropertyException $e) {
+            if ($record->exists() && $this->hasAlias()) {
                 return $record;
             }
+
+            throw $e;
         }
-        throw new Doctrine_Record_UnknownPropertyException(sprintf('Unknown record property / related component "%s" on "%s"', $propertyOrRelation, get_class($record)));
     }
 
     /**
      * Provides a way for getting property or relation value from the given record.
      *
      * @param string $propertyOrRelation
      *
-     * @return mixed
+     * @return mixed The value of the given property
      *
      * @thrown Doctrine_Record_UnknownPropertyException when this way is not available
      */
     public function filterGet(Doctrine_Record $record, $propertyOrRelation)
+    {
+        $aliasedRecord = $this->findAliasedRecordForGet($record, $propertyOrRelation);
+
+        return $aliasedRecord->get($propertyOrRelation);
+    }
+
+    /**
+     * @throws Doctrine_Table_Exception
+     */
+    private function validateAliases()
     {
         foreach ($this->_aliases as $alias) {
-            // The relationship must be fetched in order to check the field existence.
-            // Related to PHP-7.0 compatibility so an explicit call to method get is required.
-            $record[$alias];
-
-            if ( ! $record->exists()) {
-                if (isset($record[$alias][$propertyOrRelation])) {
-                    return $record[$alias][$propertyOrRelation];
-                }
-            } else {
-                if (isset($record[$alias][$propertyOrRelation])) {
-                    return $record[$alias][$propertyOrRelation];
-                }
+            $this->_table->getRelation($alias);
+        }
+    }
+
+    private function hasAlias()
+    {
+        return (bool) $this->_aliases;
+    }
+
+    /**
+     * @param string $propertyOrRelation
+     *
+     * @thrown Doctrine_Record_UnknownPropertyException when this way is not available
+     */
+    private function setAliasedRecordPropertyOrRelation(Doctrine_Record $record, $propertyOrRelation, $value)
+    {
+        $aliasedRecord = $this->findAliasedRecordForSet($record, $propertyOrRelation);
+
+        $aliasedRecord[$propertyOrRelation] = $value;
+    }
+
+    /**
+     * @return Doctrine_Record
+     *
+     * @thrown Doctrine_Record_UnknownPropertyException
+     */
+    private function findAliasedRecordForSet(Doctrine_Record $record, $propertyOrRelation)
+    {
+        foreach ($this->_aliases as $alias) {
+            if ($this->recordAliasHasPropertyOrRelation($record, $alias, $propertyOrRelation)) {
+                return $record->get($alias);
             }
+
+            $this->stopSearchIfRecordExists($record, $propertyOrRelation);
         }
-        throw new Doctrine_Record_UnknownPropertyException(sprintf('Unknown record property / related component "%s" on "%s"', $propertyOrRelation, get_class($record)));
+
+        throw Doctrine_Record_UnknownPropertyException::createFromRecordAndProperty($record, $propertyOrRelation);
+    }
+
+    /**
+     * @thrown Doctrine_Record_UnknownPropertyException
+     */
+    private function stopSearchIfRecordExists(Doctrine_Record $record, $propertyOrRelation)
+    {
+        if ($record->exists()) {
+            throw Doctrine_Record_UnknownPropertyException::createFromRecordAndProperty($record, $propertyOrRelation);
+        }
+    }
+
+    /**
+     * @return Doctrine_Record
+     *
+     * @thrown Doctrine_Record_UnknownPropertyException
+     */
+    private function findAliasedRecordForGet(Doctrine_Record $record, $propertyOrRelation)
+    {
+        foreach ($this->_aliases as $alias) {
+            if ($this->recordAliasHasPropertyOrRelation($record, $alias, $propertyOrRelation)) {
+                return $record->get($alias);
+            }
+        }
+
+        throw Doctrine_Record_UnknownPropertyException::createFromRecordAndProperty($record, $propertyOrRelation);
+    }
+
+    /**
+     * @param string $alias
+     * @param string $propertyOrRelation
+     */
+    private function recordAliasHasPropertyOrRelation(Doctrine_Record $record, $alias, $propertyOrRelation)
+    {
+        $aliasedRecord = $record->get($alias);
+
+        return isset($aliasedRecord[$propertyOrRelation]);
     }
 }
diff --git a/lib/Doctrine/Record/Filter/Standard.php b/lib/Doctrine/Record/Filter/Standard.php
@@ -40,7 +40,7 @@ class Doctrine_Record_Filter_Standard extends Doctrine_Record_Filter
      */
     public function filterSet(Doctrine_Record $record, $propertyOrRelation, $value)
     {
-        throw new Doctrine_Record_UnknownPropertyException(sprintf('Unknown record property / related component "%s" on "%s"', $propertyOrRelation, get_class($record)));
+        throw Doctrine_Record_UnknownPropertyException::createFromRecordAndProperty($record, $propertyOrRelation);
     }
 
     /**
@@ -50,6 +50,6 @@ public function filterSet(Doctrine_Record $record, $propertyOrRelation, $value)
      */
     public function filterGet(Doctrine_Record $record, $propertyOrRelation)
     {
-        throw new Doctrine_Record_UnknownPropertyException(sprintf('Unknown record property / related component "%s" on "%s"', $propertyOrRelation, get_class($record)));
+        throw Doctrine_Record_UnknownPropertyException::createFromRecordAndProperty($record, $propertyOrRelation);
     }
 }
diff --git a/lib/Doctrine/Record/UnknownPropertyException.php b/lib/Doctrine/Record/UnknownPropertyException.php
@@ -31,4 +31,14 @@
  * @author      Konsta Vesterinen <[email protected]>
  */
 class Doctrine_Record_UnknownPropertyException extends Doctrine_Record_Exception
-{ }
+{
+    public static function createFromRecordAndProperty(Doctrine_Record $record, $propertyOrRelation)
+    {
+        $message = sprintf('Unknown record property / related component "%s" on "%s"',
+            $propertyOrRelation,
+            get_class($record)
+        );
+
+        return new Doctrine_Record_UnknownPropertyException($message);
+    }
+}
diff --git a/tests/Data/ExportTestCase.php b/tests/Data/ExportTestCase.php
@@ -30,53 +30,53 @@
  * @since       1.0
  * @version     $Revision$
  */
-class Doctrine_Data_Export_TestCase extends Doctrine_UnitTestCase 
+class Doctrine_Data_Export_TestCase extends Doctrine_UnitTestCase
 {
+    public function tearDown()
+    {
+        if (file_exists('test.yml')) {
+            unlink('test.yml');
+        }
+
+        parent::tearDown();
+    }
+
     public function prepareTables()
     {
         $this->tables[] = 'I18nTestExport';
+
         parent::prepareTables();
     }
 
     public function testI18nExport()
     {
-        try {
-            $i = new I18nTestExport();
-            $i->Translation['en']->title = 'english test';
-            $i->Translation['fr']->title = 'french test';
-            $i->test_object = new stdClass();
-            $i->save();
+        $i = new I18nTestExport();
+        $i->Translation['en']->title = 'english test';
+        $i->Translation['fr']->title = 'french test';
+        $i->test_object = new stdClass();
+        $i->save();
 
-            $data = new Doctrine_Data();
-            $data->exportData('test.yml', 'yml', array('I18nTestExport', 'I18nTestExportTranslation'));
+        $data = new Doctrine_Data();
+        $data->exportData('test.yml', 'yml', array('I18nTestExport', 'I18nTestExportTranslation'));
 
-            $array = Doctrine_Parser::load('test.yml', 'yml');
+        $array = Doctrine_Parser::load('test.yml', 'yml');
 
-            $this->assertTrue( ! empty($array));
-            $this->assertTrue(isset($array['I18nTestExport']['I18nTestExport_1']));
-            $this->assertTrue(isset($array['I18nTestExportTranslation']['I18nTestExportTranslation_1_en']));
-            $this->assertTrue(isset($array['I18nTestExportTranslation']['I18nTestExportTranslation_1_fr']));
+        $this->assertTrue( ! empty($array));
+        $this->assertTrue(isset($array['I18nTestExport']['I18nTestExport_1']));
+        $this->assertTrue(isset($array['I18nTestExportTranslation']['I18nTestExportTranslation_1_en']));
+        $this->assertTrue(isset($array['I18nTestExportTranslation']['I18nTestExportTranslation_1_fr']));
 
-            $i->Translation->delete();
-            $i->delete();
+        $i->Translation->delete();
+        $i->delete();
 
-            Doctrine_Core::loadData('test.yml');
+        Doctrine_Core::loadData('test.yml');
 
-            $q = Doctrine_Query::create()
-                ->from('I18nTestExport e')
-                ->leftJoin('e.Translation t');
+        $q = Doctrine_Query::create()
+            ->from('I18nTestExport e')
+            ->leftJoin('e.Translation t');
 
-            $results = $q->execute();
-            $this->assertEqual(get_class($results[0]->test_object), 'stdClass');
-
-            $this->pass();
-        } catch (Exception $e) {
-            $this->fail($e->getMessage());
-        }
-
-        if (file_exists('test.yml')) {
-            unlink('test.yml');
-        }
+        $results = $q->execute();
+        $this->assertEqual(get_class($results[0]->test_object), 'stdClass');
     }
 }
 
@@ -93,4 +93,4 @@ public function setUp()
     {
         $this->actAs('I18n', array('fields' => array('name', 'title')));
     }
-}
+}