Provide some way to rebuild the persistent bundle field map

Hello,

@Sam152 thanks for the code snippet, it saved my day!

This has saved me multiple times as well.

I think this should be a link in the devel module or maybe admin_toolbar_tools.

I've submitted a patch using this snippet to admin_toolbar #3190279: Provide a link to rebuild the persistent bundle field map

Thanks @loze for the input and the patch done on admin_toolbar!

#2 saved my skin too, thanks @Sam152!

I've submitted a patch using this snippet to admin_toolbar

While I think it's great to be able to conveniently trigger it, I wonder if the actual functionality shouldn't live in core?

Thanks!

@AndyF - I agree it should live in core, but it will take literally years for that to happen. ;)
So I summitted it to admin_toolbar in hopes that getting it in there would be quicker.

The code in #2 will also add base fields to the map, so should probably filter these out. Not sure if $field_definition->getFieldStorageDefinition()->isBaseField() is sufficient for this.

Can confirm that code needs an improvement from #12 as jacktonkin wrote.

Also the bundle list is not a simple array, it should contain bundle also as a key:
$map[$entity_type_id][$field_name]['bundles'][$bundle] = $bundle;

So the looping part should be:

      foreach ($this->entityTypeBundleInfo->getBundleInfo($entity_type_id) as $bundle => $bundle_info) {
        foreach ($this->entityFieldManager->getFieldDefinitions($entity_type_id, $bundle) as $field_name => $field_definition) {
          if (!$field_definition->getFieldStorageDefinition()->isBaseField()) {
            $map[$entity_type_id][$field_name]['type'] = $field_definition->getType();
            $map[$entity_type_id][$field_name]['bundles'][$bundle] = $bundle;
          }
        }
      }

Last problem is that if you already used original code from #2 you have stored map items for entities which shouldn't be there - the entities which don't have any configurable field. You'll have map for file, shortcut, path_alias, redirect, and others. I highly suggest to one time delete the former map completely before you set a new one:

    // Delete former incorrect map.
    $persistent_map->deleteAll();
    // Add a recent map.
    $persistent_map->setMultiple($map);

Thanks, the comments were all very helpful. I thought I'd take a stab at moving this a step further. EntityFieldManager seemed an an appropriate place, but happy to move it if not. I've also referenced it from the config-import notice that alerted me to the issue in my case.

https://git.drupalcode.org/project/drupal/-/merge_requests/1434

I wasn't sure how to go about testing this. Perhaps setting something out of sync programmatically, triggering a rebuild, and testing that the map is now back in sync?

I think that kind of snippet could be part of the Devel module but not the Admin Toolbar module.

I think a big part of the benefit of having this in core is that the error message can give a more helpful hint as to what might be going wrong and how to solve it (as the merge request code does). The hardest part of hitting this error is actually tracking down what has gone wrong as the issue summary notes.

I suppose an alternative is to just have this cover changes to the error message, pointing in the direction of a Devel option; ie a compromise of the two.

The MR looks functionally good to me and seems to work, thanks! @romainj can I just confirm that it's Needs work for tests only?

Does it make sense (in a follow-up perhaps) to add a hook_requirements() check that your current field map matches the expected one?

I've just attached a patch using the MR diff from 6b08a1819216019fe00abb6dbd172895bc6923b7 for Composer.

Thanks!

Last problem is that if you already used original code from #2 you have stored map items for entities which shouldn't be there [snip] I highly suggest to one time delete the former map completely before you set a new one

Just wondering, is there any reason we shouldn't clear out the map completely on every rebuild?

Coming from #3125827: Prevent leftover fields from causing errors like "Call to a member function getLabel() after enabling layout_builder" - Tested with a D9.43 site that broke during field migration from D7. It seemed to find a broken leftover from Scald installation. D7 no longer have Scald installed however it seem to have left orphan field behind.

Applied path and tried to trigger a rebuild but the problem still persist. Let me know if I used it right?

$entity_field_manager = \Drupal::service('entity_field.manager');
$entity_field_manager->rebuildBundleFieldMap();

Here is a full run down of how to break a D9 site during migration:

../vendor/bin/drush migrate:import upgrade_d7_field
 33/41 [======================>-----]  80% [error]
The "scald_atom" entity type does not exist. (/Users/steinmb/sites/d9_cell/cell/web/core/lib/Drupal/Core/Entity/EntityTypeManager.php:139)
Processed 41 items (38 created, 0 updated, 1 failed, 2 ignored) - done with 'upgrade_d7_field'
In MigrateToolsCommands.php line 983: upgrade_d7_field Migration - 1 failed.


../vendor/bin/drush migrate:import upgrade_d7_field_instance
 26/51 [==============>-------------]  50% [error]
