Astrotomic / laravel-translatable

A Laravel package for multilingual models
https://docs.astrotomic.info/laravel-translatable/
MIT License
1.23k stars 156 forks source link

Include changed translations with translated models' getChanges method #229

Open Tschoatscho opened 3 years ago

Tschoatscho commented 3 years ago

Is your feature request related to a problem? Please describe. I understand that a change within a translation does not affect a translated model's database table. However, logically it changes the model's attributes, so it would be nice to also get the changed translation values with their corresponding keys within the result when calling $model->getChanges() within an update operation. I'm aware of the fact that this would be a breaking change, because it affects the outcome of a translated model's method. But so does the overridden $model->fill() method as well and personally I consider that as an improvement.

Describe the solution you'd like I would like the Translatable trait to override the $model->getChanges() method in the following way:

    public function getChanges(): array
    {
        // if (!config('translatable.publishTranslationChangesOnModel', false)) {
        //     return parent::getChanges();
        // }
        switch (config('translatable.rule_factory.format')) {
            case RuleFactory::FORMAT_KEY:
                $replacementFunc = function (string $key, string $locale): string {
                    return $key . ':' . $locale;
                };
                break;
            default:
                $replacementFunc = function (string $key, string $locale): string {
                    return $locale . '.' . $key;
                };
        }
        $translationChanges = $this->translations->reduce(function ($carry, $translation) use ($replacementFunc) {
            $locale = $translation->locale;
            foreach ($translation->getChanges() as $attribute => $value) {
                $carry[$replacementFunc($attribute, $locale)] = $value;
            }
            return $carry;
        }, array());
        if (empty($translationChanges)) {
            return parent::getChanges();
        }
        return array_merge(parent::getChanges() ?: [], $translationChanges);
    }

As mentioned in the code example the new behaviour could be hidden behind a config flag.

Describe alternatives you've considered Initially I tried to fiddle with $model->isDirty() and $model->getDirty(). But as I understand at least when using $model->fill() (and I assume the same for the other translatable methods) the changes will be saved automagicly to the database and therefore there will never be anything dirty outside of translations.

Additional context This would also affect the result of model $model->wasChanged() which is intended.

I'm not sure if overriding getChanges() is a good strategy, I'm rather a noob regarding Laravel und PHP. I used the override on several of my own models to propagate translation changes to my controllers. As the repetition had a little code smell I moved it up to common base class and so came to the conclusion that it actually should reside in a trait regarding translations, which is yours ;-)

Kind regards and thanks a lot for all your work Gio

Gummibeer commented 3 years ago

Hey, I would accept a PR adding a new method not overriding the default. Like getTranslationChanges() - should also only return the translation changes not the changes of $this. So that it's a full optional opt-in. That would prevent it from being breaking and will keep the default behavior for everyone.

In case you would PR a solution: please don't use reduce() even if it's correct and works it's a pretty unreadable API. Great that you've already used the different formats!

In case you want to override the defaults you can still do so in your own trait/base model with a simple array_merge() call.

Tschoatscho commented 3 years ago

Just created a PR.