Updated: Comment #11

Problem/Motivation

Currently, entity storage controllers have a load() method. However, it doesn't load an entity. It loads multiple entities as an array. You can't actually get a single entity out of a storage controller. That was all well and good if everyone was using entity_load() (which was 3-4 layers of procedural calls to get to the storage controller).

Proposed resolution

Rename ->load() to ->loadMultiple(), add a ->load() method that just sub-calls to ->loadMultiple() like entity_load() does, and update any current calls to ->load() as needed.

Remaining tasks

  • Replace and deprecate entity_load_multiple() and entity_load() with callings to entity storage controllers methods: ->loadMultiple() and ->load().

User interface changes

No UI changes.

API changes

  1. All entity storage controllers method ->load() will be replaced by the method ->loadMultiple(). No changes made to arguments.
  2. For retrieving a single entity, entity storage controllers will call their new ->load() method with the id as argument.
  3. All *_load() functions derived from entity_load() and also the menu loaders (*_load()) will return NULL instead of FALSE in case of failure to retrieve the object.

N/A

Original report by Crell

Currently, entity storage controllers have a load() method. However, it doesn't load an entity. It loads multiple entities as an array. You can't actually get a single entity out of a storage controller. That was all well and good if everyone was using entity_load() (which was 3-4 layers of procedural calls to get to the storage controller), but we want to stop doing that. That means we need to make load() actually do what you'd expect.

Fortunately that's easy. Rename load() to loadMultiple(), add a load() method that just sub-calls to loadMultiple() like entity_load() does, and update any current calls to load() as needed. Simple!

Someone please beat me to it. :-) I'll try to find time but make no promises.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea

Let's try...

claudiu.cristea’s picture

Title: split load() and loadMultiple() on entity storage controllers » Adopt load() and loadMultiple() on entity storage controllers
Status: Active » Needs review
FileSize
49.73 KB

Here's a first patch. Didn't manage to run many tests locally.

Status: Needs review » Needs work

The last submitted patch, drupal-split-load-2024833-2.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
49.96 KB

Fixed. Retrying...

