Problem/Motivation

Now that entity schema can change under the hood views has to react as otherwise the views will be broken.

Support the following cases:

  • rename base table
  • rename data table
  • add a data table
  • add a revision table
  • remove some fields
  • Support the following transactions:
      // base <-> base + translation
    +    // base + translation <-> base + translation + revision
    +    // base + revision <-> base + translation + revision
    +    // base <-> base + revision
    +    // base <-> base + translation + revision
    

Proposed resolution

Listen to the events fired by the entity storage. With this we can iterate over the existing views and update them
properly automatically.

Remaining tasks

User interface changes

API changes

Postponed until

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because you don't want to have broken views if you update an entity type providing entity types
Issue priority Critical because you don't want to end up with broking views
Disruption No API changes at all
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Issue tags: +beta target, +D8 upgrade path
fago’s picture

Ideally, views integration would be built upon the table mapping only and thus automatically adapt. But how do we handle stuff that goes away that has been used, just mark it as broken? E.g. revision functionality or a certain field?

dawehner’s picture

@fago
Well, sure I would love to have that, but I don't think this feasible in case we want to get some release at some point.
The steps forward for now are to store entity-type, entity-field and column name in the configured view in order to be able to update the view properly.
Is there some event triggered when the schema is changed?

xjm’s picture

Title: Figure out what to do when entity schema changes » Figure out what to do in Views when entity schema changes
fago’s picture

The steps forward for now are to store entity-type, entity-field and column name in the configured view in order to be able to update the view properly.

That doesn't really sound simpler to me, but you know better :)

Is there some event triggered when the schema is changed?

Not right now, but when we do #2332935: Allow code to respond to entity/field schema changes views could use the same event system as well to get notified on entity changes?

xjm’s picture

Issue tags: +Pre-AMS beta sprint

Tagging as a priority for an AMS beta sprint.

xjm’s picture

daffie’s picture

Status: Postponed » Active
dawehner’s picture

plach’s picture

dawehner’s picture

Title: Figure out what to do in Views when entity schema changes » [pp-1] Figure out what to do in Views when entity schema changes
Status: Active » Postponed

.

xjm’s picture

Title: [pp-1] Figure out what to do in Views when entity schema changes » Figure out what to do in Views when entity schema changes
Status: Postponed » Active
Issue tags: +Triaged D8 critical

I think this can be active again now? Or is it postponed on something else?

dawehner’s picture

Title: Figure out what to do in Views when entity schema changes » [pp-1] Figure out what to do in Views when entity schema changes

#2349553: Store entity field information in the views data adds some important meta-data, which allows us to figure out what kind of entity types we have after the update.

xjm’s picture

Issue summary: View changes
dawehner’s picture

Worked on some tests, to at least somehow make an outline what should happend at the end of the day.

#2388467: Remove \Drupal\entity_test\EntityTestViewsData and update the annotation. is one small tiny issue which resulted from that.

dawehner’s picture

Some other cases we have to support:

  • enable revision support
  • removing base fields
    • base <-> base + translation
    • base + translation <-> base + translation + revison
    • base + revision <-> base + translation + revison
    • base <-> base + revison
    • base <-> base + translation + revision
fago’s picture

+++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
@@ -0,0 +1,142 @@
+    // We expect that views which use 'entity_test' as base tables are removed.
+    $views = $entity_storage->loadMultiple();
+    $this->assertEqual(3, count($views));

I'm not sure about that. Imo, that's something not related to "storage-adjustments" but cleaning up config based on module removals. Afaik, we do generally clean-up all config depending on a module when we uninstall it, so it should work via that?

dawehner’s picture

Issue tags: +Ghent DA sprint

I'm not sure about that. Imo, that's something not related to "storage-adjustments" but cleaning up config based on module removals. Afaik, we do generally clean-up all config depending on a module when we uninstall it, so it should work via that?

That is valid, ... we can drop that test in case we simply don't want to take care about that.
It is just, what I came up with as a possible test case.

This will be fun, to even just get all the possible test cases done.

dawehner’s picture

Status: Active » Needs review
FileSize
28.65 KB
18.87 KB

Worked a bit on more test coverage.

