Posted by miro_dietiker on July 28, 2009 at 11:50am
Jump to:
| Project: | Comment CCK |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
While comparing cckasetracker with comment_cck we've found many interesting issues.
At first this patch allows comment_cck to display per-comment-diffs right out of the cache. For N comments on a page with N comment_cck based revisionned changes we therefore reduce complexity by 2*N node_load and node_build_content.
It correctly invalidates cache, based on cid(nid) on any update and should provide pretty much speedup.
Install script patch provided too.
I also changed hook_form_alter to form_FORM_ID_alter and changed few signatures and added doc.
Hope it works! :o)
| Attachment | Size |
|---|---|
| comment_cck.install.patch | 1014 bytes |
| comment_cck.module.patch | 8.38 KB |
Comments
#1
Completely forgot to ask for review!
#2
I think the DB structure should be different:
We should gave another table with NID and a text field that will get the array of the rendered diffs - keyd by the revision ID.
Like this it would be enough for a single query to extract all the comments in a node.
Also we don't need to use cache_set() and cache_clear_all() - it will be enough to have an API function to get/ set the rendered diffs.
p.s. you can create a patch your files into a single .patch file - it's easier to review ( http://drupal.org/patch/create )
#3
What i most struggle is field permissions. Currently we're ignoring them. Or at least i dunno what will happen if any restrictions occur.
Right, we can build one structure per complete node instead of revision per comment Comm.
But what's wrong with a simple caching solution as i suggest? It's pretty hi speed compared to the node_loads before and the node_build_content happened before.
Most modules rely on core caching tools/api. Keep it atomic and simple. Do you think your suggestion will be that much more perfect?
May you make a code suggestion? ;-)
btw: i think we could reduce the cid to the vid which is globally unique. We only need it for nid cache flushes...
#4
Back again after thinking about it.
Amitaibu, primary data management is about APIs. It's about introducing logic and specific stuff.
Introducing a cache is about most trivial solution that best helps the situation and remaining as much as possible to core tools. This is cid (identifier) and some value to be cached. Additionally, identifiers could be namespaces with n subspaces, to be cleared with wildcard option. That's it.
Also from architecture, our primary natural entity we need to refer is the comment. Complexity is per comment, so let's do caching per comment. If you introduce sth. like per node caching, we could cache the whole node... even the whole page. No comment api based caching needed. If comments get more and more per node and we may add paging (also views supports comment entity), it is very important to have small caching elements. Without the need to load the whole node with n (e.g. 200) revisions, cached.
And if we want to have partial updateable cache elements with sth. like a special api, we might best add a new caching module that introduces this generic functionality instead adding the api to comment_cck.
Here we go. I'm happy with my suggestion and the way it works. If you're not, please provide your solution or at least explain it in more detail.
There's still field permission consideration that may lead to an issue. But i don't know anyways if comment_cck handles field permissions right now, if any.
#5
I'm thinking like you about cahcing, but instead of using cache_set() I suggest having a table with NID and serialized array - somthing that will persist even after clear-cache.
Something along these lines:
<?php
function comment_cck_node_delete($nid) {
db_query("DELETE FROM {comment_cck_node} WHERE nid = %d", $nid);
}
function comment_cck_node_get($nid) {
$node = db_fetch_object(db_query("SELECT nid, data FROM {comment_cck_node} WHERE nid = %d", $nid));
$node->data = unserialize($node->data);
return $node;
}
/**
* Store the node and all the comments.
*
* @param $data
* The diff of a comment.
* Data is an array keyed with thecomment ID.
* $data[CID] = array(old => array(), new => array())
*/
function comment_cck_node_set($nid, $data) {
// Determine if we update or insert a new record.
$update = FALSE;
if ($node = comment_cck_node_get($nid)) {
$update = TRUE;
}
else {
$node->nid = $nid;
}
// Add the new comments data.
$node->data += $data;
// Seralize the comments data.
$node->data = serialize($node->data);
if ($update) {
drupal_write_record('comment_cck_node', $data, array('nid'));
}
else {
drupal_write_record('comment_cck_node', $data);
}
}
?>
#6
Hmm... Why in general do we need "somthing that will persist even after clear-cache"?
It can be deleted anytime without any issue (except performance impact to rebuild). But that's the same for all other caching implementations in drupal.
#7
> Why in general do we need "somthing that will persist even after clear-cache"?
>> except performance impact to rebuild
I think performance is good enough reason. The thing is that once we have the data, we'll hardly need to delete it (only if we delete the node or a comment). For me a cache is something more temporary, while this data IMO is more persistent.
#8
Changing status per discussion after patch submitted.