Move $entity->original to a separate hook argument

Would this be getting rid of the entity-generic hooks (e.g. hook_entity_update())? That has a bigger impact than removing $entity->original to a lot of modules.

Ah ok, was just confused since there was no "after" version of it.

I can't stand the structure of $entity->original, so this seems like a good idea.

At some point I'd wanted something like hook_entity_presave($type, Entity $entity, array $changes); so you can just easily see what stuff had changed, but I'm not sure how feasible that is at all. That'd be a much bigger change though so let's go for the extra parameter for now.

Here is a patch to do this.

I also added the original parameter entity to the preSave/postSave methods, which might need it too.

Rerolled. Should have far less errors (node and user tests pass locally).

I might add something that this makes impossible is being able to do token replacement using $entity->original. Moving this to a separate parameter removes the ability to fetch the original. I guess tokens could fetch it with entity_load_unchanged().

Looks like it should pass this time, comment, entity, node, taxonomy, and user pass locally.

Rerolled after yesternight's commits, just in case they broke something.

Rerolled.

FTR, I regret our not using that ternary pattern, though: in cases like this (uncomplicated but long expression members), it is both clearer than a single line and than an if/else. But that's not an issue.

Rerolled accordingly.

Actually, this needs some more changes in the api files.

Turned out to be much more significant. Patch rerolled on top of #1618136: Remove $type from entity hooks.

Forgot to set status.

Rerolled now that #1618136: Remove $type from entity hooks has been committed.

  * Implements hook_user_presave().
  */
-function contact_user_presave($account) {
+function contact_user_presave(User $account, User $original) {
   $account->data['contact'] = isset($account->contact) ? $account->contact : variable_get('contact_default_status', 1);
 }

+ *   The original entity object.
  */
-function hook_entity_presave(Drupal\entity\EntityInterface $entity) {
+function hook_entity_presave(Drupal\entity\EntityInterface $entity, Drupal\entity\EntityInterface $original) {
   $entity->changed = REQUEST_TIME;
 }

(and other places in hook_entity_presave())
$original should be default to NULL.

  * @param Drupal\Core\File\File $file
  *   The file entity that is about to be created or updated.
+ * @param Drupal\Core\File\File $file
+ *   The original file entity.
  */
-function hook_file_presave(Drupal\Core\File\File $file) {
+function hook_file_presave(Drupal\Core\File\File $file, Drupal\Core\File\File $original) {
   // Change the file timestamp to an hour prior.
   $file->timestamp -= 3600;

(a few times)

Inconsistant argument name.

--- a/core/modules/user/user.api.php
+++ b/core/modules/user/user.api.php
@@ -1,12 +1,13 @@
 <?php
 
-use Drupal\entity\EntityInterface;
-
 /**
  * @file
  * Hooks provided by the User module.
  */
 
+use Drupal\entity\EntityInterface;
+use Drupal\user\User;
+

Unrelated change.

Otherwise looks good ;)

Thanks for the review. I think it is best to include the "use" change too since we are modifying this file anyway.

Hmm, looks like this was not the final patch.

Rerolled with all these changes. All tests I checked seem to pass locally.

#29: 0001-Issue-1480696-move-entity-original-to-a-separate-hoo.patch queued for re-testing: make sure that yesterday's commits didn't break it.

I checked for more occurrences and found a few, but then something else emerged: Node::save() overrides Entity::save(), and needs to be converted to that format too for the issue to be completely addressed for the _update() hooks.

Actually, this also required changes in taxonomy.module and node tests, which implements these hooks.

All tests I've checked locally pass (book, comment, node, path, taxonomy).

Removed, along with the stray reference to the Monolog logger patch subcommit which found its way in the previous version too.

I'm in favor of this API change. I want to give more people the time to comment on it so I'll wait a couple of days before committing it.

The prospective change notice is at http://drupal.org/node/1643406

After one week, it was bound to be broken.... will fix it tonight.

All failing tests pass with this version.

Note that this required starting to use EntityInterface in field hooks.

Typo in the API file, no code change. Rerolled a better interdiff.

Hmmm, now that the original exceptions have been addressed, more appear. Looks like a lot more type hinting is required. Will be rerolling tomorrow.

Rerolled on current HEAD.

Grrr 8.x moved in the meantime. Rerolled.

Of course there had to be a merge error..

Rerolled on today's HEAD. Removed previously appeared exceptions and fail on local tests. Will they pass on q.d.o. ?

Rerolled on today's HEAD. Removed previously appeared exceptions and fail on local tests. Will they pass on q.d.o. ?

#54: 0001-Issue-1480696-Move-entity-original-to-separate-53.patch queued for re-testing: some recent change supposedly broke it w.r.t field updates.

Note that this patch and #1723892: Support for revisions for entity save and delete operations are currently on a conflicting path. The patch over there requires $entity->original in hook_field_update().

We'll need to figure out a solution, see the other issue for details.

I really don't like the DX of moving this to a separate argument. :/ I'd much rather remove $original all together, and if you need the original entity, you can fetch it yourself.

@DaveReid: The problem is that that is impossible. By the time hook_insert()/update are called we've already updated the database and it is impossible to load the original entity anymore. So you'd have to do it in hook_entity_presave() and then modules might end up doing this multiple times which is the reason this was added in the beginning.

#54: 0001-Issue-1480696-Move-entity-original-to-separate-53.patch queued for re-testing.

I don't think that's possible, seems like a large API change, we would now also have to update all pre/post methods. And there are unsolved problems when trying to access it in some function calls later on, as pointed out by dave reid.

However, what we still could do is provide a method for it, that would at least make it a bit less weird? And define it as an explicit property, possibly public for now?

Because right now ->original isn't even defined anywhere, which means it's stored in $this->values, for the current language and weird stuff like that.

I'm much in favor of this proposal, I was always annoyed by that magic property. A separate method/hook argument seems way cleaner. However, I guess this now won't happen anymore? Or is there still a chance? Really a bummer that it was already almost committed two years ago …

Anyways, if this API change won't happen anymore in D8 then, as Sascha says, we should at least properly define that property. With #2256679: Use config schema to determine which config entity properties to export regardless of whether they are public or protected landed, there shouldn't be any problem with config entities there anymore.

I'm pretty sure we can still do it as an explicit method, we can keep support for $this->original if we want to.

I'm pretty sure we can still do it as an explicit method, we can keep support for $this->original if we want to.

You mean adding getOriginalEntity()/setOriginalEntity() (or whatever) and making $original an explicit protected property? That's what I meant we should do in any case, yes, I do hope that's still possible.

What I fear won't be possible anymore (though I'd love to hear I'm mistaken) is adding $original as a separate argument to all update/presave hooks and methods, and completely getting rid of the property.

> I'm pretty sure we can still do it as an explicit method, we can keep support for $this->original if we want to.

That's now covered by #2839195: Add a method to access the original property , which I think would make this issue obsolete.