Fix scopeTransOrderBy duplicates

[mterman]

Added scope TransWhereSpecificLocale that uses where clause on only specified locale and not default locale and selected locale. I think that this is a more common use case for the transWhere and it is much faster.
This addresses the issue: #581

Also in this pull request, I added checking for multiple joins to the translation index table. With these changes, it is now possible to chain multiple transWhere clauses, also with multiple transWhereSpecificLocale and transOrderBy. It is also possible to mix locales in different clauses.

Please feel free to change name of the TransWhereSpecificLocale to something shorter.

[mjauvin]

@aurelien-roy what do you think about this proposed change?

[aurelien-roy]

@mjauvin This approach looks good to me! Nice way of solving the multi-join issue.

Could bug fix #587 be included in this too?

[mterman]

@aurelien-roy
@mjauvin

I think that this would be a solution to both problems.

$indexTableAlias = 'rainlab_translate_indexes_' . $locale;

should be changed to:

$indexTableAlias = 'rainlab_translate_indexes_' . $index . '_' . $locale;

Then join method should look like this:

protected function joinTranslateIndexesTable($query, $locale, $indexTableAlias, $index)
{
        $joinTableWithAlias = 'rainlab_translate_indexes as ' . $indexTableAlias;
        // check if table with same name and alias is already joined
        if (collect($query->getQuery()->joins)->contains('table', $joinTableWithAlias)) {
            return $query;
        }

        $query->leftJoin($joinTableWithAlias, function($join) use ($locale, $indexTableAlias, $index) {
            $join
                ->on(Db::raw(DbDongle::cast($this->model->getQualifiedKeyName(), 'TEXT')), '=', $indexTableAlias . '.model_id')
                ->where($indexTableAlias . '.model_type', '=', $this->getClass())
                ->where($indexTableAlias . '.item', '=', $index)
                ->where($indexTableAlias . '.locale', '=', $locale)
            ;
        });

        return $query;
}

[mjauvin]

@mterman can you update your PR to do this?

[mterman]

@mjauvin I updated the PR.
I tested this code on a medium set of data ~69k rows in rainlab_translate_indexes. 3 locales are active.
The example below is relatively slow but it is usable.
$query->transWhereSpecificLocale('title', '%test%', 'en', 'LIKE')->transOrderBy('title', 'asc', 'de')

[mariavilaro]

Hi! Just wanted to say that I hit this problem (and 587), I applied this pull request and everything works like a charm. When do you think that you will publish it? Thank you!

[mjauvin]

@mariavilaro I need people to test this and also stats to show before/after perfs. Also need a better naming for this.

[mariavilaro]

The test worked for me. I can't help you with the performance stats because my databases are small. What do you mean with the better naming?

[mjauvin]

@mariavilaro you can't provide details like number of records for the Rainlab.Translate tables and the time it takes for the queries before and after applying this PR?

[mjauvin]

@mterman @aurelien-roy can you guys also provide for performance metrics before/after this PR is applied?

[mariavilaro]

My rainlab_translate_indexes table has only 95 rows right now. I did some tests before and after PR and these are the results (I did the same tests with and without applying the PR changes and ran each test 5 times). I can't really see a difference.

[mjauvin]

@mariavilaro what are you saying, that the PR makes no difference?

[LukeTowers]

@acasar could you try running some performance tests on this PR on your dataset as reported in #581

[LukeTowers]

I suggest replacing the original implementation with the new one, unless of course there is a valid reason not to do it.

@acasar whenever we're evaluating changes for performance sake they must be validated to not change existing behaviour in any breaking way, and we also need benchmarks to prove that the change actually improves performance.

[mjauvin]

I tested this and there is no fallback as I expected.

I almost exclusively use the transWhere() scope in situation where I want to find a model based on a slug (which is usually provided in a cms page :slug param) and as long as the slug has a translation, this new transWhereSpecificLocale() scope will do the job, but as soon as a slug only exists in the default language, that breaks if the page is opened in one of the other languages with that slug in the default language.

