EntityFieldManager key/value field map gets out of sync, doesn't recognise bundle fields

Confirming this.

> Fields added hook_entity_bundle_field_info don't get entered into the bundle_field_map on bundle creation

But bundle creation isn't the only problem. If you add a new bundle field in code, it doesn't get added to the map either.

Basically, EntityFieldManager::getFieldMap() doesn't show bundle fields that are defined in code, only those that are config fields.

However, the reason that a state key is used for bundle fields is explained in the comment:

        // In the second step, the per-bundle fields are added, based on the
        // persistent bundle field map stored in a key value collection. This
        // data is managed in the EntityManager::onFieldDefinitionCreate()
        // and EntityManager::onFieldDefinitionDelete() methods. Rebuilding this
        // information in the same way as base fields would not scale, as the
        // time to query would grow exponentially with more fields and bundles.
        // A cache would be deleted during cache clears, which is the only time
        // it is needed, so a key value collection is used.

So if I am reading that correctly, it's stored in the key-value system because querying for all the config field entities would not scale, because they are stored in the database.

But fields added with hook_entity_bundle_field_info() are defined in code, so they don't have this problem.
EntityFieldManager::getFieldMap() could just invoke hook_entity_bundle_field_info() and return the result.

BUT field module implements that hook to return config fields to the field system (https://api.drupal.org/api/drupal/core%21modules%21field%21field.module/...), so we can't invoke that hook as it would cause the problem that the use of the key-value system is working around. I guess we could get a list of each implementation from the module manager, and skip field module's implementation? That feels pretty ugly though.

I'm changing the issue title to the effective problem, as I am not sure what the best fix is here.

> EntityFieldManager::getFieldMap() could just invoke hook_entity_bundle_field_info() and return the result.

No it can't because that's exactly how field.module exposes its own bundle fields too.

And the hook needs to be called for all entity types and all bundles.. exactly that is the performance problem about the whole thing.

Hi there! I'm adding a related issue open for Layout Builder: https://www.drupal.org/project/drupal/issues/3034979

There is a working patch there, but the general agreement seems to be that it's not a viable approach on the long term. Making sure that getFieldMap() returns fields declared in hook_entity_bundle_field_info is a much cleaner way to solve the issue.

As of now, without this patch, computed fields and layout builder can't coexist. It even breaks sites if layout builder is enabled on view modes with active computed fields, as explained on this issue: https://www.drupal.org/project/computed_field_plugin/issues/3093367 (just to be clear, the issue is open on 'Computed Field Plugin' but it actually happens for all computed fields; it's not linked to the module at all).

Any ideas on which direction to go to fix this issue with getFieldMap()? I'd like to help but I really don't know where to start.

For the moment, the only solution seems to be implementing
\Drupal::service('field_definition.listener')->onFieldDefinitionCreate($field_definition); in module's hook_install() (of module that defines any field through hook_entity_bundle_field_info()
On the larger scale, there can hardly be any solution than to introduce another hook or in general to make fields from configuration and fields from code be discovered with different mechanisms.

We ran into this in farmOS, where @symbioquine discovered that bundle fields defined via hook_entity_bundle_field_info() are not writable via JSON:API.

This is probably worth mentioning in the issue description. I would be curious what other systems depend on the field map for proper functioning. This is the first we've run into.

I guess we could get a list of each implementation from the module manager, and skip field module's implementation? That feels pretty ugly though.

@joachim seems to outline all the considerations very well in comment #2. And although it feels ugly, this approach may be the "best" way to solve this.

To summarize/restate the problem:

EntityFieldModule::getFieldMap() currently does not load bundle fields defined by hook_entity_bundle_field_info() (reference). It ONLY loads bundle fields defined in config, and it does this in its own special way. It could conceivably get ALL bundle field definitions by invoking hook_entity_bundle_field_info(), because the field module implements that to define config fields, but it sounds like there is a concern about the scalability/performance of that, per the code comment that @joachim references:

the time to query would grow exponentially with more fields and bundles

For reference, here is where that performance logic and comment were added originally: #2482295: Rebuilding field map with many bundles/fields is very slow / commit: https://git.drupalcode.org/project/drupal/-/commit/0f881613ef53745ad4bab...

