Translate-plugin: Translations are no longer loaded for models with custom morph aliases

[tobias-kuendig]

This is kind of a nasty bug:

The newly added translations morph many relation now respects any custom morph map aliases:

\October\Rain\Database\Relations\Relation::::morphMap([
    'custom.key' => MyModel::class
]);

Up until this change, get_class() was used to determine the model_type, so in the attributes table the following data was saved:

| model_type |
|--------------------|
| My\Model\Namespace |

As the translations are now loaded via a morphMany relationship, the custom alias is taken into account in the generated query:

select *
from `rainlab_translate_attributes`
where `rainlab_translate_attributes`.`model_id` in ('9')
  and `rainlab_translate_attributes`.`model_type` = 'custom.key'

This means translations are no longer loaded for any model that has a custom morph alias defined.

Swapping the get_class() call with a call to $this->model->getMorphClass() in the TranslatableModel class would fix this issue. I have a possible PR created here:
https://github.com/rainlab/translate-plugin/compare/master...OFFLINE-GmbH:morph-map

But the down side to this is that now existing translations are no longer found, as they are not using the morph key as model_type.

This means that the attributes table would have to be migrated to use the new aliases everywhere.

A nicer solution for this would be to make the morphMany relation ignore the alises completely. This way, the existing data remains compatible.

[LukeTowers]

Could you try explaining this a bit more for the less familiar with the code?

[tobias-kuendig]

The translatable attributes are essentially a morph many relationship. The rainlab_translate_attributes table contains a model_id and model_type where references to the related model are stored.

The translate plugin uses custom code to save the attributes and does not use the usual morph many methods. In this custom code, get_class($model) is used to get the model_type value. This means, the plugin always stores the class name into this column.

In Laravel, there is the morphMap method, to decouple the stored model_type from the actual class name: https://laravel.com/docs/6.x/eloquent-relationships#custom-polymorphic-types

\October\Rain\Database\Relations\Relation::::morphMap([
    'custom.alias' => MyModel::class
]);

If you use this feature, model_type won't contain the class name but the alias you provided.

Since this change, the plugin now loads the translations relation using a proper morph many relationship, which means the provided morph map alias is used to query the attributes, but it is not used to store the attributes.

The result is a attributes table that looks like this:

| ...| model_type | ... |
|-----|--------------------|-----|
| ... | My\Model\Namespace\MyModel | ... |

But a query that looks like this:

select *
from `rainlab_translate_attributes`
where `rainlab_translate_attributes`.`model_id` in ('9')
  -- Essentially this is the right behaviour, but it is no longer compatible
  -- with the custom way of storing attributes.
  and `rainlab_translate_attributes`.`model_type` = 'custom.alias'
  -- instead of
  -- and `rainlab_translate_attributes`.`model_type` = 'My\\Model\\Namespace\\MyModel'

1. Changing the insertion code to respect the morph map alias fixes the loading and saving of the attributes
2. Already stored attributes with the old model_type will have to be migrated to the provided alias

Use this test case to see a proof of this bug:

    // TranslatableModelTest.php
    public function testGetTranslationValueEagerLoadingWithMorphMap()
    {
        // Remove this statement to make the test pass.
        \October\Rain\Database\Relations\Relation::morphMap([
            'morph.key' => CountryModel::class,
        ]);

        $this->recycleSampleData();

        $obj = CountryModel::first();
        $obj->translateContext('fr');
        $obj->name = 'Australie';
        $obj->states = ['a', 'b', 'c'];
        $obj->save();

        $objList = CountryModel::with([
          'translations'
        ])->get();

        $obj = $objList[0];
        $this->assertEquals('Australia', $obj->name);
        $this->assertEquals(['NSW', 'ACT', 'QLD'], $obj->states);

        $obj->translateContext('fr');
        $this->assertEquals('Australie', $obj->name);
        $this->assertEquals(['a', 'b', 'c'], $obj->states);
    }

[LukeTowers]

@tobias-kuendig yikes. I didn't realize that setting a morph mapping for a class set it application wide, that seems like a bad idea. Honestly I'm not really a fan of the custom morph maps anyways, they seem to just make it harder to determine what's really going on without that big of a benefit (supposedly easier to change out the underlying model class, but really if you're making that sort of change you should be writing a migration anyways).

@daftspunk could you chime in on this?

[bennothommo]

@LukeTowers I'd vote for not supporting morph maps at all and ignoring those aliases. They don't really fit in with our architecture (especially in the sense of plugins) - it's more useful for Laravel apps where there's only a single app running on the codebase.

[LukeTowers]

@bennothommo so what would that look like as a change to the Translate plugin to fix the issue at hand? Also, should we add a note about that to the documentation to discourage plugin authors from using morph maps? I do agree that we should be discouraging their use in October, they're pretty dangerous in an ecosystem with plugins that do very similar things and are named similarly.

[daftspunk]

What an annoyance. After some discussion about whether we should allow overriding the morphMap, we decided it was better to just use a forward-looking fix and migrate the affected areas. Thanks for the fix @tobias-kuendig !