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

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lind101 created an issue. See original summary.

lind101’s picture

Status: Active » Needs review
lind101’s picture

The MR in #2 should allow the grouping of Bundle Specific field logic within the Bundle Class, e.g:

namespace Drupal\mymodule\Entity;

use Drupal\Core\Entity\EntityTypeInterface;
use Drupal\entity\BundleFieldDefinition;
use Drupal\node\Entity\Node;

class Book extends Node implements BookInterface {

  // Get the bundle field value
  public function getAuthor(): string {
    return $this->get('author')->value;
  }

  /**
   * {@inheritDoc}
   */
  public static function bundleFieldDefinitions(EntityTypeInterface $entity_type, $bundle, array $base_field_definitions) {
    $definitions = parent::bundleFieldDefinitions($entity_type, $bundle, $base_field_definitions);

    $definitions['author'] = BundleFieldDefinition::create('string')
      ->setName('author')
      ->setLabel(t('Author'))
      ->setTargetEntityTypeId($entity_type->id())
      ->setTargetBundle($bundle)
      ->setRequired(TRUE);

    return $definitions;
  }
}
larowlan’s picture

Version: 9.3.x-dev » 9.4.x-dev
Component: field system » entity system
Category: Bug report » Feature request
Issue tags: +Bug Smash Initiative

This feels like a feature request

joachim’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Good idea, but the problem with DRY needs figuring out.

Also, needs tests.

lind101’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Suggested changes made and tests added!

jeffam’s picture

Status: Needs review » Reviewed & tested by the community

I just ran into this after reading this bit in https://www.factorial.io/en/blog/defining-bundle-fields-drupal

For an entity type you don’t control, you could switch the entity class to a custom subclass and implement bundleFieldDefinitions() in that...

After applying this patch, bundleFieldDefinitions() in my custom subclass works!

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

Found 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.

arunkumark made their first commit to this issue’s fork.

arunkumark’s picture

Status: Needs work » Needs review

alexpott made their first commit to this issue’s fork.

alexpott’s picture

I'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.

joachim’s picture

Status: Needs review » Needs work

> 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:

 *   - class: (optional) The fully qualified class name for this bundle. If
 *     omitted, the class from the entity type definition will be used. Multiple
 *     bundles must not use the same subclass. If a class is reused by multiple
 *     bundles, an \Drupal\Core\Entity\Exception\AmbiguousBundleClassException
 *     will be thrown.

and EntityTypeBundleInfoInterface is REALLY vague:

   *   An array of bundle information where the outer array is keyed by the
   *   bundle name, or the entity type name if the entity does not have bundles.
   *   The inner arrays are associative arrays of bundle information, such as
   *   the label for the bundle.

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?

alexpott’s picture

Category: Feature request » Bug report
Status: Needs work » Needs review
Related issues: +#2570593: Allow entities to be subclassed using "bundle classes"

@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...

   * Provides field definitions for a specific bundle.
   *
   * This function can return definitions both for bundle fields (fields that
   * are not defined in $base_field_definitions, and therefore might not exist
   * on some bundles) as well as bundle-specific overrides of base fields
   * (fields that are defined in $base_field_definitions, and therefore exist
   * for all bundles). However, bundle-specific base field overrides can also
   * be provided by 'base_field_override' configuration entities, and that is
   * the recommended approach except in cases where an entity type needs to
   * provide a bundle-specific base field override that is decoupled from
   * configuration. Note that for most entity types, the bundles themselves are
   * derived from configuration (e.g., 'node' bundles are managed via
   * 'node_type' configuration entities), so decoupling bundle-specific base
   * field overrides from configuration only makes sense for entity types that
   * also decouple their bundles from configuration. In cases where both this
   * function returns a bundle-specific override of a base field and a
   * 'base_field_override' configuration entity exists, the latter takes
   * precedence.

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.

jonathanshaw’s picture

Status: Needs review » Reviewed & tested by the community

I think it is clear and expected that for anything bundle specific a bundle class should be used if it is available.

+1.
Documentation can be improved in a follow-up if desired.

  • catch committed a5d4f72 on 10.0.x
    Issue #3266287 by lind101, alexpott, joachim, larowlan, quietone: Make...
  • catch committed f7d8fe6 on 10.1.x
    Issue #3266287 by lind101, alexpott, joachim, larowlan, quietone: Make...
  • catch committed ff6c369 on 9.5.x
    Issue #3266287 by lind101, alexpott, joachim, larowlan, quietone: Make...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.1.x, 10.0.x, 9.5.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.