Problem/Motivation

\Drupal\views\EventSubscriber\ViewsEntitySchemaSubscriber::onEntityTypeUpdate() re-saves all the existing views of a site if an entity type was updated by the entity definition update manager.

This is causing a lot of problems in real-world sites because a lot of views handlers don't define their config dependencies properly, which means that some views can not be re-saved without user intervention.

Proposed resolution

Don't re-save views that weren't changed by an entity update process and catch any exception thrown while saving to avoid making the update process to fail. If an error occurs on save, it most certainly means that the view was not set up correctly or that it does not support the entity schema changes. In both cases there's no point in making the update fail because the view will need some (likely manual) tweak anyway.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Release notes snippet

Nope.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu created an issue. See original summary.

amateescu’s picture

Status: Active » Needs review
FileSize
12.74 KB

This patch restricts the views that are being re-saved after an entity type update to only those that needed to be changed.

amateescu’s picture

Issue tags: +8.7.0 update

Tagging because this should greatly reduce the number of failures of the 8.7.0 upgrade path.

plach’s picture

Looks good to me!

  1. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -270,12 +276,14 @@ protected function baseTableRename($all_views, $entity_type_id, $old_base_table,
    +        $view->set('_updated', TRUE);
    

    👍

    This is ok to do since _updated is not an exported property, so there's no risk of it being persisted.

  2. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -194,11 +195,16 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
    +      $view->set('_updated', NULL);
           $view->trustData()->save();
    

    I'm wondering whether it would make sense to wrap these lines in a try/catch block and just log any exception: if we have an error it most certainly means that the view was not set up correctly or that it does not support the entity schema changes. In both cases there's no point in making the update fail because the view will need some (likely manual) tweak anyway, as you were pointing out in the OP.

  3. +++ b/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
    @@ -429,4 +480,24 @@ protected function getUpdatedViewAndDisplay($revision = FALSE) {
    +    $all_view_ids = array_keys($this->entityTypeManager->getStorage('view')->loadMultiple());
    

    I manually checked by debugging the test that this array contains always more views than the updated ones. Since this is the case, I think it would make sense to add the following assertion to make it more explicit:

    $this->assertTrue(count($all_view_ids) > count($updated_view_ids));
    
amateescu’s picture

Priority: Major » Critical
FileSize
18.65 KB
7.8 KB

Thanks for the review!

2. I was wondering as well if we should do that, your argument convinced to bite the bullet :)

3. Sure thing, it can't hurt I guess..

Raising to critical because this will now solve a lot of the failures that were reported in #3052318: Update from 8.6 to 8.7 fails due to corrupt menu_link_content or taxonomy_term entity data and various other 8.7.0 update issues.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

If we happen to reroll this, I just realized we can do the following:

+++ b/core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php
@@ -429,4 +519,28 @@ protected function getUpdatedViewAndDisplay($revision = FALSE) {
+    $this->assertTrue(count($all_view_ids) > count($updated_view_ids));

This could be rewritten as

$this->assertGreaterThan(count($updated_view_ids), count($all_view_ids));
plach’s picture

Title: ViewsEntitySchemaSubscriber should not update views that weren't changed by an entity update process » ViewsEntitySchemaSubscriber should make an entity update fail if a view cannot be resaved
plach’s picture

Title: ViewsEntitySchemaSubscriber should make an entity update fail if a view cannot be resaved » ViewsEntitySchemaSubscriber should not make an entity update fail if a view cannot be resaved

NOT

plach’s picture

Issue summary: View changes

And now the IS as well.

plach’s picture

Btw, it would be good to add a follow-up to add the same try/catch to ::onEntityTypeDelete(), I'll do that once this is committed, if the latest approach is confirmed.

amateescu’s picture

Fixed #6 :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. Here are some minor nits - this looks like an excellent improvement to a more robust upgrade path
  2. +++ b/core/modules/system/tests/modules/entity_test_update/entity_test_update.module
    @@ -45,6 +46,15 @@ function entity_test_update_entity_type_alter(array &$entity_types) {
    +  if (\Drupal::state()->get('entity_test_update.throw_view_exception', FALSE) && $entity->id() === 'test_view_entity_test') {
    +    throw new \LogicException('The view could not be saved.');
    +  }
    

    This would be slightly easier to grok if entity_test_update.throw_view_exception was set to 'test_view_entity_test' rather than TRUE.

  3. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -81,14 +83,28 @@ class ViewsEntitySchemaSubscriber implements EntityTypeListenerInterface, EventS
    +      @trigger_error('Calling ViewsEntitySchemaSubscriber::__construct() with the $logger argument is supported in drupal:8.7.0 and will be required before drupal:9.0.0. See https://www.drupal.org/project/drupal/issues/3052492.', E_USER_DEPRECATED);
    

    8.7.1 no?

  4. +++ b/core/modules/views/src/EventSubscriber/ViewsEntitySchemaSubscriber.php
    @@ -194,12 +210,28 @@ public function onEntityTypeUpdate(EntityTypeInterface $entity_type, EntityTypeI
    +        $this->logger->critical("The '@view_id' view could not be updated automatically while processing an entity schema update for the '@entity_type_id' entity type.", [
    +          '@view_id' => $view->id(),
    +          '@entity_type_id' => $entity_type->id(),
    +        ]);
    

    How about using placeholders ie % and not '@..'

  5. Can we open a follow-up to refactor \Drupal\views\EventSubscriber\ViewsEntitySchemaSubscriber::onEntityTypeUpdate() so that we pass a single view to each of the methods like baseTableRename() - that way we could do something better than adding the _update state. I think it might also make the code much easier to read and more sensible.
amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
18.63 KB
3.79 KB
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed d9e8e19c29 to 8.8.x and 601744f25c to 8.7.x. Thanks!

  • alexpott committed d9e8e19 on 8.8.x
    Issue #3052492 by amateescu, plach, alexpott:...

  • alexpott committed 601744f on 8.7.x
    Issue #3052492 by amateescu, plach, alexpott:...
waverate’s picture

Did this patch make it into D8.7.1?

amateescu’s picture

@waverate, nope, 8.7.1 was just a security release. It will be included in 8.7.2 :)

XSramik’s picture

Did this patch made it into D8.7.2? I just tried 8.6.16 > 8.7.2 and ended with the same error.

waverate’s picture

@XSramik. Yes it did. See changelog.

crash_springfield’s picture

@waverate This is still breaking for me as well

crash_springfield’s picture

Status: Fixed » Active
catch’s picture

XSramik and crash_springfield could you post the exact error messages you're getting for the update to 8.7.2?

XSramik’s picture

Big thanks to all contributors to fix this issue! Great job!
@waverate Thanks, in the end I managed to update to D8.7.2.

The reason why this patch didn't worked for me right away was that updatedb was performed on the db with artifacts from previous failed update. I didn't know that mysql -u dbdev -ppassword < /www/backup/db.sql rewrites just the tables that exists in a source db.sql and keeps all other tables in dbdev.sql intact (the artifacts from previous failed update).

I realized this when I followed #36, #39 and #60 and it suddenly worked (on dbdev).

So to solve this, either:

  • You have db before performing the failed-update, then you can just update to D8.7.2 and it should work (if you revert from the backup, avoid mistake I did).
  • You have no backup and already run into this failure before - then you have to manually DROP some tables (#39 suggested steps that worked for some users, however I am not competent to judge the consequences of this actions).
catch’s picture

Status: Active » Fixed

XSramik thanks for reporting back! I'm going to move this back to fixed.

Mitriy-Bug’s picture

Applied patches, such error:
Drupal\Core\Entity\EntityStorageException: Exception thrown while performing a schema update. Невозможно переименовать tmp_d3cbdbmenu_link_content_revision в menu_link_content_revision: таблица menu_link_content_revision уже существует. in Drupal\Core\Entity\Sql\SqlContentEntityStorage->wrapSchemaException() (line 1611 of /home/sites/rating-avtosalon.ru/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Status: Fixed » Closed (fixed)

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

plach’s picture