In summary, if 'en' is the default locale and 'fr' is an alternate locale:

• transWhere('key', 'value', 'fr') will return records that have key==value in the FR locale OR fallback to records where key==value in EN locale
• transWhereSpecificLocale('key', 'value', 'fr') will return records that have key==value in FR locale OR null

Personally, this is of no use to me as you would require another query to get the fallback record.

What am I missing?

[mterman]

@mterman Ok, so if I understand correctly, 'scopeTransWhereSpecificLocale()` does not use the fallback translation if no translation is found for the requested locale, correct?

Yes, that is correct.

I tested this and there is no fallback as I expected.

I almost exclusively use the transWhere() scope in situation where I want to find a model based on a slug (which is usually provided in a cms page :slug param) and as long as the slug has a translation, this new transWhereSpecificLocale() scope will do the job, but as soon as a slug only exists in the default language, that breaks if the page is opened in one of the other languages with that slug in the default language.

In summary, if 'en' is the default locale and 'fr' is an alternate locale:

• transWhere('key', 'value', 'fr') will return records that have key==value in the FR locale OR fallback to records where key==value in EN locale
• transWhereSpecificLocale('key', 'value', 'fr') will return records that have key==value in FR locale OR null

Personally, this is of no use to me as you would require another query to get the fallback record.

What am I missing?

You are not missing anything. That is the expected behaviour of the transWhereSpecificLocale scope. For me that is useful, we usually don't fallback to the default locale it some content is not available in selected locale.

As per your example, if there are no records in FR locale we will not show content in EN locale but go to 404 or redirect to homepage.

[mariavilaro]

Maybe, instead of adding a new scope "transWhereSpecificLocale", we could just add a new param $fallbackToDefaultLocale (default true,so we don't introduce a breaking change) to transWhere to say if we want to fallback to the default locale or not:

Classic transWhere with fallback:
transWhere('key', 'value', 'fr', '=')

New transWhere without fallback that can use the new code:
transWhere('key', 'value', 'fr', '=', false)

I think that other laravel translation libraries do it like this.

[acasar]

To preserve the original fallback behavior I made an alternative suggestion here: #623

[mjauvin]

@mterman would you mind updating this to keep the scopeTransOrderBy() changes, remove the changes to scopeTransWhere() and drop scopeTransWhereSpecificLocale?

Thanks.

[LukeTowers]

@mjauvin are you planning on merging both?

[mjauvin]

@LukeTowers yes, but this one only for the transOrderBy changes.

[mjauvin]

@mterman if you could make the requested changes, I'd like to merge this for upcoming 1.7.4. I'd like to make the release today or tomorrow.

Thanks.

[mjauvin]

Thanks @mterman, can you pull changes from master in your branch? Tests have been fixed in latest master.

@aurelien-roy can you retest this to make sure it works as expected?

Thanks guys!

[mjauvin]

@mterman please pull latest changes for the upstream master branch before merging, looks like you didn't.

[mterman]

@mterman please pull latest changes for the upstream master branch before merging, looks like you didn't.

@mjauvin Is this the correct upsteram url? And have I been merging from the right branch?

% git checkout specific_where_and_joins                                                                                                                                                                 
Already on 'specific_where_and_joins'
% git remote -v                                                                                                                                                                                         
origin	https://github.com/mterman/translate-plugin.git (fetch)
origin	https://github.com/mterman/translate-plugin.git (push)
upstream	https://github.com/rainlab/translate-plugin.git (fetch)
upstream	https://github.com/rainlab/translate-plugin.git (push)
% git merge upstream/master                                                                                                                                                                             
Already up to date.
% git push
Everything up-to-date

[mjauvin]

@mterman I'm sorry, looks like this is good on your end... not sure why those tests are going through fine when using your branch locally, though... I'll find the problem and get back to you. How long until you're offline?

[mjauvin]

Ok, I made a change to your repo to fix this, we're good now.