Most of core uses drupal_write_record() when saving records. The comment module uses hardcoded fields in a database call. This means that any schema changes through hook_schema_alter are ignored and so hacks need to be implemented in hook_comment_insert() / hook_comment_update() to work around this limitation.

At this stage I don't have a patch for this, just filing a bug report so I remember why I had to implement the hacks in a contrib module.

CommentFileSizeAuthor
#14 drupal-comments_support_additional_columns-1120412-14.patch2.54 KBskwashd
#7 drupal-comments_support_additional_columns-1120412-7.patch2.73 KBskwashd
#4 drupal-comments_support_additional_columns-1120412-4.patch2.79 KBskwashd

Comments

ralt’s picture

ralt commented
Status: Active » Closed (works as designed)

Hello,

Since drupal 7, drupal_write_record() is almost not used anymore in core functions. See /modules/node/node.module or /modules/user/user.module for some examples, where drupal_write_record() does not appear while db_insert() does.

skwashd’s picture

skwashd commented
Title: comment_save() doesn't use drupal_write_record() » comment_save() doesn't support additional columns added by hook_schema_alter()
Status: Closed (works as designed) » Active

You pointed to node.module as an example of where drupal_write_record() wasn't used, but node_save() does use it:

...
    // Save the node and node revision.
    if ($node->is_new) {
      // For new nodes, save new records for both the node itself and the node
      // revision.
      drupal_write_record('node', $node);
      _node_save_revision($node, $user->uid);
      $op = 'insert';
    }
...

user_save() also uses drupal_write_record() at line 586 in user.module.

Either way, there is still the problem of comment_save() not supporting columns which have been added via hook_schema_alter(). I have amended the title to better reflect the underlying problem.

catch’s picture

catch
he/him
Primary language English
commented
Version: 7.x-dev » 8.x-dev

Seems sensible to me. Either you should be able to alter the columns in, or just use drupal_write_record() here. However this would need to get into 8.x first.

skwashd’s picture

skwashd commented
StatusFileSize
new drupal-comments_support_additional_columns-1120412-4.patch2.79 KB

This is a rough cut first attempt at the trying to fix this.

gordon’s picture

gordon commented
Status: Active » Needs review 
+++ b/modules/comment/comment.module
@@ -1553,23 +1546,17 @@ function comment_save($comment) {
+      $comment->cid = $comment->cid;

Shouldn't this be set by drupal_write_record()

Other than that it looks ok, however I think that it needs to be run thought some testing before we commit to core.

Powered by Dreditor.

gordon’s picture

gordon commented
Issue tags: +Needs backport to D7
skwashd’s picture

skwashd commented
StatusFileSize
new drupal-comments_support_additional_columns-1120412-7.patch2.73 KB

@gordonh Thanks for noticing the wasted CPU cycles. Rerolled.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs review » Reviewed & tested by the community

Looks good, although there was talk about 'death to drupal_write_record' back when Drupal 8 battle plans were drawn up. Personally I'd weep at it's removal.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

http://1drupal.blogspot.com/2010/12/re-development-using-dbinsert-with-b...

skwashd’s picture

skwashd commented

I acknowledge that not everyone loves drupal_write_record(), but in D7 and currently in D8 it is the best option we have for dealing with the use case here.

I'd suggest that it is best to get comment_save() using drupal_write_record() for consistency and not breaking schema_alter()d columns in {comment}. If/when drupal_write_record() is replaced comment_save() can be fixed at the same time.

dries’s picture

dries commented

- I'd love to see a few other people chime on this. While this looks reasonable, it certainly trades off flexibility for more ugly core code. Admittedly, it makes contributed module code more elegant.

- I'd love to figure out if this introduces clean-up opportunities elsewhere in core. We don't necessarily have to fix this in the current patch, but at least we could create follow-up issues for it.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

It doesn't look necessary to do the clone and unset to me. While comment module doesn't specifically set those values, nothing stops modules from manipulating them now via hook_comment_update() and hook_comment_insert() if they really wanted to, so they can just be passed in - I assume this is part of what Dries is referring to as the ugliness.

Also the comments in the second hunk could use some clean up - full sentences, explain the 'missing' values are those that aren't passed into the function or similar.

We don't have any core modules that schema alter the comment table afaik, so there are unlikely to be cleanup opportunities here. However, bringing comment_save() a bit closer to other $entity_save() functions will eventually make them easier to consolidate in to a single entity_save().

damien tournoud’s picture

damien tournoud commented

This is a must have. All the other entity save logic uses drupal_write_record().

skwashd’s picture

skwashd commented
Status: Needs work » Needs review
StatusFileSize
new drupal-comments_support_additional_columns-1120412-14.patch2.54 KB

Rerolled patch based on feedback from catch.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

This looks great now. Thanks for the reviews and the re-rolls.

I didn't see any harm in back-porting this so I've committed it to both 7.x and 8.x.

We should be able to make some progress with UUIDs now -- at least in contrib.

Automatically closed -- issue fixed for 2 weeks with no activity.