Crell’s picture

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -141,6 +141,14 @@ public function load(array $ids = NULL) {
+    return isset($entities[$id]) ? $entities[$id] : FALSE;

This should return NULL, not FALSE. See http://www.garfieldtech.com/blog/empty-return-values for why. ;-)

+++ b/core/lib/Drupal/Core/Entity/EntityListController.php
@@ -87,7 +87,7 @@ public function getStorageController() {
   public function load() {
-    return $this->storage->load();
+    return $this->storage->loadMultiple();
   }

I'm not fully up on EntityListController, but this just looks suspicious.

claudiu.cristea’s picture

Status: Needs review » Needs work

This should return NULL, not FALSE. See http://www.garfieldtech.com/blog/empty-return-values for why. ;-)

I agree. That was how I would implement but I "copied" from the old entity_load():

function entity_load($entity_type, $id, $reset = FALSE) {
  $entities = entity_load_multiple($entity_type, array($id), $reset);
  return isset($entities[$id]) ? $entities[$id] : FALSE;
}
I'm not fully up on EntityListController, but this just looks suspicious.

$this->storage is an instance of \Drupal\Core\Entity\EntityStorageControllerInterface. So it seems OK.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
49.92 KB

Here we go again...

Status: Needs review » Needs work

The last submitted patch, drupal-split-load-2024833-7.patch, failed testing.

claudiu.cristea’s picture

Hmm. Now, returning NULL instead of FALSE we've ran in the next issue: As entity_load() has been refactored to use the new ->load() method it returns NULL instead of FALSE, as it used till now. This breaks things on the site. I retested failed tests with the old entity_load() and they passed.

I can see 2 main options (with some variations):

  1. [Workaround] Keep entity_load() returning FALSE (not NULL) if no entity found. This is kind of dirty solution but it's effective.
  2. Check each entity_load() call (~282 calls) and refactor to accept NULL as returned value. This would be the right approach. I see here 2 sub-options:
    1. Keep entity_load()
    2. Drop the procedural entity_load() in favour of ->load() but, here, I don't know what are the core team plans.
claudiu.cristea’s picture

Issue tags: +API clean-up
Crell’s picture

I don't think entity_load() is slated to be removed, but it should at least be @deprecated. (Same for entity_load_multiple()).

Since there's only 6 fails and 4 exceptions, I'd say let's just switch to NULL and fix anywhere that breaks. NULL and FALSE both evaluate to boolean FALSE on a weak check, so most code apparently can't tell the difference.

claudiu.cristea’s picture

Issue summary: View changes

Issue summary update.

claudiu.cristea’s picture

I narrowed the error zone and it seems they are triggered by the menu system, in fact when evaluating the menu loaders (*_load() functions). The menu system expects there a === FALSE in case of object missing.

core/includes/menu.inc

function _menu_load_objects(&$item, &$map) {
        ...
        // If callback returned an error or there is no callback, trigger 404.
        if ($return === FALSE) {
          $item['access'] = FALSE;
          $map = FALSE;
          return FALSE;
        }
}

My proposal is to replace if ($return === FALSE) with if (empty($return)).

This will fix those failures. I'm testing locally this solution right now.

Crell’s picture

Of course it's the menu system... :-) +1 to #12.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
18.38 KB
69.23 KB

Voilà!

Status: Needs review » Needs work

The last submitted patch, drupal-split-load-2024833-14.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
608 bytes
69.64 KB

Drupal\filter\Tests\FilterAdminTest->testFormatAdmin() passes when testing manually with this latest patch. However running the automated test doesn't succeed locally. I cannot find any reason.

Let's see what Drupal testing says.

Status: Needs review » Needs work

The last submitted patch, drupal-split-load-2024833-16.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
798 bytes
70.23 KB

Oh, forgot the editor.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Looks OK to me. Thanks, Claudiu!

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -141,6 +141,14 @@ public function load(array $ids = NULL) {
+   * Implements Drupal\Core\Entity\EntityStorageControllerInterface::load().

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -178,6 +178,14 @@ public function load(array $ids = NULL) {
+   * Implements \Drupal\Core\Entity\EntityStorageControllerInterface::load().

@inheritdoc.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.phpundefined
@@ -38,7 +38,18 @@ public function resetCache(array $ids = NULL);
+   * @return

@return is missing the type: Drupal\Core\Entity\EntityInterface.

Not sure about the NULL/FALSE change here. It might be a correct thing to do but it means we're doing two things at the same time here and it makes the patch considerable bigger, including a lot of unrelated changes in completely unrelated menu load callbacks. I would suggest to keep the existing behavior here and just add that method.

Crell’s picture

Berdir: While I'd nominally agree with you, at this point the work has already been done and we're less than a week to freeze; I really don't want to ship another release with FALSE being used incorrectly. :-)

The docs changes should be easy enough to make.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
2.54 KB
70.34 KB

Thank you @Berdir for review.

Updating doxygen lines I found that in entity_load(), when resetting the cache, I forgot to pass the $id to ->resetCache(), in #18. This patch is fixing that. Even it's out of scope, I noticed that entity_load_multiple() does the same -- it invalidates the whole cache for that entity. This is not OK unless there's a special reason for doing that but I cannot get that point. I fixed entity_load_multiple() too, see interdiff.txt.

claudiu.cristea’s picture

Issue tags: +Needs change record

Needs change notification.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a reroll...

curl https://drupal.org/files/drupal-split-load-2024833-22.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 72028  100 72028    0     0  14984      0  0:00:04  0:00:04 --:--:-- 17465
error: patch failed: core/modules/editor/editor.module:290
error: core/modules/editor/editor.module: patch does not apply
claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
70.37 KB

Let's see.

claudiu.cristea’s picture

Issue tags: -Needs reroll

Don't know what are the best practices when re-rolling. Should I set to "needs review" or to "reviewed & tested by the community" assuming that was previously RTBC?

Status: Needs review » Needs work

The last submitted patch, drupal-split-load-2024833-26.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
73.62 KB

HEAD is advancing... :(

Status: Needs review » Needs work
Issue tags: -API clean-up, -Needs change record

The last submitted patch, drupal-split-load-2024833-29.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review

#29: drupal-split-load-2024833-29.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API clean-up, +Needs change record

The last submitted patch, drupal-split-load-2024833-29.patch, failed testing.

claudiu.cristea’s picture

The test is failing before beginning. I opened a ticket here: #2029473: Test fail before starting tests.

Can anybody help with this?

alexpott’s picture

So I agree with @berdir in #20 we should be doing less change here... lets return FALSE and change to NULL in a followup... it will mean less rerolls.

claudiu.cristea’s picture

Finally I have to agree. It's too big to be handled.

alexpott’s picture

So in answer to the test fails... this patch currently breaks login :)

claudiu.cristea’s picture

Ok but can you explain where? :)

claudiu.cristea’s picture

Ok. Found :)

 }
diff --git a/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php
index 93236f1..ab29ffa 100644
--- a/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php
+++ b/core/modules/user/lib/Drupal/user/Form/UserLoginForm.php
@@ -132,8 +132,7 @@ public function validateForm(array &$form, array &$form_state) {
    * {@inheritdoc}
    */
   public function submitForm(array &$form, array &$form_state) {
-    $accounts = $this->storageController->load(array($form_state['uid']));
-    $account = reset($accounts)->getBCEntity();
+    $account = $this->storageController->load($form_state['uid']);
     $form_state['redirect'] = 'user/' . $account->id();
 
     user_login_finalize($account);

Are you OK to fix that and try again with both: load split & NULL?

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
73.63 KB

Let's try :)

claudiu.cristea’s picture

Now that it's green it needs a RTBC -- it was RTBC before reroll.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Avoid commit conflicts

That's the downside of so much great work this week. Lots of patches colliding. :-)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs reroll

curl https://drupal.org/files/drupal-split-load-2024833-39.patch | git apply --check
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 75402  100 75402    0     0  46785      0  0:00:01  0:00:01 --:--:-- 54246
error: patch failed: core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.php:74
error: core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.php: patch does not apply
claudiu.cristea’s picture

Status: Needs work » Needs review

Here we go... :)

claudiu.cristea’s picture

Forgot the patch :)

claudiu.cristea’s picture

Needs a re-RTBC before it's not too late :)

jhodgdon’s picture

I was just scanning this patch to avoid committing a conflict, and I happened to notice that right at the top, there are namespaces used in documentation that do not start with \ -- they MUST start with \ in documentation, or else leave out the namespace entirely (if it is clear from the context of the file you can probably just leave it out as you would in code).

claudiu.cristea’s picture

@jhodgdon, Thank you for pointing out. I think that I inherited most them. Not an excuse, I should have been more careful.

I did some cleanup but only on docs related to this patch (and sometime a little bit around) because I wanted to keep the patch readable while it'a already big enough.

jhodgdon’s picture

Yes, most of them were inherited. I do not know why people are putting them into documentation all over the place. Very bad. Thanks for cleaning them up!

claudiu.cristea’s picture

The patch fails to apply after last commits. Here's a rerolled version.

claudiu.cristea’s picture

@Crell, desperately need a re-RTBC here. This HEAD race is gonna drive me crazy :)