As @plach said, we could leverage a couple of things of the EntityDefinitionUpdateTest ... extracted that into a dedicated
trait and used it in the test ...which is still not completed, but at least we do test something already.

Status: Needs review » Needs work

The last submitted patch, 19: 2341323-19.patch, failed testing.

xjm’s picture

Title: [pp-1] Figure out what to do in Views when entity schema changes » Figure out what to do in Views when entity schema changes
dawehner’s picture

Issue summary: View changes

Update the IS for some cases we should better support.

dawehner’s picture

FileSize
17.44 KB
34.83 KB

Reroll and some initial work.

plach’s picture

Status: Needs work » Needs review

Just curious to see what the bot says. I will have a look to this later...

The last submitted patch, 15: 2341323-15.patch, failed testing.

dawehner’s picture

FileSize
13.44 KB
39.33 KB

Worked a bit on it, now tests pass now.

Current problem

Currently I'm a bit helpless what we can do in case a data table is added, aka. an entity type got marked as translatable.

At this context you get a call to onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeInterface $original). In order to figure out which fields needs to be updated, you somehow need to generate the new
table mapping, but sadly there seems to be now way to get a new one. $storage::getTableMapping()
allows you to pass along a list of field storage definitions, but there seems to be no api to generate one given an $entity_type (EntityManagerInterface::getFieldStorageDefinitions() is nearly it, but it just allows you to pass along the $entity_type_id. Is there a different way to generate a new table mapping?

The last submitted patch, 23: 2341323-23.patch, failed testing.

jhodgdon’s picture

Really, if you have for instance a Node content type that is not translatable, and you suddenly decide it needs to be translatable, that causes the entity schema to change? ?!? That seems utterly ridiculous to me.

jhodgdon’s picture

Oh, maybe you're talking about: you have a module that defines entity type Foo. Then next week you update the module and Foo is now a translatable entity, so the schema changes. That seems more reasonable...

dawehner’s picture

Issue summary: View changes

Oh, maybe you're talking about: you have a module that defines entity type Foo. Then next week you update the module and Foo is now a translatable entity, so the schema changes. That seems more reasonable...

Well yeah, that is basically the usecase. We want to ensure that we don't end up with totally broken views,
especially because it is not obvious for site builders what happens and how they could fix it.

Status: Needs review » Needs work

The last submitted patch, 26: 2341323-24.patch, failed testing.

Status: Needs work » Needs review

plach queued 26: 2341323-24.patch for re-testing.

plach’s picture

@26:

At the moment we are working around that impossibility to generate a table mapping starting from an entity type + field storage definitions by temporarily changing the wrapped entity type in the storage class, see http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/Entity/Sql/S....
#2274017: Make SqlContentEntityStorage table mapping agnostic should provide a clean solution for this, but I don't think it's worth postponing this on that.

Status: Needs review » Needs work

The last submitted patch, 26: 2341323-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.07 KB
43.2 KB

Thank you @plach, this helped both in term of motivation as well as on the technical side.

We now handle the basic cases, not sure whether we should ever deal with <onEntityTypeDelete() for example.

General reviews, comments on the test coverage etc. would be highly welcomed.˚

Status: Needs review » Needs work

The last submitted patch, 35: 2341323-35.patch, failed testing.

plach’s picture

Overall this seems to be going the right way, I did not immediately realize it's still a work in progress so forgive some remarks :)

  1. +++ b/core/modules/system/src/Tests/Entity/EntityDefinitionTestTrait.php
    @@ -0,0 +1,195 @@
    +  protected function renameBaseTable() {
    ...
    +  protected function renameDataTable() {
    
    +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,270 @@
    +  public function __construct(EntityManagerInterface $entity_manager) {
    

    Missing PHP docs

  2. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,270 @@
    +  public function onEntityTypeDelete(EntityTypeInterface $entity_type) {
    

    I guess we could disable all views dealing with the deleted entity type in this case...

  3. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,270 @@
    +  }
    

    Are we sure there's nothing to do here? What would happen if a column were added or removed?

  4. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,270 @@
    +                $handler_config['table'] = $entity_type->getDataTable();
    

    Will Views automatically take care of possibly fixing joins in this case?

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
44.99 KB

I guess we could disable all views dealing with the deleted entity type in this case...

True, we could also drop some relationships/fields automatically.

Are we sure there's nothing to do here? What would happen if a column were added or removed?

At least for adding we should not anything, this is for sure. Removing
a field is a different case. Sadly removing every handler is problematic as there might be implicit dependencies somehow.

Will Views automatically take care of possibly fixing joins in this case?

Yes, it " fixes" it, because the joins are always just defined implicit in the hook_views_data() anyway.

plach’s picture

+++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
@@ -101,6 +124,37 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
+      if ($view->get('base_table') == $base_table) {
+        $view->delete();
+        unset($all_views[$id]);
+      }
+

Wouldn't it be better to disable the view? Just in case the entity type reappears later.

At least for adding we should not anything, this is for sure. Removing
a field is a different case. Sadly removing every handler is problematic as there might be implicit dependencies somehow.

Sorry if it wasn't clear, I was referring to the case of a field storage definition update, in particular if the storage definition schema changes. Not sure we should cover this case, just wanted to bring it up.

plach’s picture

Issue tags: +entity storage
dawehner’s picture

FileSize
45.54 KB
3.92 KB

Sorry if it wasn't clear, I was referring to the case of a field storage definition update, in particular if the storage definition schema changes. Not sure we should cover this case, just wanted to bring it up.

What could happen in this case? Would we magically move a field from the base table to the data table?

dawehner’s picture

Issue summary: View changes
FileSize
2.53 KB
45.53 KB

Test coverage is not a lie!

jibran’s picture

Found some doc issues.

  1. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,324 @@
    +  public function onEntityTypeCreate(EntityTypeInterface $entity_type) {
    

    Do we have to clear ViewsData cache here or not?

  2. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,324 @@
    +    $all_views = $this->entityManager->getStorage('view')->loadMultiple(NULL);
    

    No need to pass NULL.

  3. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,324 @@
    +      // Continue and look at all handlers and check whether the entity type
    +      // disappears.
    

    We are also unsetting it. So can you please update the comment as well.

  4. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,324 @@
    +      // @todo How can we ensure that we don't break views.
    

    Issue id please.

  5. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,324 @@
    +        $display =& $view->getDisplay($display_id);
    ...
    +        $display =& $view->getDisplay($display_id);
    ...
    +        $display =& $view->getDisplay($display_id);
    ...
    +        $display =& $view->getDisplay($display_id);
    ...
    +        $display =& $view->getDisplay($display_id);
    ...
    +        $display =& $view->getDisplay($display_id);
    

    I think it should be = &$view->

  6. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,324 @@
    +  /**
    +   * @param \Drupal\views\Entity\View[] $all_views
    +   * @param string $entity_type_id
    +   * @param string $old_base_table
    +   * @param string $new_base_table
    +   */
    ...
    +  /**
    +   * @param \Drupal\views\Entity\View[] $all_views
    +   * @param string $entity_type_id
    +   * @param string $old_data_table
    +   * @param string $new_data_table
    ...
    +  /**
    +   * @param \Drupal\views\Entity\View[] $all_views
    +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
    +   * @param string $data_table
    +   * @throws \Drupal\Core\Entity\EntityStorageException
    +   */
    ...
    +  /**
    +   * @param \Drupal\views\Entity\View[] $all_views
    +   * @param string $entity_type_id
    +   * @param string $field_name
    +   */
    

    doc block all messed up.

  7. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,324 @@
    +  protected function dataTableRemoval($all_views, $entity_type_id, $old_data_table, $base_table) {
    

    doc block missing.

dawehner’s picture

FileSize
46.33 KB
6.46 KB

Found some doc issues.

Amazing, so should we next time just put everything into a single function ;)

Do we have to clear ViewsData cache here or not?

I'd argue that this event will just be fired if a module is installed, which invalidates views cache already.

No need to pass NULL.

I did it on purpose to make things more explicit.

We are also unsetting it. So can you please update the comment as well.

Good point, fixed it.

Issue id please.

I'm still convinced that forcing issues causes more harm than good, because people won't think or rather never mention the todo.

I think it should be = &$view->

Fixed

Fixed all comments as well

plach’s picture

Status: Needs review » Needs work

A more in-depth review :)

  1. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,357 @@
    +  /**
    +   * Indicates that a base table got renamed.
    +   */
    +  const BASE_TABLE_RENAME = 0;
    +
    +  /**
    +   * Indicates that a data table got renamed.
    +   */
    +  const DATA_TABLE_RENAME = 1;
    +
    +  /**
    +   * Indicates that a data table got added.
    +   */
    +  const DATA_TABLE_ADDITION = 2;
    +
    +  /**
    +   * Indicates that a data table got removed.
    +   */
    +  const DATA_TABLE_REMOVAL = 3;
    +
    

    We are missing the same cases for the revision table here.

  2. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,357 @@
    +          debug("base table rename");
    ...
    +          debug("data table rename");
    ...
    +          debug("data table addition");
    ...
    +          debug("data table removal");
    

    debug leftover

  3. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,357 @@
    +  protected function baseTableRename($all_views, $entity_type_id, $old_base_table, $new_base_table) {
    ...
    +  protected function dataTableRename($all_views, $entity_type_id, $old_data_table, $new_data_table) {
    

    This code is virtually identical, can we call a common function and just pass the table key name and the old/new values?

  4. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,357 @@
    +   *
    

    Surplus blank line.

  5. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,357 @@
    +                $handler_config['table'] = $entity_type->getDataTable();
    

    Can we move this method call out of the loop?

  6. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,357 @@
    +              unset($display['display_options'][$handler_type][$id]);
    

    Shouldn't we leave a broken handler in place, so people can figure out something is broken and maybe restore the field definition? I'd avoid performing irreversible changes under the hood.

  7. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,338 @@
    +  public function testDeleteEntityType() {
    ...
    +  public function testBaseTableRename() {
    ...
    +  public function testDataTableRename() {
    ...
    +  public function testDataTableAddition() {
    ...
    +  public function testRevisionEnabling() {
    ...
    +  public function testRemoveFields() {
    ...
    +  public function testVariousTableUpdates() {
    ...
    +  protected function getUpdatedViewAndDisplay() {
    

    Missing PHP docs :)

  8. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,338 @@
    +    // user module provides 3 views for themselves.
    

    Missing capital U :)

  9. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,338 @@
    +    // We expect that views which use 'entity_test_Update' as base tables are
    

    Surplus capital U :D

  10. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,338 @@
    +    $disabled = 0;
    +    array_walk($views, function(View $view) use (&$disabled) {
    +      if (!$view->status()) {
    +        $disabled++;
    +      }
    +    });
    +    // Ensure that all related views are disabled.
    +    $this->assertEqual(3, $disabled);
    

    Wouldn't it be safer to check actual view names?

  11. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,338 @@
    +    $entity_manager = \Drupal::entityManager();
    

    Why don't we have an entity manager service in the test?

  12. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,338 @@
    +    $this->assertEqual('entity_test_update', $display['display_options']['fields']['id']['table']);
    +    $this->assertEqual('entity_test_update', $display['display_options']['fields']['name']['table']);
    ...
    +    // base + translation <-> base + translation + revision
    ...
    +    // base + revision <-> base + translation + revision
    ...
    +    // base <-> base + revision
    

    We should check the langcode field too: it will be moved from the base table to the revision table.

  13. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,338 @@
    +  // @todo think about translation, and revision tests.
    

    :)

dawehner’s picture

Status: Needs work » Needs review
FileSize
48.8 KB
13.56 KB

Thank you a LOT for the review. Here is just some small continuous work, still a lot is too be done.

This code is virtually identical, can we call a common function and just pass the table key name and the old/new values?

There is a hell lot of code which basically just iterates over all tables, we can certainly improve / simplify the code a lot, but I would love that we first have the complete test coverage.

Shouldn't we leave a broken handler in place, so people can figure out something is broken and maybe restore the field definition? I'd avoid performing irreversible changes under the hood.

For now I added a todo so we don't forget to discuss it.

Can we move this method call out of the loop?

Sure

Missing PHP docs :)

MEH

Wouldn't it be safer to check actual view names?

It is not only more secure, it is also more easy to read.

Why don't we have an entity manager service in the test?

We do have one already, we just don't use it at the moment.

We should check the langcode field too: it will be moved from the base table to the revision table.

Add a fixme for now.

Status: Needs review » Needs work

The last submitted patch, 46: 2341323-46.patch, failed testing.

dawehner’s picture

Title: Figure out what to do in Views when entity schema changes » Adapt the references field / table names in views, when corresponding entity schema changes

better title.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.49 KB
59.03 KB

We (plach, dawehner) worked together on that and discussed a lot.

We figured out which steps we want to support, so here is the working version for all changes we intend to apply onto the existing views.
One thing we aren't clear yet is how we handle if your entity type has no revision support anymore.

You could simply drop the corresponding fields / views, or maybe just disable them and rely on broken handlers. The later
one has the advantage that you can still look at your views and try to fix things.

plach’s picture

This is looking great, just a few notes:

  1. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -131,7 +131,16 @@ public function requiresEntityStorageSchemaChanges(EntityTypeInterface $entity_t
    +      $this->hasSharedTableNameChanges($entity_type, $original);
    

    I'd move this before index change detection, since it should be more performant.

  2. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php
    @@ -131,7 +131,16 @@ public function requiresEntityStorageSchemaChanges(EntityTypeInterface $entity_t
    +  protected function hasSharedTableNameChanges(EntityTypeInterface $entity_type, EntityTypeInterface $original) {
    

    Missing PHP doc

  3. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -155,27 +155,32 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
    +          // @FIXME the update on revision table rename is not triggered.
    

    Is this still the case?

  4. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -100,7 +100,7 @@ public function testDeleteEntityType() {
    +    $this->assertEqual(7, count($views));
    
    @@ -108,7 +108,7 @@ public function testDeleteEntityType() {
    +    $this->assertEqual(7, count($views));
    

    I thought we agreed on using views names :)

  5. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -155,6 +155,49 @@ public function testDataTableRename() {
    +    $this->updateEntityTypeToRevisionable();
    +    $this->entityDefinitionUpdateManager->applyUpdates();
    +
    +    $this->renameRevisionBaseTable();
    +    $this->entityDefinitionUpdateManager->applyUpdates();
    ...
    +    $this->updateEntityTypeToRevisionable();
    +    $this->updateEntityTypeToTranslatable();
    +    $this->entityDefinitionUpdateManager->applyUpdates();
    +
    +    $this->renameRevisionDataTable();
    +    $this->entityDefinitionUpdateManager->applyUpdates();
    
    @@ -191,6 +234,23 @@ public function testRevisionEnabling() {
    +    $this->updateEntityTypeToRevisionable();
    +    $this->entityDefinitionUpdateManager->applyUpdates();
    +
    +    $this->updateEntityTypeToNotRevisionable();
    +    $this->entityDefinitionUpdateManager->applyUpdates();
    +
    

    What about adding a few assertions in between to ensure we have the expected state after applying the fist set of updates?

plach’s picture

Status: Needs review » Needs work

We are also missing a check that the storage is an instance of SqlContentEntityStorage and thus implements the table layout(s) we are assuming here. As discussed, we should just bail out if not.

dawehner’s picture

Status: Needs work » Needs review
FileSize
62.09 KB
8.84 KB

We are also missing a check that the storage is an instance of SqlContentEntityStorage and thus implements the table layout(s) we are assuming here. As discussed, we should just bail out if not.

Totally forgot about that, sorry.

I thought we agreed on using views names :)

Expanded test coverage found more bugs.

dawehner’s picture

FileSize
60.93 KB
8.47 KB

Refactored the code a bit, to reduce some lines of boilerplate code.

plach’s picture

Status: Needs review » Needs work

Looks great, just a few more remarks and then we are ready to go :)

  1. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,436 @@
    +    elseif ($original->isRevisionable() && !$entity_type->isRevisionable()) {
    

    Mmh, this is a bit inconsistent with the rest of the code here.

  2. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,436 @@
    +          $this->dataTableAddition($all_views, $entity_type, $entity_type->getDataTable(),$entity_type->getBaseTable());
    ...
    +          $this->dataTableAddition($all_views, $entity_type, $entity_type->getRevisionDataTable(),$entity_type->getRevisionTable());
    

    Missing space after comma :)

  3. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,436 @@
    +      // Unset all handlers which point to no longer existing entity types.
    ...
    +    $this->removeFieldStorage($all_views, $storage_definition->getTargetEntityTypeId(), $storage_definition->getName());
    

    I'd still prefer to avoid making stuff disappear without warning ;)

  4. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,436 @@
    +  protected function processHandlers(array $all_views, callable $process) {
    

    Missing PHP doc.

  5. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -0,0 +1,436 @@
    +        // @todo Discuss whether we might even want to remove them. In general
    

    Can we remove this now? I think we discussed this in IRC.

dawehner’s picture

Status: Needs work » Needs review
FileSize
60.09 KB
7.54 KB

Mmh, this is a bit inconsistent with the rest of the code here.

Let's adapt it, as talked about in IRC.

Missing space after comma :)

Sure.

dawehner’s picture

FileSize
1.84 KB
59.05 KB

Let's not longer drop fields.

dawehner’s picture

FileSize
58.32 KB
1.33 KB

This is what you get if you have test coverage.

The last submitted patch, 56: 2341323-58.patch, failed testing.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Cool, I'd say this is ready to go.

yched’s picture

ViewsEntitySchemaSubscriber uses FieldStorageDefinitionEventSubscriberTrait, but doesn't seem to actually do anything in any of the the corresponding onField*() methods ?

plach’s picture

Status: Reviewed & tested by the community » Needs work

Oh, right. We removed the content of onFieldStorageDefinitionDelete() in one of the latest iterations.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
57.8 KB
1.26 KB

Good catch.

plach’s picture

RTBC +1

plach’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
@@ -0,0 +1,379 @@
+class ViewsEntitySchemaSubscriber implements EventSubscriberInterface {

I'm terribly sorry, I didn't notice this is not implementing EntityTypeListenerInterface, although it's implementing the related methods.

dawehner’s picture

Status: Needs work » Needs review
FileSize
57.76 KB
1.1 KB

No problem.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Hopefully this will be the good one ;)

jibran’s picture

+++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
@@ -0,0 +1,378 @@
+  public function onEntityTypeCreate(EntityTypeInterface $entity_type) {
+  }

Can we add a comment here?

plach’s picture

The last submitted patch, 62: 2341323-62.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 65: 2341323-65.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
57.89 KB
1.49 KB

Ups :)

plach’s picture

Status: Needs review » Reviewed & tested by the community

Now for reals

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,456 @@
    +    // @fixme ... include a revision field as well.
    +    // @fixme ... Add an explicit test for the langcode.
    

    Are these done?

  2. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,456 @@
    +
    +  public function testVariousTableUpdatesForRevisionView() {
    +    // base + revision <-> base + translation + revision
    

    Missing docblock

  3. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,456 @@
    +  /**
    +   * Gets a view and its display.
    +   */
    +  protected function getUpdatedViewAndDisplay($revision = FALSE) {
    

    docblock is needs @param and @return.

  4. +++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -0,0 +1,456 @@
    +  }
    +
    +  // @todo think about translation, and revision tests.
    +
    +}
    

    This @todo looks very lonely.

dawehner’s picture

Status: Needs work » Needs review
FileSize
58.01 KB
1.92 KB

Yeah the @todostatements are adressed in the meantime.

plach’s picture

+++ b/core/modules/views/src/Tests/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
@@ -451,6 +457,5 @@ protected function getUpdatedViewAndDisplay($revision = FALSE) {
 
-  // @todo think about translation, and revision tests.
 

Ubernitpick: I think there's a surplus blank line here :)

plach’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
908 bytes
57.99 KB

Zapped!

Also slightly reworded a comment.

dawehner’s picture

Ha, there is always room for one more nitpick :P

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 23e9d7f and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

dawehner’s picture

high five @all

  • alexpott committed 23e9d7f on 8.0.x
    Issue #2341323 by dawehner, plach: Adapt the references field / table...
plach’s picture

Yay!

dawehner’s picture

Issue tags: +D8 Accelerate

Retrospective sky tagging.

Status: Fixed » Closed (fixed)

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