Issue #3314741: EntityFieldManager::getFieldMap() doesn't show farmOS bundle fields #583

mstenta · 2022-10-11T16:40:02Z

See https://www.drupal.org/project/farm/issues/3314741

… bundle fields

mstenta · 2022-10-11T16:44:12Z

Note that this PR currently includes two commits that change the equipment and quick fields from bundle fields to base fields.

If we want to split those out to review separately that's fine with me.

symbioquine · 2022-10-12T15:44:07Z

modules/core/entity/farm_entity.module

+  foreach ($entity_type_definitions as $entity_type => $entity_type_definition) {
+
+    // Only proceed for entity types that use bundle plugins.
+    if (!in_array($entity_type, ['asset', 'log', 'plan', 'quantity'])) {


I'm seeing this array in a lot of places - sometimes in different orders or with/without the 'data_stream' entry.

Would it make sense to refactor it into a constant - or maybe just additional attributes on the entity definitions themselves?

e.g. here;

if (!$entity_type_definition->supportsBundlePlugins()) { continue; }

Oh good idea! Didn't know about the supportsBundlePlugins() method!

I think we could use EntityTypeInterface::getBundleEntityType()

/** * Gets the name of the entity type which provides bundles. * * @return string|null * The name of the entity type which provides bundles, or NULL if the entity * type does not have a bundle entity type. */ public function getBundleEntityType();

symbioquine · 2022-10-12T15:48:28Z

modules/core/entity/farm_entity.module

+    }
+
+    // Set the bundle field map key value collection.
+    if (!empty($bundle_field_map)) {


What if it became empty as part of the changes above? Shouldn't it still get set in that case?

symbioquine · 2022-10-12T15:55:17Z

modules/core/entity/farm_entity.post_update.php

+
+  // Iterate through entity types.
+  $entity_type_definitions = \Drupal::service('entity_type.manager')->getDefinitions();
+  foreach ($entity_type_definitions as $entity_type => $entity_type_definition) {
+
+    // Only proceed for entity types that use bundle plugins.
+    if (!in_array($entity_type, ['asset', 'log', 'plan', 'quantity'])) {
+      continue;
+    }
+
+    // Get the bundle field map key value collection.
+    $bundle_field_map = \Drupal::service('keyvalue')->get('entity.definitions.bundle_field_map')->get($entity_type) ?? [];
+
+    // Get a list of installed bundles for this entity type.
+    $bundles = \Drupal::service('entity_type.bundle.info')->getBundleInfo($entity_type);
+
+    // Iterate through bundles.
+    foreach ($bundles as $bundle => $bundle_info) {
+
+      // Invoke hook_farm_entity_bundle_field_info() on all modules with a
+      // callback function to add bundle fields to the entity field map.
+      \Drupal::service('module_handler')->invokeAllWith(
+        'farm_entity_bundle_field_info',
+        function (callable $hook) use ($entity_type_definition, $bundle, &$bundle_field_map) {
+
+          // Get bundle fields defined by the module.
+          $bundle_fields = $hook($entity_type_definition, $bundle) ?? [];
+
+          // If bundle fields are empty, bail.
+          if (empty($bundle_fields)) {
+            return;
+          }
+
+          // Iterate through the bundle field definitions to add fields to the
+          // bundle field map. This mimics the field_definition.listener
+          // service's onFieldDefinitionCreate() behavior.
+          // @see Drupal\Core\Field\FieldDefinitionListener::onFieldDefinitionCreate()
+          foreach ($bundle_fields as $field_name => $bundle_field) {
+            if (!isset($bundle_field_map[$field_name])) {
+              // This field did not exist yet, initialize it with the type and
+              // empty bundle list.
+              $bundle_field_map[$field_name] = [
+                'type' => $bundle_field->getType(),
+                'bundles' => [],
+              ];
+            }
+            $bundle_field_map[$field_name]['bundles'][$bundle] = $bundle;
+          }
+        }
+      );
+    }
+
+    // Set the bundle field map key value collection.
+    if (!empty($bundle_field_map)) {
+      \Drupal::service('keyvalue')->get('entity.definitions.bundle_field_map')->set($entity_type, $bundle_field_map);
+    }
+  }
+
+  // Delete the entity field map cache entry.
+  \Drupal::service('cache.discovery')->delete('entity_field_map');


Why implement this all again? Couldn't it just call _farm_entity_rebuild_bundle_field_map('install', $modules) with a list of all the modules that implement hook_farm_entity_bundle_field_info?

symbioquine · 2022-10-12T15:56:56Z

modules/core/entity/farm_entity.post_update.php

+/**
+ * Add hook_farm_entity_bundle_field_info() fields to bundle field maps.
+ */
+function farm_entity_post_update_add_farm_entity_bundle_field_maps(&$sandbox = NULL) {


What's this $sandbox parameter? Do we need to use/honor it?

symbioquine · 2022-10-12T15:58:55Z

modules/core/entity/farm_entity.post_update.php

+/**
+ * Add hook_farm_entity_bundle_field_info() fields to bundle field maps.
+ */
+function farm_entity_post_update_add_farm_entity_bundle_field_maps(&$sandbox = NULL) {


Probably just my Drupal naivete showing here, but does this implement a hook or is it called some other way?

symbioquine · 2022-10-12T16:01:37Z

modules/core/entity/tests/src/Functional/FarmEntityBundleFieldTest.php

+  /**
+   * Test that bundle field maps are updated on install/uninstall.
+   */
+  public function testBundleFieldMapUpdates() {
+
+    // Get the entity field map.
+    $field_map = $this->entityFieldManager->getFieldMap();
+
+    // Confirm that the 'test_default_bundle_field' exists in the log field map.
+    $this->assertArrayHasKey('test_default_bundle_field', $field_map['log']);
+
+    // Confirm that the 'test_contrib_hook_bundle_field' does NOT exist (yet).
+    $this->assertArrayNotHasKey('test_contrib_hook_bundle_field', $field_map['log']);
+
+    // Install the farm_entity_contrib_test module.
+    $result = $this->moduleInstaller->install(['farm_entity_contrib_test']);
+    $this->assertTrue($result);
+
+    // Reload the entity field map.
+    $this->entityFieldManager->setFieldMap([]);
+    \Drupal::service('cache.discovery')->delete('entity_field_map');
+    $field_map = $this->entityFieldManager->getFieldMap();
+
+    // Confirm that the 'test_contrib_hook_bundle_field' exists in the log field
+    // map, and exists in the 'test' bundle, but not in 'test_override'.
+    $this->assertArrayHasKey('test_contrib_hook_bundle_field', $field_map['log']);
+    $this->assertContains('test', $field_map['log']['test_contrib_hook_bundle_field']['bundles']);
+    $this->assertNotContains('test_override', $field_map['log']['test_contrib_hook_bundle_field']['bundles']);
+
+    // Uninstall the farm_entity_contrib_test module.
+    $result = $this->moduleInstaller->uninstall(['farm_entity_contrib_test']);
+    $this->assertTrue($result);
+
+    // Reload the entity field map.
+    $this->entityFieldManager->setFieldMap([]);
+    \Drupal::service('cache.discovery')->delete('entity_field_map');
+    $field_map = $this->entityFieldManager->getFieldMap();
+
+    // Confirm that the 'test_contrib_hook_bundle_field' no longer exists in the
+    // log field map.
+    $this->assertArrayNotHasKey('test_contrib_hook_bundle_field', $field_map['log']);
+  }


Should this also test the case where more than one module provides a given bundle field? i.e. that the bundle gets added, the field mapping doesn't get overwritten or prematurely deleted?

symbioquine · 2022-10-12T16:04:08Z

modules/core/quick/tests/src/Kernel/QuickFormTest.php

-    $this->assertEquals('test', $storage['assets'][0]->quick[0]);
+    $this->assertEquals('test', $storage['assets'][0]->get('quick')[0]->value);


Why does this need to change?

paul121

Just to capture what we chatted about on the monthly call..

This reminded me of this issue which provides a solution for a problem I have run into in update hooks adding/removing a bundle field in a custom module: https://www.drupal.org/project/drupal/issues/3129179

So considering that, it would be nice if there was a helper function we could call at any time to rebuild this field map during update hooks. To simplify things this helper function could rebuild the entire mapping, regardless of modules installed/uninstalled.

This issue I referenced may provide a solution for Drupal core, but we may need to determine if it works for the farmOS bundle field hook. It sounds like providing our own helper method for farmOS core would be the easiest near term solution (and call this helper function to do this logic as is being done by this PR)

mstenta · 2022-12-08T17:15:29Z

@paul121 raised an important question on the dev call today: if we convert the equipment field to a base field, will that stop it from showing up in Log Views?

mstenta added 3 commits October 10, 2022 16:14

Change equipment from a bundle field to a base field.

7f86409

Change quick from a bundle field to a base field.

fe0d719

Issue #3314741: EntityFieldManager::getFieldMap() doesn't show farmOS…

099149a

… bundle fields

symbioquine reviewed Oct 12, 2022

View reviewed changes

paul121 reviewed Oct 12, 2022

View reviewed changes

paul121 mentioned this pull request Nov 16, 2022

Add action to bulk categorize logs #590

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3314741: EntityFieldManager::getFieldMap() doesn't show farmOS bundle fields #583

Issue #3314741: EntityFieldManager::getFieldMap() doesn't show farmOS bundle fields #583

mstenta commented Oct 11, 2022

mstenta commented Oct 11, 2022

symbioquine Oct 12, 2022

mstenta Oct 12, 2022

symbioquine Oct 12, 2022

paul121 Oct 13, 2022

symbioquine Oct 12, 2022

symbioquine Oct 12, 2022

symbioquine Oct 12, 2022

symbioquine Oct 12, 2022

symbioquine Oct 12, 2022

symbioquine Oct 12, 2022

paul121 left a comment •

edited

mstenta commented Dec 8, 2022

		$this->assertEquals('test', $storage['assets'][0]->quick[0]);
		$this->assertEquals('test', $storage['assets'][0]->get('quick')[0]->value);

Issue #3314741: EntityFieldManager::getFieldMap() doesn't show farmOS bundle fields #583

Are you sure you want to change the base?

Issue #3314741: EntityFieldManager::getFieldMap() doesn't show farmOS bundle fields #583

Conversation

mstenta commented Oct 11, 2022

mstenta commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul121 left a comment • edited

Choose a reason for hiding this comment

mstenta commented Dec 8, 2022

paul121 left a comment •

edited