[TASK] Move convert code from ViewHelperNode to VH #787
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This aims to solve a class structure issue that makes various things more complicated than they should be, and prevents fixing various issues with compiled templates. When a template is parsed, view helpers are represented by the ViewHelperNode in the syntax tree. At compile time, ViewHelperNode creates the PHP representation of a view helper in convert(). convert() now creates the "outer" stuff - it takes care of VH argument representation and of embedding compiled children. For the "inner" stuff, the specific ViewHelper compile() method is called. Various ViewHelpers thus override compile(): For instance, f:comment returns an empty string, since it does not want anything to be rendered. ViewHelperNode convert() however still compiles children and adds them to the output, even though this is not needed. This isn't great for f:comment, but it's a huge issue with f:if, which takes care of children on it's own, but ViewHelperNode happily adds them a *second* time, which can be massive with bigger f:then or f:else bodies. There have been attempts to change this already: The StopCompilingChildrenException can be thrown in compile() in VH's, but then arguments aren't compiled, either. The true reason for these headaches is that the "outer" stuff done in ViewHelperNode->convert() does not belong there: Single VH's should have full control over their compile representation, without ViewHelperNode->convert() assuming things. The patch does exactly this: Move the convert() body from ViewHelperNode to a new method in AbstractViewHelper, which allows us to not only solve the f:if / f:comment and friends issues by overriding that method again, but also allows us to phase out StopCompilingChildrenException again. Strictly speaking, this patch is breaking for two reasons: First, we're changing ViewHelperInterface, and second, this fails when an existing VH currently has a method called "convert()" already. We however consider this patch *not* being breaking *in practice* for these reasons, which is why we will add to next v2 minor: First, we did not find a single Flow or TYPO3 related case where VH classes fully implement ViewHelperInterface on their own and do not extend AbstractViewHelper directly or indirectly. We're now declaring ViewHelperInterface @internal along the way and say that VH's *must* extend AbstractViewHelper. This should be ok and there are various internal ViewHelperInterface methods already. Second, we could not find any public VH class with a method called "convert()" in neither Flow, nor TYPO3 extensions. It seems this risk is very low.
sbuerk
approved these changes
May 13, 2023
sbuerk
added a commit
that referenced
this pull request
Jun 18, 2023
Fluid have been designed without DependencyInjection and therefore shared classes, for example ViewHelper, in mind. Using systems can implement custom ViewHelperResolvers to determine how ViewHelper are resolved and created, using custom configuration. For example, the TYPO3 CMS implements such a custom ViewHelperResolver providing the ability to instanciate them throug Symfony DI. This allows shared ViewHelper instances now and may become more likely, especial for custom ViewHelper using the `CompileWithRenderStatic` trait. ViewHelper and ViewHelperNodes are connected through cycling references. This has not been a problem until the execution code for `convert()` have been moved from ViewHelperNode to the AbstractViewHelper (#787) to avoid creating unnessesary children codes, which may be duplicated and not needed. However, that enforced the code to work with the `ViewHelperNode` set in the `ViewHelper`. Using DI and shared `ViewHelper` now leads to the state, that the wrong `ViewHelperNode` instance is referenced in the shared `ViewHelper`. That compiled template code retrieving the wrong argument in some circumstances, for example in a for-loop when the viewhelper are additionaly used after the for-loop. Moving the `convert()` code from the node to the ViewHelper is valid, but `shared ViewHelper instances` are valid too. This change now updates the ViewHelperNode reference in the ViewHelper before dispatching to the ViewHelper. We do not have any evidence yet, but the same thing is done in the `ViewHelperNode->evaluate()` method. This is baked by a regression test. A cross check is added, but skipped for now as it makes the test green even without the fix. That points to another issue, test setup related or other, and have to be tackled in a dedicated change. The skipped test is added to the phpstan baseline. Used command(s): ```shell composer phpstan:generate-baseline ``` Resolves #804 Related: #787 1f1dc5a
sbuerk
added a commit
that referenced
this pull request
Jun 19, 2023
Fluid have been designed without DependencyInjection and therefore shared classes, for example ViewHelper, in mind. Using systems can implement custom ViewHelperResolvers to determine how ViewHelper are resolved and created, using custom configuration. For example, the TYPO3 CMS implements such a custom ViewHelperResolver providing the ability to instanciate them throug Symfony DI. This allows shared ViewHelper instances now and may become more likely, especial for custom ViewHelper using the `CompileWithRenderStatic` trait. ViewHelper and ViewHelperNodes are connected through cycling references. This has not been a problem until the execution code for `convert()` have been moved from ViewHelperNode to the AbstractViewHelper (#787) to avoid creating unnessesary children codes, which may be duplicated and not needed. However, that enforced the code to work with the `ViewHelperNode` set in the `ViewHelper`. Using DI and shared `ViewHelper` now leads to the state, that the wrong `ViewHelperNode` instance is referenced in the shared `ViewHelper`. That compiled template code retrieving the wrong argument in some circumstances, for example in a for-loop when the viewhelper are additionaly used after the for-loop. Moving the `convert()` code from the node to the ViewHelper is valid, but `shared ViewHelper instances` are valid too. This change now updates the ViewHelperNode reference in the ViewHelper before dispatching to the ViewHelper. We do not have any evidence yet, but the same thing is done in the `ViewHelperNode->evaluate()` method. This is baked by a regression test. A cross check is added, but skipped for now as it makes the test green even without the fix. That points to another issue, test setup related or other, and have to be tackled in a dedicated change. The skipped test is added to the phpstan baseline. Used command(s): ```shell composer phpstan:generate-baseline ``` Resolves #804 Related: #787 1f1dc5a
sbuerk
added a commit
that referenced
this pull request
Jun 19, 2023
Fluid have been designed without DependencyInjection and therefore shared classes, for example ViewHelper, in mind. Using systems can implement custom ViewHelperResolvers to determine how ViewHelper are resolved and created, using custom configuration. For example, the TYPO3 CMS implements such a custom ViewHelperResolver providing the ability to instanciate them throug Symfony DI. This allows shared ViewHelper instances now and may become more likely, especial for custom ViewHelper using the `CompileWithRenderStatic` trait. ViewHelper and ViewHelperNodes are connected through cycling references. This has not been a problem until the execution code for `convert()` have been moved from ViewHelperNode to the AbstractViewHelper (#787) to avoid creating unnessesary children codes, which may be duplicated and not needed. However, that enforced the code to work with the `ViewHelperNode` set in the `ViewHelper`. Using DI and shared `ViewHelper` now leads to the state, that the wrong `ViewHelperNode` instance is referenced in the shared `ViewHelper`. That compiled template code retrieving the wrong argument in some circumstances, for example in a for-loop when the viewhelper are additionaly used after the for-loop. Moving the `convert()` code from the node to the ViewHelper is valid, but `shared ViewHelper instances` are valid too. This change now updates the ViewHelperNode reference in the ViewHelper before dispatching to the ViewHelper. We do not have any evidence yet, but the same thing is done in the `ViewHelperNode->evaluate()` method. This is baked by a regression test. A cross check is added, but skipped for now as it makes the test green even without the fix. That points to another issue, test setup related or other, and have to be tackled in a dedicated change. The skipped test is added to the phpstan baseline. Used command(s): ```shell composer phpstan:generate-baseline ``` Resolves #804 Related: #787 1f1dc5a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This aims to solve a class structure issue that
makes various things more complicated than they
should be, and prevents fixing various issues
with compiled templates.
When a template is parsed, view helpers are
represented by the ViewHelperNode in the
syntax tree.
At compile time, ViewHelperNode creates the PHP
representation of a view helper in convert().
convert() now creates the "outer" stuff - it
takes care of VH argument representation and
of embedding compiled children. For the "inner"
stuff, the specific ViewHelper compile()
method is called.
Various ViewHelpers thus override compile():
For instance, f:comment returns an empty string,
since it does not want anything to be rendered.
ViewHelperNode convert() however still compiles
children and adds them to the output, even though
this is not needed. This isn't great for f:comment, but it's a huge issue with f:if, which takes care
of children on it's own, but ViewHelperNode happily adds them a second time, which can be massive
with bigger f:then or f:else bodies.
There have been attempts to change this already:
The StopCompilingChildrenException can be thrown
in compile() in VH's, but then arguments
aren't compiled, either.
The true reason for these headaches is that the
"outer" stuff done in ViewHelperNode->convert()
does not belong there: Single VH's should
have full control over their compile representation, without ViewHelperNode->convert() assuming things.
The patch does exactly this: Move the convert()
body from ViewHelperNode to a new method in
AbstractViewHelper, which allows us to not only
solve the f:if / f:comment and friends issues
by overriding that method again, but also allows
us to phase out StopCompilingChildrenException
again.
Strictly speaking, this patch is breaking for two
reasons: First, we're changing ViewHelperInterface, and second, this fails when an existing VH currently has a method called "convert()" already.
We however consider this patch not being breaking in practice for these reasons, which is why we will add to next v2 minor:
First, we did not find a single Flow or TYPO3 related case where VH classes fully implement ViewHelperInterface on their own and do not extend AbstractViewHelper
directly or indirectly. We're now declaring
ViewHelperInterface @internal along the way and say that VH's must extend AbstractViewHelper. This should be ok and there are various internal ViewHelperInterface methods already.
Second, we could not find any public VH class with a method called "convert()" in neither Flow, nor
TYPO3 extensions. It seems this risk is very low.