Project

General

Profile

Actions

Bug #86890

closed

AbstractTagBasedViewHelper recycles the TagBuilder

Added by Steffen Keuper about 6 years ago. Updated over 5 years ago.

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

100%

Estimated time:
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 5 (0 open5 closed)

Related to TYPO3 Core - Bug #87586: ext:form field properties get inherited to subsequent fieldsClosed2019-01-30

Actions
Related to TYPO3 Core - Bug #87811: EXT:form makes all fields requiredClosed2019-02-28

Actions
Related to TYPO3 Core - Bug #86164: CheckboxViewHelper sets automatically each checkbox on checked, if just one of them is checkedClosed2018-09-06

Actions
Has duplicate TYPO3 Core - Bug #86910: All text input fields get required="required" attributeClosed2018-11-12

Actions
Has duplicate TYPO3 Core - Bug #87430: Fields which are not required render with required attributeClosed2019-01-14

Actions
Actions #1

Updated by Jonas Eberle about 6 years ago

can reproduce (master).

Actions #2

Updated by Wouter Wolters about 6 years ago

  • Has duplicate Bug #86910: All text input fields get required="required" attribute added
Actions #3

Updated by Jonas Eberle about 6 years 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!

Actions #4

Updated by Jonas Eberle about 6 years 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.

Actions #5

Updated by Ralf Zimmermann about 6 years 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.

Actions #6

Updated by Steffen Keuper about 6 years 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)

Actions #7

Updated by Ralf Zimmermann about 6 years 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.

Actions #8

Updated by Ralf Zimmermann about 6 years 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.

Actions #9

Updated by Ralf Zimmermann about 6 years ago

  • Category changed from Form Framework to Fluid
Actions #10

Updated by Jonas Eberle about 6 years 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']);

Actions #11

Updated by Jonas Eberle about 6 years ago

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

Actions #12

Updated by Jonas Eberle about 6 years 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.

Actions #13

Updated by Josef Glatz almost 6 years ago

  • Has duplicate Bug #87430: Fields which are not required render with required attribute added
Actions #14

Updated by Ralf Zimmermann almost 6 years ago

  • Related to Bug #87586: ext:form field properties get inherited to subsequent fields added
Actions #15

Updated by Jonas Eberle almost 6 years ago

Fluid 2.6 is released which fixes the bug.

But it is not included in TYPO3 master yet.

Actions #16

Updated by Gerrit Code Review almost 6 years ago

  • Status changed from New to Under Review

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/59554

Actions #17

Updated by Gerrit Code Review almost 6 years ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/59554

Actions #18

Updated by Gerrit Code Review almost 6 years ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/59554

Actions #19

Updated by Gerrit Code Review almost 6 years ago

Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/59609

Actions #20

Updated by Gerrit Mohrmann almost 6 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 90 to 100
Actions #21

Updated by Ralf Zimmermann over 5 years ago

  • Related to Bug #87811: EXT:form makes all fields required added
Actions #22

Updated by Rémy DANIEL over 5 years ago

  • Related to Bug #86164: CheckboxViewHelper sets automatically each checkbox on checked, if just one of them is checked added
Actions #23

Updated by Benni Mack over 5 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF