ViewsEntitySchemaSubscriber does not update views base table when making an entity type translatable

I looked a bit at the failures and I think that at least the test_view_entity_test_data view used by \Drupal\Tests\views\Kernel\EventSubscriber\ViewsEntitySchemaSubscriberIntegrationTest::testDataTableRename() is wrong.

It is supposed to be a view of a translatable entity type, so I just tested manually by creating a view of nodes through the UI and the base table of the view is indeed the data table of the entity type, not its base table.

$this->assertEqual('entity_test_update', $display['display_options']['fields']['id']['table']); this assertion from that test is also wrong. In the same view that I created earlier I added the ID field and, surely enough, it was using the data table to retrieve it, not the base table :)

In conclusion, the "needs tests" part of this issue is actually "fix broken tests" :)

+++ b/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
@@ -150,9 +150,9 @@ public function testDataTableRename() {
-    $this->assertEqual('entity_test_update', $view->get('base_table'));
+    $this->assertEqual('entity_test_update_data', $view->get('base_table'));

How come we can change this assertion before running the updates and not get any fails? The actual view has base_table: entity_test_update in the yml file.

Ahh, you're right, I wrote #7 from memory, without looking at the actual test method.

+++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
index 2319a7064a..0ecfc2b08a 100644
--- a/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php

--- a/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
+++ b/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php

+++ b/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
+++ b/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
@@ -150,9 +150,9 @@ public function testDataTableRename() {

Does this bug basically mean we need some more kind of integration level testing?

+++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
@@ -97,6 +107,9 @@ public static function getSubscribedEvents() {
+    // Invalidate stored views data.
+    $this->viewsData->clear();
+

@@ -217,6 +232,9 @@ public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
     }
+
+    // Invalidate stored views data.
+    $this->viewsData->clear();

Instead of saying what happens, could we document why we need to invalidate?

1. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
   @@ -140,7 +150,9 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
   +    // if an entity type is still revisioanble but no longer translatable.
   +    elseif ($original->isRevisionable() && $original->isTranslatable() && !$revision_remove && $translation_remove) {
   

   The comment talks about the concept of revisioan-ble, is this a new concept just berdir understands?

2. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
   @@ -318,14 +338,21 @@ protected function dataTableAddition($all_views, EntityTypeInterface $entity_typ
   +    // If the entity base table was the view base table, then switch to the
   +    // entity data table.
   +    foreach ($all_views as $view) {
   +      if ($view->get('base_table') === $base_table) {
   +        $view->set('base_table', $data_table);
   +      }
   +    }
   
   @@ -345,6 +372,15 @@ protected function dataTableAddition($all_views, EntityTypeInterface $entity_typ
   +
   +    // If the entity data table was the view base table, then switch to the
   +    // entity base table.
   +    foreach ($all_views as $view) {
   +      if ($view->get('base_table') === $old_data_table) {
   +        $view->set('base_table', $base_table);
   +      }
   +    }
   +
   

   Nice, this does make sense.

3. +++ b/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
   @@ -139,6 +139,9 @@ public function testBaseTableRename() {
   +
   +    // Ensure the view can be executed without errors.
   +    $view->getExecutable()->preview('default');
   
   @@ -164,10 +167,13 @@ public function testDataTableRename() {
   +
   +    // Ensure the view can be executed without errors.
   +    $view->getExecutable()->preview('default');
   
   @@ -197,6 +203,9 @@ public function testRevisionBaseTableRename() {
   +
   +    // Ensure the view can be executed without errors.
   +    $view->getExecutable()->preview('default');
   
   @@ -226,10 +235,13 @@ public function testRevisionDataTableRename() {
   +
   +    // Ensure the view can be executed without errors.
   +    $view->getExecutable()->preview('default');
   
   @@ -244,10 +256,13 @@ public function testDataTableAddition() {
   +
   +    // Ensure the view can be executed without errors.
   +    $view->getExecutable()->preview('default');
   
   @@ -266,6 +281,9 @@ public function testRevisionEnabling() {
   +
   +    // Ensure the view can be executed without errors.
   +    $view->getExecutable()->preview('default');
   
   @@ -312,6 +331,7 @@ public function testVariousTableUpdates() {
   +    $view->getExecutable()->preview('default');
   
   @@ -320,25 +340,28 @@ public function testVariousTableUpdates() {
   +    $view->getExecutable()->preview('default');
   ...
   +    $view->getExecutable()->preview('default');
   ...
   +    $view->getExecutable()->preview('default');
   
   @@ -349,14 +372,16 @@ public function testVariousTableUpdates() {
   +    $view->getExecutable()->preview('default');
   ...
   +    $view->getExecutable()->preview('default');
   
   @@ -365,6 +390,7 @@ public function testVariousTableUpdates() {
   +    $view->getExecutable()->preview('default');
   
   @@ -376,6 +402,7 @@ public function testVariousTableUpdates() {
   +    $view->getExecutable()->preview('default');
   
   @@ -393,9 +421,13 @@ public function testVariousTableUpdates() {
   +    $view->getExecutable()->preview('default');
   

   Shouldn't we check somehow the result of that as well? Maybe check whether $view->build_info['fail'] wasn't set?

Finally managed to take a look at this, sorry @Berdir for the delay :/ The patch looks very very good to me, it clearly shows all the deficiencies of this subscriber and its test coverage in HEAD.

For the problem detailed in #21, the fix is to make \Drupal\views\EntityViewsData and views_views_data() use the "active" entity type and field storage definitions.

That means we're blocked on #3056816: Installing a new field storage definition during a fieldable entity type update is not possible, so posting a combined patch as well to prove that everything works nicely together :)

#3056816: Installing a new field storage definition during a fieldable entity type update is not possible was committed (only to 8.8.x for now, but it should be backported to 8.7.x), so here's a fix for the remaining test failures.

#3056816: Installing a new field storage definition during a fieldable entity type update is not possible has been reverted and the patch that will be re-committed won't have the cache clears needed by this issue, so we need to add them here.

A new test method was added to EntityViewsDataTest in #2973137: EntityViewsData missed revisionable validation so we need to apply the same changes as the ones from the interdiff in #26.

#3056816: Installing a new field storage definition during a fieldable entity type update is not possible was re-committed so the non-combined patch should be good to go for 8.8.x at least.

Rerolled after #3056816: Installing a new field storage definition during a fieldable entity type update is not possible, which was re-re-committed with the necessary cache clears.

Patch #31 re-rolled.

Patch #33 re-rolled for D 8.7.x.

Fixed inconsistences (tested on live site).

Patch #36 re-rolled for 8.9.x-dev.

The changes in #36 were wrong, the entity manager was deprecated and shouldn't be used anymore.

Here's a proper reroll for 9.1.x. Some tests are still failing and needs to be looked at properly so leaving at NW.