dimsav / laravel-translatable

[Deprecated] A Laravel package for multilingual models
MIT License
1.95k stars 320 forks source link

[Possible improvement] firstOrCreate method not working with translations relation. #442

Closed BarryPooter closed 6 years ago

BarryPooter commented 6 years ago

Hello!

I'm currently running into a limitation of the Translatable module which causes me to write more code than necessary - to my understanding. It's not really a big issue since the 'issue' can be easily circumenvented, but it's forcing me to execute more queries than necessary, which causes extra wasted time (which adds up on processing thousands of resources).

Currently when I'm trying to use the firstOrCreate() method from Laravel it causes the following QueryException:

Illuminate\Database\QueryException with message 'SQLSTATE[42S22]: Column not found: 1054 Unknown column 'nl' in 'where clause' (SQL: select * from `features` where (`nl` = test) and `features`.`deleted_at` is null limit 1)'`.

The code that causes this error is as follows;

        $_cCategory = \App\Category::firstOrCreate([
            $sLocale => [
                'name'          => $sName,
                'description'   => $sDescription
            ]
        ]);

(The $sLocale variable would in this case be equal to 'nl'.)

A way to circumenvent this issue using the Translatable module is by refactoring the code into this;

        // Prepare an Eloquent statement.
        $_eQuery = \App\CategoryTranslation::where('name', $sName)
            ->where('locale', $sLocale);

        // Check if the Category Translation exists.
        if ($_eQuery->exists()) {
            // Retrieve the corresponding Category Resource.
            $_cCategory = \App\Category::findOrFail($_eQuery->firstOrFail()->category_id);

            // And return it to the caller.
            return $_cCategory;
        }

        // Create a new Category with
        // it's translation data.
        $_cCategory = \App\Category::create([
            $sLocale => [
                'name'          => $sName,
                'description'   => $sDescription,
            ]
        ]);

        // Return the base Category resource.
        return $_cCategory;

As you can see it requires a few more steps and possibly a few more queries compared to the firstOrCreate() method - adding up time if you need to process thousands of resources.

Consider this 'issue' as a Quality of Code improvement to allow for cleaner - and possibly faster - code.

Gummibeer commented 6 years ago

Even this could be reduced to two queries, the same findOrCreate() produces.

try {
    return Category::whereHas('translations', function(Builder $query) use ($name, $locale) {
        $query->where('name', $name)->where('locale', $locale);
    })->firstOrFail();
} catch(ModelNotFoundException $e) {
    return Category::create([
        $locale => [
            'name' => $name,
            'description' => $description,
        ],
    ]);
}

To solve this in the package in a dynamic way it would produce tons of more code and reduces performance a lot. The following things have to be done in the package: 1) detect if it's a translatable attribute 2) get locale and attribute name 3) put it into a structured array 4) create the query

Cause of the fact that it could be solved in 5 lines and two queries (same as internal laravel logic) by your own I recommend to create your own method like findOrCreateByTranslation.