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

CommentFileSizeAuthor
#27 2256219-27--datasource_language_options.patch14.59 KBdrunken monkey
#27 2256219-27--datasource_language_options--interdiff.txt593 bytesdrunken monkey
#24 2256219-24--datasource_language_options.patch14.01 KBdrunken monkey
#24 2256219-24--datasource_language_options--interdiff.txt578 bytesdrunken monkey
#21 2256219-21--datasource_language_options.patch.patch13.93 KBdrunken monkey
#21 2256219-21--datasource_language_options.patch--interdiff.txt7.37 KBdrunken monkey
#20 interdiff.txt628 bytesborisson_
#20 allow_the_content-2256219-20.patch14.35 KBborisson_
#16 allow_the_content-2256219-16.patch13.73 KBborisson_
#16 interdiff.txt1.43 KBborisson_
#13 allow_the_content-2256219-13.patch13.61 KBborisson_
#13 interdiff.txt5.18 KBborisson_
#11 2256219-11--datasource_language_option.patch13.12 KBdrunken monkey
#11 2256219-11--datasource_language_option--interdiff.txt3.95 KBdrunken monkey
#10 allow_the_content-2256219-10.patch13.08 KBborisson_
#10 interdiff.txt2.71 KBborisson_
#8 allow_the_content-2256219-7.patch12.26 KBborisson_
#7 interdiff.txt2.41 KBborisson_
#5 allow_the_content-2256219-5.patch8.03 KBborisson_
#5 interdiff.txt2.85 KBborisson_
#4 allow_the_content-2256219-4.patch5.43 KBborisson_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey commented
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

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey commented
Project: Search API (8.x) » Search API
Version: » 8.x-1.x-dev
Component: Framework » Plugins
Nick_vh’s picture

Nick_vh
he/him
Location Ghent
CreditAttribution: Nick_vh at Acquia commented
Issue summary: View changes
Issue tags: +beta blocker
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Active » Needs review
FileSize
allow_the_content-2256219-4.patch5.43 KB

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

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
interdiff.txt2.85 KB
allow_the_content-2256219-5.patch8.03 KB

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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.41 KB

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

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
FileSize
allow_the_content-2256219-7.patch12.26 KB

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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.71 KB
allow_the_content-2256219-10.patch13.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

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
FileSize
2256219-11--datasource_language_option--interdiff.txt3.95 KB
2256219-11--datasource_language_option.patch13.12 KB

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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt5.18 KB
allow_the_content-2256219-13.patch13.61 KB

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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.43 KB
allow_the_content-2256219-16.patch13.73 KB

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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented

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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented
Status: Needs work » Needs review
FileSize
allow_the_content-2256219-20.patch14.35 KB
interdiff.txt628 bytes

Test should be green now.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
FileSize
2256219-21--datasource_language_options.patch--interdiff.txt7.37 KB
2256219-21--datasource_language_options.patch.patch13.93 KB

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

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Needs work » Needs review
FileSize
2256219-24--datasource_language_options--interdiff.txt578 bytes
2256219-24--datasource_language_options.patch14.01 KB

Re-rolling and fixing one stupid mistake.

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

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
Status: Needs work » Needs review
FileSize
2256219-27--datasource_language_options--interdiff.txt593 bytes
2256219-27--datasource_language_options.patch14.59 KB

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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ for Acquia commented
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

drunken monkey
he/him
Primary language German
Location Vienna, Austria
CreditAttribution: drunken monkey for Acquia commented
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.