Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include changed translations with translated models' getChanges method #229

Open
Tschoatscho opened this issue May 28, 2021 · 2 comments
Open

Comments

@Tschoatscho
Copy link

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
Copy link
Member

Gummibeer commented May 31, 2021

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
Copy link
Author

Just created a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants