Problem/Motivation

During each phase of entity CRUD, hooks are invoked.
For example, when saving a new entity, hook_entity_presave() and then hook_entity_insert() are invoked.
When saving an existing entity, hook_entity_presave() and then hook_entity_update() are invoked.

create, presave, predelete, delete, load: these are all clear.
postsave is divided into insert and update. This is not clear.

Proposed resolution

Add hook_entity_postsave(), deprecate hook_entity_insert() and hook_entity_update()

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

claudiu.cristea’s picture

I think hook_entity_insert() still make sense. There are specific tasks that are applying only to entity insertion.

fago’s picture

>I think hook_entity_insert() still make sense. There are specific tasks that are applying only to entity insertion.

Well for that postSave has the $update boolean. I'm +1 for adding postSave as it helps with consistency also, I'm not so sure about deprecating insert/update. Seems to be a too large change to do between to minor versions to me (for everyone who wants to be a good citizen and follow deprecations).

fago’s picture

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: +DrupalWTF

Just had someone else hit this today. It continues to be very confusing.

Adding postsave is nice, but that won't solve the problem. Deprecating (and eventually removing) the hooks that don't do what you think is the fix here.

Status: Needs review » Needs work

The last submitted patch, 2: 2798783-entity-2.patch, failed testing.

tim.plunkett’s picture

That is an extremely brittle test. I think I'll just deal with it here, but #2824165: Remove brittleness from ConfigEntityStorageTest would help if it happens first

Berdir’s picture

Should we include some conversions, to see how that looks?

The deprecation message isn't following the standard yet :)

Berdir’s picture

I guess the only argument against this is the long-term goal of removing hooks completely in favor of events? So adding new hooks is going a bit against that. Personally, I'm in no hurry to go there, though :)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
10.53 KB
15.46 KB

In order to keep the "events for hooks" issue clean, I was hoping to keep it 1:1. Therefore to have the right events, we'd want to do this first.

Did a couple conversions and fixed the tests.

Berdir’s picture

+++ b/core/modules/content_moderation/content_moderation.module
@@ -91,21 +91,16 @@ function content_moderation_entity_presave(EntityInterface $entity) {
+  if ($entity->isNew()) {
+    return $entity_operation->entityInsert($entity);
+  }
+  else {
+    return $entity_operation->entityUpdate($entity);

this doesn't work AFAIK, isNew() is already updated at this point and it is no longer new. That's why ::postSave() has an $update argument.

Status: Needs review » Needs work

The last submitted patch, 11: 2798783-presave-11.patch, failed testing.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dpi’s picture

This issue is a dupe of #2221347: Add hook_entity_postsave hook. Although this issue has progress...

tim.plunkett’s picture

Status: Needs work » Closed (duplicate)