CakePHP-Bootstrap / cakephp3-bootstrap-helpers

CakePHP 3.x Helpers for Bootstrap 3 and 4.
https://holt59.github.io/cakephp3-bootstrap-helpers/
MIT License
130 stars 79 forks source link

FormHelper :: control(), less forceful defaults #182

Closed starlocke closed 5 years ago

starlocke commented 5 years ago

Per issue #181.

After migrating from CakePHP 3.4 to 3.7 / 3.8, it took me a while to debug the fact that the ('required' => false) configuration for $options imposed by this package actually TURNS OFF the "required" decorators.

CakePHP's evolved "_magicOptions" function uses:

        if (!isset($options['required']) && $options['type'] !== 'hidden') {
            $options['required'] = $context->isRequired($fieldName);
        }

... which expects NULL values (ie: neutral, not-yet-decided) instead of FALSE values (already decided in the negative direction).

Holt59 commented 5 years ago

Thanks for the feedback and the PR. Cannot check this right now so waiting on travis.

starlocke commented 5 years ago

Hadn't looked at how far backward-compatibility goes with the change. Is there a process for backporting that should be followed here?

It was unclear why ['prepend', 'append', 'help', 'inline'] were false in the first place, as no comment was around. I had the gut feeling that a similar "be neutral" default value and check later-on might be useful around those options, too. Should they stick to being false by default, instead? I hadn't yet fully traced out how they're used.

Holt59 commented 5 years ago

Hadn't looked at how far backward-compatibility goes with the change. Is there a process for backporting that should be followed here?

Unit tests should check that the changes made do not impact these helpers for the currently targeted version (3.7.x), but these do not check the impact on the standard CakePHP helpers.

Since the changes you made are already present in CakePHP 3.7.9 (https://github.com/cakephp/cakephp/blob/3.7.9/src/View/Helper/FormHelper.php#L1161), this should be good.

It was unclear why ['prepend', 'append', 'help', 'inline'] were false in the first place, as no comment was around. I had the gut feeling that a similar "be neutral" default value and check later-on might be useful around those options, too. Should they stick to being false by default, instead? I hadn't yet fully traced out how they're used.

These are (partially) documented in the control() method documentation. As I understand it, the new way of setting null is to distinguish between automatic mode (with _magicOptions) and explicit false.

Defaulting preprend, append and help to null makes sense since these options are meant to accept a string or an array that is used, but inline is a pure boolean check with no magic-option associated, so setting it to null does not make sense.

If you could update the PR to default inline to false instead of null, I would accept it.

Holt59 commented 5 years ago

I merged these changes to master with 63064f255ce1479fffe468a548acf0ff99a1c289 and 4.0.1-alpha with 4296ee78b4db85922ac3cb1258319f62af599b15.

I made a small edit to your commit for the inline option, but you keep the authorship (I did not want to add an extra commit).

Thanks again for the PR, if you found other compatibility issues with CakePHP 3.8, feel free to open a new issue or send a new PR.