Problem/Motivation

The main functionality for supporting entity translations has been done in #2241429: Support translations, but it would be nice to have a setting in the content entity datasource to only include (or exclude) some languages.

Proposed resolution

Implement this setting and test it :)

Estimated Value and Story Points

This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.

Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.

Value : 5
Story Points: 5

Comments

drunken monkey’s picture

Component: Backend » Framework
Issue summary: View changes

Clarifying the phrasing.

Also, we could probably even move this to the normal Search API issue queue, as this is really not of a high priority. Especially when we now want to encourage using a single index for everything, and when the Solr integration will hopefully support multilingual sites a lot better, I don't think this will be used much.
However, since I dropped the "Language filter" processor, this would otherwise be a regression, and I do think we want to avoid those.

drunken monkey’s picture

Project: Search API (8.x) » Search API
Version: » 8.x-1.x-dev
Component: Framework » Plugins
Nick_vh’s picture

Issue summary: View changes
Issue tags: +beta blocker
borisson_’s picture

Status: Active » Needs review
FileSize
5.43 KB

I think this is the right direction; needs more work though.

borisson_’s picture

I've written a new test and that test fails, but I can't figure out what I should do to make it pass.

Status: Needs review » Needs work

The last submitted patch, 5: allow_the_content-2256219-5.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
2.41 KB

Tests still fail in all the same ways; but it now has less unused code.

borisson_’s picture

Forgot the actual patch; woops.

Status: Needs review » Needs work

The last submitted patch, 8: allow_the_content-2256219-7.patch, failed testing.

borisson_’s picture

Status: Needs work » Needs review
FileSize
2.71 KB
13.08 KB

I updated the patch and expanded the tests for this (in IndexIntegrationTest).

The patch will still fail, but instead of 6 failing tests, locally this gives me 2.
CliTest is still failing, as well as the IndexIntegrationTest. I can't figure out why the tests are failing.

drunken monkey’s picture

Thanks a lot for the great patch!
On a first glance, this looks pretty good to me.
The test case could/should probably be merged with #2540668-11: Reindex misses translations, though, since they test a lot of the same stuff.

I also got the test failures (2 exceptions for CliTest, 2 fails for the new IndexIntegrationTest). A bit too late to investigate today, though, maybe tomorrow.

And, as usual, I also found a few smaller issues with the patch, see my attached revision.

Status: Needs review » Needs work

The last submitted patch, 11: 2256219-11--datasource_language_option.patch, failed testing.

borisson_’s picture

This should fix the IndexIntegrationTest, no idea why the CliTest is still failing.

Status: Needs review » Needs work

The last submitted patch, 13: allow_the_content-2256219-13.patch, failed testing.

The last submitted patch, 13: allow_the_content-2256219-13.patch, failed testing.

borisson_’s picture

Status: Needs review » Needs work

The last submitted patch, 16: allow_the_content-2256219-16.patch, failed testing.

The last submitted patch, 16: allow_the_content-2256219-16.patch, failed testing.

borisson_’s picture

I can't seem to figure out why the patch in #16 is failing.

I agree with #11 that the new test here can be integrated in the new test in #2540668: Reindex misses translations. Do you want me to postpone this issue on that one?

borisson_’s picture

Test should be green now.

drunken monkey’s picture

I agree with #11 that the new test here can be integrated in the new test in #2540668: Reindex misses translations. Do you want me to postpone this issue on that one?

No, I don't think that's necessary. We can just as well commit here and then add the tests from the other issue to this one. I renamed the test to LanguageIntegrationTest, in any case, so that will be a fit.

Otherwise, the patch looks pretty good, nice work!

However, what confuses me is ContentEntity::setConfiguration(): there is a huge amount of code there to determine whether the selected bundles changed and, if they did, to change tracking accordingly. Isn't the same needed for the selected languages, too?
And, if it is, it is of course worrying that the tests don't catch this bug. (But should only be a few lines to add to the test.)

Status: Needs review » Needs work

The last submitted patch, 21: 2256219-21--datasource_language_options.patch.patch, failed testing.

The last submitted patch, 21: 2256219-21--datasource_language_options.patch.patch, failed testing.

drunken monkey’s picture

Status: Needs review » Needs work

The last submitted patch, 24: 2256219-24--datasource_language_options.patch, failed testing.

The last submitted patch, 24: 2256219-24--datasource_language_options.patch, failed testing.

drunken monkey’s picture

Huh. Somehow, this isn't going entirely in the direction I hoped it would …
But, interestingly, I just seem to have found a bug in a completely unrelated part of the code. Good to know!
Tests pass for me now locally, let's see whether the test bot agrees.

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I'm pretty happy with the results here, when this patch applies I'll reroll #2540668: Reindex misses translations.

drunken monkey’s picture

Status: Reviewed & tested by the community » Fixed

OK, great! Committed.
Thanks again!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.