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

Compatibility with spatie/laravel-translatable is broken #870

Closed
alexrififi opened this issue Sep 24, 2024 · 3 comments
Closed

Compatibility with spatie/laravel-translatable is broken #870

alexrififi opened this issue Sep 24, 2024 · 3 comments

Comments

@alexrififi
Copy link
Contributor

In spatie/laravel-translatable translatable attributes are stored as an array and converted to a string like this

✏️ Describe the bug

Version 4.6.0 introduced a new NormalizedModel.

In which, after some time, the method for obtaining model attributes was changed. And that broke the compatibility.

Because of this, translatable attributes began to come to the data class as arrays, instead of strings.

An we got this error

Argument ... must be of type string, array given

We can write workarounds to avoid problems.

But... It looks terrible

    // trait HasTranslations
    public function attributesToArray(): array
    {
        $result = parent::attributesToArray();

        [, $caller] = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2);
        if ($caller['class'] === NormalizedModel::class) {
            foreach ($this->getTranslatableAttributes() as $attribute) {
                $result[$attribute] = $this->getAttributeValue($attribute);
            }
        }

        return $result;
    }

I can create a custom ModelSerializer, but then I'll have to completely repeat NormalizedModel and make changes there.

The simplest solution is to make the $properties property of the NormalizedModel class public and then I can use my custom normalizer right after ModelNormalizer and cast the translatable properties to a string. Also adding a setter as mentioned in #803 would also solve the problem...

Please tell me, is this possible? Or maybe there is another way to solve the problem?

This problem was also mentioned in #851

Thank you for reading.

↪️ To Reproduce

Unfortunately there is no test to show the problem

🖥️ Versions

Laravel: 10.48.22
Laravel Data: 4.9.0
PHP: 8.3

@rubenvanassche
Copy link
Member

rubenvanassche commented Oct 4, 2024

I think I've fixed it, but could be that the change breaks everything 😅, this Eloquent attributes thing a magic black box and every time it seems to be working differently.

Solution would be not to initialize the properties in the beginning, then we'll try to call getAttributeValue if a property is required which then will use the translation (normally).

Commit: d017301

@Tofandel
Copy link
Contributor

Tofandel commented Oct 10, 2024

It did break stuff, I'm just testing this and now it's the opposite of this issue in another translation lib https://github.com/Astrotomic/laravel-translatable/blob/main/src/Translatable/Translatable.php#L143
https://github.com/Astrotomic/laravel-translatable/blob/main/src/Translatable/Translatable.php#L72

It uses getAttribute instead of getAttributeValue so now we don't retrieve anything when we try to get a translated attribute

Seeing as in laravel getAttribute calls getAttributeValue I think it should go back to using getAttribute https://github.com/laravel/framework/blob/8fbb94079dad0f959d19b9de1c452e970e61c3ab/src/Illuminate/Database/Eloquent/Concerns/HasAttributes.php#L471

@rubenvanassche
Copy link
Member

@Tofandel You're referencing the Astronomic package, as far as I know it now works with the Spatie package. You're always welcome to send in a PR with solution which works for both.

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

No branches or pull requests

3 participants