entity_load_unchanged() should be part of the storage controller

Looks fine to me.

Just one thing: Can we make the method body in ConfigStorageController identical to DatabaseStorageController, please? The resetCache method exists, and is merely empty right now, but we want to change that in #1885830: Enable static caching for config entities

Thanks!

Yeah, I totally thought of a EntityStorageControllerBase a couple of times already, too. We might want to do that as part of #1893772: Move entity-type specific storage logic into entity classes.

+++ b/core/lib/Drupal/Core/Entity/EntityStorageControllerInterface.phpundefined
@@ -41,6 +41,17 @@ public function resetCache(array $ids = NULL);
+   * @param $id
+   *   The ID of the entity to load.
+   *
+   * @return
+   *   The unchanged entity, or FALSE if the entity cannot be loaded.
+   */
+  public function loadUnchanged($id);

Nitpick, but the @param and @return should specify the type.

Thanks.

Hmm while we're moving this could we rename it to loadUncached() or similar?

Hmm yeah I'm not sure either. This was originally added for hook_node_*() implementations that want to see the original entity before it was loaded rather than the one as it's being changed, but we now have $entity->original (which is messy in itself but for the moment that's what we have).

The only place this is used outside of storage controllers and tests is http://api.drupal.org/api/drupal/core%21modules%21translation_entity%21l...

But could that use $entity->original instead?

If it can, then we could probably refactor the tests a bit and make this a protected method.

Separate issue seems fine but could we create that now and add a @todo?

I think that loadUnchanged() is actually much more appropriate than loadUncached().

The focus of this facility is not on the cache, because loading the [database-]cached copy of the entity would be totally OK. loadUnchanged() actually works around the static cache, because the static cache is compromised with edited, in-process, submitted form values.

I'm not sure about #16, and likewise, also not sure about the added @todo.

I could foresee situations in which you want to populate $entity->original with a fake/dummy entity that isn't actually loaded from storage. But turning ->original into a method would enforce storage-loading on all possible scenarios.

In any case, this particular and focused change looks good to me. Let's handle any further/additional questions in separate issues instead.

Good clean-up! I found a nit-pick:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -143,6 +143,15 @@ public function load(array $ids = NULL) {
+  function loadUnchanged($id) {

"public function"

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.php
@@ -236,6 +236,15 @@ public function load(array $ids = NULL) {
+  function loadUnchanged($id) {

should be "public function"

I could foresee situations in which you want to populate $entity->original with a fake/dummy entity that isn't actually loaded from storage. But turning ->original into a method would enforce storage-loading on all possible scenarios.

Not necessarily, we could add a setter as well. If we had it as a method, we'd only need to load the entity when accessed, right now it's wasted effort when it's not needed.

Thanks plach, back to RTBC then.

Not necessarily, we could add a setter as well. If we had it as a method, we'd only need to load the entity when accessed, right now it's wasted effort when it's not needed.

Ideally, we'd have the entity itself tracking what has changed imo - so we could save the load operation at whole. Howsoever that discussion belongs into its own issue.

Yes that's an even better point. I thought we had an issue for this already, but it may be stuck in one of the very early entity API dicsussions on Drupal.org. At the risk of a duplicate issue opened #1926714: Track changes for entity property changes and make available to hooks.

Committed/pushed to 8.x, thanks!

Will need a change notice.

The change record looks good. I searched through d.o including examples module, where this change could be reflected but did not find anything. If no objections, I change the status to fixed.
Good job, plach!