alexpott’s picture

Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +DX (Developer Experience)

Marked as major so this change is not subject to API freeze as we are just moving functionality from the procedural wrapper to the object. And this change will make working with entities an better developer experience. If we do not want to break BC then we can always just add a loadSingle method (nice idea @berdir).

We need to remove all unrelated change include the change of the return value to NULL and the unnecessary fixes to coding standards.

+++ b/core/includes/entity.incundefined
@@ -192,17 +192,19 @@ function entity_get_view_modes($entity_type = NULL) {
- * @see Drupal\Core\Entity\EntityStorageControllerInterface
- * @see Drupal\Core\Entity\DatabaseStorageController
- * @see Drupal\Core\Entity\Query\QueryInterface
+ * @see \Drupal\Core\Entity\EntityStorageControllerInterface
+ * @see \Drupal\Core\Entity\DatabaseStorageController
+ * @see \Drupal\Core\Entity\Query\QueryInterface

For example, this is out-of-scope

Crell’s picture

Status: Needs work » Reviewed & tested by the community

Alex: I don't see why we need to back out the FALSE/NULL fix. It's done, we know we want to do it (lest we suffer another 3 years of a totally stupid and easily fixable API bug), and the only challenge here is really just commit conflicts. :-)

Fixing up docs along the way is what jhodgdon just asked be done, and it's something we've been doing. "Leave everything cleaner than you found it." I don't see any dead kittens here.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