Ultimately we want all the bundle fields defined by hook_entity_bundle_field_info() to be included in the field map, including the ones that are defined by field module, but because of that performance concern, it sounds like the field module's definitions need to be handled separately. It feels ugly to have to make that exception for the field module, but perhaps that is the "least bad" option.

One question this brings to mind, though: do we need to worry about the same performance concern with other implementations of hook_entity_bundle_field_info()? Or is the concern mostly with config fields managed by the field module, I wonder?

I am exploring a workaround for this in the farmOS codebase... if I have any success I'll share it here to keep the conversation going...

Merged #3034979: Layout builder doesn't support bundle computed field into this.

Adding basis version & MR to 10.3.x & 11.x trying at least to keep functionallity for layout builder of https://www.drupal.org/project/drupal/issues/3034979

The work being done in #3129179: Provide some way to rebuild the persistent bundle field map may provided a way forward for us in farmOS.

That issue adds a new method for rebuilding the bundle field map, like so:

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

A quick workaround for this issue is to implement hook_modules_installed() and hook_modules_uninstalled() to call that method whenever modules are installed/uninstalled. That covers fields that are added/removed with a module. However, if the module adds a bundle field in a new version, it will also need to include an update hook that runs the method.

If it's useful for anyone, this is the pull request we are considering for fixing this in the farmOS ecosystem (until a proper core fix emerges): https://github.com/farmOS/farmOS/pull/854

Just ran into #3034979: Layout builder doesn't support bundle computed field

I think we should take this out of key/value.

We could have field module implement similar logic in it's bundle field info hook instead maybe.

I just did a workaround for this problem over at #3522951: ContentEntityDenormalizer uses the field map, and so is unaware of bundle fields.

uploaded patch from https://git.drupalcode.org/project/drupal/-/merge_requests/8374/diffs to use in composer

Ran into this via #3557435: Bundle fields provided by module are missing in the field map

I think the field map key value collection is rendered redundant by #3537863: Optimize field module's hook_entity_bundle_info() implementation which optimized field_entity_bundle_field_info() itself, not to mention that was getting called despite the field map on cold cache requests.

So I'm looking at whether we can remove it entirely.

Yep it's redundant - performance tests are more or less neutral, arguably a small improvement.

Following tests broke in a notable way:

1. A couple of unit tests that are testing the key/value behaviour specifically. Tending towards deleting those methods because they are very much testing the implementation and having to mock too much.

2. One unit test that creates a bundle on the entity_test_update entity type, which never actually gets created because entity_test doesn't manage the entity type - this is the key/value implementation hiding some bugs - I think the field gets created for a bundle that doesn't exist.

Not quite ready for review yet but getting closer.

Started to look at removing the key/value implementation itself, but there's a fair amount to do there.

I think the current MR is sufficient to fix the bug here, so moving to needs review.

We'll also need to remove the key/value implementation, and the supporting code + test coverage, but we could consider backporting the bug fix to 11.3.x, which would be more or less the current MR. That leaves either having two MRs here, one with the full removal/deprecation, or possibly doing that part in a follow-up to this issue.

Profiled with xhprof on the front page of Umami, cold cache:

HEAD:

Overall Summary	
Total Incl. Wall Time (microsec):	6,486,853 microsecs
Total Incl. MemUse (bytes):	44,286,080 bytes
Total Incl. PeakMemUse (bytes):	48,065,536 bytes
Number of Function Calls:	1,698,642

MR:

Overall Summary	
Total Incl. Wall Time (microsec):	6,492,833 microsecs
Total Incl. MemUse (bytes):	44,348,560 bytes
Total Incl. PeakMemUse (bytes):	46,360,624 bytes
Number of Function Calls:	1,697,197

At a high level, it's not making a lot of difference either way. Some of the memory usage changes are probably from switching branches etc., I don't think this MR has any chance of actually reducing memory usage, but it does show it's not hugely inflating it either.

I also checked EntityFieldManager::getFieldDefinitions() specifically, since that's the method that is most likely to be called more often. It's called 370 times with the MR vs. 350 times without.

This difference translates into an extra four calls to ::buildBundleFieldDefinitions - 16 in HEAD, 20 with the MR.

So the field map is preventing some processing, but the percentage is quite small compared to what has to happen anyway.

