Problem/Motivation
With Drupal 9.3.x came the very useful addition of Bundle Classes.
When I jumped into using this new feature, the Bundle Classes seemed like the perfect place to provide bundle specific field definitions via the bundleFieldDefinitions method, as all variations of bundle fields could live in their own class.
However, the buildBundleFieldDefinitions method on the EntityFieldManager responsible for building the list of bundle specific field definitions does not use Bundle Classes and will only ever try to call bundleFieldDefinitions on the base Entity class, meaning that you cannot define bundle specific fields in the Bundle Class.
Steps to reproduce
Create a Bundle Class as per the documentation and add a method of bundleFieldDefinitions to it. This method is never called and so any field definitions returned for it are never registered.
Proposed resolution
Modify the buildBundleFieldDefinitions method to check for and use a Bundle Class for locating bundleFieldDefinitions before falling back to the Entity Class as default.
Remaining tasks
Review
Commit
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3266287
Show commands
Start 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:
Comments
Comment #3
lind101 CreditAttribution: lind101 commentedComment #4
lind101 CreditAttribution: lind101 commentedThe MR in #2 should allow the grouping of Bundle Specific field logic within the Bundle Class, e.g:
Comment #5
larowlanThis feels like a feature request
Comment #6
joachim CreditAttribution: joachim as a volunteer commentedGood idea, but the problem with DRY needs figuring out.
Also, needs tests.
Comment #7
lind101 CreditAttribution: lind101 commentedSuggested changes made and tests added!
Comment #8
jeffamI just ran into this after reading this bit in https://www.factorial.io/en/blog/defining-bundle-fields-drupal
After applying this patch,
bundleFieldDefinitions()
in my custom subclass works!Comment #10
quietone CreditAttribution: quietone at PreviousNext commentedFound some coding standards for the comments in the test that should be fixed. The changes needed are suitable for a first time contribution, adding novice tag.
Comment #12
arunkumarkComment #14
alexpottI've reverted the commit from @arunkumark - https://git.drupalcode.org/project/drupal/-/merge_requests/1882/diffs?co...
The dependency is not used and it was adding a completely unrelated test.
Comment #15
joachim CreditAttribution: joachim as a volunteer commented> However, the buildBundleFieldDefinitions method on the EntityFieldManager responsible for building the list of bundle specific field definitions does not use Bundle Classes and will only ever try to call bundleFieldDefinitions on the base Entity class, meaning that you cannot define bundle specific fields in the Bundle Class.
This sounds like missing documentation -- I can't see where it's defined what responsibilities a bundle class will take on and which ones are left to the entity class. For example, currently buildBundleFieldDefinitions() is not called on the bundle class.
hook_entity_bundle_info() only says:
and EntityTypeBundleInfoInterface is REALLY vague:
The behaviour for buildBundleFieldDefinitions() this issue is changing needs documenting, so we need to figure out where that documentation should go. Probably in entity.api.php?
Comment #16
alexpott@joachim I would argue that on reading the CR for the original issue https://www.drupal.org/node/3191609 and reading the current documentation for the method...
That this issue is bug report and not a feature request.
The first line Provides field definitions for a specific bundle. points to the fact that this method is for bundle specific logic. And since the entity bundle class change we are supposed to be able to define all bundle specific logic in a bundle class.
I arrived at the issue because I assumed that this would be how it worked and I was really surprised it didn't work. I discussed with @larowlan and I’ve decided to reclassify this as a bug for the above reason.
I'm not 100% convinced that we need to document this change in the code. I think it is clear and expected that for anything bundle specific a bundle class should be used if it is available.
Comment #17
jonathanshaw+1.
Documentation can be improved in a follow-up if desired.
Comment #19
catchCommitted/pushed to 10.1.x, 10.0.x, 9.5.x, thanks!