Currently, for entities defined in code, only the delete hook is called on reverts. I think that's incomplete as, logically, that's not what happens. We can either say we edit the entity to look like the default again (-> update) or that we discard the current custom entity and create the default one again (-> delete followed by insert).

Currently, I don't think it's possible to react on a revert after the properties have been changed to the defaults, right? That's also a functional shortcoming, in addition to the (possible) conceptual one.

(I currently have this problem in #1414078: Revert of exportables not working.)

CommentFileSizeAuthor
#2 entity_features_update-1643184-2.patch3.56 KBderhasi

Comments

derhasi’s picture

derhasi commented

I'm trying to build a patch for that. My first approach would be, to simply try to store the object provided by the default implementation with using the old one's entity_id. Another approach would be, to replace each property of the old one, with the one from the default object.

Do you know of any barriers regarding that issue or the mentioned approaches?

derhasi’s picture

derhasi commented
Status: Active » Needs review
StatusFileSize
new entity_features_update-1643184-2.patch3.56 KB

I had a look at it. The first approach (storing the default with the old id) seems to work fine. So I attached a patch with my changes.

Besides replacing the revert() method with the new approach, there is now a EntityLegacyFeaturesController that holds the old implementation.

jaxxed’s picture

jaxxed commented

I tested #2 patch on a related issue with search_api, and it solved the problem without creating any obvious new ones.

http://drupal.org/node/1414078

[edit: clarified which patch]

jantoine’s picture

jantoine commented

This patch also worked for me in relation to the same issue: #1414078: Revert of exportables not working.

traviscarden’s picture

traviscarden commented

I'm not competent to comment on the code in this case, but the patch in #2 fixes the problem in #1414078: Revert of exportables not working for me as well.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented 
+++ b/entity.features.inc
@@ -172,3 +186,54 @@ function entity_features_export_render($module, $data, $export = NULL, $entity_t
+      // Update the status to be in code.
+      $entity->{$keys['status']} |= ENTITY_IN_CODE;

Shouldn't we set this only to ENTITY_IN_CODE (instead of a combination of that and the previous flags)? When reverting, we end up with the default entity after all, not an overridden one.

Also, does this take care of when we call entity_delete() (or something similar) directly? That was actually my main concern with this issue: that the Entity API doesn't handle (or at least allow us to handle) this case satisfyingly. Reverts via the Features module could be left using entity_delete() if that would work properly for reverts.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

Currently, for entities defined in code, only the delete hook is called on reverts.

I doubt this is really the case, because if so you'd end up with no entity in the database also. _entity_defaults_rebuild will chime in and re-save the new default, i.e. trigger the insert hook. If that's not happening for you there is more general problem with applying your defaults I guess?

derhasi’s picture

derhasi commented
Status: Needs work » Needs review

@fago, simply lookg at EntityDefaultFeaturesController::revert() - it only utilises entity_delete(). Yes, and then the insert hook will we be called in the rebuild process, as a new(!) entity will get created. And that's exactly the problem. The new entity will get a new id, and therefore may loose configuration simply attached to the entity.

@drunken monkey, I did not set to only ENTITY_IN_CODE, as this code only knows bout the ENTITY_IN_CODE. Any other part of code might use another flag on that same field (in theory).

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Postponed (maintainer needs more info)

Yes, and then the insert hook will we be called in the rebuild process, as a new(!) entity will get created. And that's exactly the problem. The new entity will get a new id, and therefore may loose configuration simply attached to the entity.

ok, but that's not what the issue title says it is about? ;-)

The new entity will get a new id, and therefore may loose configuration simply attached to the entity.

It will get a new numeric ID, the machine stays. Configuration should relate to other configuration items by using machine names.

derhasi’s picture

derhasi commented
Title: Invoke update or insert hook on reverts » Invoke update hook on existing entities on revert

It will get a new numeric ID, the machine stays. Configuration should relate to other configuration items by using machine names.

I feared you'd play that card. It's right, that good modules might use the machine name for referencing. But that is independent from how a revert on an existing entity should work. When there is a entity with the exact machine name, that should be updated. Everything else (delete + create) feels really weird.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Postponed (maintainer needs more info) » Closed (works as designed)

I'm sorry, it seems I had wrong information here, you do indeed already invoke both the delete and the insert hook. And the new complaints by derhasi are of course a different issue. And while I still think doing an update instead of a delete and an insert would be better/clearer, I admit it probably wouldn't be worth the API break to change this now.

So, closing here. (@ derhasi: Create a new issue, if you want.)
Sorry again for needlessly bothering you!

derhasi’s picture

derhasi commented
Title: Invoke update hook on existing entities on revert » Invoke insert hook on existing entities on revert

I added a follow-up in #2051079: Do not delete entities on revert.