On completely cold caches, which is the only place that we access the key/value map because it's behind a cache item, we also have #3537962: EntityFieldManager::getFieldDefinitions() per-bundle caching can be expensive still to do, which would further reduce the number of cache operations from building this data. That might be enough to cancel any performance regression out entirely.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

#3537962: EntityFieldManager::getFieldDefinitions() per-bundle caching can be expensive now has an MR, so combined with the changes here:

11.x latest:

Overall Summary	
Total Incl. Wall Time (microsec):	2,593,851 microsecs
Total Incl. MemUse (bytes):	44,158,248 bytes
Total Incl. PeakMemUse (bytes):	46,170,232 bytes
Number of Function Calls:	1,650,670

This issue + #3537962: EntityFieldManager::getFieldDefinitions() per-bundle caching can be expensive rebased on 11.x:

Total Incl. Wall Time (microsec):	2,209,596 microsecs
Total Incl. MemUse (bytes):	44,374,416 bytes
Total Incl. PeakMemUse (bytes):	46,390,112 bytes
Number of Function Calls:	1,650,909

There is still the additional four calls to ::buildBundleFieldDefinitions() as expected, but the overall difference in function calls and memory usage is pretty imperceptible. Pretty confident that this is the reduction in database queries, cache sets and gets vs. the additional hook invocations. Given it also allows us to remove a lot of code and fix several bugs not really seeing a downside.

We can go even further as well - opened #3565128: Avoid calls to EntityFieldManager::getFieldMap() where unnecessary which theoretically could remove all the cold cache calls to the field map in core, which ought to remove those four additional buildBundleField() calls again.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rebased.

A couple questions on the MR. One has to do with deprecation and clean up, which is a remaining task in the IS. I think we can do the clean up here? There's also code in Drupal\Core\Field\FieldDefinitionListener::onFieldDefinitionCreate() that gets and sets the map from keyvalue, and maybe that (and the associated test code) can be removed as well. Presumably an entry in the keyvalue store is not API, so not sure if that needs to be deprecated. But we can add deprecation warnings to the class constructors for the key value argument removal.

One has to do with deprecation and clean up, which is a remaining task in the IS. I think we can do the clean up here?

I think it depends if we think the current MR is backportable to 11.3.x, if it is, I'd rather open a follow-up for the cleanup + deprecations because there's going to be a lot of it. Otherwise we'll need separate 11.x and 11.3.x MRs here anyway. I don't mind very much, but that's what made me stop (shortly after I started ripping things out).

I think it depends if we think the current MR is backportable to 11.3.x, if it is, I'd rather open a follow-up for the cleanup + deprecations because there's going to be a lot of it.

Makes sense. Though maybe getting/setting keyvalue in Drupal\Core\Field\FieldDefinitionListener::onFieldDefinitionCreate() and ::onFieldDefinitionDelete()can be removed here? It will save the select and insert queries.

Think the bot took some time off but this one has merge conflicts it appears.

I was about to suggest using the cache pre-warming mechanism for the field map, but looked at the code and found out that's already done.

Still NW for #42 and the merge conflicts, otherwise I've read this a few times in past weeks and I think it looks great.

Rebased and addressed #42.

Reverted the commit that addresses #42, it causes unit test failures, we'll then have to adjust those tests to expect the key value store to do nothing, when we'll be able to remove it in the follow-up which will require adjusting them again. I still think being able to backport this to a patch release is more important than the deprecations.

Will open that follow-up now so what needs to get done doesn't get lost.

Opened #3585986: [PP-1] Remove remaining key/value implementation from the entity field map and supporting code

This lgtm now.

@catch is that latest commit (ea6bd872) something pushed to the wrong branch?

@godotislate - it is, but also I removed it again and force-pushed so it's no longer there (except as a ghost on this issue).

Committed 502de2e and pushed to main. Thanks!

We'll need a backport MR for 11.x and 11.3.x :)

Made an 11.x backport MR. The only conflicts were in performance test assertions so moving directly back to RTBC.

Committed bef5231 and pushed to 11.x. Thanks!

Needs another MR for 11.3.x...

11.3 MR is up - just performance tests again.

Committed 69b779f and pushed to 11.3.x. Thanks!