[BUGFIX] Prevent property spill-over to next call #419

jonaseberle · 2018-11-16T10:01:32Z

We have found an issue where consecutive viewhelpers take over properties from the previous one.

This is the TYPO3 bug report: https://forge.typo3.org/issues/86890

coveralls · 2018-11-16T10:05:17Z

Pull Request Test Coverage Report for Build 1106

2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
3 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.002%) to 97.431%

Files with Coverage Reduction	New Missed Lines	%
src/Core/Parser/TemplateParser.php	1	83.77%
src/View/AbstractTemplateView.php	2	77.33%

Totals
Change from base Build 1105:	0.002%
Covered Lines:	2617
Relevant Lines:	2686

💛 - Coveralls

mbrodala · 2018-11-16T10:19:32Z

src/Core/ViewHelper/AbstractTagBasedViewHelper.php

@@ -88,6 +88,9 @@ public function initializeArguments()
    public function initialize()
    {
        parent::initialize();
+        $this->tag->reset();
+        $this->tag->setTagName($this->tagName);


I wonder if this could be moved to setTagBuilder() and drop the $this->tagName injection from the constructor ...

Rather, I'd remove the line that sets the tag name in setTagBuilder since setter methods shouldn't mutate the object that is passed.

TagBuilders are already initialized with tag name as constructor argument ;)

In the future, tag based VH should become compilable in a way that TagBuilder is constructed outside and has one instance per execution, then passed to a static method that will render the tag. Then the TagBuilder reset and mutability can be reworked. Point being, if we aim for that then anything that treats the TagBuilder instance as a more stupid thing that needs init on every call, is imho a good thing. Even if it didn't also solve a bug ;)

NamelessCoder · 2018-11-16T13:55:54Z

src/Core/ViewHelper/AbstractTagBasedViewHelper.php

@@ -88,6 +88,9 @@ public function initializeArguments()
    public function initialize()
    {
        parent::initialize();
+        $this->tag->reset();
+        $this->tag->setTagName($this->tagName);


Rather, I'd remove the line that sets the tag name in setTagBuilder since setter methods shouldn't mutate the object that is passed.

TagBuilders are already initialized with tag name as constructor argument ;)

In the future, tag based VH should become compilable in a way that TagBuilder is constructed outside and has one instance per execution, then passed to a static method that will render the tag. Then the TagBuilder reset and mutability can be reworked. Point being, if we aim for that then anything that treats the TagBuilder instance as a more stupid thing that needs init on every call, is imho a good thing. Even if it didn't also solve a bug ;)

mbrodala · 2018-11-16T14:00:40Z

@NamelessCoder so are you fine with the change as is?

NamelessCoder · 2018-11-16T14:04:26Z

This is okay with me. The one risk I see is someone overriding setTagBuilder to do manipulation which in the first place is not the right approach - init done there would be reset after this patch. I've never seen an implementation of this in the wild so I'd say: safe!

mbrodala · 2018-11-16T14:07:13Z

OK, fine with me.

the-hotmann · 2019-01-30T19:44:58Z

This particular Problem is fixed at the attribute "required" at inputfields but still is here at the attribute "checked" at radio buttons.

All of my radiobuttons have this field "checked='checked'".
I insert 8 radio buttons and did not even select one to be selected but all of them got the attribute.
This leads to: the last of them will be standard selected.
If I select one to be selected this is having no effect as all already will have the attribute checked and also the last of them will be standard selected.

As I think this is related to the same issue I will just post this here.
I'm already using the latest fluidpages-master (and also vhs-master and flux-master)

Hmm.. after updating again to the latest fluidpages-master this issue is gone now.. sorry for this false alarm

NamelessCoder · 2020-10-31T11:59:31Z

A regression has been detected as a consequence of this patch.

TagBasedViewHelpers with data- prefixed arguments will now discard these prefixed attributes, since they are assigned to the TagBuilder through handleAdditionalArguments which is called before initializeArgumentsAndRender, but with this patch, initializeArgumentsAndRender will reset the TagBuilder which discards the previously registered attributes.

