tattersoftware / codeigniter4-relations

Entity relationships for CodeIgniter 4
MIT License
87 stars 20 forks source link

$tmpWith and $tmpWithout not defined for ModelTrait #8

Closed sfadschm closed 3 years ago

sfadschm commented 3 years ago

When using the ModelTrait and setting the models $with, the error array_diff(): Expected parameter 1 to be an array, null given appears in: https://github.com/tattersoftware/codeigniter4-relations/blob/b964ae2410da9b93c14167b2d396d1768cec9572/src/Traits/ModelTrait.php#L241

After researching, this seems to be because the tmpWith and tmpWithout properties do not exist in the Model/ModelTrait. Therefore, these properties are still null after the following lines although $this->getWith[out]() returns the correct values: https://github.com/tattersoftware/codeigniter4-relations/blob/b964ae2410da9b93c14167b2d396d1768cec9572/src/Traits/ModelTrait.php#L227 https://github.com/tattersoftware/codeigniter4-relations/blob/b964ae2410da9b93c14167b2d396d1768cec9572/src/Traits/ModelTrait.php#L232

This can be fixed by adding both properties to either the model using the traits or to the ModelTrait itself.

protected $tmpWith = null;
protected $tmpWithout    = null;

What I am wondering, however, is, why this is working without the fix in MGatner\playground with the Monsters model. Any ideas?

MGatner commented 3 years ago

I'm unsure why it is not working for you. The code you referenced above should be setting the temp values if they don't exist:

    if (! isset($this->tmpWith))
        {
            $this->tmpWith = $this->getWith();
        }
        // If no tmpWithout was set then use this model's default
        if (! isset($this->tmpWithout))
        {
            $this->tmpWithout = $this->getWithout();
        }

Are you using the latest version?

sfadschm commented 3 years ago

I thought so, too... Both CI4 and relations have just been updated.

If I try this:

    // If no tmpWith was set then use this model's default
    var_dump($this->tmpWith);
    if (!isset($this->tmpWith))
    {
        var_dump($this->getWith());
        $this->tmpWith   = $test     = $this->getWith();
        var_dump($this->tmpWith);
        var_dump($test);
    }

I get the output:

    NULL                                  // $this->tmpWith before setting
    array(1) { [0]=> string(5) "teams" }  // $this->getWith()
    NULL                                  // $this->tmpWith after setting
    array(1) { [0]=> string(5) "teams" }  // $test

So it seems setting the model property is not working. But its not throwing an error either.

MGatner commented 3 years ago

Hmm fascinating! It must be some weird trait-class property interaction. I will have to look into this more and get back.

MGatner commented 3 years ago

Just out of curiosity, do you have a $tmpWith property on your Model that is using the trait? Or a child of it? If so what is the scope of the property? and it's initial value?

sfadschm commented 3 years ago

Just out of curiosity, do you have a $tmpWith property on your Model that is using the trait? Or a child of it? If so what is the scope of the property? and it's initial value?

Not at all. The only file using such properties right now is ModelTrait and BaseTrait. Like noted above, I tried to workaround by adding the properties to either the model or the Trait, but this yields other errors...

Is PHP supposed to automatically create new object properties, if they are not defined when doing $this->propertyName = propertyValue?

sfadschm commented 3 years ago

Okay, found the curlpit. My BaseModel hat the magic __set defined as

/**
 * Sets as porperty of the object.
 *
 * @param string $key
 * @param mixed $value
 */
public function __set($key, $value)
{
    if (property_exists($this, $key))
    {
        $this->$key = $value;
    }
}

So if the property does not exists yet, it is not set. Don't ask me where this came from... Closing as this is definitely not an issue of the plugin.

MGatner commented 3 years ago

Haha whoops! Glad you found it.

Generally speaking I would like to see trait properties handled better in the future. I will probably define an actual class or something to handle them, so there aren't so many individual points of failure.

sfadschm commented 3 years ago

Might be worth the effort. Using property overloading is convenient but does not make too much sense when the properties are predefined and not really dynamic.