From uc_roles:
/**
* Delete all data associated with a given node.
*
* @param $nid
* A Drupal node ID.
*/
function uc_roles_node_delete($nid) {
### NOTE: this is called from hook_nodeapi delete op.
db_query("DELETE FROM {uc_roles_products} WHERE nid = %d", $nid);
}
/**
* Delete all data associated with a given product feature.
*
* @param $pfid
* An Ubercart product feature ID.
*/
function uc_roles_feature_delete($pfid) {
db_query("DELETE FROM {uc_roles_products} WHERE pfid = %d", $pfid);
}
Why should every single module that implements a product feature have to check for node deletion?
Surely UC core also knows when a product node is delete. It can then take care of finding the features on this node (since presumable UC core is cleaning {uc_product_features}), then calling the feature deletion callbacks, in this particular case uc_roles_feature_delete().
This would mean less work for feature modules.
In the case of uc_roles, hook_nodeapi could be *dropped entirely* -- IMO dropping a hook is a big bonus.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 584506-D6-feature-delete.patch | 1.69 KB | tr |
Comments
Comment #1
rszrama commentedI'm torn... I think the product features in general is pretty lame and could have been a lot stronger. As is, though, while I think we can just delete product features when a product node is deleted, we don't have a strict data model that would clear us to delete any associated data. We can figure out if it's safe with a little investigation... for example, making sure that the file download data doesn't need to be kept around even if the product isn't to keep restrictions in place for people who purchased those files in the past. It should be fine if the code was written properly... that's a big if. : )
Comment #2
joachim commentedIf the product isn't for sale any more but you still want its file download feature to work, wouldn't you unpublish the node? I'd expect deleting node with a file download to maybe even delete the file!
And more generally: yes, +1 on a stronger product features system. Is there a general roadmappy issue for this already?
Comment #3
longwaveBumping for consideration in 7.x.
Comment #4
arski commentedjust ran into the same issue when working on my feature-based module http://drupal.org/project/uc_product_panes - there is really no reason why every such module should be implementing its own hook_nodeapi to delete the features when this could be 100% safely done by UC itself. It's pretty simple, if a node is deleted for whatever reason, the features saved along with it will never be of any use to anyone, so keeping those is just collecting trash in the DB.
Would be great if this were implemented in both D6 and D7.
Cheers
Comment #5
tr commentedClosely related issue: #943048: Add hook_nodeapi() to bring in uc_file (and uc_roles) feature data
Comment #6
longwaveFixed in 7.x: http://drupalcode.org/project/ubercart.git/commit/f5c9dda
I think this can be backported safely, as it shouldn't be a problem if both uc_product and a feature module both try to delete the same thing.
Comment #7
tr commentedHere's the backport to D6. Can someone test this?
Comment #8
tr commentedNot going to backport anything to D6 anymore..