Bug #86890

AbstractTagBasedViewHelper recycles the TagBuilder

Added by Steffen Keuper 10 days ago. Updated 2 days ago.

Status:
New
Priority:
Must have
Assignee:
-
Category:
Fluid
Target version:
-
Start date:
2018-11-08
Due date:
% Done:

90%

TYPO3 Version:
9
PHP Version:
7.2
Tags:
Complexity:
Is Regression:
Sprint Focus:

Description

The same ViewHelper instance is used for multiple generations of Tags in \TYPO3Fluid\Fluid\Core\ViewHelper\AbstractTagBasedViewHelper.
This causes arguments like required to get set in the generated HTML even though it wasn't set in the ViewHelper call.
As a result I'm getting required fields which shouldn't be required and aren't marked like required either.

Screenshot of the var_dump of $this->tag directly at the top of the render function in the TextfieldViewHelper:

It only occurred in 30-50% of the cases when I first accessed the page after clearing the cache in the Install Tool for which I have no possible explanation.

I made a simple test form to reproduce this bug(form fields after the first required field also get the required tag even though they may not be required):


renderingOptions:
  submitButtonLabel: Submit
type: Form
identifier: testForm
label: 'Test Form'
prototypeName: standard
renderables:
  -
    renderingOptions:
      previousButtonLabel: 'Previous step'
      nextButtonLabel: 'Next step'
    type: Page
    identifier: page-1
    label: Step
    renderables:
      -
        type: GridRow
        identifier: gridrow-1
        label: 'Grid: Row'
        renderables:
          -
            defaultValue: ''
            type: Text
            identifier: text-1
            label: 'Text 1'
            validators:
              -
                identifier: NotEmpty
            properties:
              fluidAdditionalAttributes:
                required: required
                placeholder: 'text 1'
          -
            defaultValue: ''
            type: Text
            identifier: text-2
            label: Text
            properties:
              fluidAdditionalAttributes:
                placeholder: 'text 2'
      -
        type: GridRow
        identifier: gridrow-2
        label: 'Grid: Row'
        renderables:
          -
            defaultValue: ''
            type: Text
            identifier: text-3
            label: Text
            properties:
              fluidAdditionalAttributes:
                placeholder: 'text 3'
          -
            defaultValue: ''
            type: Text
            identifier: text-4
            label: Text
            properties:
              fluidAdditionalAttributes:
                placeholder: 'text 4'
                required: required
            validators:
              -
                identifier: NotEmpty
      -
        type: GridRow
        identifier: gridrow-3
        label: 'Grid: Row'

I also couldn't reproduce the bug directly with Fluid templates which is why I decided to use the Form Framework Category.


Related issues

Duplicated by TYPO3 Core - Bug #86910: All text input fields get required="required" attribute Closed 2018-11-12

History

#1 Updated by Jonas Eberle 10 days ago

can reproduce (master).

#2 Updated by Wouter Wolters 6 days ago

  • Duplicated by Bug #86910: All text input fields get required="required" attribute added

#3 Updated by Jonas Eberle 6 days ago

Minimal test case that renders the erroneous

required=required

on the second element.

type: Form
identifier: testForm
prototypeName: standard
renderables:
  -
    type: Page
    identifier: page-1
    renderables:
      -
        type: Text
        identifier: text-1
        properties:
          fluidAdditionalAttributes:
            required: required
      -
        type: Text
        identifier: text-2

Mind that it can change behaviour on the 2nd reload after a cache clear and that it is not consistent!

#4 Updated by Jonas Eberle 6 days ago

... we just found out that wrapping the fields inside GridRow->Fieldset->... greatly reduces the probability of seeing the error... It is getting weird...

As such you can also reproduce it with the introduction distribution aka bootstrap_package with typo3conf/ext/bootstrap_package/Resources/Private/Forms/Contact.form.yaml on current master, although I had to cache clear+reload ~10 times. Btw: Testing is easier if you remove the template typo3conf/ext/bootstrap_package/Resources/Private/Templates/Form/Form.html (or at least the line novalidate=) as then you do not have to use browser dev tools to see the error but you get erroneous HTML5-validation straight away.

Fun fact: typo3conf/ext/bootstrap_package/Resources/Private/Templates/Form/Form.html does only add the line novalidate= to the default Form.html template.

This actually is a workaround for this bug. You get wrong HTML then, but it does not interfere with your HTML5-validation.

#5 Updated by Ralf Zimmermann 4 days ago

