contao
Fix missing template fields for forms
IMHO that's not a bug in the core. The Form class does set the attributes and novalidate parameter.
its a bug because the current HTML5 templates work just fine without these, but the Twig templates break it.
No, it's not a bug only because the old template system suppressed an error?
No that's not really an error within the old template system. In the old template system variables never have to be defined. It is specifically allowed to access variables that do not exist - otherwise we'd have to throw an exception within __get() for example.
But not by design and simply because older PHP versions (or now not using strict types) did not complain. Or am I missing something?
It is by design. Template::__get() returns the value in $this->arrData if set and null otherwise. There will never be an error or exception if you do $this->variableThatHasNotBeenSet.
With that argumentation, you would need |default on every variable. This is just wrong.
With that argumentation, you would need
|defaulton every variable. This is just wrong.
yes, which will be necessary for all HTML5-to-Twig templates, otherwise people will get random errors now that they did not get with the HTML5-versions.
…people will get random errors now…
I don’t think that is correct. The errors are not random but instead point to an error in the template or in the controller and should be fixed appropriately. As these errors are suppressed in prod I don’t see a BC issue here. And templates are not BC anyways.
I’d prefer to fix it this in Haste instead. However I’m fine with merging this PR as the added |defaults do not seem to break functionality (the affected variables can be interpreted as being optional IMO) and it improves BC for the legacy templates.
But I want to note that I’m against adding |default to any new (non-legacy) Twig templates as these undefined variable errors are actually an important feature of Twig that we should definitely not bypass everywhere.
As these errors are suppressed in
prodI don’t see a BC issue here
But still, it will be unexpected in dev, as the PHP template would not generate the same error.
But I want to note that I’m against adding
|defaultto any new (non-legacy) Twig templates as these undefined variable errors are actually an important feature of Twig that we should definitely not bypass everywhere.
Of course, this is only about legacy templates. In regular Twig templates this issue cannot occur, as there was never this expectation / behaviour to begin with.
As these errors are suppressed in
prodI don’t see a BC issue hereBut still, it will be unexpected in
dev, as the PHP template would not generate the same error.
If required variables for the template are missing, the error is expected (and important) IMO. This can be seen as a new feature of 5.7.
I agree it must not be added to every variable, but these are optional for sure, otherwise it would not have worked with Haste. There are a lot of cases of mandatory variables, e.g. a label for a button cannot be empty. I would just fix them as we find them, same as we did with the PHP8 array property issues.