Config entity static cache doesn't get reset and isn't override aware

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -255,6 +266,46 @@ protected function has($id, EntityInterface $entity) {
+            $entities[$id] = $this->entities[$id][$key];

This does change the structure of $this->entities, which was recently improved to document what it contains. So it would be different for this. Not sure how to properly document that.

Ouch, no idea why I missed that, yes that looks great. You're just too good at this, only a bit thinking out loud below, no real feedback :)

Would also appreciate your thoughts in #1885830: Enable static caching for config entities in regards to enabling it by default vs. only doing it for config entities where we know it's worth it, like roles.

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   @@ -742,12 +738,9 @@ protected function doSave($id, EntityInterface $entity) {
        $this->invokeFieldMethod($is_new ? 'insert' : 'update', $entity);
        $this->saveFieldItems($entity, !$is_new);
   -    $this->resetCache($cache_ids);
    
   

   Apparently it's not a problem that we're moving when exactly the caches are cleared, and there's almost nothing after that, so that seems fine, even logical to do it after the translation hooks were called too.

   That said, there is #1729812: Separate storage operations from reactions, not exactly sure what this would mean. I guess it would still be fine as the hooks already happen in the parent and the current implementation needs to override save() to be able to have them inside the transaction.

2. +++ b/core/modules/config/src/Tests/ConfigEntityStaticCacheTest.php
   @@ -0,0 +1,96 @@
   +   */
   +  public function testCacheHit() {
   +    $entity_1 = entity_load($this->entityTypeId, $this->entityId);
   +    $entity_2 = entity_load($this->entityTypeId, $this->entityId);
   +    // config_entity_static_cache_test_config_test_load() sets _loadToken to a
   +    // random string. If they match, it means $entity_2 was retrieved from the
   +    // static cache rather than going through a separate load sequence.
   +    $this->assertIdentical($entity_1->_loadToken, $entity_2->_loadToken);
   +  }
   

   It worked pretty well to use unit tests for cache hit/miss testing in #597236: Add entity caching to core because you can just mock the inner load method and easily define if and how often it should be called. Maybe that would be an easier way to test it?

   Downside is that unit tests are always a bit constructed, and it's kind of an integration test here...

   That said, very nice work on the tests.

3. +++ b/core/modules/config/tests/config_entity_static_cache_test/config_entity_static_cache_test.module
   @@ -0,0 +1,31 @@
   +  static $random;
   +  if (!$random) {
   +    $random = new Random();
   +  }
   

   Was wondering if this is really necessary for tests. But it is, because only if we do it like this, we ensure that the random strings are unique.

Ok, looks good to me, let's get this in, so we can get back the other issue.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
@@ -255,6 +266,46 @@ protected function has($id, EntityInterface $entity) {
+          $key = $this->configFactory->getCacheKey($this->getConfigPrefix() . $id);
...
+        $key = $this->configFactory->getCacheKey($this->getConfigPrefix() . $id);

I wonder if it is worth delegating cache key generation to a protected method and moving outside the foreach since there is no need for each ID to be passed to the ConfigFactory::getCacheKey() - we could pass an empty string for all we care.

I also think it might be worth adding a test for disabling overrides missing the cache and then enabling overrides and getting a cache hit and then disabling again and getting a cache hit.

Also needs a reroll for #2306455: ConfigEntityStorage adds too many dots since ConfigEntityStorage::getConfigPrefix has become protected and now called getPrefix()

Something like this.

Yeah I think that is a good option. Plus we could look at what actually needs to be on the public ConfigFactory interface.

#2312135: Rename and protect ConfigFactory::getCacheKey(s)(); add a separate ConfigFactoryInterface::getCacheKeys() just landed, this is now unblocked!

The test coverage looks solid to me. Only minor stuff and nitpicks.

1. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php
   @@ -271,6 +282,46 @@ protected function has($id, EntityInterface $entity) {
   +    if ($this->entityType->isStaticallyCacheable() && !empty($this->entities)) {
   

   Isn't this tied to a single entity type, i.e. ConfigEntityType? Is it then still sensible to perform this check?

2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityDatabaseStorage.php
   index 3dc3bb6..5b831bd 100644
   --- a/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   
   --- a/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   +++ b/core/lib/Drupal/Core/Entity/EntityStorageBase.php
   

   I like how everything moves from ContentEntityStorageBase to EntityStorageBase: consistency FTW! :)

3. +++ b/core/modules/config/src/Tests/ConfigEntityStaticCacheTest.php
   @@ -0,0 +1,110 @@
   +class ConfigEntityStaticCacheTest extends KernelTestBase {
   

   Why can't this be a unit test?

4. +++ b/core/modules/config/src/Tests/ConfigEntityStaticCacheTest.php
   @@ -0,0 +1,110 @@
   +    $entity_1 = entity_load($this->entityTypeId, $this->entityId);
   

   I've been told that I should never use entity_load() anymore, but <EntityTypeId>::load() instead. Since this tests always uses $this->entityTypeId === 'config_test, can't this be changed to ConfigTest::load()?

RE: #17.1: ok, that makes sense, thanks.

RE: #17.3: then let's not hold up this patch on that.

#17.4 is a nitpick, so it'd be silly to delay this patch for that. But at the same time, I'm not at all deeply familiar with the config system, so it'd be great if somebody more familiar with the config system could do the final review.

I think its better to not do this as a unit test since we would need to mock way too much of what we actually want to test here. This is about interaction of several pieces. Otherwise looks good and resolves prior concerns.

Random fail.

Committed and pushed to 8.x. Thanks!

This change broke Postgresql even more. I think this is due to the changes from ContentEntityDatabaseStorage to EntityDatabaseStorage.

I think it's possible that the resetCache call in ContentEntityDatabaseStorage will create the cache table for the first time, and since that doesn't happen, it fails on other things in the installation.

Adding the WIP patch for mimicking implicit commits in transactions allows Postgresql to continue through the install, but that requires a lot of testing because of what it does. See #2181291: Prevent a query from aborting the entire transaction in pgsql.

I guess that this patch clears the cache inside a transaction (like most entity save operations), which is going to fail hard because the cache table has not been created yet. The transaction is completely rolled back and no other queries are allowed.

I'll keep this open, but we shouldn't revert the commit.

Yup, this needs some work ;-) PostgreSQL testbot exceptions before patch ~240 - after patch ~1440. All errors are about:

A non well formed numeric value
    encounteredException->__construct('SQLSTATE[25P02]: In failed sql
    transaction: 7 ERROR:  current transaction is aborted, commands ignored
    until end of transaction block', '25P02', Object)

Thanks for keeping this open as a reminder. Closing as other issue was committed.