Recalculation of display cacheability metadata during module install causing error [#3060081]

This was discovered while testing #2824640: Views cached results are not taking full cacheability metadata into account in Search API.

I'm seeing failures in my project when I enable a custom module that defines a Search API view with some custom facets. During the enabling of the module I am getting the error "No facet source defined for facet."

The problem does not occur when importing single config, or when syncing config. It only happens when enabling a module that contains the view. I did some searching and there are other reports of similar issues (eg #2170579: Race conditions on "default config import", #2805645: Config can be created twice during module install.).

From the backtrace I can see that Views is recalculating the cacheability metadata when the view is saved from config. This causes the problem because at this time the facet config has not yet been imported and does not exist.

Views should not recalculate the cacheability metadata when importing config due to enabling a module. This is only needed when the view is saved through the UI or API, as explained in \Drupal\views\Entity\View::addCacheMetadata():

  /**
   * Fills in the cache metadata of this view.
   *
   * Cache metadata is set per view and per display, and ends up being stored in
   * the view's configuration. This allows Views to determine very efficiently:
   * - the max-age
   * - the cache contexts
   * - the cache tags
   *
   * In other words: this allows us to do the (expensive) work of initializing
   * Views plugins and handlers to determine their effect on the cacheability of
   * a view at save time rather than at runtime.
   */
  protected function addCacheMetadata() {}

Here is the backtrace that leads to the error, it shows that the cacheability metadata is recalculated when the view is imported due to a module installation:

./web/modules/contrib/facets/src/Entity/Facet.php:450
./web/modules/contrib/facets/src/FacetManager/DefaultFacetManager.php:115
./web/modules/contrib/facets/facets.module:89
./web/core/lib/Drupal/Core/Extension/ModuleHandler.php:539
./web/modules/contrib/search_api/src/Query/Query.php:555
./web/modules/contrib/search_api/src/Query/Query.php:709
./web/modules/contrib/search_api/src/Plugin/views/query/SearchApiQuery.php:699
./web/core/lib/Drupal/Core/Cache/CacheableMetadata.php:171
./web/core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:2289
./web/core/modules/views/src/Entity/View.php:379
./web/core/modules/views/src/Entity/View.php:312
./web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:490
./web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:445
./web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:263
./web/core/lib/Drupal/Core/Entity/EntityBase.php:394
./web/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:613
./web/core/lib/Drupal/Core/Config/ConfigInstaller.php:364
./web/core/lib/Drupal/Core/Config/ConfigInstaller.php:136
./web/modules/contrib/config_provider/src/ProxyClass/ConfigProviderConfigInstaller.php:75
./web/core/lib/Drupal/Core/Extension/ModuleInstaller.php:268

Comment	File	Size	Author
#15	interdiff.txt	790 bytes	pfrenssen
#15	3060081-14-test-only.patch	8.93 KB	pfrenssen
#15	8.7.x: PHP 7.1 & MySQL 5.7 26,343 pass, 1 fail 8.8.x: PHP 7.1 & MySQL 5.7 26,455 pass, 1 fail
#15	3060081-14.patch	9.7 KB	pfrenssen
#15	8.7.x: PHP 7.1 & MySQL 5.7 26,352 pass
#12	interdiff.txt	2.03 KB	borisson_
#12	3060081-12.patch	9.7 KB	borisson_
#12	8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass, 1 fail
#9	3060081-9-test-only.patch	8.92 KB	pfrenssen
#9	8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass, 1 fail
#9	interdiff.txt	1.3 KB	pfrenssen
#9	3060081-9.patch	9.7 KB	pfrenssen
#9	8.7.x: PHP 7.1 & MySQL 5.7 26,342 pass
#4	interdiff.txt	790 bytes	pfrenssen
#4	3060081-4-test-only.patch	8.85 KB	pfrenssen
#4	8.8.x: PHP 7.1 & MySQL 5.7 CI aborted PHP 7.1 & MySQL 5.7 26,431 pass, 1 fail 8.7.x: PHP 7.1 & MySQL 5.7 26,340 pass, 1 fail
#4	3060081-4.patch	9.62 KB	pfrenssen
#4	8.8.x: PHP 7.1 & MySQL 5.7 26,432 pass 8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass
#2	3060081-2.patch	587 bytes	pfrenssen
#2	8.8.x: PHP 7.2 & MySQL 5.5 26,429 pass

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

6 June 2019 at 22:03

pfrenssen created an issue. See original summary.

Comment #2

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 6 June 2019 at 22:06

Issue tags:

+Needs tests

File	Size
3060081-2.patch	587 bytes
8.8.x: PHP 7.2 & MySQL 5.5 26,429 pass

We already have a switch that will skip the recalculation of the cacheability metadata when config is being synced (and yes, I can confirm that this is correct, so we can get rid of the @todo).

We can extend this by checking if the view config is trusted. This flag is set only when the config is imported during a module or theme install.

Ref. ConfigEntityInterface::trustData():

  /**
   * Sets that the data should be trusted.
   *
   * If the data is trusted then dependencies will not be calculated on save and
   * schema will not be used to cast the values. Generally this is only used
   * during module and theme installation. Once the config entity has been saved
   * the data will no longer be marked as trusted. This is an optimization for
   * creation of configuration during installation.
   *
   * @return $this
   *
   * @see \Drupal\Core\Config\ConfigInstaller::createConfiguration()
   */
  public function trustData();

Comment #3

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 6 June 2019 at 22:10

Issue summary:

View changes

Comment #4

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 7 June 2019 at 12:43

Status:	Active	» Needs review
Issue tags:	-Needs tests

File	Size
3060081-4.patch	9.62 KB
8.8.x: PHP 7.1 & MySQL 5.7 26,432 pass 8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass
3060081-4-test-only.patch	8.85 KB
8.8.x: PHP 7.1 & MySQL 5.7 CI aborted PHP 7.1 & MySQL 5.7 26,431 pass, 1 fail 8.7.x: PHP 7.1 & MySQL 5.7 26,340 pass, 1 fail
interdiff.txt	790 bytes

1 file was hidden/shown/deleted

File	Size
3060081-2.patch	587 bytes
8.8.x: PHP 7.2 & MySQL 5.5 26,429 pass

Added a test. Found inspiration in \Drupal\Tests\Core\Config\Entity\ConfigEntityBaseUnitTest::testPreSaveDuringSync() and \Drupal\KernelTests\Core\Config\ConfigInstallTest::testModuleInstallation().

Comment #5

7 June 2019 at 13:30

Status:

Needs review

» Needs work

The last submitted patch, 4: 3060081-4-test-only.patch, failed testing. View results

Comment #6

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 7 June 2019 at 13:41

Status:

Needs work

» Needs review

Comment #7

claudiu.cristea

Romanian

Arad 🇷🇴

CreditAttribution: claudiu.cristea as a volunteer and at Webikon for European Commission and European Union Institutions, Agencies and Bodies commented 8 June 2019 at 09:39

Version:

8.8.x-dev

» 8.7.x-dev

Bugs are going in 8.7.x.

Comment #8

Lendude

Dutch

Amsterdam

CreditAttribution: Lendude at ezCompany commented 8 June 2019 at 12:06

Looking really good, nice coverage.

```
+++ b/core/modules/views/src/Entity/View.php
@@ -307,8 +307,10 @@ public function preSave(EntityStorageInterface $storage) {
+    // saved through the UI or API. It should not be done when we are syncing
```
Do we have coverage for the 'through the UI' statement? We are not adding any here, but maybe some existing coverage takes care of this? If not, I think it might be worth adding to make sure we are not messing with the flow that will impact the most users.

+++ b/core/modules/views/tests/src/Kernel/CachableMetadataCalculationTest.php
@@ -0,0 +1,106 @@
+    // Reset the state so we are ready for the next test.
+    $this->state->set('views_test_cacheable_metadata_has_been_accessed', FALSE);

Maybe not do this in the assert but add a resetState method to make this more explicit? Or update the method name and docblock to indicate this is doing more than just an assert?

Comment #9

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 10 June 2019 at 10:04

Issue tags:

+drupaldevdays

File	Size
3060081-9.patch	9.7 KB
8.7.x: PHP 7.1 & MySQL 5.7 26,342 pass
interdiff.txt	1.3 KB
3060081-9-test-only.patch	8.92 KB
8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass, 1 fail

3 files were hidden/shown/deleted

File	Size
3060081-4.patch	9.62 KB
8.8.x: PHP 7.1 & MySQL 5.7 26,432 pass 8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass
3060081-4-test-only.patch	8.85 KB
8.8.x: PHP 7.1 & MySQL 5.7 CI aborted PHP 7.1 & MySQL 5.7 26,431 pass, 1 fail 8.7.x: PHP 7.1 & MySQL 5.7 26,340 pass, 1 fail
interdiff.txt	790 bytes

Thanks for the review!

Do we have coverage for the 'through the UI' statement? We are not adding any here, but maybe some existing coverage takes care of this? If not, I think it might be worth adding to make sure we are not messing with the flow that will impact the most users.

Good suggestion. I did a search in the core/modules/views_ui/tests/src/Functional folder and it seems we are well covered. There are 86 instances of clicking the "Save" button in 31 different tests.
Fixed.

Comment #10

10 June 2019 at 10:51

Status:

Needs review

» Needs work

The last submitted patch, 9: 3060081-9-test-only.patch, failed testing. View results

Comment #11

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 10 June 2019 at 11:35

Status:

Needs work

» Needs review

Comment #12

borisson_

Dutch

Mechelen, 🇧🇪

CreditAttribution: borisson_ at Calibrate commented 12 June 2019 at 06:19

File	Size
3060081-12.patch	9.7 KB
8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass, 1 fail
interdiff.txt	2.03 KB

3 files were hidden/shown/deleted

File	Size
3060081-9.patch	9.7 KB
8.7.x: PHP 7.1 & MySQL 5.7 26,342 pass
interdiff.txt	1.3 KB
3060081-9-test-only.patch	8.92 KB
8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass, 1 fail

I reviewed this at drupal dev days with @phenaproxima. He pointed out that we should rename the assert and use assertEquals over assertSame.

Comment #13

phenaproxima

he/him

English

Massachusetts

CreditAttribution: phenaproxima at Acquia commented 12 June 2019 at 06:21

Status:

Needs review

» Reviewed & tested by the community

@borisson_ walked me through the patch and I think I grok it. The explicit test coverage is nice, and as @pfrenssen points out, there seems to be plenty of coverage from the UI angle. So, although my familiarity with Views' internals is not all that high, I think this patch makes perfect sense and elegantly fixes this edge case. I feel okay RTBCing it, once testbot goes green.

Comment #14

12 June 2019 at 07:12

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 12: 3060081-12.patch, failed testing. View results

Comment #15

pfrenssen

Sofia

CreditAttribution: pfrenssen at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commented 12 June 2019 at 10:05

Status:

Needs work

» Needs review

File	Size
3060081-14.patch	9.7 KB
8.7.x: PHP 7.1 & MySQL 5.7 26,352 pass
3060081-14-test-only.patch	8.93 KB
8.7.x: PHP 7.1 & MySQL 5.7 26,343 pass, 1 fail 8.8.x: PHP 7.1 & MySQL 5.7 26,455 pass, 1 fail
interdiff.txt	790 bytes

1 file was hidden/shown/deleted

File	Size
3060081-12.patch	9.7 KB
8.7.x: PHP 7.1 & MySQL 5.7 26,341 pass, 1 fail

We can do the assertSame() but then we need to provide a default value because NULL is not a boolean.

Comment #16

12 June 2019 at 10:45

Status:

Needs review

» Needs work

The last submitted patch, 15: 3060081-14-test-only.patch, failed testing. View results

Comment #17

borisson_

Dutch

Mechelen, 🇧🇪

CreditAttribution: borisson_ at Calibrate commented 12 June 2019 at 12:11

Status:

Needs work

» Needs review

The second patch was test-only, so back to Needs review.

Comment #18

phenaproxima

he/him

English

Massachusetts

CreditAttribution: phenaproxima at Acquia commented 12 June 2019 at 12:18

Status:	Needs review	» Reviewed & tested by the community
Issue tags:	-drupaldevdays	+DevDaysCluj

I think that's fine.

Comment #19

rosinegrean CreditAttribution: rosinegrean at PitechPlus commented 20 June 2019 at 09:01

Issue tags:

-DevDaysCluj

+DevDaysTransylvania

Comment #20

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 20 June 2019 at 20:51

Status:

Reviewed & tested by the community

» Fixed

Committed 9550d9d and pushed to 8.8.x, and cherry-picked to 8.7.x. Thanks!

Comment #21

20 June 2019 at 20:52

catch committed 9550d9d on 8.8.x

Issue #3060081 by pfrenssen, borisson_: Recalculation of display...

Comment #22

20 June 2019 at 20:52

catch committed d025b16 on 8.7.x

Issue #3060081 by pfrenssen, borisson_: Recalculation of display...

Comment #23

Wim Leers

Ghent 🇧🇪🇪🇺

CreditAttribution: Wim Leers at Acquia commented 20 June 2019 at 21:24

Issue tags:

+D8 cacheability

Nice one!

+++ b/core/modules/views/tests/modules/views_test_cacheable_metadata_calculation/src/Plugin/views/access/CachableMetadataCalculationTest.php
@@ -0,0 +1,104 @@
+class CachableMetadataCalculationTest extends AccessPluginBase implements CacheableDependencyInterface {

Übernit: s/Cachable/Cacheable/ 🤓