In #3045509: EntityFieldManager key/value field map gets out of sync, doesn't recognise bundle fields we stopped relying on key/value in the entity field map, but left some of the integration in place to support backporting to a patch release.
In this issue, we should be able to remove/deprecate key/value usage in the EntityFieldManager and FieldDefinitionListener classes.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
catchComment #3
catchComment #6
danielvezaI removed the remaining keyValue code from
FieldDefinitionListenerand deprecated the property inEntityFieldManagerandFieldDefinitionListener.Fixed the tests so they're now green, and I've added a change record. I reckon this is ready for review.
Comment #7
mstrelan commentedI think we might also want to use
\Drupal\Core\DependencyInjection\DeprecatedServicePropertyTraitand list the deprecated properties, in case anyone is extending this class and calling these directly. See\Drupal\Core\Routing\RouteBuilderfor example.Can we also expand the CR with FQCNs?
Comment #8
danielvezaI debated that too, but IMO DeprecatedServicePropertyTrait shouldn't be used until #3519400: Update DeprecatedServicePropertyTrait for Drupal 12 is sorted. Otherwise it's just confusing. You get the error when you construct it that says it will be removed in D12, but then you'll get another that says it's removed it D11, which you're probably alraedy on.
Happy to be overridden, but I think that provides more mess than value.
In the meantime I've addressed catched feedback and updated the CR.
Comment #9
catchhttps://git.drupalcode.org/project/registration/-/blob/b35a5fab75f96b56d... doesn't access the key value property, and nor does https://git.drupalcode.org/project/test_helpers/-/blob/cf37e25460d7d31b3...
Unless I missed something the only other occurrences were in core tests.
So I think we're OK without the deprecated property here.
Comment #10
catchNoticed another thing we can remove.
When profiling FieldResolverTest, about 700ms+ was spent loading and saving the field map when saving fields. This is trying to 'pre-cache' the field map, but it just doesn't work with the new approach. Rather than trying to make it work, which might not even be worth it, let's just drop the code.
Comment #11
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.
Comment #12
catchComment #13
smustgrave commentedCould we bump these to 12.1/13 please.
Also could we fill in the proposed solution section. please
Comment #14
longwaveLet's also add a post update hook to remove the unused key-value entry.
Comment #16
longwaveAdded an update hook, removed another unused service, added some tests and combined other tests that are now duplicates.
This round of changes was assisted by GPT 6.