Fix retrieval of property to work for Astrotomic/translatable #883
Conversation
|
Ended up giving up on the idea of using strict mode, because it has a lot of conditions to throw (model exists and was not recently created) and a callback that can change the behavior so this wouldn't have been a reliable method |
| if ($this->hasModelAttribute($name)) { | ||
| return $this->properties[$name] = $this->model->getAttributeValue($name); | ||
| } | ||
|
|
||
| $camelName = Str::camel($name); |
There was a problem hiding this comment.
I think we should get rid of this in the automatic camel, snake casing in the next major, after looking back at it, we should not do this manually for relations and rely on the content mapping of the lib https://spatie.be/docs/laravel-data/v4/as-a-data-transfer-object/model-to-data-object#content-mapping-property-names
There was a problem hiding this comment.
Here is the commit for my proposal on how it should be handled in the next major 380942b
Attributes are still snake cased if necessary but not relations
| if (! isset($this->attributesProperty)) { | ||
| $this->attributesProperty = new ReflectionProperty($this->model, 'attributes'); | ||
| $this->attributesProperty->setAccessible(true); |
There was a problem hiding this comment.
This line does nothing because: https://www.php.net/manual/en/reflectionproperty.setaccessible.php
Note: As of PHP 8.1.0, calling this method has no effect; all properties are accessible by default.
And we require php 8.1 in composer.json
As seen in #870 the move to
getAttributeValuebypasses the overriden method https://github.com/Astrotomic/laravel-translatable/edit/main/src/Translatable/Translatable.php#L143This PR goes back to using
getAttributebut after already checking if it's a relation (because getAttribute loads the relation otherwise, which we don't want), like this both spatie/translatable and astrotomic/translatable will work with this package