Sterc / FormIt

A dynamic form processing Snippet for MODX Revolution
https://docs.modx.com/current/en/extras/formit
33 stars 58 forks source link

FormIt 3.0.3 problem with Checkboxes #139

Closed jolichter closed 6 years ago

jolichter commented 7 years ago

Hi, since FormIt 3.0.3-pl my checkboxes doesn't work, e.g.:

[[!FormIt? &validate=`color:required`]]
...
<label>Color: [[!+fi.error.color]]</label>
<input type="hidden" name="color[]" value="" />
<input type="checkbox" name="color[]" value="blue" [[!+fi.color:FormItIsChecked=`blue`]] > Blue 
<input type="checkbox" name="color[]" value="red" [[!+fi.color:FormItIsChecked=`red`]] > Red 
<input type="checkbox" name="color[]" value="green" [[!+fi.color:FormItIsChecked=`green`]] > Green

I got nothing displayed by placeholder [[+color]] and have no Error Log. Therefore I switched back to FormIt 3.0.2-pl and it works again. Maybe it is a bug?

signalfeuer commented 7 years ago

Same problem! :-( Is there any fix/workaround beside the switch to 3.0.2?

jolichter commented 7 years ago

This solves it. Thanks Jako!

joeke commented 7 years ago

@jolichter This was indeed caused by a PHP7 compatibility warning fix we've added in 3.0.3. I've merged the PR from @jako (thanks Thomas), so it will be included in the 3.0.4 release.

Jako commented 7 years ago

@joeke: I don't think that the PHP7 compatibility warnings are fixed right in https://github.com/Sterc/FormIt/commit/d0636355a868bc70d6bb0dd292f97d00f497a9a7. All the references should be restored to work with the original $modx object and not with a copy (the inner values of the copy should be the same but not the object itself). Please read http://php.net/manual/en/language.oop5.references.php#101900 (2nd principle and the 3rd principle) on this.

Since the $modx object should be the same in every method, it should be always assigned as reference.

joeke commented 7 years ago

@Jako But the $modx object is already passed as reference in the main formit class file (https://github.com/Sterc/FormIt/blob/develop/core/components/formit/model/formit/formit.class.php#L81-L82), and every class that uses a reference as well (see: https://github.com/Sterc/FormIt/blob/develop/core/components/formit/model/formit/fihooks.class.php#L84-L85). So I feel that passing $formit->modx (https://github.com/Sterc/FormIt/blob/develop/core/components/formit/model/formit/fihooks.class.php#L86) as reference is unnecessary.

Jako commented 7 years ago

@joeke You are right, the $modx object is already passed as reference in the formit class. But it is wrong to remove the reference inside the other classs and use a copy. You can't change the object via this variable afterwards.

Since the patches in https://github.com/Sterc/FormIt/commit/d0636355a868bc70d6bb0dd292f97d00f497a9a7 are done to solve 'PHP by reference errors': References are not stripped in PHP7 and references in function definitions are allowed in PHP7. Only in a method call a reference is not allowed: $whatever->method(&$param) is invalid (since PHP 5.4).

Maybe it could be stripped off totally, but not only the reference. For BC the $modx object should stay in the hooks class and maybe in the validator class, since it could be modified inside of the hooks and the validator with $this->modx and $this->formit->$modx.