The patch in #49 actually applies, including the changes from FALSE to NULL. I decided to commit it to 8.x. Thanks all!

tim.plunkett’s picture

Title: Adopt load() and loadMultiple() on entity storage controllers » HEAD BROKEN: Adopt load() and loadMultiple() on entity storage controllers
Category: task » bug
Priority: Major » Critical
Status: Fixed » Needs review
FileSize
1.67 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

These seems to be the only places

core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php
458:    $instances = $this->entityManager->getStorageController('field_instance')->load(array($instance_id));

core/modules/system/lib/Drupal/system/Tests/Path/AliasTest.php
56:      $loadedAlias = $path->load(array('pid' => $pid));

core/modules/path/path.module
236:    $path = ($term->id() ? Drupal::service('path.crud')->load(array('source' => 'taxonomy/term/' . $term->id())) : array());

core/modules/path/lib/Drupal/path/Form/DeleteForm.php
79:    $this->pathAlias = $this->path->load(array('pid' => $pid));
Dries’s picture

I just committed Tim's patch in #54, but based on #55, it looks like we may be missing two more.

neclimdul’s picture

I think he meant we found all the locations since he RTBC'ed it. We'll see when https://qa.drupal.org/8.x-status comes back.

tim.plunkett’s picture

Title: HEAD BROKEN: Adopt load() and loadMultiple() on entity storage controllers » Needs change notice: Adopt load() and loadMultiple() on entity storage controllers
Category: bug » task
Status: Reviewed & tested by the community » Active

Those other instances are on \Drupal\Core\Path\Path, so we don't need to worry. Thanks @Dries!

claudiu.cristea’s picture

Status: Active » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.phpundefined
@@ -455,7 +455,7 @@ function testEntityTranslationAPI() {
-    $instances = $this->entityManager->getStorageController('field_instance')->load(array($instance_id));
+    $instances = $this->entityManager->getStorageController('field_instance')->loadMultiple(array($instance_id));
     $instance = reset($instances);

In fact you should use the new "load single" approach :)

EDITED!

$instance = $this->entityManager->getStorageController('field_instance')->load($instance_id);

instead of reset() stuff.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityUnitTestBase.phpundefined
@@ -94,7 +94,7 @@ protected function reloadEntity(EntityInterface $entity) {
-    $entities = $controller->load($ids);
+    $entities = $controller->loadMultiple($ids);
     return reset($entities);

Same here. The last lines of function should go as

     ...
     $controller = $this->entityManager->getStorageController($entity->entityType());
     $controller->resetCache(array($entity->id()));
     return $controller->load($entity->id());
  }
tim.plunkett’s picture

Status: Needs work » Active

I was merely unbreaking HEAD, not getting fancy with it. I think that could be a follow-up issue, it's not urgent.

claudiu.cristea’s picture

Priority: Critical » Normal
Status: Active » Needs review
FileSize
2.39 KB

@tim.plunkett, thanks for fixing HEAD.

While I'm preparing a change notice for this issue, I leave a patch here for testing, reviewing and RTBC. Don't need a separate issue for this.

claudiu.cristea’s picture

Title: Needs change notice: Adopt load() and loadMultiple() on entity storage controllers » Adopt load() and loadMultiple() on entity storage controllers

Added 2 change notices as they seems different API changes.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

#61 looks correct to me.

claudiu.cristea’s picture

Untagging.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3b0e0c5 and pushed to 8.x. Thanks!

amateescu’s picture

I corrected https://drupal.org/node/2032185 a bit, because there was no entity_load_multiple() in D7 and the loading code in D8 is much simpler, you just call entity_load() and entity_load_multiple().

https://drupal.org/node/2032185/revisions/view/2748581/2764149

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

Anonymous’s picture

Issue summary: View changes

Typo.