I can reproduce this with the testcase from #86890#note-3 with TYPO3 9.5.2-dev. TYPO3 8.7-x is not affected.
The issue occurs after clearing the system cache and after a reload of the form page.
After the fluid templates have been cached, the error disappears.
I'll try to find a solution.

#6 Updated by Steffen Keuper 4 days ago

  • Description updated (diff)

Sometimes the error disappears after refreshing the page after the first access, sometimes it doesn't, couldn't find any pattern what causes the irregularity of that.
The result on the 2nd access doesn't change on further reloads.

I can't reproduce the error in plain fluid(also tried using the original form partials/ViewHelpers for that but without results), so it looks like something form specific to me.
(The initial workaround I posted broke some unit tests so I removed it)

#7 Updated by Ralf Zimmermann 4 days ago

It is reproducible with plain fluid:

This works as expected:

<f:form>
    <f:form.textfield additionalAttributes="{required: 'required'}" />
    <f:form.textfield additionalAttributes="{}" />
</f:form>

Result:

<form action="/" method="post">
    <input required="required" type="text" name="">
    <input type="text" name="">
</form>

but with a foreach:

<f:form>
    <f:for each="{0: {required: 'required'}, 1: '{}'}" as="attribs">
        <f:form.textfield additionalAttributes="{attribs}" />
    </f:for>
</f:form>

Result:

<form action="/" method="post">
    <input required="required" type="text" name="">
    <input required="required" type="text" name="">
</form>

You can disable the fluid template cache to reproduce the issue all the time:

$GLOBALS['TYPO3_CONF_VARS']['SYS']['caching']['cacheConfigurations']['fluid_template']['backend'] = \TYPO3\CMS\Core\Cache\Backend\NullBackend::class;

I continue to debug this.

#8 Updated by Ralf Zimmermann 4 days ago

The TYPO3 CMS AbstractTagBasedViewHelper implementation (https://github.com/TYPO3/TYPO3.CMS/blob/TYPO3_8-7/typo3/sysext/fluid/Classes/Core/ViewHelper/AbstractTagBasedViewHelper.php) is removed since (https://github.com/TYPO3/TYPO3.CMS/commit/533a1a349e99570a9e59357f43277add5a74b636#diff-37c6a16d3e2de976d66e17b797cda2ba).
The initialize() method differs a little bit.

https://github.com/TYPO3/TYPO3.CMS/blob/TYPO3_8-7/typo3/sysext/fluid/Classes/Core/ViewHelper/AbstractTagBasedViewHelper.php#L83 vs https://github.com/TYPO3/Fluid/blob/2.5.4/src/Core/ViewHelper/AbstractTagBasedViewHelper.php#L88

Within typo3/sysext/fluid/Classes/Core/ViewHelper/AbstractTagBasedViewHelper.php (TYPO3 8.7) there was this code

$this->tag->reset();
$this->tag->setTagName($this->tagName);

which reset the tagbuilder.
Now this reset will never happened at least with the usage of the Textfield ViewHelper.

#9 Updated by Ralf Zimmermann 4 days ago

  • Category changed from Form Framework to Fluid

#10 Updated by Jonas Eberle 2 days ago

Amazing find, Ralf!

For the current fluid in master (2.5.2) this fixes it for me:

--- fluid_orig/src/Core/ViewHelper/AbstractTagBasedViewHelper.php       2018-11-16 10:13:58.885022862 +0100
+++ fluid/src/Core/ViewHelper/AbstractTagBasedViewHelper.php    2018-11-16 10:14:14.961016742 +0100
@@ -88,6 +88,8 @@
     public function initialize()
     {
         parent::initialize();
+        $this->tag->reset();
+        $this->tag->setTagName($this->tagName);

         if ($this->hasArgument('additionalAttributes') && is_array($this->arguments['additionalAttributes'])) {
             $this->tag->addAttributes($this->arguments['additionalAttributes']);

#11 Updated by Jonas Eberle 2 days ago

Pull request is upstream: https://github.com/TYPO3/Fluid/pull/419

#12 Updated by Jonas Eberle 2 days ago

  • % Done changed from 0 to 90

Patch is merged https://github.com/TYPO3/Fluid/commit/60545b6cbb6d66de7413ec515c8ae64ef84414b7

I can't say how the roadmap is from here (bug fixed upstream, but upstream has added features, maybe breaking, too). I hope somebody can jump in and provide some pointers.

Also available in: Atom PDF