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.
Comment | File | Size | Author |
---|---|---|---|
#13 | interdiff-13.txt | 3.79 KB | amateescu |
#13 | 3052492-13.patch | 18.63 KB | amateescu |
#11 | interdiff-11.txt | 799 bytes | amateescu |
#11 | 3052492-11.patch | 18.65 KB | amateescu |
#5 | interdiff-5.txt | 7.8 KB | amateescu |
Comments
Comment #2
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThis patch restricts the views that are being re-saved after an entity type update to only those that needed to be changed.
Comment #3
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedTagging because this should greatly reduce the number of failures of the 8.7.0 upgrade path.
Comment #4
plachLooks good to me!
👍
This is ok to do since
_updated
is not an exported property, so there's no risk of it being persisted.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.
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:
Comment #5
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks 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.
Comment #6
plachGreat, thanks!
If we happen to reroll this, I just realized we can do the following:
This could be rewritten as
Comment #7
plachComment #8
plachNOT
Comment #9
plachAnd now the IS as well.
Comment #10
plachBtw, 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.Comment #11
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedFixed #6 :)
Comment #12
alexpottThis would be slightly easier to grok if entity_test_update.throw_view_exception was set to 'test_view_entity_test' rather than TRUE.
8.7.1 no?
How about using placeholders ie
%
and not'@..'
Comment #13
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks for the review! Fixed all points and opened the followup: #3052918: Improve code readability in ViewsEntitySchemaSubscriber by invoking each update operation per view
Comment #14
alexpottCommitted and pushed d9e8e19c29 to 8.8.x and 601744f25c to 8.7.x. Thanks!
Comment #17
waverate CreditAttribution: waverate commentedDid this patch make it into D8.7.1?
Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@waverate, nope, 8.7.1 was just a security release. It will be included in 8.7.2 :)
Comment #19
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedComment #20
XSramik CreditAttribution: XSramik commentedDid this patch made it into D8.7.2? I just tried 8.6.16 > 8.7.2 and ended with the same error.
Comment #21
waverate CreditAttribution: waverate commented@XSramik. Yes it did. See changelog.
Comment #22
crash_springfield CreditAttribution: crash_springfield commented@waverate This is still breaking for me as well
Comment #23
crash_springfield CreditAttribution: crash_springfield commentedComment #24
catchXSramik and crash_springfield could you post the exact error messages you're getting for the update to 8.7.2?
Comment #25
XSramik CreditAttribution: XSramik commentedBig 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 sourcedb.sql
and keeps all other tables indbdev.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:
Comment #26
catchXSramik thanks for reporting back! I'm going to move this back to fixed.
Comment #27
Mitriy-Bug CreditAttribution: Mitriy-Bug commentedApplied 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).
Comment #29
plachA related issue was reported at #3006815: ViewsEntitySchemaSubscriber may fail when a view has a broken handler.