From c0e1b5fb31df1c2b22a3282d151d6ad475c6b20b Mon Sep 17 00:00:00 2001
From: Christian Kuhn <lolli@schwarzbu.ch>
Date: Sun, 22 Aug 2021 11:52:30 +0200
Subject: [PATCH] [TASK] Explicitly set ContentObjectRenderer in USER cObj

Handing around ContentObjectRenderer in the frontend
is tricky and intransparent. Especially class instances
created via ContentObjectRenderer->callUserFunction()
get an instance of it set through public property $cObj
in the consuming class. This is typically the case for
USER and USER_INT type frontend plugins.

The patch deprecates using this public property and
calls the method setContentObjectRenderer() instead.

This explicit method call is much easier to see,
break point and follow. The state handling of
ContentObjectRenderer is far from obvious, the
change helps debugging and refactoring this further.

Change-Id: Ib6ca11e7d6660dae0e82ad08ad1f9b39095e99da
Resolves: #94956
Releases: master
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/70717
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Tested-by: core-ci <typo3@b13.com>
Tested-by: Benni Mack <benni@typo3.org>
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Benni Mack <benni@typo3.org>
---
 .../master/Deprecation-94956-PublicCObj.rst   | 77 +++++++++++++++++++
 .../sysext/extbase/Classes/Core/Bootstrap.php | 17 +++-
 .../ContentObject/ContentObjectRenderer.php   | 16 +++-
 .../DataProcessing/LanguageMenuProcessor.php  | 13 +++-
 .../Classes/DataProcessing/MenuProcessor.php  | 13 +++-
 .../frontend/Classes/Imaging/GifBuilder.php   |  2 +
 .../Classes/Plugin/AbstractPlugin.php         | 13 ++++
 .../Fixtures/LinkHandlingController.php       | 14 +++-
 .../Classes/HrefLang/HrefLangGenerator.php    |  2 +-
 9 files changed, 157 insertions(+), 10 deletions(-)
 create mode 100644 typo3/sysext/core/Documentation/Changelog/master/Deprecation-94956-PublicCObj.rst

diff --git a/typo3/sysext/core/Documentation/Changelog/master/Deprecation-94956-PublicCObj.rst b/typo3/sysext/core/Documentation/Changelog/master/Deprecation-94956-PublicCObj.rst
new file mode 100644
index 000000000000..c50910267e07
--- /dev/null
+++ b/typo3/sysext/core/Documentation/Changelog/master/Deprecation-94956-PublicCObj.rst
@@ -0,0 +1,77 @@
+.. include:: ../../Includes.txt
+
+==================================
+Deprecation: #94956 - Public $cObj
+==================================
+
+See :issue:`94956`
+
+Description
+===========
+
+Frontend plugins receive an instance of :php:`ContentObjectRenderer` when
+called via :php:`ContentObjectRenderer->callUserFunction()`. This is
+typically the case for plugins called as :typoscript:`USER` or indirectly
+as :typoscript:`USER_INT` type.
+
+The instance of :php:`ContentObjectRenderer` has previously been set by
+declaring a public (!) property :php:`cObj` in the consuming class.
+
+Handing a :php:`ContentObjectRenderer` instance around this way is hard to
+follow and has thus been deprecated: Declaring :php:`public $cObj` should
+be avoided. Frontend plugins that need the current :php:`ContentObjectRenderer`
+should have a :php:`public setContentObjectRenderer()` method instead.
+
+
+Impact
+======
+
+Declaring :php:`public $cObj` in a class called by
+:php:`ContentObjectRenderer->callUserFunction()` triggers a PHP :php:`E_USER_DEPRECATED` error.
+
+
+Affected Installations
+======================
+
+Frontend extension classes that neither extend :php:`TYPO3\CMS\Frontend\Plugin\AbstractPlugin`
+("pibase") nor extbase :php:`TYPO3\CMS\Extbase\Mvc\Controller\ActionController`
+and have a public property :php:`cObj` are affected.
+
+
+Migration
+=========
+
+When instantiating the frontend plugin, :php:`ContentObjectRenderer->callUserFunction()`
+now checks for a public method :php:`setContentObjectRenderer()` to explicitly set
+an instance of the :php:`ContentObjectRenderer`.
+
+Many plugins may not need this instance at all. If the ContentObjectRenderer instance
+used within the plugin does not rely on further ContentObjectRenderer state, for instance
+if it only calls :php:`stdWrap()` or similar without using state like :typoscript:`LOAD_REGISTER`,
+the :php:`cObj` class property should be avoided and an own instance of  ContentObjectRenderer
+should be created.
+
+Classes that do rely on current ContentObjectRenderer state should adapt their code.
+
+Before::
+
+    class Foo
+    {
+        public $cObj;
+    }
+
+
+After::
+
+    class Foo
+    {
+        protected $cObj;
+
+        public function setContentObjectRenderer(ContentObjectRenderer $cObj): void
+        {
+            $this->cObj = $cObj;
+        }
+    }
+
+
+.. index:: Frontend, PHP-API, NotScanned, ext:frontend
diff --git a/typo3/sysext/extbase/Classes/Core/Bootstrap.php b/typo3/sysext/extbase/Classes/Core/Bootstrap.php
index a7f6223c8180..9c66953e10c3 100644
--- a/typo3/sysext/extbase/Classes/Core/Bootstrap.php
+++ b/typo3/sysext/extbase/Classes/Core/Bootstrap.php
@@ -50,12 +50,11 @@ class Bootstrap
     public static $persistenceClasses = [];
 
     /**
-     * Back reference to the parent content object
-     * This has to be public as it is set directly from TYPO3
+     * Set by UserContentObject (USER) via setContentObjectRenderer() in frontend
      *
-     * @var \TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer
+     * @var ContentObjectRenderer|null
      */
