[BUGFIX] Fix inconsitency of file reference property getters 23/18223/2
authorHelmut Hummel <h.hummel@bitmotion.de>
Tue, 12 Feb 2013 17:32:23 +0000 (18:32 +0100)
committerHelmut Hummel <helmut.hummel@typo3.org>
Tue, 5 Mar 2013 13:22:35 +0000 (14:22 +0100)
The FileReference object has the following getters
for getting properties:

getProperties()
getReferenceProperties()
getProperty($key)
getReferenceProperty($key)

The result from these getters is inconsistent:

getReferenceProperty($key)
Returns the value of a property which is only
in the reference properties

getReferenceProperties()
Returns merged properties form the original
file and the reference properties without
respecting overriding of reference properties
if they are not set to NULL

getProperty($key)
Returns the merged value from original file
and reference, respecting the NULL override
handling. But if a property is only available
in the original file an Exception is thrown.

getProperties()
Returns merged properties form the original
file and the reference properties
respecting overriding of reference properties
if they are not set to NULL
Properties only available in the original file
will be available in the resulting array.

Streamline the behaviour so that all getters
starting with "getReference" will only return
the properties of the reference record and all
other will return the merged properties but
respecting the NULL override handling.

Resolves: #45416
Releases: 6.0, 6.1

Change-Id: I35a84da83be765991b357c5cee89ce018a1f1e24
Reviewed-on: https://review.typo3.org/18223
Reviewed-by: Steffen Ritter
Tested-by: Wouter Wolters
Reviewed-by: Helmut Hummel
Tested-by: Helmut Hummel
typo3/sysext/core/Classes/Resource/FileReference.php
typo3/sysext/core/Tests/Unit/Resource/FileReferenceTest.php [new file with mode: 0644]

