kirkbushell / eloquence

A drop-in library for certain database functionality in Laravel, that allows for extra features that may never make it into the main project.
MIT License
537 stars 58 forks source link

Logic reversed in getAttribute() #59

Closed valorin closed 12 months ago

valorin commented 8 years ago

I've just noticed an interesting discrepancy in getAttribute() between how the CamelCasing trait works and the original Eloquent Model.

Consider the Eloquent Model function:

public function getAttribute($key)
{
    if (array_key_exists($key, $this->attributes) || $this->hasGetMutator($key)) {
        return $this->getAttributeValue($key);
    }
    return $this->getRelationValue($key);
}

Now look at CamelCasing:

public function getAttribute($key)
{
    if (method_exists($this, $key)) {
        return $this->getRelationValue($key);
    }
    return parent::getAttribute($this->getSnakeKey($key));
}

Eloquent allows attributes (even read-only ones) precedence over relations. CamelCasing favours relationships first.

It seems to me that following Eloquence's order of preference would be a good idea, so the behaviour is consistent between both.

kirkbushell commented 8 years ago

@valorin cheers! i have a feeling this may have been changed in the core codebase, as I'm fairly certain I duplicated the first bit of it. Something in the back of my mind is saying that I felt it was weird to begin with (placing relations over attributes).

Happy to change this - it should definitely maintain the same behaviour as Eloquent.

bicatu commented 6 years ago

any chance this will be updated?

kirkbushell commented 6 years ago

Certainly, soon as I have time.

askmrsinh commented 1 year ago

Anyone who is surprised by this, please have a look at the new getAttribute function code (10.x): https://github.com/laravel/framework/blob/b4ab62986000eaa411a22dbeb112b21c10420fdc/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L428

kirkbushell commented 12 months ago

This is resolved in the coming 11.0 release.