-    public $cObj;
+    protected ?ContentObjectRenderer $cObj = null;
 
     protected ContainerInterface $container;
     protected ConfigurationManagerInterface $configurationManager;
@@ -83,6 +82,16 @@ class Bootstrap
         $this->extbaseRequestBuilder = $extbaseRequestBuilder;
     }
 
+    /**
+     * Called for frontend plugins from UserContentObject via ContentObjectRenderer->callUserFunction().
+     *
+     * @param ContentObjectRenderer $cObj
+     */
+    public function setContentObjectRenderer(ContentObjectRenderer $cObj)
+    {
+        $this->cObj = $cObj;
+    }
+
     /**
      * Explicitly initializes all necessary Extbase objects by invoking the various initialize* methods.
      *
diff --git a/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php b/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
index 3cc0f750f62e..91dd439a767e 100644
--- a/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
+++ b/typo3/sysext/frontend/Classes/ContentObject/ContentObjectRenderer.php
@@ -5233,8 +5233,22 @@ class ContentObjectRenderer implements LoggerAwareInterface
                 }
                 $methodName = (string)$parts[1];
                 $callable = [$classObj, $methodName];
+
                 if (is_object($classObj) && method_exists($classObj, $parts[1]) && is_callable($callable)) {
-                    $classObj->cObj = $this;
+                    if (method_exists($classObj, 'setContentObjectRenderer') && is_callable([$classObj, 'setContentObjectRenderer'])) {
+                        $classObj->setContentObjectRenderer($this);
+                    } elseif (property_exists($classObj, 'cObj')) {
+                        trigger_error(
+                            'Setting public property "cObj" is deprecated since v11 and will be removed in v12. Use explicit setter'
+                            . ' "public function setContentObjectRenderer(ContentObjectRenderer $cObj)" if your plugin needs an instance of ContentObjectRenderer instead.',
+                            E_USER_DEPRECATED
+                        );
+                        // Note this will still fatal if that property is protected. There is no way to
+                        // detect property visibility in PHP without reflection, so we'll deal with this in v11.
+                        // Extensions should either drop the property altogether if they don't need current instance
+                        // of ContentObjectRenderer, or set the property to protected and use the setter above.
+                        $classObj->cObj = $this;
+                    }
                     $content = $callable($content, $conf, $this->getRequest());
                 } else {
                     $this->getTimeTracker()->setTSlogMessage('Method "' . $parts[1] . '" did not exist in class "' . $parts[0] . '"', LogLevel::ERROR);
diff --git a/typo3/sysext/frontend/Classes/DataProcessing/LanguageMenuProcessor.php b/typo3/sysext/frontend/Classes/DataProcessing/LanguageMenuProcessor.php
index 19b83dd1019e..fbd6714c91dc 100644
--- a/typo3/sysext/frontend/Classes/DataProcessing/LanguageMenuProcessor.php
+++ b/typo3/sysext/frontend/Classes/DataProcessing/LanguageMenuProcessor.php
@@ -50,7 +50,7 @@ class LanguageMenuProcessor implements DataProcessorInterface
      *
      * @var ContentObjectRenderer
      */
-    public $cObj;
+    protected ?ContentObjectRenderer $cObj = null;
 
     /**
      * The processor configuration
@@ -257,6 +257,17 @@ class LanguageMenuProcessor implements DataProcessorInterface
         $this->contentDataProcessor = GeneralUtility::makeInstance(ContentDataProcessor::class);
     }
 
+    /**
+     * This is called from UserContentObject via ContentObjectRenderer->callUserFunction()
+     * for nested menu items - those use a USER content object for getFieldAsJson().
+     *
+     * @param ContentObjectRenderer $cObj
+     */
+    public function setContentObjectRenderer(ContentObjectRenderer $cObj): void
+    {
+        $this->cObj = $cObj;
+    }
+
     /**
      * Get configuration value from processorConfiguration
      *
diff --git a/typo3/sysext/frontend/Classes/DataProcessing/MenuProcessor.php b/typo3/sysext/frontend/Classes/DataProcessing/MenuProcessor.php
index 4f1afe907117..a364eff3cd4a 100644
--- a/typo3/sysext/frontend/Classes/DataProcessing/MenuProcessor.php
+++ b/typo3/sysext/frontend/Classes/DataProcessing/MenuProcessor.php
@@ -68,7 +68,7 @@ class MenuProcessor implements DataProcessorInterface
      *
      * @var ContentObjectRenderer
      */
