From 97aa45a6ed0fa3959cfa7d790077a4a5a9cef53f Mon Sep 17 00:00:00 2001 From: Simon Praetorius Date: Sun, 25 Aug 2024 00:20:18 +0200 Subject: [PATCH] [TASK] Use render() as primary ViewHelper method (#983) Until now it was best practice to differentiate between object-based ViewHelpers, using the object method `render()` as their primary render method, and static ViewHelpers, using the static method `renderStatic()`. This distinction has been introduced to Fluid mainly for performance reasons: With earlier PHP versions it was quite expensive performance-wise to create a new ViewHelper object for each ViewHelper call. However, PHP's performance characteristics for objects have changed quite a bit with newer versions, which questions the usefulness of this performance optimization nowadays. Our performance tests have shown that the performance gains are still existent today if you compare `renderStatic()` to `render()` ViewHelpers, especially if no further optimizations are applied to Fluid's core implementation. However, the impact is quite small and almost not measurable in real-world scenarios. In contrast to that, we see meaningful opportunities for the further development and optimization for Fluid if we streamline and simplify Fluid's APIs. It improves the developer experience if there's only one correct way to implement ViewHelpers and if all ViewHelpers use the same API. For Fluid internally, this opens up new opportunities of optimization and refactoring, which are currently not feasible to tackle. Ultimately, we have taken the decision to deprecate `renderStatic()` with Fluid v4. ViewHelpers should use `render()` as their primary render method from now on. This patch deprecates both the trait `CompileWithRenderStatic` and the usage of `renderStatic()` without any trait applied to the ViewHelper (which is probably only an edge case). Instead, the `ViewHelperInvoker` is called both for uncached and cached templates to initiate and perform each ViewHelper call. We will backport the `@deprecated` annotation on `CompileWithRenderStatic` to Fluid v2.15 to let users know in advance of this change. `AbstractConditionViewHelper` will be adjusted to this change in a follow-up patch. There will probably also be further code optimization patches that are made possible because of this decision, either still in Fluid v4 or later in Fluid v5, when `renderStatic()` will be removed completely. --- Documentation/Changelog/2.x.rst | 3 + Documentation/Changelog/4.x.rst | 5 ++ src/Core/ViewHelper/AbstractViewHelper.php | 81 ++++++++----------- .../Traits/CompileWithRenderStatic.php | 4 +- src/Core/ViewHelper/ViewHelperInterface.php | 21 ----- .../ViewHelpers/GroupedForViewHelperTest.php | 6 +- .../ViewHelpers/CompilableViewHelper.php | 1 + .../SharedStaticCompilableViewHelperTest.php | 2 + .../ViewHelper/AbstractViewHelperTest.php | 8 +- 9 files changed, 57 insertions(+), 74 deletions(-) diff --git a/Documentation/Changelog/2.x.rst b/Documentation/Changelog/2.x.rst index 3d5b16097..999cff587 100644 --- a/Documentation/Changelog/2.x.rst +++ b/Documentation/Changelog/2.x.rst @@ -9,6 +9,9 @@ Changelog 2.x 2.15 ---- +* Deprecation: Trait :php:`TYPO3Fluid\Fluid\Core\ViewHelper\Traits\CompileWithRenderStatic` + has been marked as deprecated. It will log a deprecation level error message when called in + Fluid v4. It will be removed in Fluid v5. * Deprecation: Trait :php:`TYPO3Fluid\Fluid\Core\ViewHelper\Traits\CompileWithContentArgumentAndRenderStatic` has been marked as deprecated. It will log a deprecation level error message when called in Fluid v4. It will be removed in Fluid v5. diff --git a/Documentation/Changelog/4.x.rst b/Documentation/Changelog/4.x.rst index bc1369636..8c49a6acc 100644 --- a/Documentation/Changelog/4.x.rst +++ b/Documentation/Changelog/4.x.rst @@ -25,8 +25,13 @@ Changelog 4.x :php:`TYPO3Fluid\Fluid\View\AbstractTemplateView::RENDERING_LAYOUT` * Breaking: Careful addition of method and property type hints throughout the system. This should be only mildly breaking and projects should be able to adapt easily. +* Deprecation: Trait :php:`TYPO3Fluid\Fluid\Core\ViewHelper\Traits\CompileWithRenderStatic` + now emits a E_USER_DEPRECATED level error. * Deprecation: Trait :php:`TYPO3Fluid\Fluid\Core\ViewHelper\Traits\CompileWithContentArgumentAndRenderStatic` now emits a E_USER_DEPRECATED level error. +* Deprecation: Static method :php:`renderStatic()` on ViewHelpers that don't use :php:`TYPO3Fluid\Fluid\Core\ViewHelper\Traits\CompileWithRenderStatic` + or :php:`TYPO3Fluid\Fluid\Core\ViewHelper\Traits\CompileWithContentArgumentAndRenderStatic` now emits a + E_USER_DEPRECATED level error. * Deprecation: Method :php:`TYPO3Fluid\Fluid\Core\ViewHelper\AbstractViewHelper->overrideArgument()` now emits a E_USER_DEPRECATED level error. * Deprecation: Calling method :php:`TYPO3Fluid\Fluid\Core\ViewHelper\AbstractTagBasedViewHelper->registerUniversalTagAttributes()` diff --git a/src/Core/ViewHelper/AbstractViewHelper.php b/src/Core/ViewHelper/AbstractViewHelper.php index e54dae609..babacecc2 100644 --- a/src/Core/ViewHelper/AbstractViewHelper.php +++ b/src/Core/ViewHelper/AbstractViewHelper.php @@ -258,33 +258,7 @@ public function initializeArgumentsAndRender() $this->validateArguments(); $this->initialize(); - return $this->callRenderMethod(); - } - - /** - * Call the render() method and handle errors. - * - * @return string the rendered ViewHelper - * @throws Exception - */ - protected function callRenderMethod() - { - if (method_exists($this, 'render')) { - return $this->render(); - } - if ((new \ReflectionMethod($this, 'renderStatic'))->getDeclaringClass()->getName() !== AbstractViewHelper::class) { - // Method is safe to call - will not recurse through ViewHelperInvoker via the default - // implementation of renderStatic() on this class. - return static::renderStatic($this->arguments, $this->buildRenderChildrenClosure(), $this->renderingContext); - } - throw new Exception( - sprintf( - 'ViewHelper class "%s" does not declare a "render()" method and inherits the default "renderStatic". ' . - 'Executing this ViewHelper would cause infinite recursion - please either implement "render()" or ' . - '"renderStatic()" on your ViewHelper class', - get_class($this), - ), - ); + return $this->render(); } /** @@ -309,8 +283,9 @@ public function renderChildren() } /** - * Helper which is mostly needed when calling renderStatic() from within - * render(). + * Creates a closure that renders a view helper's child nodes. It also takes + * into account the contentArgumentName, which if defined leads to that argument + * being rendered instead. * * No public API yet. * @@ -491,6 +466,36 @@ public function validateAdditionalArguments(array $arguments) } } + /** + * Main render method of the ViewHelper. Every modern ViewHelper implementation + * must implement this method. + * + * @todo Remove fallback implementation for renderStatic() and declare as abstract with Fluid v5 + * + * @return mixed + */ + public function render() + { + if (!method_exists(static::class, 'renderStatic')) { + throw new Exception( + sprintf( + 'ViewHelper class "%s" does not declare a "render()" method. Also, no implementation of "renderStatic"' . + 'could be found to use as fallback. Please implement "render()" on your ViewHelper class', + static::class, + ), + ); + } + + // This covers the edge case where a ViewHelper implements neither CompileWithRenderStatic nor + // CompileWithContentArgumentAndRenderStatic, but still uses renderStatic(). + trigger_error('renderStatic() has been deprecated and will be removed in Fluid v5.', E_USER_DEPRECATED); + return static::renderStatic( + $this->arguments, + $this->buildRenderChildrenClosure(), + $this->renderingContext, + ); + } + /** * You only should override this method *when you absolutely know what you * are doing*, and really want to influence the generated PHP code during @@ -528,7 +533,7 @@ public function validateAdditionalArguments(array $arguments) public function compile($argumentsName, $closureName, &$initializationPhpCode, ViewHelperNode $node, TemplateCompiler $compiler) { $execution = sprintf( - '%s::renderStatic(%s, %s, $renderingContext)', + '$renderingContext->getViewHelperInvoker()->invoke(%s::class, %s, $renderingContext, %s)', static::class, $argumentsName, $closureName, @@ -551,22 +556,6 @@ public function compile($argumentsName, $closureName, &$initializationPhpCode, V return $execution; } - /** - * Default implementation of static rendering; useful API method if your ViewHelper - * when compiled is able to render itself statically to increase performance. This - * default implementation will simply delegate to the ViewHelperInvoker. - * - * @param array $arguments - * @param \Closure $renderChildrenClosure - * @param RenderingContextInterface $renderingContext - * @return mixed - */ - public static function renderStatic(array $arguments, \Closure $renderChildrenClosure, RenderingContextInterface $renderingContext) - { - $viewHelperClassName = get_called_class(); - return $renderingContext->getViewHelperInvoker()->invoke($viewHelperClassName, $arguments, $renderingContext, $renderChildrenClosure); - } - /** * Save the associated ViewHelper node in a static public class variable. * called directly after the ViewHelper was built. diff --git a/src/Core/ViewHelper/Traits/CompileWithRenderStatic.php b/src/Core/ViewHelper/Traits/CompileWithRenderStatic.php index 2fc6c7257..cfb8f0b2c 100644 --- a/src/Core/ViewHelper/Traits/CompileWithRenderStatic.php +++ b/src/Core/ViewHelper/Traits/CompileWithRenderStatic.php @@ -14,7 +14,8 @@ * any ViewHelper that conforms to the `renderStatic` * method pattern. * - * @todo add missing types with Fluid v5 + * @deprecated Will be removed in v5. The non-static render() method + * should be used instead */ trait CompileWithRenderStatic { @@ -27,6 +28,7 @@ trait CompileWithRenderStatic */ public function render() { + trigger_error('CompileWithRenderStatic has been deprecated and will be removed in Fluid v5.', E_USER_DEPRECATED); return static::renderStatic( $this->arguments, $this->buildRenderChildrenClosure(), diff --git a/src/Core/ViewHelper/ViewHelperInterface.php b/src/Core/ViewHelper/ViewHelperInterface.php index 75db65858..b2bcbd4e3 100644 --- a/src/Core/ViewHelper/ViewHelperInterface.php +++ b/src/Core/ViewHelper/ViewHelperInterface.php @@ -77,27 +77,6 @@ public function handleAdditionalArguments(array $arguments); */ public function validateAdditionalArguments(array $arguments); - /** - * Here follows a more detailed description of the arguments of this function: - * - * $arguments contains a plain array of all arguments this ViewHelper has received, - * including the default argument values if an argument has not been specified - * in the ViewHelper invocation. - * - * $renderChildrenClosure is a closure you can execute instead of $this->renderChildren(). - * It returns the rendered child nodes, so you can simply do $renderChildrenClosure() to execute - * it. It does not take any parameters. - * - * $renderingContext contains references to the VariableProvider and the - * ViewHelperVariableContainer. - * - * @param array $arguments - * @param \Closure $renderChildrenClosure - * @param RenderingContextInterface $renderingContext - * @return mixed the resulting value from the ViewHelper - */ - public static function renderStatic(array $arguments, \Closure $renderChildrenClosure, RenderingContextInterface $renderingContext); - /** * Called when being inside a cached template. * diff --git a/tests/Functional/ViewHelpers/GroupedForViewHelperTest.php b/tests/Functional/ViewHelpers/GroupedForViewHelperTest.php index 92cdacdca..8498fbfda 100644 --- a/tests/Functional/ViewHelpers/GroupedForViewHelperTest.php +++ b/tests/Functional/ViewHelpers/GroupedForViewHelperTest.php @@ -19,7 +19,7 @@ final class GroupedForViewHelperTest extends AbstractFunctionalTestCase { #[Test] - public function renderStaticThrowsExceptionWhenEachIsNotTraversable(): void + public function renderThrowsExceptionWhenEachIsNotTraversable(): void { $this->expectException(Exception::class); $this->expectExceptionCode(1253108907); @@ -33,7 +33,7 @@ public function renderStaticThrowsExceptionWhenEachIsNotTraversable(): void } #[Test] - public function renderStaticThrowsExceptionWhenEachIsOneDimensionalArray(): void + public function renderThrowsExceptionWhenEachIsOneDimensionalArray(): void { $this->expectException(Exception::class); $this->expectExceptionCode(1253120365); @@ -47,7 +47,7 @@ public function renderStaticThrowsExceptionWhenEachIsOneDimensionalArray(): void } #[Test] - public function renderStaticReturnsEmptyStringWhenEachIsNull(): void + public function renderReturnsEmptyStringWhenEachIsNull(): void { $source = ''; diff --git a/tests/Functional/ViewHelpers/StaticCacheable/Fixtures/ViewHelpers/CompilableViewHelper.php b/tests/Functional/ViewHelpers/StaticCacheable/Fixtures/ViewHelpers/CompilableViewHelper.php index 20d0ce01b..3b230ae5b 100644 --- a/tests/Functional/ViewHelpers/StaticCacheable/Fixtures/ViewHelpers/CompilableViewHelper.php +++ b/tests/Functional/ViewHelpers/StaticCacheable/Fixtures/ViewHelpers/CompilableViewHelper.php @@ -15,6 +15,7 @@ final class CompilableViewHelper extends AbstractViewHelper { + // We leave this here as a test case for the deprecated feature use CompileWithRenderStatic; /** diff --git a/tests/Functional/ViewHelpers/StaticCacheable/SharedStaticCompilableViewHelperTest.php b/tests/Functional/ViewHelpers/StaticCacheable/SharedStaticCompilableViewHelperTest.php index 757a2a7d3..3f51308fe 100644 --- a/tests/Functional/ViewHelpers/StaticCacheable/SharedStaticCompilableViewHelperTest.php +++ b/tests/Functional/ViewHelpers/StaticCacheable/SharedStaticCompilableViewHelperTest.php @@ -9,6 +9,7 @@ namespace TYPO3Fluid\Fluid\Tests\Functional\ViewHelpers\StaticCacheable; +use PHPUnit\Framework\Attributes\IgnoreDeprecations; use PHPUnit\Framework\Attributes\Test; use TYPO3Fluid\Fluid\Core\ViewHelper\ViewHelperInterface; use TYPO3Fluid\Fluid\Core\ViewHelper\ViewHelperResolver; @@ -22,6 +23,7 @@ final class SharedStaticCompilableViewHelperTest extends AbstractFunctionalTestCase { #[Test] + #[IgnoreDeprecations] public function renderWithSharedCompilableViewHelper(): void { // TYPO3 implements a custom ViewHelperResolver to provide DI-able ViewHelper instances. This allows diff --git a/tests/Unit/Core/ViewHelper/AbstractViewHelperTest.php b/tests/Unit/Core/ViewHelper/AbstractViewHelperTest.php index 85b2f6881..7539586a6 100644 --- a/tests/Unit/Core/ViewHelper/AbstractViewHelperTest.php +++ b/tests/Unit/Core/ViewHelper/AbstractViewHelperTest.php @@ -10,6 +10,7 @@ namespace TYPO3Fluid\Fluid\Tests\Unit\Core\ViewHelper; use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\IgnoreDeprecations; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use TYPO3Fluid\Fluid\Core\Compiler\TemplateCompiler; @@ -144,14 +145,15 @@ public function testCompileReturnsAndAssignsExpectedPhpCode(): void $subject = $this->getMockBuilder(AbstractViewHelper::class)->onlyMethods([])->getMock(); $result = $subject->compile('foobar', 'baz', $init, $node, new TemplateCompiler()); self::assertEmpty($init); - self::assertEquals(get_class($subject) . '::renderStatic(foobar, baz, $renderingContext)', $result); + self::assertEquals('$renderingContext->getViewHelperInvoker()->invoke(' . get_class($subject) . '::class, foobar, $renderingContext, baz)', $result); } #[Test] + #[IgnoreDeprecations] public function testCallRenderMethodCanRenderViewHelperWithoutRenderMethodAndCallsRenderStatic(): void { $subject = new RenderMethodFreeViewHelper(); - $method = new \ReflectionMethod($subject, 'callRenderMethod'); + $method = new \ReflectionMethod($subject, 'render'); $subject->setRenderingContext(new RenderingContext()); $result = $method->invoke($subject); self::assertSame('I was rendered', $result); @@ -162,7 +164,7 @@ public function testCallRenderMethodOnViewHelperWithoutRenderMethodWithDefaultRe { $this->expectException(Exception::class); $subject = new RenderMethodFreeDefaultRenderStaticViewHelper(); - $method = new \ReflectionMethod($subject, 'callRenderMethod'); + $method = new \ReflectionMethod($subject, 'render'); $subject->setRenderingContext(new RenderingContext()); $method->invoke($subject); }