Fix retrieval of property to work for Astrotomic/translatable

[Tofandel]

As seen in #870 the move to getAttributeValue bypasses the overriden method https://github.com/Astrotomic/laravel-translatable/edit/main/src/Translatable/Translatable.php#L143

This PR goes back to using getAttribute but 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

[Tofandel]

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

[Tofandel]

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

[Tofandel]

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

[rubenvanassche]

Yeah I get the idea, I'm also always confused working in this code. But atm we don't have a system in place where we can have different mapping systems for input data. So by setting a mapper on a data object for eloquent models to be snake case all the data you parse from the frontend should also be snake case (which is probably the best idea). But I think we still want people to have an option the manually define another input mapping type for frontend data.

There are complicated solutions to solve this by adding special attributes for this for example, config flags, ... but for now the best solution to me looks like keeping it as is.

[rubenvanassche]

In its current state I'm not sure if this is something we want to merge just to solve problems with another package. It introduces some extra performance penalties which we probably want to avoid.

Since normalizers can be overwritten on a data specific object and in config maybe the easiest solution is to add your own normalizer into your project.

If you find a way where we can have less performance issues, please let me know!

[rubenvanassche]

Since we remove this piece of code the whole method on the bottom won't be called anymore so can it be removed?

[rubenvanassche]

Yeah I get the idea, I'm also always confused working in this code. But atm we don't have a system in place where we can have different mapping systems for input data. So by setting a mapper on a data object for eloquent models to be snake case all the data you parse from the frontend should also be snake case (which is probably the best idea). But I think we still want people to have an option the manually define another input mapping type for frontend data.

There are complicated solutions to solve this by adding special attributes for this for example, config flags, ... but for now the best solution to me looks like keeping it as is.

[rubenvanassche]

Not sure if we want to do this:

1. Probably 90% of the properties on a model will be regular properties without special logic, now we're checking for every property the relations, attributes, ... this is going to affect performance quite a lot

2. I hope everyone has a minimal amount of missing attributes, but always waiting on exceptions makes code slow and is something to avoid.

[Tofandel]

Well the call to getAttribute was done prior to 4.9.0 (which broke this) and has never been a bottleneck

isRelation is not an expensive call. It will do the reverse of before 4.9.0 with in_array($name, $this->model->getMutatedAttributes()

It does basically just 1 isset($mutators[$key]) and if the isset didn't pass (which it will for all attributes) one method_exists($this, $key)

The exception catching is there as a bugfix. If you don't enable laravel strict mode it will never happen. But if you do, and try to access a property that doesn't exist on the model then it will throw an exception rather than do the UnknownProperty logic of the package which we don't want

[rubenvanassche]

I'm more worried about the $dataProperty->attributes->contains I think we can skip that one by adding a new property to DataProperty isLoadingRelation that way we don't have to check it all the time. Once we have a cached version of that you might be right that the performance wouldn't be affected too much.

Then take a look at the comment on line 41.

If that's done this can be merged.

[Tofandel]

In its current state I'm not sure if this is something we want to merge just to solve problems with another package. It introduces some extra performance penalties which we probably want to avoid.

Well this is a problem that has been introduced recently and forces me to pin the package as it breaks everywhere I have it installed

This still fixes the issue while being still way more performant than before d017301

A more performant solution could be simply to check if the getAttribute method has been overriden or comes from laravel with some reflection (needed only once for the model) and decide which method to use

Or add a compatibility layer for the Astrotomic package by checking the isTranslationAttribute method and if it is a translation attribute then use getAttribute instead of getAttributeValue

[Tofandel]

I looked into caching all PropertyAttributes, so it should become O(n) instead of O(9n) as multiple contains are made throughout the code and we can just cache it in a single loop, let me know what you think

I'll look into benchmarking before and after to see if it really provides an optimisation as the number of items should still be fairly small in all cases because there is I would say on average 0.3 attributes per property