amcgowanca / discoverable_entity_bundle_classes

Enables entity type provider entity base class overrides on a per-bundle basis.
GNU General Public License v2.0
15 stars 7 forks source link

postLoad() hook not called correctly in SqlContentEntityStorageTrait #8

Open simonbaese opened 6 years ago

simonbaese commented 6 years ago

Current implementation

protected function postLoad(array &$entities) {
    $entity_class = $this->getEntityClass();
    $entity_class::postLoad($this, $entities);
    // Call hook_entity_load().
    foreach ($this->moduleHandler()
               ->getImplementations('entity_load') as $module) {
      $function = $module . '_entity_load';
      $function($entities, $this->entityTypeId);
    }
    // Call hook_TYPE_load().
    foreach ($this->moduleHandler()
               ->getImplementations($this->entityTypeId . '_load') as $module) {
      $function = $module . '_' . $this->entityTypeId . '_load';
      $function($entities);
    }
  }

does not determine entity_class of used entity bundle, but rather of the given parent entity, as there is no parameter given to getEntityClass().

I suggest to invoke the postLoad hook by bundle. Maybe like following.

protected function postLoad(array &$entities) {
    // We have to determine the bundle first.
    $bundle = FALSE;
    if (!empty($entities) && isset($this->bundleKey)) {
      $entity = reset($entities);
      $bundle = $entity->get($this->bundleKey)->getString();
    }
    $entity_class = $this->getEntityClass($bundle);
    $entity_class::postLoad($this, $entities);
    // Call hook_entity_load().
    foreach ($this->moduleHandler()
               ->getImplementations('entity_load') as $module) {
      $function = $module . '_entity_load';
      $function($entities, $this->entityTypeId);
    }
    // Call hook_TYPE_load().
    foreach ($this->moduleHandler()
               ->getImplementations($this->entityTypeId . '_load') as $module) {
      $function = $module . '_' . $this->entityTypeId . '_load';
      $function($entities);
    }
  }
amcgowanca commented 6 years ago

@simonbaese Thanks for submitting this! I haven't take a deep enough look, but the intent here is excellent. We should explore getting a PR open for this and merged as I do see the benefits :)

weitzman commented 4 years ago

@amcgowanca looks like you got 2 PRs for this. Still want to merge it?