fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Error while getting value of NULLable column on Model_Crud. #1589

Closed yuya-miyake closed 10 years ago

yuya-miyake commented 10 years ago

I tried:

$val = $model_crud->property;

got:

Error - Property "property_name" not found for Some_Model_Crud_Class.

FuelPHP 1.7.1's Model_Crud::__get():

if (isset($this->_data[$property]))

and I think it should be

if (array_key_exists($this->_data[$property]))

I may overlook something. Should I write like below anytime?

$val = isset($model_crud->property) ? $model_crud->property :  null;

Thanks.

WanWizard commented 10 years ago

Hmm.... Yeah, isset is probably not a good idea. I'll have a look.

yuya-miyake commented 10 years ago

Great! Thank you very much (and sorry my wrong usage of array_key_exists()...). BTW, I noticed that isset() is the implementation of isset() functinality on inaccessible properties, so it's ok to use isset() in isset(), isn't it?

frankdejonge commented 10 years ago

@yuya-miyake correct, @WanWizard I think the one in __isset should be reverted, agreed?

WanWizard commented 10 years ago

Well, that depends.

If it's your point of view that when a property is set with the value null (which I think was why this issue was opened) the __isset() method should return true, you can no use isset() there either.

yuya-miyake commented 10 years ago

According to PHP manual, I think this is correct behavior.

$model = Some_Model_Crud::forge(array('property_name' => NULL));
$model->property_name; // NULL without errors. This is the reason why I opened this issue.
isset($model->property_name); // FALSE

http://jp.php.net/manual/en/function.isset.php isset determines if a variable is set and is not NULL.

WanWizard commented 10 years ago

which is why I used array_key_exists() in __isset(). So it returns true instead of false.

frankdejonge commented 10 years ago

@WanWizard the array_key_exists is correct to for the __get method, not for the __isset one, I've reverted that part. isset is expected to be false on null.

yuya-miyake commented 10 years ago

@FrenkyNet that's what I want to explain. Thanks.

WanWizard commented 10 years ago

I understand that from a PHP point of view, you want like to have it this way.

However, consider the following. Say I have a table, and in this table the following column: property NULL DEFAULT 'A'

And this bit of code:

public function something($property = null)
{
    $model = Model_Something::forge();
    $model->property = $property;

    // ....

    if (something)
    {
        $model->property = null;
    }

    // ...

    if ( ! isset($model->property))
    {
        $model->property = 'B';
    }

    $model->save();
}

What is stored in the property column of the record? And what would you expect to be stored?

I would expect NULL to be stored, since NULL is a valid value for the column. But with Model_Crud's isset behaving like a standard isset(), 'B' will be saved.

And say I have this code:

public function something($property = null)
{
    $model = Model_Something::forge();
    $model->property = $property;

    // ....

    if (something)
    {
        unset($model->property);
    }

    // ...

    if ( ! isset($model->property))
    {
        $model->property = 'B';
    }

    $model->save();
}

Now I would expect 'B' to be stored, when the property is explicitly unset.

frankdejonge commented 10 years ago

@WanWizard I would expect it to be 'B'?

WanWizard commented 10 years ago

If NULL is a valid value, you can never see the difference between "not there" and "=== null".

// this will save NULL
$model->property = null;
$model->save();

// this will save 'A'
unset($model->property);
$model->save();

How can I distinguish between those two values before I call save?

yuya-miyake commented 10 years ago

Indeed. There is no way to see the difference. I guess property_exists is supposed to be the way, but it does not work with magically accessible properties using get magic method. I've no idea what is the best way, but in my opinion, `issetshould satisfyisset` specification at least.

Thank you for your understandable explanations.

WanWizard commented 10 years ago

I still have a problem with it, because with the column definition in my example, there is a difference to what is stored in the record, based on whether the property value exists and is null, or the property value is absent from $this->data.

So imo this leads to possible unexpected behaviour, with no way of checking or validating it.

WanWizard commented 10 years ago

p.s. a request to properly fix this is open since 2008, with the usual response from the PHP community: nothing.

https://bugs.php.net/bug.php?id=44857

yuya-miyake commented 10 years ago

@WanWizard I agree with you. This is messy problem.