index 2da3b9d..d8e538b 100644 (file)
@@ -82,17 +82,13 @@ class FileReference implements FileInterface {
        protected $originalFile;
 
        /**
-        * Defines properties that are merged with the parent object (File) if
+        * Properties merged with the parent object (File) if
         * the value is not defined (NULL). Thus, FileReference properties act
         * as overlays for the defined File properties.
         *
         * @var array
         */
-       protected $parentFallbackProperties = array(
-               'title' => 'title',
-               'description' => 'description',
-               'alternative' => 'alternative',
-       );
+       protected $mergedProperties = array();
 
        /**
         * Constructor for a file in use object. Should normally not be used
@@ -131,7 +127,7 @@ class FileReference implements FileInterface {
         * @return boolean
         */
        public function hasProperty($key) {
-               return array_key_exists($key, $this->propertiesOfFileReference);
+               return array_key_exists($key, $this->getProperties());
        }
 
        /**
@@ -139,27 +135,26 @@ class FileReference implements FileInterface {
         *
         * @param string $key The property to be looked up
         * @return mixed
+        * @throws \InvalidArgumentException
         */
        public function getProperty($key) {
-               $value = $this->getReferenceProperty($key);
-
-               if ($value === NULL && !empty($this->parentFallbackProperties[$key])) {
-                       $value = $this->originalFile->getProperty($key);
+               if (!$this->hasProperty($key)) {
+                       throw new \InvalidArgumentException('Property "' . $key . '" was not found in file reference or original file.', 1314226805);
                }
-
-               return $value;
+               $properties = $this->getProperties();
+               return $properties[$key];
        }
 
        /**
-        * Gets a property.
+        * Gets a property of the file reference.
         *
         * @param string $key The property to be looked up
         * @return mixed
         * @throws \InvalidArgumentException
         */
        public function getReferenceProperty($key) {
-               if (!$this->hasProperty($key)) {
-                       throw new \InvalidArgumentException('Property "' . $key . '" was not found.', 1314226805);
+               if (!array_key_exists($key, $this->propertiesOfFileReference)) {
+                       throw new \InvalidArgumentException('Property "' . $key . '" of file reference was not found.', 1360684914);
                }
                return $this->propertiesOfFileReference[$key];
        }
@@ -170,26 +165,39 @@ class FileReference implements FileInterface {
         * @return array
         */
        public function getProperties() {
-               $properties = $this->getReferenceProperties();
-               $keys = array_keys($properties);
+               if (empty($this->mergedProperties)) {
+                       $this->mergedProperties = \TYPO3\CMS\Core\Utility\GeneralUtility::array_merge_recursive_overrule(
+                               $this->propertiesOfFileReference,
+                               $this->originalFile->getProperties(),
+                               FALSE,
+                               TRUE,
+                               FALSE
+                       );
+                       array_walk($this->mergedProperties, array($this, 'restoreNonNullValuesCallback'));
+               }
 
-               foreach ($this->parentFallbackProperties as $localKey => $parentKey) {
-                       if (array_key_exists($localKey, $keys) && $properties[$localKey] === NULL) {
-                               $properties[$localKey] = $this->originalFile->getProperty($parentKey);
-                       }
+               return $this->mergedProperties;
+       }
+
+       /**
+        * Callback to handle the NULL value feature
+        *
+        * @param mixed $value
+        * @param mixed $key
+        */
+       protected function restoreNonNullValuesCallback(&$value, $key) {
+               if (array_key_exists($key, $this->propertiesOfFileReference) && $this->propertiesOfFileReference[$key] !== NULL) {
+                       $value = $this->propertiesOfFileReference[$key];
                }
        }
 
        /**
-        * Gets all properties.
+        * Gets all properties of the file reference.
         *
         * @return array
         */
        public function getReferenceProperties() {
-               return \TYPO3\CMS\Core\Utility\GeneralUtility::array_merge_recursive_overrule(
-                       $this->originalFile->getProperties(),
-                       $this->propertiesOfFileReference
-               );
+               return $this->propertiesOfFileReference;
        }
 
        /**
diff --git a/typo3/sysext/core/Tests/Unit/Resource/FileReferenceTest.php b/typo3/sysext/core/Tests/Unit/Resource/FileReferenceTest.php
new file mode 100644 (file)
index 0000000..08d810d
--- /dev/null
@@ -0,0 +1,173 @@
+<?php
+namespace TYPO3\CMS\Core\Tests\Unit\Resource;
+
+/***************************************************************
+ * Copyright notice
+ *
+ * (c) 2013 Helmut Hummel <helmut.hummel@typo3.org>
+ * All rights reserved
+ *
+ * This script is part of the TYPO3 project. The TYPO3 project is
+ * free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * The GNU General Public License can be found at
+ * http://www.gnu.org/copyleft/gpl.html.
+ *
+ * This script is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This copyright notice MUST APPEAR in all copies of the script!
+ ***************************************************************/
+
+
+/**
+ * Testcase for the file class of the TYPO3 FAL
+ *
+ */
+class FileReferenceTest extends \TYPO3\CMS\Core\Tests\UnitTestCase {
+
+       /**
+        * @var array A backup of registered singleton instances
+        */
+       protected $singletonInstances = array();
+
+       public function setUp() {
+               $this->singletonInstances = \TYPO3\CMS\Core\Utility\GeneralUtility::getSingletonInstances();
+               \TYPO3\CMS\Core\Utility\GeneralUtility::purgeInstances();
+       }
+
+       public function tearDown() {
+               \TYPO3\CMS\Core\Utility\GeneralUtility::resetSingletonInstances($this->singletonInstances);
+       }
+
+       /**
+        * @param array $fileReferenceProperties
+        * @param array $originalFileProperties
+        * @return \TYPO3\CMS\Core\Resource\FileReference|\PHPUnit_Framework_MockObject_MockObject|\TYPO3\CMS\Core\Tests\AccessibleObjectInterface
+        */
+       protected function prepareFixture(array $fileReferenceProperties, array $originalFileProperties) {
+               $fixture = $this->getAccessibleMock('TYPO3\\CMS\\Core\\Resource\\FileReference', array('dummy'), array(), '', FALSE);
+               $originalFileMock = $this->getAccessibleMock('TYPO3\\CMS\\Core\\Resource\\File', array(), array(), '', FALSE);
+               $originalFileMock->expects($this->any())
+                       ->method('getProperties')
+                       ->will($this->returnValue($originalFileProperties)
+               );
+               $fixture->_set('originalFile', $originalFileMock);
+               $fixture->_set('propertiesOfFileReference', $fileReferenceProperties);
+
+               return $fixture;
+       }
+
+       /**
+        * @return array
+        */
+       public function propertiesDataProvider() {
+               return array(
+                       'File properties correctly override file reference properties' => array(
+                               array(
+                                       'title' => NULL,
+                                       'description' => 'fileReferenceDescription',
+                                       'alternative' => '',
+                               ),
+                               array(
+                                       'title' => 'fileTitle',
+                                       'description' => 'fileDescription',
+                                       'alternative' => 'fileAlternative',
+                                       'file_only_property' => 'fileOnlyPropertyValue',
+                               ),
+                               array(
+                                       'title' => 'fileTitle',
+                                       'description' => 'fileReferenceDescription',
+                                       'alternative' => '',
+                                       'file_only_property' => 'fileOnlyPropertyValue',
+                               ),
+                       )
+               );
+       }
+
+       /**
+        * @param array $fileReferenceProperties
+        * @param array $originalFileProperties
+        * @param array $expectedMergedProperties
+        * @test
+        * @dataProvider propertiesDataProvider
+        */
+       public function getPropertiesReturnsMergedPropertiesAndRespectsNullValues(array $fileReferenceProperties, array $originalFileProperties, array $expectedMergedProperties) {
+               $fixture = $this->prepareFixture($fileReferenceProperties, $originalFileProperties);
+               $actual = $fixture->getProperties();
+               $this->assertSame($expectedMergedProperties, $actual);
+       }
+
+       /**
+        * @param array $fileReferenceProperties
+        * @param array $originalFileProperties
+        * @param array $expectedMergedProperties
+        * @test
+        * @dataProvider propertiesDataProvider
+        */
+       public function hasPropertyReturnsTrueForAllMergedPropertyKeys($fileReferenceProperties, $originalFileProperties, $expectedMergedProperties) {
+               $fixture = $this->prepareFixture($fileReferenceProperties, $originalFileProperties);
+               foreach (array_keys($expectedMergedProperties) as $key) {
+                       $this->assertTrue($fixture->hasProperty($key));
+               }
+       }
+
+       /**
+        * @param array $fileReferenceProperties
+        * @param array $originalFileProperties
+        * @param array $expectedMergedProperties
+        * @test
+        * @dataProvider propertiesDataProvider
+        */
+       public function getPropertyReturnsAllMergedPropertyKeys($fileReferenceProperties, $originalFileProperties, $expectedMergedProperties) {
+               $fixture = $this->prepareFixture($fileReferenceProperties, $originalFileProperties);
+               foreach ($expectedMergedProperties as $key => $expectedValue) {
+                       $this->assertSame($expectedValue, $fixture->getProperty($key));
+               }
+       }
+
+       /**
+        * @param array $fileReferenceProperties
+        * @param array $originalFileProperties
+        * @param array $expectedMergedProperties
+        * @test
+        * @dataProvider propertiesDataProvider
+        * @expectedException \InvalidArgumentException
+        */
+       public function getPropertyThrowsExceptionForNotAvailableProperty($fileReferenceProperties, $originalFileProperties) {
+               $fixture = $this->prepareFixture($fileReferenceProperties, $originalFileProperties);
+               $fixture->getProperty(uniqid('nothingHere'));
+       }
+
+       /**
+        * @param array $fileReferenceProperties
+        * @param array $originalFileProperties
+        * @param array $expectedMergedProperties
+        * @test
+        * @dataProvider propertiesDataProvider
+        */
+       public function getPropertyDoesNotThrowExceptionForPropertyOnlyAvailableInOriginalFile($fileReferenceProperties, $originalFileProperties) {
+               $fixture = $this->prepareFixture($fileReferenceProperties, $originalFileProperties);
+               $this->assertSame($originalFileProperties['file_only_property'], $fixture->getProperty('file_only_property'));
+       }
+
+       /**
+        * @param array $fileReferenceProperties
+        * @param array $originalFileProperties
+        * @param array $expectedMergedProperties
+        * @test
+        * @dataProvider propertiesDataProvider
+        * @expectedException \InvalidArgumentException
+        */
+       public function getReferencePropertyThrowsExceptionForPropertyOnlyAvailableInOriginalFile($fileReferenceProperties, $originalFileProperties) {
+               $fixture = $this->prepareFixture($fileReferenceProperties, $originalFileProperties);
+               $fixture->getReferenceProperty('file_only_property');
+       }
+}
+
+?>
\ No newline at end of file