modul-is / orm

Simple lightweight ORM
11 stars 0 forks source link

Reject empty value from form #10

Closed SlavaAurim closed 3 years ago

SlavaAurim commented 3 years ago

This seems to be a inaccurate algorithm:

https://github.com/modul-is/orm/blob/25cb6f24c64d22637c4798ecff81555fc8e95530/src/ModulIS/Entity.php#L177-L184

When I fill Entity from form values with not filled int field - this field obtain value '' (empty string) from form, and I need cast it to int or null (if it is empty in form).

It seems to me that it should be something like this:

/**
* Set NULL for nullable properties without value
* Skip if property not set and is not nullable
*/
if($property->isNullable() && empty($values[$name]))

Do you think this is correct code for your intent?

SlavaAurim commented 3 years ago

It is expected that such code will work normally:

public function onFormSuccessed(PageEntity $pageEntity) {
 // $pageEntity filled by form values and all empty strings cast to null for not-string nullable properties
}
SlavaAurim commented 3 years ago

I assume that the code below implements the original intent. Check it, is it correct?

if (!isset($values[$name]) ) {
    if ($property->isNullable() && !isset($this->$name)) {
        $values[$name] = null;
    } else {
        continue;
    }
}

if (empty($values[$name]) && $property->isNullable()) {
    $values[$name] = null;
}
kravcik commented 3 years ago

Thats good point. We discuss this and create new "convert table" and along this we will prepare some tests. I guess code you mentioned was old workaround for int values inside nette form ->addText('user_id'), new nette forms has possibility to use ->addInteger(). So empty value '' will throw exception for int property. This can be changed by some custom wrapper if somebody needs cast it to int automaticly.

https://github.com/modul-is/orm/tree/v2#special-types--behavior

Makes sense for you?

SlavaAurim commented 3 years ago

Yes, it makes sense. I have already made my own ORM, inspired by your great work and other well-known Nette-ORMs.

Here are my conclusions at the moment:

kravcik commented 3 years ago

Thanks for ideas!

  1. Properties agree, we just use custom types (json, date), for us its always responsibility of entity.
  2. We never use id in form, for me is antipattern. If we do - its always good idea rehydrate entity in success ($repo->getBy())
  3. Thats true, we need figure this out.
  4. Null is default value, solution can be similar like in nette form, so maybe you can introduce new Attribute #[Omitted]