Complete entity CRUD hook invocations

Important API addition!

I think it's a bug to have field level hooks invoked when there's no entity level hook - it effectively makes it impossible to make things fieldable from contrib, or for modules to react in those places. Since those would only be additions to the API, not changes in any sense at all, we might be able to squeeze this in (although I'm expecting to get thumped by webchick in SF).

I personally don't think we need to add tests just to check that a module_invoke_all() fires, but yeah system.api.php and taxonomy.api.php would need to be updated.

What about comment.module and presave?

This is really 'entity' system, but we don't have a proper component, which is part of the reason we have this issue at all.

@hefox: See the media project. We are doing just that (fieldable files).

Best,
J

So here's what we have:

hook_$entity_load()
hook_entity_load()

hook_$entity_update()
hook_entity_update()

hook_$entity_insert()
hook_entity_insert()

here's what we don't have consistently:

hook_$entity_delete()
hook_entity_delete()

hook_$entity_presave()
hook_entity_presave()

I think we can argue it's a bug not to have these hooks invoked in all the equivalent CRUD places, certainly it makes it impossible to add entity cache support for all entities without individually implementing the _delete() hook for each core entity. I think it's also causing problems for mongodb field storage too. Anyone want to roll a patch?

Yeah, these hooks would be great. If it's possible without an API change in D7 it would be amazing to add hook_entity_view() to the list. I think it could easily be added next to field_attach_view for all fieldable entities. Then we'd have a complete set of entity hooks ready.

Subscribing.

Especially hook_entity_delete() would be very helpful for my current project (indexing entities for searching -> can't (easily) remove deleted entities from index unless there is a hook_entity_delete()).

Is it OK for all these to be in the same issue or should this be split?

I created a patch which adds the entity_delete hook invocations.

#10 looks good to me and seems to work. Attached is a patch adding documentation to system.api.php.
(There are only problems with e.g. the entity_metadata module in contrib, which now unintentionally "implements" the hook.)

* Combine both improvements into one.
* As this issue is about making entity hooks complete, also add the presave hook.

> module_invoke_all('taxonomy', 'delete', 'vocabulary', $vocabulary);
What's that !? You just stumbled over another core bug. Just fix that too.

* You have forgotten about files.

Here is a revised patch, invocations plus documentation, including for files and also fixing the mentioned taxonomy bug.
I only didn't follow the suggestion to include hook_entity_presave as well, as a) I couldn't find any hook_user_presave() invocations (only implementations), b) there is no file_presave() and c) I found it better to fix one half of the issue right away (and also arguably the more important one) instead of waiting for a complete fix.

>I couldn't find any hook_user_presave() invocations (only implementations)

There is hook_user_presave(), however it's working differently as it uses $edit for the stuff to be saved. As a consequence we cannot do a useful hook_entity_presave() in D7. :(

Thus the patch in #13 makes entity hooks already as complete as possible in D7. Note that the added hook_entity_delete() is needed to solve real world use cases; e.g. in #9. Beside that the patch fixes hook_taxonomy_vocabulary_delete() to work at all. Patch looks good and comes with API docs -> good to go!

Re-roll.

Don't know, if the error really was with the patch (test result looks weird), but still: re-roll.

#18: entity_crud_delete-18.patch queued for re-testing.

OK, test bot seems to act normal again. Setting this back to RTBC.

Btw, could this be considered major? Generic entity handling is considerably hampered by this—but then, probably few modules use these features anyways.
For my project, it would be a real pita if this wouldn't get in, as then I'd have to manually check which entities still exist during cron runs, or employ similarly nonsensical workarounds.

I think this is a critical API brokage which is easy to fix, so I'd rather say that this issue is at least major if not critical.

I confirm #18 is a good re-roll.

For my project, it would be a real pita if this wouldn't get in, as then I'd have to manually check which entities still exist during cron runs, or employ similarly nonsensical workarounds.

Indeed this is a missing link. We have hook_entity_insert() + hook_entity_update(), so we also need to hook_entity_delete() for them to be useful.

OK, since there is some agreement, let's set this to "critical". It's a missing part of the API which makes some use cases at least a lot harder, it has a well-tested patch already and will hopefully finally get in this way.
If someone disagrees, they can just set it back to "major".

No this is still 'major', it'd be possible to add these hooks even after a release if it came to it.

#18: entity_crud_delete-18.patch queued for re-testing.

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

As this is critical for a number of use cases of the new entity system, I hardly think this can wait until D8.
Setting this back to D7.

This looks like a straight-forward "oversight in the API" bug-fix to me, but we need some example code in that new hook. I have less than 0% confidence level we will clean it up after this patch is committed, and we are just about at 100% compliance in D7 finally.

Also, this?

-  module_invoke_all('taxonomy', 'delete', 'vocabulary', $vocabulary);
+  module_invoke_all('taxonomy_vocabulary_delete', $vocabulary);

Sounds like we are in serious need of tests for our entity deletion stuff. This seems as good an issue as any to add them.

