TYPO3
Boolean values in TagBuilder attribute and rendering result
Right now
$tagBuilder->addAttribute('attr', NULL); // ->getAttribute('attr') returns NULL, and ->render() results attr=""
$tagBuilder->addAttribute('attr', TRUE); // ->getAttribute('attr') returns 'attr', and ->render() results attr="attr"
$tagBuilder->addAttribute('attr', FALSE); // ->getAttribute('attr') returns NULL as the attribute has been removed, and ->render() results nothing.
I think it's a little messing. What I expected are:
$tagBuilder->addAttribute('attr', NULL); // removes the attribute, and ->render() results nothing
$tagBuilder->addAttribute('attr', TRUE); // ->getAttribute('attr') returns TRUE, and ->render() results attr or attr="attr"
$tagBuilder->addAttribute('attr', FALSE); // ->getAttribute('attr') returns FALSE, and ->render() results nothing or maybe attr="".
So for ->render(), considering the output for HTML5 and XHTML, I think it's better to introduce a new property and affect the final result according to its value.
Thanks!
Sorry, after thinking a while, I think it's better to render nothing for $tagBuilder->addAttribute('attr', FALSE);, rather than attr="", so both $tagBuilder->addAttribute('attr', NULL); and $tagBuilder->addAttribute('attr', FALSE); will have the same result.
So to sum up, you would like to change the behavior for NULL values, everything else is fine?
I think we previously chose to leave the NULL behavior as-is to prevent a breaking change. If we would want to change that, we would need to investigate possible unexpected outcomes, for example in the TYPO3 core. But it would be feasible to do this in Fluid v5, I think.
Would you be able to provide a PR for that?
Can you give more context about your API usage? If this was purely PHP you could simply not call addAttribute() if you want to have an attribute skipped. So I'd assume you have another layer on top where such conditions are not as easy to implement.
@s2b Yes, if we look at the result, only the behavior for NULL is changed. But in the process, there are also differences in how TRUE and FALSE are handled.
The PR has been submit, see #1230, but it will fail one test (for sure). Feel free to optimize my changes if needed.
@mbrodala Originally, I was tring to figure out how to output <input disalbed /> instead of <input disabled="disabled" /> in my project. But soon, I noticed that although disalbed is defined as boolean in VH, it gets added to TagBuilder via
$this->tag->addAttribute('disabled', 'disabled');
And any boolean value added to TagBuilder would be modified again by TagBuilder, which feels inconsistent.
If my suggestion could be accepted, boolean attributes like this could simply be registered in VH with $this->registerTagAttribute() and passed on to TagBuilder, so no extra code in render(). This could also help the issue #90181 on forge.
I'm still confused about the fixation to switch from disabled="disabled" to just disabled, I've heard this one several times. Isn't it just the same result, only a different syntax?
Indeed, the former purely exists for XHTML where attributes always must have values. I'd argue that this is barely relevant nowadays, now that everything uses HTML5.
Yup, in HTML5 foo="foo" is perfectly fine. But the issue I understood is different: completely skip the attribute if null is passed as value. Currently this would lead to foo="" instead, thus an "active" attribute.
E.g. by setting disabled="" on a form field, you basically write disabled or disabled="disabled", thus the field is disabled. Only by omitting the attribute the field is enabled. And this can be achieved in the TagBuilder only by not calling addattribute() as mentioned above.
Yes, that's why I asked if it's mainly about null. Let's not mix code style choices with actual issues that have consequences in functionality.
So do you mean null should behaviors the same as false, and remove all other changes?
In a first step, yes. The null change would be breaking and needs to be evaluated, e. g. against the TYPO3 Core. This is a runtime change for ViewHelpers, which is always a bit tricky to test (both for us and users out there, who e. g. perform a TYPO3 Upgrade), so we need to be sure of the consequences.
The boolean changes can be discussed afterwards and added in a follow-up PR.
Hm, from what I see this should already work?
$tagBuilder->setTagName('input');
$tagBuilder->ignoreEmptyAttributes(true);
$tagBuilder->addAttribute('disabled', '');
$tagBuilder->ignoreEmptyAttributes(false);
$tagBuilder->addAttribute('readonly', '');
$tagBuilder->render(); // <input readonly=""/>
The "ignore empty attributes" flag can be set at any point, it will only affect the attributes added afterwards. If you unset it again, following empty attributes will be added again.
Caveat: you must use an empty string, null won't work ATM:
https://github.com/TYPO3/Fluid/blob/83a314972330ee876c2028c0cb104333908a8172/src/Core/ViewHelper/TagBuilder.php#L167 https://github.com/TYPO3/Fluid/blob/83a314972330ee876c2028c0cb104333908a8172/src/Core/ViewHelper/TagBuilder.php#L232
From what I understand, this is the actual issue:
<f:form.textfield someattribute="{null}" />
will lead to:
<input type="text" someattribute="" />
instead of removing that attribute altogether. This comment shows that we already thought about that but decided to not fix it at that point:
https://github.com/TYPO3/Fluid/blob/83a314972330ee876c2028c0cb104333908a8172/src/Core/ViewHelper/TagBuilder.php#L216
That's what I meant with the context in my initial remark. If you directly use the TagBuilder API as suggested in the OP, there is no problem.
But yeah, the case you describe doesn't allow for any behavior adjustments since the TagBuilder is used and configured internally.
Well, kind of. It should not be necessary to skip empty attributes if you supply null imho.
Whatever. Let's create a small patch for Fluid v5 (with adjusted test cases, [!!!] and changelog entry) to address that issue, validate that it works with projects out there, and move on. :)
Thank you Mathias and Simon! Feel free to let me know if anything I can help further on this issue.
Thank you Mathias and Simon! Feel free to let me know if anything I can help further on this issue.
This would mean that your current PR would need to be split into two:
- breaking change for
null - non-breaking task for adjusted markup for
trueandfalse