Bundle fields not accounted if they are created through hook_entity_bundle_field_info()#356
Bundle fields not accounted if they are created through hook_entity_bundle_field_info()#356idimopoulos wants to merge 2 commits intojhedstrom:2.xfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for the PR. Before we land a fix, let me walk through how this driver currently decides which fields get routed through its handler pipeline, so the right shape of a fix falls out naturally. The two axes of a fieldEvery Drupal field carries two independent attributes that matter here: Origin (where the definition is declared):
Storage profile (how the value is persisted):
What
|
| Origin | Storage | In getFieldStorageDefinitions() |
In getBaseFieldDefinitions() |
In getFieldDefinitions($bundle) |
|---|---|---|---|---|
baseFieldDefinitions() |
standard | yes | yes | yes |
baseFieldDefinitions() |
computed | no (filtered at EntityFieldManager::getFieldStorageDefinitions() line 472) |
yes | yes |
baseFieldDefinitions() |
custom | yes | yes | yes |
FieldStorageConfig + FieldConfig |
standard | yes | no | yes |
bundleFieldDefinitions() alone |
computed | no | no | yes |
bundleFieldDefinitions() alone |
custom | no | no | yes |
bundleFieldDefinitions() alone |
standard (not supported by default content-entity schema) | no | no | yes |
hook_entity_field_storage_info() + bundleFieldDefinitions() |
standard | yes | no | yes |
The "bundle-only with standard storage" row exists in the entity-field tables but has nowhere for Drupal's SqlContentEntityStorage to read from or write to, because the default schema generator only considers entity-type-wide definitions. In practice, bundle-only fields declare either computed, custom storage, or come paired with hook_entity_field_storage_info().
How the driver consumes those lookups
getEntityFieldTypes() iterates getFieldStorageDefinitions() + (optionally) getBaseFieldDefinitions(). A field is kept for handler expansion only if either predicate returns TRUE:
isField()/fieldExists()- TRUE only if the field is aFieldStorageConfiginstance. Covers configurable fields.isBaseField()/fieldIsBase()- TRUE only if the field is ingetBaseFieldDefinitions(), AND the caller passed the field name in the$base_fieldsargument. Covers entity-type-wide base fields.
Applying that to the matrix above:
- Standard-storage entity-type-wide base fields: picked up. Handler expansion works.
- Computed entity-type-wide base fields: classified as base, but setting a value is meaningless (Drupal derives, not stores).
- Custom-storage entity-type-wide base fields: classified as base, handler expands the value, but the module's custom storage layer may or may not consume the expanded form.
- Configurable fields: picked up. Handler expansion works.
- Bundle-only fields, any storage profile: not picked up. Absent from both predicates' source lookups.
- Bundle-declared fields paired with
hook_entity_field_storage_info(): the storage lands ingetFieldStorageDefinitions(), so the loop iterates the field, but both predicates return FALSE (not aFieldStorageConfiginstance, not ingetBaseFieldDefinitions()). The field is iterated and discarded.
The real gap
The one case worth caring about is the last one: a non-computed, standard-storage field declared at the entity-type level through hook_entity_field_storage_info() and attached per-bundle through hook_entity_bundle_field_info(). It has persistable storage, scenario authors can reasonably want to set values for it, and the driver currently does not route it through handler expansion.
Closing that properly needs coordinated changes at both the predicate layer and the iteration-source layer, with test coverage for the round-trip. That is something I am happy to take on as a maintainer, so there is no need to rework this PR on your side.
What would help finalize the fix
Could you share a concrete example of what you are running into? Specifically:
- The actual field declarations: the
hook_entity_bundle_field_info()implementation (orbundleFieldDefinitions()method), and whether ahook_entity_field_storage_info()sibling exists for the same field. - Whether the field is computed and whether it uses custom storage.
- A step definition, scenario, or PHPUnit test that fails today without this change - ideally showing the value being set via a stub and what you expect to read back.
That gives us a reproducible starting point and confirms which of the paths above is actually in play for your setup.
|
Apologies for the delay @AlexSkrypnyk .. As we have (tens of) thousands of steps in behat in our project, these latest updates caused a crazy amount of failures and I wanted to first fix them all and verify our project before coming back to this as I was not sure if it was an issue.
Now, we have actually many more cases but I don't want to put them all here, it will get long and boring, but I can still add them if you want ofc. I would say that recognizing these computed fields mostly, would not really be a problem because the user that uses them, would bear the burden of handling them in a before/after node create hook. For reference, here are the two code blocks from the above modules that create the fields: # Meta entity
function meta_entity_entity_bundle_field_info(EntityTypeInterface $entity_type, ?string $bundle, array $base_field_definitions): array {
/** @var \Drupal\Core\Entity\EntityTypeBundleInfoInterface $bundle_info */
$bundle_info = \Drupal::service('entity_type.bundle.info');
$entity_type_id = $entity_type->id();
$bundle = $bundle ?? '';
$fields = [];
foreach (\Drupal::getContainer()->getParameter('meta_entity.repositories') as $meta_entity_type_id => $service_id) {
/** @var \Drupal\meta_entity\MetaEntityRepositoryInterface $repository */
$repository = \Drupal::service($service_id);
foreach ($repository->getReverseReferenceFieldNames($entity_type_id, $bundle) as $meta_entity_bundle => $field_name) {
/** @var \Drupal\Core\Field\BaseFieldDefinition $definition */
$bundle_label = $bundle_info->getBundleInfo($meta_entity_type_id)[$meta_entity_bundle]['label'];
$fields[$field_name] = BaseFieldDefinition::create('entity_reference')
->setName($field_name)
->setTargetEntityTypeId($entity_type_id)
->setTargetBundle($bundle)
->setLabel(t('@label reference', ['@label' => $bundle_label]))
->setDescription(t('@label attached metadata.'))
->setSetting('target_type', $meta_entity_type_id)
->setSetting('meta_entity_type_id', $meta_entity_bundle)
->setCardinality(1)
->setDisplayConfigurable('view', TRUE)
->setComputed(TRUE)
->setClass(MetaEntityReverseReferenceItemList::class);
}
}
return $fields;
}# RDF Sync
function rdf_sync_entity_bundle_field_info(EntityTypeInterface $entityType, ?string $bundle, array $baseFieldDefinitions): array {
$fields = [];
$mapper = \Drupal::getContainer()->get('rdf_sync.mapper');
if ($bundle && $mapper->isMappedBundle($entityType->id(), $bundle)) {
$fieldName = $mapper->getRdfUriFieldName($entityType->id(), $bundle);
$fields[$fieldName] = BundleFieldDefinition::create('uri')
->setName($fieldName)
->setTargetEntityTypeId($entityType->id())
->setTargetBundle($bundle)
->setLabel(t('URI'))
->setRequired(TRUE)
->setComputed(TRUE)
->setCustomStorage(TRUE)
->setDisplayConfigurable('form', TRUE)
->setDisplayConfigurable('view', TRUE)
->setRevisionable(FALSE)
->setClass(RdfSyncUriFieldItemList::class);
}
return $fields;
} |
|
@idimopoulos
Thank you for your patience. |
|
Thank you for your prompt response @AlexSkrypnyk . I will not work anymore on this then. In any case, the patch worked for us and I have downloaded it locally. For further updates, I will wait to see how 6.x will be :) Thanks. |
So the Drupal8 driver seems to have been expanded in 71a97a3 as far as I can track down, but it seems to be missing the bundle fields that are created through
hook_entity_bundle_field_infoand are computed.Disclaimer
I am not really good with Unit tests and I have to sum up my day to go to the DDD the upcoming days so the test was LLM assisted. Feel free to disregard.