Bug #86890
closedAbstractTagBasedViewHelper recycles the TagBuilder
100%
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.
Updated by Wouter Wolters about 6 years ago
- Has duplicate Bug #86910: All text input fields get required="required" attribute added
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!
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.
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.
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)
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.
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.
Updated by Ralf Zimmermann about 6 years ago
- Category changed from Form Framework to Fluid
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']);
Updated by Jonas Eberle about 6 years ago
Pull request is upstream: https://github.com/TYPO3/Fluid/pull/419
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.
Updated by Josef Glatz almost 6 years ago
- Has duplicate Bug #87430: Fields which are not required render with required attribute added
Updated by Ralf Zimmermann almost 6 years ago
- Related to Bug #87586: ext:form field properties get inherited to subsequent fields added
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.
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
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
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
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
Updated by Gerrit Mohrmann almost 6 years ago
- Status changed from Under Review to Resolved
- % Done changed from 90 to 100
Applied in changeset 3469afec25bf599e7c5782a5ac6f4d71ffcf946d.
Updated by Ralf Zimmermann over 5 years ago
- Related to Bug #87811: EXT:form makes all fields required added
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