[!!!][BUGFIX] Content Object instances are reused 86/27586/7
authorOliver Hader <oliver@typo3.org>
Wed, 12 Feb 2014 16:38:04 +0000 (17:38 +0100)
committerHelmut Hummel <helmut.hummel@typo3.org>
Mon, 17 Feb 2014 14:58:03 +0000 (15:58 +0100)
The ContentObjectRenderer creates instances for each content
object to be rendered, e.g. TEXT, COA, CONTENT, etc. However,
these instances are re-used and therefore we introduced work
arounds since serialization of FLUIDTEMPLATE failed back then.
If dealing with nested record sets using RECORDS or CONTENT,
reusing objects might be tricky since the parent pointer to the
ContentObjectRenderer might be wrong and strange things happen.

This patch removes the pseudo singleton approach since the
objects are not state-less and might lead to unexpected
results in rare cases.

Resolves: #55941
Releases: 6.2
Change-Id: I043a31403c05b87e1591f0e0237effa21bf93c98
Reviewed-on: https://review.typo3.org/27586
Reviewed-by: Markus Klein
Tested-by: Markus Klein
Reviewed-by: Stefan Neufeind
Tested-by: Stefan Neufeind
Reviewed-by: Helmut Hummel
Tested-by: Helmut Hummel
typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
typo3/sysext/frontend/Tests/Unit/ContentObject/ContentObjectRendererTest.php

index 4745ee6..ee6249f 100644 (file)
@@ -230,6 +230,48 @@ class ContentObjectRenderer {
        );
 
        /**
+        * Class names for accordant content objects
+        *
+        * @var array
+        */
+       protected $contentObjectClassMapping = array(
+               'TEXT' => 'Text',
+               'CASE' => 'Case',
+               'CLEARGIF' => 'ClearGif',
+               'COBJ_ARRAY' => 'ContentObjectArray',
+               'COA' => 'ContentObjectArray',
+               'COA_INT' => 'ContentObjectArrayInternal',
+               'USER' => 'User',
+               'USER_INT' => 'UserInternal',
+               'FILE' => 'File',
+               'FILES' => 'Files',
+               'IMAGE' => 'Image',
+               'IMG_RESOURCE' => 'ImageResource',
+               'IMGTEXT' => 'ImageText',
+               'CONTENT' => 'Content',
+               'RECORDS' => 'Records',
+               'HMENU' => 'HierarchicalMenu',
+               'CTABLE' => 'ContentTable',
+               'OTABLE' => 'OffsetTable',
+               'COLUMNS' => 'Columns',
+               'HRULER' => 'HorizontalRuler',
+               'CASEFUNC' => 'Case',
+               'LOAD_REGISTER' => 'LoadRegister',
+               'RESTORE_REGISTER' => 'RestoreRegister',
+               'FORM' => 'Form',
+               'SEARCHRESULT' => 'SearchResult',
+               'TEMPLATE' => 'Template',
+               'FLUIDTEMPLATE' => 'FluidTemplate',
+               'MULTIMEDIA' => 'Multimedia',
+               'MEDIA' => 'Media',
+               'SWFOBJECT' => 'ShockwaveFlashObject',
+               'FLOWPLAYER' => 'FlowPlayer',
+               'QTOBJECT' => 'QuicktimeObject',
+               'SVG' => 'ScalableVectorGraphics',
+               'EDITPANEL' => 'EditPanel',
+       );
+
+       /**
         * Holds ImageMagick parameters and extensions used for compression
         *
         * @see IMGTEXT()
@@ -477,11 +519,6 @@ class ContentObjectRenderer {
        protected $getImgResourceHookObjects;
 
        /**
-        * @var array with members of AbstractContentObject
-        */
-       protected $contentObjects = array();
-
-       /**
         * @var \TYPO3\CMS\Core\Resource\File Current file objects (during iterations over files)
         */
        protected $currentFile = NULL;
@@ -565,44 +602,6 @@ class ContentObjectRenderer {
        }
 
        /**
-        * Clone helper.
-        *
-        * Resets the references to the TypoScript Content Object implementation
-        * objects of tslib_content_*. Otherwise they would still point to the
-        * original ContentObjectRender instance's tslib_content_* instances, they in return
-        * would back-reference to the original ContentObjectRender instance instead of the
-        * newly cloned ContentObjectRender instance.
-        *
-        * @see http://forge.typo3.org/issues/24204
-        */
-       public function __clone() {
-               $this->contentObjects = array();
-       }
-
-       /**
-        * Serialization (sleep) helper.
-        *
-        * Removes properties of this object from serialization.
-        * This action is necessary, since there might be closures used
-        * in the accordant content objects (e.g. in FLUIDTEMPLATE) which
-        * cannot be serialized. It's fine to reset $this->contentObjects
-        * since elements will be recreated and are just a local cache,
-        * but not required for runtime logic and behaviour.
-        *
-        * @return array Names of the properties to be serialized
-        * @see http://forge.typo3.org/issues/36820
-        */
-       public function __sleep() {
-               // Use get_objects_vars() instead of
-               // a much more expensive Reflection:
-               $properties = get_object_vars($this);
-               if (isset($properties['contentObjects'])) {
-                       unset($properties['contentObjects']);
-               }
-               return array_keys($properties);
-       }
-
-       /**
         * Gets the 'getImgResource' hook objects.
         * The first call initializes the accordant objects.
         *
@@ -776,58 +775,14 @@ class ContentObjectRenderer {
         * Returns a new content object of type $name.
         *
         * @param string $name
-        * @return AbstractContentObject
+        * @return NULL|AbstractContentObject
         */
        public function getContentObject($name) {
-               $classMapping = array(
-                       'TEXT' => 'Text',
-                       'CASE' => 'Case',
-                       'CLEARGIF' => 'ClearGif',
-                       'COBJ_ARRAY' => 'ContentObjectArray',
-                       'COA' => 'ContentObjectArray',
-                       'COA_INT' => 'ContentObjectArrayInternal',
-                       'USER' => 'User',
-                       'USER_INT' => 'UserInternal',
-                       'FILE' => 'File',
-                       'FILES' => 'Files',
-                       'IMAGE' => 'Image',
-                       'IMG_RESOURCE' => 'ImageResource',
-                       'IMGTEXT' => 'ImageText',
-                       'CONTENT' => 'Content',
-                       'RECORDS' => 'Records',
-                       'HMENU' => 'HierarchicalMenu',
-                       'CTABLE' => 'ContentTable',
-                       'OTABLE' => 'OffsetTable',
-                       'COLUMNS' => 'Columns',
-                       'HRULER' => 'HorizontalRuler',
-                       'CASEFUNC' => 'Case',
-                       'LOAD_REGISTER' => 'LoadRegister',
-                       'RESTORE_REGISTER' => 'RestoreRegister',
-                       'FORM' => 'Form',
-                       'SEARCHRESULT' => 'SearchResult',
-                       'TEMPLATE' => 'Template',
-                       'FLUIDTEMPLATE' => 'FluidTemplate',
-                       'MULTIMEDIA' => 'Multimedia',
-                       'MEDIA' => 'Media',
-                       'SWFOBJECT' => 'ShockwaveFlashObject',
-                       'FLOWPLAYER' => 'FlowPlayer',
-                       'QTOBJECT' => 'QuicktimeObject',
-                       'SVG' => 'ScalableVectorGraphics',
-                       'EDITPANEL' => 'EditPanel',
-               );
-               $name = $classMapping[$name];
-               if (!array_key_exists($name, $this->contentObjects)) {
-                       $fullyQualifiedClassName = 'TYPO3\\CMS\\Frontend\\ContentObject\\' . $name . 'ContentObject';
-                       if (class_exists($fullyQualifiedClassName)) {
-                               $this->contentObjects[$name] = GeneralUtility::makeInstance(
-                                       $fullyQualifiedClassName,
-                                       $this
-                               );
-                       } else {
-                               $this->contentObjects[$name] = NULL;
-                       }
+               if (!isset($this->contentObjectClassMapping[$name])) {
+                       return NULL;
                }
-               return $this->contentObjects[$name];
+               $fullyQualifiedClassName = 'TYPO3\\CMS\\Frontend\\ContentObject\\' . $this->contentObjectClassMapping[$name] . 'ContentObject';
+               return GeneralUtility::makeInstance($fullyQualifiedClassName, $this);
        }
 
        /********************************************
index d9ccf99..784a838 100644 (file)
@@ -176,19 +176,6 @@ class ContentObjectRendererTest extends \TYPO3\CMS\Core\Tests\UnitTestCase {
         * @param string $name TypoScript name of content object
         * @param string $className Expected class name
         */
-       public function getContentObjectUsesExistingInstanceOfRequestedObjectType($name, $className) {
-               $fullClassName = 'TYPO3\\CMS\\Frontend\\ContentObject\\' . $className . 'ContentObject';
-               $contentObjectInstance = $this->getMock($fullClassName, array(), array(), '', FALSE);
-               $this->cObj->_set('contentObjects', array($className => $contentObjectInstance));
-               $this->assertSame($contentObjectInstance, $this->cObj->getContentObject($name));
-       }
-
-       /**
-        * @test
-        * @dataProvider getContentObjectValidContentObjectsDataProvider
-        * @param string $name TypoScript name of content object
-        * @param string $className Expected class name
-        */
        public function getContentObjectCallsMakeInstanceForNewContentObjectInstance($name, $className) {
                $fullClassName = 'TYPO3\\CMS\\Frontend\\ContentObject\\' . $className . 'ContentObject';
                $contentObjectInstance = $this->getMock($fullClassName, array(), array(), '', FALSE);