Thanks for still considering this! Really takes a load off my mind!
I think the additional tests would have justified a seperate issue, but to celebrate this going in I just oblidged. ;)
Hope all the tests pass now, locally the new tests worked. I'm not entirely sure about their style, but I think the way taken was the best one to check for the hook invocations without too much needless fuss.

Sorry, the latest patch still had a debug() call in it, nearly forgot that.
It wouldn't have hurt badly, though …

Wow, great job on the tests.

+function crud_hook_test_user_delete() {
+  $_SESSION['crud_hook_test'][] = (check_plain(__FUNCTION__) . ' called');
+}

The function name is not output, so check_plain() doesn't make much sense in that context.

Also I'd expect the test file to use the entity_* prefix we already have for the other entity related tests.

The function name is not output, so check_plain() doesn't make much sense in that context.

Ah, you're right of course! Just a remainder from when I tried it with drupal_set_message() …
A second point would be that function names can't contain HTML special characters anyways.

Also I'd expect the test file to use the entity_* prefix we already have for the other entity related tests.

I chose just crud_* because specific hooks were also tested, not only hook_entity_*(). But yeah, it is about entities (and it's in the name, too), so renamed it as requested.

New patch attached, should work just the same (if I didn't forget to change the name somewhere).

+  $_SESSION['entity_crud_hook_test'][] = (__FUNCTION__ . ' called for type ' . check_plain($type));

You missed a check_plain() there.

Beside that, I think it's RTBC.

OK, then I hope this on is complete, then.

Sry, one more minor thing I missed in the last review:

+class CrudHookTestCase extends DrupalWebTestCase {

The class name should reflect the Test case name and include file, thus probably be EntityCRUDHookTestCase.

Ah, you're right.
Good, that gives me the innocent opportunity to try my luck again with the test bot. ^^"

thanks, ready to go!

One minor thing: I would argue that in the sequence of the hook parameters $type should come first, to be consistent with for example entity_load() or entity_extract_ids(). But that would be an API change to the existing hook invocations as well, so this is definitively a D8 wish.

+++ modules/simpletest/tests/entity_crud_hook_test.test	10 Oct 2010 15:34:13 -0000
@@ -0,0 +1,299 @@
+  /**
+   * Overrides assertText() to automatically set the message to the asserted
+   * text.
+   */

no, assertText() is not overridden by this method? Comment does not make sense to me.

Otherwise RTBC from me.

Powered by Dreditor.

no, assertText() is not overridden by this method? Comment does not make sense to me.

God, how could I miss that one? That was from the earlier version with drupal_set_message(), where I just used it as the documented convenience override. Then I changed it to something completely else (and renamed it accordingly), but forgot to update the documentation.
Upside is: Now there is even a pretty Doxygen comment there. ;)

RTBC now :-)

Um. Holy cow!! You are officially hired to be the test writer for every patch! :D This is great!!!

The only thing that gives me pause is this concept:

+  $_SESSION['entity_crud_hook_test'][] = (__FUNCTION__ . ' called');

+  protected function assertHookMessage($text, $message = NULL, $group = 'Other') {
+    if (!isset($message)) {
+      $message = $text;
+    }
+    return $this->assertTrue(array_search($text, $_SESSION['entity_crud_hook_test']) !== FALSE, $message, $group);
+  }

Is there a reason you went this way rather than using drupal_set_message() and just plain assertText as you talked about before? That would certainly make these tests a little easier to read for people unfamiliar with them, if they were using standard tricks found in other tests.

Thanks, really bad that I won't have quite the time to do that. I just really want this patch to go in. ;)

As to your question: As said, I tried it, but found out that apparently it doesn't work to just do (e.g.) a node_save() and then some $this->drupalGet() – the message didn't get displayed. I would have had to write page callbacks that call those functions, thereby seperating the code and also bloating the test module further. I just thought that this way was not only a bit easier to program, but also a more concise and understandable way, as the assertHookMessage() call is also quite a self-explaining statement.
But if you would find the drupal_set_message() way better, it could probably be easily rewritten.

Or if you can think of another way, that is maybe used somewhere else for such purposes, I could also go with that. I just didn't find one and thought this was quite a good idea.

Ok, that's a pretty good justification to me. I don't see a huge reason to change the tests around, since someone can eventually figure out what's going on, just it seemed inconsistent with other tests.

Committed to HEAD. Yeehaw! Thanks again for diving in here and fleshing out not only the tests/docs for entity delete hooks, but also those for insert/update too. You rock! :D

Problem in the committed code :

+++ modules/taxonomy/taxonomy.module	12 Oct 2010 16:34:24 -0000
@@ -425,7 +425,8 @@ function taxonomy_vocabulary_delete($vid
+  module_invoke_all('taxonomy_vocabulary_delete', $vocabulary);
+  module_invoke_all('entity_delete', $vocabulary, 'taxonomy_vocabulary');

at this place in taxonomy_vocabulary_delete(), $vocabulary has been cast to an array (not sure why)
hook_taxonomy_vocabulary_delete() and hook_entity_delete() receive an array, while their doc claims they get an object.

See you over in #965348: hook_taxo_vocab_delete gets $vocab as an array