ryangjchandler / orbit

A flat-file database driver for Eloquent. 🗄
MIT License
864 stars 39 forks source link

Not consistent behaviour with column mutator and accessor #148

Open dtorras opened 2 years ago

dtorras commented 2 years ago

Found this possible issue while trying to use together Spatie's Laravel Translatable and Orbit but I think it's extensive to any stored column with mutator/accessor. (maybe related to this comment? https://github.com/ryangjchandler/orbit/pull/56#issuecomment-819048606)

I'm not sure if the behaviour for mutator/accessor is the expected for simple cases, but it doesn't work with Laravel Translatable.

Steps to reproduce

To keep this example simple, I'll use a simple attribute. Create a Test model with this schema and attribute:

class Test extends Model
{
    use Orbital;

    protected $fillable = [
        'title',
        'amount',
    ];

    public static function schema(Blueprint $table)
    {
        $table->id();
        $table->integer('amount');
    }

    protected function amount(): Attribute
    {
        return Attribute::make(
            get: fn($value) => $value / 100,
            set: fn($value) => $value * 100,
        );
    }
}

When you create a new model like this:

$model = Test::create([
  'amount' => 12.34,
]);

If you retrieve the model later, it'll work and pass the tests, because it's stored correctly on the SQLite as 1234 but not in the Markdown, stored as amount: 12.34.

Even if you remove the cache after creating and before retrieving it still works because the code uses the setter to create the SQLite.

If this is the expected behaviour, it won't work with packages like Laravel Translatable because they're managing JSON columns with multiple languages but assigning a default locale on the getter, so you lose other translations on every save. I can expand this issue with a replicable example for Laravel Translate if the previous behaviour for attributes is the expected one.

Proposed solution

Don't use the getter during the file store and the setter during the get from the file to create the SQLite in order to store the same original content on SQLite and files.

  1. Don't use the Model getter to store the file. That would keep the 1234 value on the file and also the JSON string for translations. That happens on this line of the FileDriver: https://github.com/ryangjchandler/orbit/blob/2a812a57ce295283e3fe7840bf51e7c08990bc11/src/Drivers/FileDriver.php#L88
  2. Don't use the setter to retrieve values from files to create the SQLite: https://github.com/ryangjchandler/orbit/blob/2a812a57ce295283e3fe7840bf51e7c08990bc11/src/Concerns/Orbital.php#L185

With these changes, SQLite and files should store the data the same way. I've found some collateral benefits of this, like datetimes that would be stored in the same format as the database, but that would be a breaking change for existing installations.

I've tried to find a non-breaking solution to make it compatible with Spatie's Laravel Translatable but didn't find it.

I can work on a PR if you confirm which should be the expected behaviour for accessors and mutators on Orbit. Thanks!

ryangjchandler commented 2 years ago

Thanks for looking into this @dtorras.

It definitely looks like a problem with how data is transformed during serialisation.

Regarding the breaking changes, I think this could fit into a new major version which has been worked on sparsely.

dtorras commented 2 years ago

I just saw there's a new major version. So if you think that can't be solved without breaking changes, you can close this issue or label it.

Let me know if you need some help checking this issue on the new version.