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 | 3060081-14.patch | 9.7 KB | pfrenssen |
Comments
Comment #2
pfrenssenWe 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()
:Comment #3
pfrenssenComment #4
pfrenssenAdded a test. Found inspiration in
\Drupal\Tests\Core\Config\Entity\ConfigEntityBaseUnitTest::testPreSaveDuringSync()
and\Drupal\KernelTests\Core\Config\ConfigInstallTest::testModuleInstallation()
.Comment #6
pfrenssenComment #7
claudiu.cristeaBugs are going in 8.7.x.
Comment #8
LendudeLooking really good, nice coverage.
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.
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
pfrenssenThanks for the review!
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.Comment #11
pfrenssenComment #12
borisson_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@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 #15
pfrenssenWe can do the
assertSame()
but then we need to provide a default value because NULL is not a boolean.Comment #17
borisson_The second patch was test-only, so back to Needs review.
Comment #18
phenaproximaI think that's fine.
Comment #19
rosinegrean CreditAttribution: rosinegrean at PitechPlus commentedComment #20
catchCommitted 9550d9d and pushed to 8.8.x, and cherry-picked to 8.7.x. Thanks!
Comment #23
Wim LeersNice one!
Übernit: s/Cachable/Cacheable/ 🤓