Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Not sure if it is the proper way to invoke the entity CRUD hooks.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | flag-invoke_entity_crud_hooks-2119467-4.patch | 1.36 KB | foopang |
| #2 | flag-invoke_entity_crud_hooks-2119467-2.patch | 1.37 KB | foopang |
| flag-invoke_entity_crud_hooks.patch | 1.36 KB | foopang | |
Comments
Comment #1
joachim CreditAttribution: joachim commentedGood call.
Though I think these invocations belong inside the crud methods: _insert_flagging, _update_flagging _delete_flagging.
Comment #2
foopang CreditAttribution: foopang commentedHi joachim, I have updated the patch, please review.
Comment #3
joachim CreditAttribution: joachim commentedOne more thing -- sorry, should have said earlier :(
Could you change the comments to say 'Invoke hook_foo()' please? That way it's easier to find the places the hook is invoked because you can search for the hook name. Then I think we're good to go!
I think I'm going to have to file an issue after this one to review the whole of the flow round this part of the code, as in reviewing what your patch changes I have spotted that we call field_attach_presave() AFTER we've saved data! It's getting quite messy in there :/
Comment #4
foopang CreditAttribution: foopang commentedHi joachim, updated the patch please review again.
I was wondering if it is possible to integrate flagging entity with Entity API so that it can be more extendable. For example, I try to use Search API to index the flagging entity, but there aren't any properties available for indexing unless we implement hook_entity_property_info() for the flagging entity type. And inserting/updating/deleting the flagging entity won't reflect in the Search API index because entity hooks were not invoked.
Comment #5
joachim CreditAttribution: joachim commentedCommitted. Thanks!
(And changed this to a bug, which I think it counts as.)
git commit -m "Issue #2119467 by foopang: Fixed entity CRUD hooks not invoked." --author="beansboxchrispang "
> I was wondering if it is possible to integrate flagging entity with Entity API so that it can be more extendable. For example, I try to use Search API to index the flagging entity, but there aren't any properties available for indexing unless we implement hook_entity_property_info() for the flagging entity type.
It's already been proposed that we depend on Entity API: #1866624: Add requirement for Entity API. . I'm against that, because I don't feel it's necessary for us to add a dependency and I don't think it adds anything to what Flag does at the moment. However, I'd be fine with adding support for Entity API in ways that don't require the dependency. Either hook_entity_property_info or declaring our use of an entity property controller would be fine, as without Entity API present those would just be ignored.
Entity API is part of D8 anyway, so doing that moves us along our eventual upgrade path.