Test case:

    public function testTagBasedViewHelperWithDataPrefixedArgument()
    {
        $invoker = new ViewHelperInvoker();
        $viewHelper = new TagBasedTestViewHelper();
        $arguments = [
            'data-foo' => 'bar',
        ];
        $result = $invoker->invoke($viewHelper, $arguments, new RenderingContextFixture());
        $this->assertSame('<div data-foo="bar" />', $result);
    }

Test output:

Failed asserting that two strings are identical.
Expected :'<div data-foo="bar" />'
Actual   :'<div />'

Due to a regression in TYPO3#419, a TagBasedViewHelper which was called with one or more data- prefixed arguments would discard the prefixed arguments and the output would not contain the attribute. This patch fixes that regression by moving the handling of unregistered arguments to the initialize() function in order to make sure the additional attributes are handled *after* the reset() method has been called on TagBuilder. References: TYPO3#419

Due to a regression in #419, a TagBasedViewHelper which was called with one or more data- prefixed arguments would discard the prefixed arguments and the output would not contain the attribute. This patch fixes that regression by moving the handling of unregistered arguments to the initialize() function in order to make sure the additional attributes are handled *after* the reset() method has been called on TagBuilder. References: #419

Due to a regression in TYPO3#419, a TagBasedViewHelper which was called with one or more data- prefixed arguments would discard the prefixed arguments and the output would not contain the attribute. This patch fixes that regression by moving the handling of unregistered arguments to the initialize() function in order to make sure the additional attributes are handled *after* the reset() method has been called on TagBuilder. References: TYPO3#419

[BUGFIX] Prevent settings spill-over in cached context

c407370

mbrodala reviewed Nov 16, 2018

View reviewed changes

NamelessCoder approved these changes Nov 16, 2018

View reviewed changes

mbrodala approved these changes Nov 16, 2018

View reviewed changes

mbrodala merged commit 60545b6 into TYPO3:master Nov 16, 2018

benjaminpick mentioned this pull request Mar 5, 2019

Bug in parsley validation with no cache in2code-de/powermail#382

Closed

NamelessCoder pushed a commit that referenced this pull request Sep 8, 2019

[BUGFIX] Prevent settings spill-over in cached context (#419)

f2ffbba

NamelessCoder mentioned this pull request Oct 31, 2020

[BUGFIX] Restore support for data- prefixed arguments #528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX] Prevent property spill-over to next call #419

[BUGFIX] Prevent property spill-over to next call #419

jonaseberle commented Nov 16, 2018

coveralls commented Nov 16, 2018 •

edited

Loading

mbrodala Nov 16, 2018

NamelessCoder Nov 16, 2018

NamelessCoder Nov 16, 2018

mbrodala commented Nov 16, 2018

NamelessCoder commented Nov 16, 2018

mbrodala commented Nov 16, 2018

the-hotmann commented Jan 30, 2019 •

edited

Loading

NamelessCoder commented Oct 31, 2020

[BUGFIX] Prevent property spill-over to next call #419

[BUGFIX] Prevent property spill-over to next call #419

Conversation

jonaseberle commented Nov 16, 2018

coveralls commented Nov 16, 2018 • edited Loading

Pull Request Test Coverage Report for Build 1106

💛 - Coveralls

mbrodala Nov 16, 2018

Choose a reason for hiding this comment

NamelessCoder Nov 16, 2018

Choose a reason for hiding this comment

NamelessCoder Nov 16, 2018

Choose a reason for hiding this comment

mbrodala commented Nov 16, 2018

NamelessCoder commented Nov 16, 2018

mbrodala commented Nov 16, 2018

the-hotmann commented Jan 30, 2019 • edited Loading

NamelessCoder commented Oct 31, 2020

coveralls commented Nov 16, 2018 •

edited

Loading

the-hotmann commented Jan 30, 2019 •

edited

Loading