Cannot load entity by uuid after rename

+++ b/core/modules/system/system.post_update.php
@@ -130,3 +131,28 @@ function system_post_update_change_delete_action_plugins() {
+function system_post_update_resave_non_loadable_config_entities() {

We should use the new config.entity_updater service instead of manually re-saving all config entities :) See https://www.drupal.org/node/2949630

Wow, absolutely crazy find and really nice fix. The new code fits really neatly in with the existing code - very unfortunate that we forgot to put it there in the first place.

Some notes:

1. +++ b/core/modules/system/system.post_update.php
   @@ -130,3 +132,70 @@ function system_post_update_change_delete_action_plugins() {
   +            if ($lookup_key === 'uuid') {
   ...
   +                // If the values contain more than one record then there is
   +                // custom code which is adding information to the records and
   +                // we cannot simply delete that information. Instead we inform
   +                // of the problem.
   +                else {
   +                  \Drupal::logger('system')
   +                    ->warning('Because of an error by renaming config entities a record in key value store for fast lookups still contains the previous name of the config entity. As there are multiple entries in the record it could not be cleaned automatically and requires your attention. Please take a look at the record with the name %record in the key value store for the collection %collection.', [
   +                      '%record' => $key,
   +                      '%collection' => $query_factory::CONFIG_LOOKUP_PREFIX . $entity_type_id
   +                    ]);
   +                }
   

   So if we only clean-up the UUID ones I think we can simplify the update path and simpy query the key-value store directly for items in the "config.entity.key_store.$entity_type_id" collection with the name/key "uuid:$entity_uuid".

   I am wondering, though, why we do are not updating the other keys, as well? All that is stored is the ID, so regardless of the actual keys, we know what we have to update, right?

   Having written that, it seems to me that - even if we do update all possible keys - we should be able to perform the update without using reflection, by first building up a map of renames using the loadEntityByUuid() check just like you are doing now and then in a second step iterating over all values in "config.entity.key_store.$entity_type_id" key-value collection and replacing any IDs that match our rename map.

   What do you think?

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/ConfigEntityQueryTest.php
   @@ -109,6 +109,31 @@ protected function setUp() {
   +  public function testLoadByUuidAfterRename() {
   

   I think this test could be added to ::testLookupKeys() instead, no? We already test the save and delete event there, so I think that would make sense.

+++ b/core/modules/system/system.post_update.php
@@ -130,3 +132,70 @@ function system_post_update_change_delete_action_plugins() {
+ * Re-saves config entities that cannot be loaded by uuid.
...
+ * By re-saving the entities that cannot be loaded by uuid the config key store
+ * for fast lookups will be updated, which will make the entities loadable by
+ * uuid.

uuid -> UUID

Also, instead of having the longer description in the docblock, let's remove it there and add it as in inline comment in the function. The docblock will be parsed and displayed when running the updates via update.php and updates with multi-line descriptions always look pretty weird there.

Needs a re-roll for System post-updates added in the meantime.

Reroll for #11 patch for 8.7.x

OK, this needed an update for #2624770: Use more specific entity.manager services in core.services.yml .

Ahh sorry, I accidentally included #3005137: Register the node/add path as link template "add-page" in #18. Reverted that and also fixed the coding style violations.

Some updates to the post_update hook implementation. As is noticed in #14, what stands for uuid, stands for other keys as well (in core, I can see just uuid and theme). So I removed uuid condition. Entity updater should care to re-save unloadable entities, and it saves those for which updater callback returns TRUE, so we need to negate $loadable.

Extended test with checks that renaming config object through Entity API works correctly (including removing obsolete entries from key-value store) and added upgrade path test reproducing renaming config object both through Entity API and Config API.
The test failure in #23 still needs to be taken care of.

Erroneously uploaded previous patch before.

1. +++ b/core/lib/Drupal/Core/Config/ConfigRenameEvent.php
   @@ -21,10 +28,13 @@ class ConfigRenameEvent extends ConfigCrudEvent {
       *   The old configuration object name.
   +   * @param \Drupal\Core\Config\Config $old_config
   +   *   (optional) The configuration before it has been renamed.
       */
   -  public function __construct(Config $config, $old_name) {
   -    $this->config = $config;
   +  public function __construct(Config $config, $old_name, $old_config = NULL) {
   +    parent::__construct($config);
        $this->oldName = $old_name;
   +    $this->oldConfig = $old_config;
      }
    
   

   You can still add a type hint while keeping it optional. Not sure if not passing it should be deprecated so that we can eventually make it required (has to be in D10 now)

2. +++ b/core/lib/Drupal/Core/Config/ConfigRenameEvent.php
   @@ -37,4 +47,14 @@ public function getOldName() {
   +   * Gets the old configuration object.
   +   *
   +   * @return \Drupal\Core\Config\Config
   +   *   The configuration object the renaming of which caused the event to fire.
   +   */
   +  public function getOldConfig() {
   +    return $this->oldConfig;
   

   Because this doesn't document a NULL return value, so it is kind of expected to be there, and we can't provide BC for it.

   Maybe we should just require it? The classname is hardcoded, this isn't a service, there is no reason for it to be subclassed.

3. +++ b/core/modules/system/system.post_update.php
   @@ -140,6 +141,71 @@ function system_post_update_change_delete_action_plugins() {
   + * Re-saves config entities that cannot be loaded by uuid.
   + *
   + * By re-saving the entities that cannot be loaded by uuid the config key store
   + * for fast lookups will be updated, which will make the entities loadable by
   + * uuid.
   + */
   +function system_post_update_resave_non_loadable_config_entities(&$sandbox = NULL) {
   

   not sure if the second paragraph is needed and afaik both drush and the UI don't display that properly.

4. +++ b/core/modules/system/system.post_update.php
   @@ -140,6 +141,71 @@ function system_post_update_change_delete_action_plugins() {
   +  $entity_types = $entity_type_manager->getDefinitions();
   +  foreach ($entity_types as $entity_type_id => $entity_type) {
   +    if ($entity_type instanceof ConfigEntityTypeInterface) {
   +      $config_entity_updater->update($sandbox, $entity_type_id, function ($entity) {
   +        /** @var \Drupal\Core\Entity\EntityRepositoryInterface $entity_repository */
   +        $entity_repository = \Drupal::service('entity.repository');
   +        $entity_type = $entity->getEntityType();
   

   I don't think this foreach loop works like you think it should.

   The trick with the config entity update and $sandbox is that is going to do updates in batch and is going to be called many times. But your loop is going to start from the start every time.

   We likely can't use that service here but need to duplicate that logic, it does too much on its own.

   I think it is likely much easier to implement if you instead loop over *all* config and don't use entity API here.

   That means we need to use $config_factory->listAll() and then probably use array_chunk to split them into chunks of e.g. 100, put that in sandbox (unfortunately makes it quite big as there can be thousands and then each call needs to process one chunk, remove it and return. And then #finished = 1.

   And for each config, you use similar logic as the rename event implementation and interact with the key value service directly.

Yeah, I'm not 100% sure, I see that there is a $sandbox_key and it's done inside of that, but the global #finished is then set based on the current key. So honestly, I don't know what is going to happen exactly? Probably the last call wins, so it would process $batch_size of each entity type, and then if the last one has less than $batch_size, it would be done. And if not, then would continue until the last one is done.

So, it *kinda* works I suppose as long has the last entity type has the highest entity count? which is obviously not something that you can rely on.

> In this case the #finished should be computed based on all sandbox keys

Which is easier said than done because update() has no knowledge about which keys even exist and it doesn't know in advance how many that there are.

Maybe we could add a key inside that's basically a config_entity_updater_was_here flag, then it can loop over all sandbox keys with that key, and then each run stores its own progress and calculates the total progress of all that did run so far. As it also doesn't know if it's the last. Plus a check that just skips if the current sandbox_key is already complete (except updating total). Still quite a bit of overhead and complexity.

Which is why I think that a custom batch loop that just loops over all config directly would be easier here.

Going to have a look at this.

So, here's my proposal for an update function that is much simpler and IMHO sufficient here as in at least as good as the other, faster and doesn't have any problematic assumptions:

* The entity type fail is I think because that entity type hasn't been installed yet, we can't rely on that being the case. My approach doesn't have this problem because the only thing we need from a config is its entity type id, that's always going to be available if there's config for it.
* I honestly didn't fully understand the logic around having one or multiple entries and that warning log message. One one side, we IMHO do not need to support any third party messing with the data, this is private. So UUID must always be exactly one key. However, at the same time, it is perfectly valid for other lookup keys to have many values, one example is blocks with the theme lookup key.

So my update function relies on the expectation that UUID must be exactly one config name, and if it doesn't match that, we delete that and resave it. That's enough to fix both conditions in the test.

What it would *not* clean up is exactly that example with block themes, so doing an entity query on a block theme would still return stale keys, but that's less of an issue as they are expected to be passed to loadMultiple() which would then just ignore non-existing keys. Might be a small overhead, but the cached config storage explicitly supports caching non-existing lookups as well, so that's very minimal.

If we'd decide to have to fix that too, then we should probably switch the the test from image styles to blocks.

There's two things that I'm not quite sure about in the update test:
* Many comments explain what "we" do. I think that's not really how we comment things, should be more neutral and just explains how things are. Too tired to try and rewrite that.
* We're using the entity API before running updates. It's "just" config entities, so less tricky, but still, that's not really supported We've had problems with that before, if we'd add some kind of new lookup/check relying on things that aren't there yet... We're only testing our own mock data, so I'm not sure how important is.

Updated database dump from 8.4 to 8.8.

Added test checks for correctness of value in lookup table and extensively reworked comments to make clear there are 2 separate use cases which are tested on separate entities and fixtures.

Further updated fixtures and comments for the sake of clarity and consistency.

Rerolled.

+++ b/core/lib/Drupal/Core/Config/ConfigRenameEvent.php
@@ -21,10 +28,13 @@ class ConfigRenameEvent extends ConfigCrudEvent {
+  public function __construct(Config $config, $old_name, Config $old_config) {

Do we need to allow this to be null for BC (And trigger a deprecation if it is not provided)?

It is possible that someone in contrib/custom code is firing their own config events right?

If so we'd also need a deprecation test

See #27 for why I think BC for this is pointless. The event must provide the old config or we can't fix the bug. Only mentions of this class are on the event subscribers: http://grep.xnddx.ru/search?text=ConfigRenameEvent

Fair enough - thanks

Needs reroll and looks suitable for a novice.

Rerolled patch for 9.2.x

+++ b/core/lib/Drupal/Core/Config/ConfigRenameEvent.php
@@ -37,4 +47,14 @@ public function getOldName() {
+  public function getOldConfig() {

The entity API uses the term "original" when referring to the object that holds the previous values. Is there any reason to go with "old" here?

I see we use "old name" when referring to the previous name, but I'm not sure being consistent with that is worth it in the long run..

On this case we're using $this_model
$old_config = $this->get($old_name);

And here we're using $thisModel

protected $oldConfig;

Both are correct but we can use one or another in the same file to be consistent

More information here:
https://www.drupal.org/docs/develop/standards/coding-standards#naming

Converted $old_config to $oldConfig as per coding standards.

> Both are correct but we can use one or another in the same file to be consistent

Actually, the not-mixing part refers to local variables only. properties must camelCase. local variables can be either but should be consistent across the whole file. And since there are existing variables that use snake_case, #58/#59 isn't correct.

#57: Hm. the existence of $old_name is exactly why old_config made sense to me, but I have no strong feelings about that. Interestingly, https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21... already does exist, that does make we wonder if we could rely on that instead, then we wouldn't even need the extra argument?

Rerolled #59 for 9.5

Patch #64 applied successfully on drupal version 9.5.x and working fine but this patch does not applied on drupal version 10.1.x please reroll the patch for drupal 10.1.x

Thank you

Rerolled for 10.1.x.
But i could not understand how to make changes in the file (core/modules/system/system.post_update.php).
Please review.

This needs the post update adding back from #64. You should be able to just copy the code over from the patch as a last resort given it's all new.

Add changes in system.post_update.php also.
thanks

+++ b/core/modules/system/system.post_update.php
@@ -93,3 +94,41 @@ function system_post_update_timestamp_formatter(array &$sandbox = NULL): void {
+  foreach ($config_factory->loadMultiple($config_names) as $name => $config) {
+    $entity_type_id = $config_manager->getEntityTypeIdByName($name);
+    if ($entity_type_id) {
+      $key_value_store = \Drupal::keyValue(QueryFactory::CONFIG_LOOKUP_PREFIX . $entity_type_id);

What do you think if we do the opposite using early-return?

Ex:

  foreach ($config_factory->loadMultiple($config_names) as $name => $config) {
    $entity_type_id = $config_manager->getEntityTypeIdByName($name);
    if (!$entity_type_id) {
      continue;
    }

It'll reduce one level of indentation, you know?

made changes as per #70

#71 needs work for phpcs errors

added updated patch fixed CCF #71

#71 seems to be removing the code of the post_update hook

#73 seems to be adding additional changes from #71

#74 seems to be the same as #73 and adding additional changes

So patch #69 should be starting point but #70 needs to be addressed.

Let me work on #75.

I have tried to fixed the issue mentioned in #75. Please review.

#77 does seem to address #70.

No longer applies

as patch #77 does not applies

error: while searching for:
    return $update;
  });
}

error: patch failed: core/modules/system/system.post_update.php:93
error: core/modules/system/system.post_update.php: patch does not apply

adding a reroll for #77, please review.

Reroll seems good

I don't see any exploration of the ::getOriginal method highlighted in #60, we've just been re-rolling the previous patch without seeing if that means we don't even need the new arguments.

Can we explore that first please? If it works the patch will be dramatically simplified.