Missing bundle entity, entity type paragraphs_type, entity id y_creator. (web/core/lib/Drupal/Core/Entity/EntityType.php:877)
 [error]  Missing bundle entity, entity type paragraphs_type, entity id y_creator. (web/core/lib/Drupal/Core/Entity/EntityType.php:877)
 [error]  Missing bundle entity, entity type paragraphs_type, entity id y_creator. (web/core/lib/Drupal/Core/Entity/EntityType.php:877)
 [error]  Missing bundle entity, entity type paragraphs_type, entity id her. (web/core/lib/Drupal/Core/Entity/EntityType.php:877)
 [error]  Missing bundle entity, entity type paragraphs_type, entity id her. (web/core/lib/Drupal/Core/Entity/EntityType.php:877)
 31/51 [=================>----------]  60% [error]  The "scald_atom" entity type does not exist. (web/core/lib/Drupal/Core/Entity/EntityTypeManager.php:139)
 [error]  Attempted to create an instance of field with name scald_thumbnail on entity type scald_atom when the field storage does not exist. (web/core/modules/field/src/Entity/FieldConfig.php:315)
 [notice] Processed 51 items (40 created, 0 updated, 7 failed, 4 ignored) - done with 'upgrade_d7_field_instance'

In MigrateToolsCommands.php line 983:
upgrade_d7_field_instance Migration - 7 failed.

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getLabel() on null in Drupal\layout_builder\Plugin\Derivative\FieldBlockDeriver->getDerivativeDefinitions() (line 113 of core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php).
Drupal\layout_builder\Plugin\Derivative\FieldBlockDeriver->getDerivativeDefinitions(Array) (Line: 101)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives(Array) (Line: 87)
Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions() (Line: 285)
Drupal\Core\Plugin\DefaultPluginManager->findDefinitions() (Line: 175)
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions() (Line: 22)
…

Also tried to rollback the field migration and then rebuild with rebuildBundleFieldMap() but the problem still persist.

../vendor/bin/drush migrate:rollback upgrade_d7_field_instance
../vendor/bin/drush migrate:rollback upgrade_d7_field

Error: Call to a member function getLabel() on null in Drupal\layout_builder\Plugin\Derivative\FieldBlockDeriver->getDerivativeDefinitions() (line 113 of core/modules/layout_builder/src/Plugin/Derivative/FieldBlockDeriver.php).
...

Let me know if I used it right?

$entity_field_manager = \Drupal::service('entity_field.manager');
$entity_field_manager->rebuildBundleFieldMap();

As @hideaway mentions in #14, while the above will rebuild the bundle field map for those entity types that should have an entry, it won't clear entries for entity types that shouldn't be in there in the first place. So to be really confident I think you'll want:

\Drupal::keyValue('entity.definitions.bundle_field_map')->deleteAll();
$entity_field_manager = \Drupal::service('entity_field.manager');
$entity_field_manager->rebuildBundleFieldMap();

And I'm thinking that should probably be built into the rebuild command personally... does anyone see an issue with that?

I've just rebased against 9.5.x. I don't think I have the rights to change the MR target branch tho... @scott_euser could you possibly change that?

Thanks!

I've added a simple test: @scott_euser if you're able to change the target branch, could you also switch the issue to Needs Review> to trigger testbot?

Thanks!

Hi AndyF,
Thank you for your work on this, target branch updated.
Thanks!
Scott

Cheers @scott_euser, looks good!

I've just made a minor update to avoid a deprecation in the test, fingers crossed.

If that does the job, I think I'm gonna be bold and update the method to completely clear out the bundle field map (rather than just overwriting existing data), and slightly modify the test to verify that.

Thanks!

OK, so I've updated it now to clear out the bundle field map before rebuilding it. (So #28 is no longer accurate.)

Regarding the test, there were a couple of things I was uncertain about:

1. Should I assert against an explicit expected bundle field map, or just test that it's the same before and after a rebuild. Normally I like to be really explicit in tests, but in this case I thought that would introduce additional brittleness, so I just test it's the same before and after.
2. How complex a bundle field map should I be making for the test? I've kept it simple for the time being.

Thanks!

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Adding a file patch in case anyone want to link. Identical to #38

If someone wanted to use the code in the patch:

function MYMODULE_update_10000(&$sandbox) {
  \Drupal::service('entity_field.manager')->rebuildBundleFieldMap();
}

Seems to be an open question in #34 could that be answered?

> Should I assert against an explicit expected bundle field map, or just test that it's the same before and after a rebuild. Normally I like to be really explicit in tests, but in this case I thought that would introduce additional brittleness, so I just test it's the same before and after.

I think testing it's the same before and after is potentially good, in that as you say it keeps the test simple.

And I see that the test is deleting the map -- so the intention is that we're checking that the rebuild actually can rebuild from scratch. However, we're not checking that after the deletion the map is empty!

> How complex a bundle field map should I be making for the test? I've kept it simple for the time being.

What are the different cases we should consider?

Also,

> public function rebuildBundleFieldMap() {

The other methods that work with the field map call it just 'field map', so the use of 'bundle' in this method name is confusing.

Scratch that, the field map methods show base fields as well.

rebuildBundleFieldMap() should finish off by clearing the 'entity_field_map cache item, as otherwise it won't have any apparent effect.