Workspaces views query alter multiplies result set by number of languages

1. +++ b/core/modules/workspaces/src/ViewsQueryAlter.php
   @@ -373,10 +373,13 @@ protected function getRevisionTableJoin($relationship, $table, $field, $workspac
   +      'extra' => [[]],
   

   It took me a while to figure it out, but extra can be a simple (string) expression instead of an array, in which case it will be added as-is. So we can replace this and the new revision_table join plugin with 'extra' => "$table.langcode = $relationship.langcode", :)

   Additionally, we need to do this only if the entity type has a langcode entity key, so we need to pass the $entity_type object from ensureRevisionTable() to getRevisionTableJoin() and only add the extra join condition if $entity_type->hasKey('langcode').

2. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspaceIntegrationTest.php
   @@ -78,20 +88,34 @@ protected function setUp() {
   +    $this->container->get('content_translation.manager')->setEnabled('node', 'page', TRUE);
   

   Let's move this line under the one which creates the content type, for better readability :)

3. +++ b/core/modules/workspaces/tests/src/Kernel/WorkspaceIntegrationTest.php
   @@ -78,20 +88,34 @@ protected function setUp() {
   +    // Ensure we are building a new Language object for each test.
   +    $this->languageManager = $this->container->get('language_manager');
   +    $this->languageManager->reset();
   

   The test runner executes the setUp() method for each test method, so this is not needed. This means we can also remove the protected $languageManager member added above.

Looks great :D Let's see if the testbot agrees.

Ok, the testbot is happy, here's also a test-only patch to prove the failure.

Not sure what happened with the patch from #7, it applies fine locally on both 8.7.x and 8.8.x, let's try again :)

Edit: Oh, I queued that test on the Drupal 7.x branch :/

+++ b/core/modules/workspaces/src/ViewsQueryAlter.php
@@ -375,6 +378,10 @@ protected function getRevisionTableJoin($relationship, $table, $field, $workspac
+    if ($entity_type && $langcode_field = $entity_type->getKey('langcode')) {

I think we should be checking EntityTypeInterface::isTranslatable() here instead of the langcode key directly.

It's the same check in practice (unless we checked if bundles were translatable but that's not really reliable for views).

Hi guys,

I added the recommended change from @catch regarding checking for EntityTypeInterface::isTranslatable(). I hope everything is in order.

Thank you!

Nice, thanks for the update :)

Sorry that change got me thinking, also thanks @plach who checked this was sensible.

At the moment, the logic of the patch is that if the entity type supports translation, then it should join, but this is resulting in a join in at least two cases where it's not necessary:

1. The site only has one language configured.

2. The site has multiple languages configured, but the entity type we're working with has no bundles with translation enabled.

I think we should do all three checks here - the site is multilingual, the entity type is translatable, at least one bundle has translation enabled.

There's a third issue that plach pointed out, is that bundles where translation has been disabled may have old translations in the database. This may mean we can't rely on 'at least one bundle has translation enabled' but we could check that the site is multilingual.

At the moment, the logic of the patch is that if the entity type supports translation, then it should join, but this is resulting in a join in at least two cases where it's not necessary

That's not entirely accurate :) We need to perform the join every time to ensure that we select a possible workspace-specific revision, and this patch is only about adding an additional langcode condition to that join.

There's a third issue that plach pointed out, is that bundles where translation has been disabled may have old translations in the database. This may mean we can't rely on 'at least one bundle has translation enabled' but we could check that the site is multilingual.

That's a really nice point. Since we can't rely on any bundle being translatable or not, I only added the "site is multilingual" check.

Oh good point extra condition not extra join.

Committed 88fede1 and pushed to 8.8.x, and cherry-picked to 8.7.x. Thanks!