-    public $cObj;
+    protected ?ContentObjectRenderer $cObj = null;
 
     /**
      * The processor configuration
@@ -257,6 +257,17 @@ class MenuProcessor implements DataProcessorInterface
         $this->contentDataProcessor = GeneralUtility::makeInstance(ContentDataProcessor::class);
     }
 
+    /**
+     * This is called from UserContentObject via ContentObjectRenderer->callUserFunction()
+     * for nested menu items - those use a USER content object for getDataAsJson().
+     *
+     * @param ContentObjectRenderer $cObj
+     */
+    public function setContentObjectRenderer(ContentObjectRenderer $cObj): void
+    {
+        $this->cObj = $cObj;
+    }
+
     /**
      * Get configuration value from processorConfiguration
      *
diff --git a/typo3/sysext/frontend/Classes/Imaging/GifBuilder.php b/typo3/sysext/frontend/Classes/Imaging/GifBuilder.php
index 9d50af659488..cbb47849f6cd 100644
--- a/typo3/sysext/frontend/Classes/Imaging/GifBuilder.php
+++ b/typo3/sysext/frontend/Classes/Imaging/GifBuilder.php
@@ -97,6 +97,8 @@ class GifBuilder extends GraphicalFunctions
 
     /**
      * @var ContentObjectRenderer
+     * @deprecated Set to protected in v12.
+     * @todo: Signature in v12: protected ?ContentObjectRenderer $cObj = null;
      */
     public $cObj;
 
diff --git a/typo3/sysext/frontend/Classes/Plugin/AbstractPlugin.php b/typo3/sysext/frontend/Classes/Plugin/AbstractPlugin.php
index 938c3ba5ea82..ac6ef3b52fe0 100644
--- a/typo3/sysext/frontend/Classes/Plugin/AbstractPlugin.php
+++ b/typo3/sysext/frontend/Classes/Plugin/AbstractPlugin.php
@@ -44,6 +44,8 @@ class AbstractPlugin
      * The backReference to the mother cObj object set at call time
      *
      * @var ContentObjectRenderer
+     * @deprecated Set to protected in v12.
+     * @todo: Signature in v12: protected ?ContentObjectRenderer $cObj = null;
      */
     public $cObj;
 
@@ -246,6 +248,17 @@ class AbstractPlugin
         }
     }
 
+    /**
+     * This setter is called when the plugin is called from UserContentObject (USER)
+     * via ContentObjectRenderer->callUserFunction().
+     *
+     * @param ContentObjectRenderer $cObj
+     */
+    public function setContentObjectRenderer(ContentObjectRenderer $cObj): void
+    {
+        $this->cObj = $cObj;
+    }
+
     /**
      * Recursively looks for stdWrap and executes it
      *
diff --git a/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/LinkHandlingController.php b/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/LinkHandlingController.php
index 7495bb190399..ba3033c213f3 100644
--- a/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/LinkHandlingController.php
+++ b/typo3/sysext/frontend/Tests/Functional/SiteHandling/Fixtures/LinkHandlingController.php
@@ -30,9 +30,19 @@ use TYPO3\TestingFramework\Core\Functional\Framework\Frontend\RequestBootstrap;
 class LinkHandlingController
 {
     /**
-     * @var ContentObjectRenderer
+     * @var ContentObjectRenderer|null
      */
-    public $cObj;
+    protected ?ContentObjectRenderer $cObj = null;
+
+    /**
+     * This is called from UserContentObject via ContentObjectRenderer->callUserFunction().
+     *
+     * @param ContentObjectRenderer $cObj
+     */
+    public function setContentObjectRenderer(ContentObjectRenderer $cObj): void
+    {
+        $this->cObj = $cObj;
+    }
 
     /**
      * @return string
diff --git a/typo3/sysext/seo/Classes/HrefLang/HrefLangGenerator.php b/typo3/sysext/seo/Classes/HrefLang/HrefLangGenerator.php
index e6fc4805b3c8..741129faa95f 100644
--- a/typo3/sysext/seo/Classes/HrefLang/HrefLangGenerator.php
+++ b/typo3/sysext/seo/Classes/HrefLang/HrefLangGenerator.php
@@ -41,7 +41,7 @@ class HrefLangGenerator
      *
      * @var ContentObjectRenderer
      */
-    public $cObj;
+    protected $cObj;
 
     /**
      * @var LanguageMenuProcessor
-- 
GitLab