Add a generic oid column as a foreign key
pribeh - October 20, 2009 - 20:39
| Project: | Activity |
| Version: | 6.x-2.x-dev |
| Component: | Activity Contrib |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
| Issue tags: | Facebook-style Statuses |
Description
Thanks again to the developers of this module for all their hard work. I've brought this issue up with the maintainer of FBSS (http://drupal.org/node/606070 comment #3-4) and he's not sure there is a solution for this. The request is to have a way of deleting an FBS status and the activity message (tied to the status) simultaneously. We could go the route of simply letting users delete the activity messages but then there still exists all these extra FBS statuses. Any ideas?
Thanks,
Thomas
#1
So there is a function
<?php**
* Delete an activity message.
*
* @param int $aid
* The activity message id for the activity message table.
*/
function activity_delete($aid)
?>
So if there is a way to determine the Activity id from the FBSS then you could delete the activity allow with the status
#2
Should I bring this up with Ice from FBSS? Meaning, do you think this is something that could be done on the FBSS side of things? Thanks.
#3
As I said in the referenced issue,
So there are two issues raised: how to record the association between the Status ID and the Activity ID, and how to find out that there is an association. I've figured out the second part (see the end of this post) so the first part is most relevant.
One way to keep track of the association between the Status ID and the Activity ID would be to have a table specifically devoted to the SID - AID relationship. That's way out of scope for FBSS, at least unless I decided to split Activity integration into a separate module, which I think is not necessary.
To me, the most straightforward way to address this would be to have a content_id column somewhere in the Activity schema, since FBSS is not the only case in which users might want to delete an entity and its associated Activity records simultaneously. There is already a "nid" column in the {activity} table, but it is hard-coded to only hold a Node ID. So I'd like to change the purpose of this feature request to generalize/abstract the NID column into a content_id column so it can be used more broadly. (The "type" column should already be the name of the content type [except possibly in the obnoxious special case of "nodeapi" instead of "node"].) Any thoughts on that approach?
=====
For what it's worth, I did figure out a way to get the Activity AID and FBSS Status ID, although it is untested:
<?php
//Assumes the name of the module is "example."
/**
* Implementation of hook_activity_message_recorded().
*/
function example_activity_message_recorded($record, $context) {
$aid = $record->aid;
$sid = $context['poster']->facebook_status->sid;
db_query("INSERT INTO {example} SET amid = %d, sid = %d", $amid, $sid);
}
/**
* Implementation of hook_facebook_status_delete().
*/
function example_facebook_status_delete($sid) {
//Get the AIDs.
$result = db_query("SELECT aid FROM {example} WHERE sid = %d", $sid);
//Delete Activity records.
while ($record = db_fetch_object($result)) {
activity_delete($record->aid);
}
}
?>
I can't find a way to react when an Activity record is deleted at the moment, but I may be missing something. Anyway it doesn't seem particularly important in this case, although it may be something to keep in mind in the future.
#4
This has been discussed regarding NID column. It is important that it is just an NID column for node_access and the node relationship. It will not change. Same with the uid column.
It has also been discussed to avoid an oid column (that is a generic foreign key).
But I don't remember the arguments. Willing to listen to some though. (I will drag up previous discussions).
#5
I'm actually thinking that in almost every case I'm employing Activity (in a facebookish/twitteresque way) that I would want every activity and its corresponding node or status to be deletable straight from that activity view. This would make total sense from a usability standpoint (particularly if activity is being employed as a primary view for user navigation like on Facebook).
PS, is this at all related to my other request http://drupal.org/node/582230 ? Sounds similar but my conceptualization of msqyl sucks.
Thanks so much for your attention to this issue guys.
#6
ok here is a patch.
Basically, adds to the api an eid_field. So the Activity info object specifies which field is the entity id. This allows for the module to find all its entities and delete them from Activity. Like this:
<?php
function comment_activity_info() {
$info = _activity_activity_info();
$info->name = 'comment';
$info->object_type = 'comment';
$info->objects = array('comment author' => 'comment', 'node author' => 'node', 'comment author is the node author' => 'node_comment_author'); // array key is the label
$info->hooks = array('comment' => array('insert', 'update'));
$info->realms = array('comment' => 'Comment'); // do we need a t()?
$info->eid_field = 'cid'; // <<<< RIGHT HERE
foreach (node_get_types() as $type) {
$info->type_options[$type->type] = t($type->name);
}
return $info;
}
/**
* Implementation of hook_comment().
*/
function activity_comment(&$comment, $op) {
if ($op == 'delete') {
$db_result = db_query("SELECT aid FROM {activity} WHERE type = 'comment' AND eid = %d'", $comment->cid);
$aids = array();
while ($aid_obj = db_fetch_object($db_result)) {
$aids[] = $aid_obj->aid;
}
activity_delete($aids);
}
}
?>
The activity_delete function now accepts an array of aids as well as the single aid. This allows for efficient deletion.
#7
Needs review
#8
Patch looks good from looking over it. Unfortunately it will probably be around 2 weeks before I have time to do a more complete review.
#9
Tagging so I remember to come back to this.
#10
Well the important question really is, the 'eid_field' a DX fail? Does the api make it obvious and does it feel obvious?
#11
To me it certainly does. Honestly, one of the first things I noticed when I started working with the Activity API is that there wasn't an eid_field. I hadn't had any actual uses for one until now, so I never brought it up, but I think that conceptually it is not particularly difficult to grasp the idea that Activity records are related to the entity that they describe. If others are like me, recognizing that concept will actually help. The structure of Activity -- that is, the way it uses the core Actions/Triggers API -- is still clear.
#12
#457256: Use an oid column
Found the old issue. There the argument was so we could load up for theming. Which isn't what this is supposed to achieve. So I give this a go!
#13
Definitely a couple very good reasons. I'll support this idea.
Patch still applies cleanly but did not test other then that. If you're comfortable with it and it can be used by Ice in this way, I say we add it and roll a beta. I think this should probably we the last major api change and we start thinking about feature freeze in prep for alpha or rc.
#14
updated the patch a little bit to fix the following bugs
http://drupal.org/cvs?commit=284684
#15
#16
I just applied the patch and tested it, and it works as expected! Good job.
#17
Oops, cross-posted. Woot!
#18
FWIW, this change isn't much use to most developers until a hook_update_N() is written to add this column to the database. Otherwise, supporting the OID column will cause most modules to break. In the mean time, I've come up with a function for FBSS that does a good enough job of checking that the eid column exists, for now:
<?phpfunction _facebook_status_check_activity_eid_exists() {
$result = db_fetch_array(db_query_range("SELECT * FROM {activity}", 0, 1));
//There should be 7 columns in the database in the latest version of Activity
//(with the eid column in the {activity} table). If there are no columns
//returned, we don't have data yet, so we assume this is a new install of the
//latest version.
if (count($result) == 6) {
return FALSE;
}
return TRUE;
}
?>
I've just added the feature to FBSS that was the original reason for this issue: activity records will now be deleted when statuses are deleted if the eid column exists in the {activity} table.
#19
hmm i wouldn't do that.
Not writing update functions for a dev module with no recommended releases.
When users have a problem I generally just provide the SQL to update their code base. Besides, there has to be a better way to count the columns :-D. I know there is.
#20
Hey IceCreamYou and Scott, thanks so much for implementing this. I'm sure this will prevent a lot of headaches for users in the future. One question to Scott, though: will this allow for the same type of integration with nodes? ie, delete a node and the associated activity goes bye-bye? Or will there be another coding step required for that.
#21
@pribeh: It works for nodes too.
@Scott: I wasn't necessarily saying that you should write update functions for dev releases, just that no existing Activity install will be able to take advantage of this new feature until an upgrade path exists, which also means that all modules that implement this feature will be broken for those users.
Having said that, why not have update functions in a dev module? The worst that could happen is you have to write another one that un-does the work of the previous one, which is not harmful at all. Benefits include getting rid of problems like the one I've described, where module developers are stuck having to choose which group to support when writing integration for the development release. It also helps users who have intentionally decided to use the development releases -- with full understanding of the potential consequences -- keep up with the cutting-edge changes which are the reason they're using the development version in the first place. I suppose one could take the perspective that upgrade functions encourage users to upgrade to potentially unstable development releases, but that's the user's prerogative. It's their responsibility to understand what they're doing, not your responsibility to make sure they don't hurt themselves. ;-)
EDIT: Also, there's no better way to find out if the EID column exists. I researched rather thoroughly. A more robust alternative is the SHOW COLUMNS syntax, but IIRC that is not cross-database compatible and it's slower than my solution.
#22
Automatically closed -- issue fixed for 2 weeks with no activity.