Having a column named "fields" is not a great idea. This is a reserved word in mysql:

http://dev.mysql.com/doc/mysqld-version-reference/en/mysqld-version-refe...

Causes db_write_record to fail under mysql 4.1 at a minimum. If you can't add to mollom_forms, the module doesn't work so well.

CommentFileSizeAuthor
#10 mollom-HEAD.schema.10.patch12.49 KBsun
#7 mollom-DRUPAL-6--1.schema.7.patch12.31 KBsun

Comments

joshk’s picture

joshk commented

Here's the doc for 4.x

http://dev.mysql.com/doc/refman/4.1/en/reserved-words.html

Trying to do a basic insert fails:

mysql> INSERT INTO mollom_form (form_id, mode, fields, module) VALUES  ('comment_form', 2, 'a:2:{i:0;s:7:"subject";i:1;s:7:"comment";}', 'comment');
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'fields, module) VALUES  ('comment_form', 2, 'a:2:{i:0;s:7:"subject";i:1;s:7:"com' at line 1
dries’s picture

dries commented
Title: Mollom's DB schema should now use "fields" as a column name » Mollom's DB schema should not use "fields" as a column name

Fixing title: "should now use" -> "should not use".

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Dang. Need to think of a new name that works.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Maybe just use 'data' or 'field_data'. If we used data it could be a serialized array with array('fields' => $fields_array). That way it could be extendable in the future.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Normal » Critical

Bumping to critical.

joshk’s picture

joshk commented

Sorry for mistyping the initial title.

+1 for Dave Reid's suggestion of "field_data". Most tables that store serialized info call it "data".

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Active » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.schema.7.patch12.31 KB

'field_data' would technically not be correct. The column does not hold any data, but rather references to form element keys.

I therefore went with 'enabled_fields'.

Since a couple of sites are already running on 6.x-dev, the update path is a bit tricky, but we need to re-learn how to do proper HEAD-HEAD updates anyway. :)

dries’s picture

dries commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Needs work

Committed to DRUPAL-6--1. Moving to CVS HEAD.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Patch (to be ported)
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Reviewed & tested by the community
StatusFileSize
new mollom-HEAD.schema.10.patch12.49 KB

Straight port to HEAD.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ mollom.module	17 Feb 2010 15:01:33 -0000
@@ -503,10 +503,10 @@ function mollom_get_form_info($form_id =
+    $mollom_form['enabled_fields'] = unserialize($mollom_form['enabled_fields']);
     // @todo Is this a bug in D7?
...
+    if (!$mollom_form['enabled_fields']) {
+      $mollom_form['enabled_fields'] = array();
     }

Just found the corresponding bug report for core:

#353918: drupal_write_record writes empty string instead of empty serialized array

Powered by Dreditor.

dries’s picture

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

Committed to CVS HEAD. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-1.x-dev » 6.x-1.12

Let's move this back to 6.12, so users are able to find it. (special forward-porting scenario)

Status: Fixed » Closed (fixed)

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

  • Commit 0d87293 on master, fai6, 8.x-2.x, fbajs, actions by Dries: 
    - Patch #701010 by sun: Mollom's DB schema should not use 'fields' as a...
  • Commit 27282d4 on master, fai6, 8.x-2.x, fbajs, actions by Dave Reid:
    #701010, #683648 follow-up by sun: Mollom's DB schema should not use '...

  • Commit 0d87293 on master, fai6, 8.x-2.x, fbajs, actions by Dries: 
    - Patch #701010 by sun: Mollom's DB schema should not use 'fields' as a...
  • Commit 27282d4 on master, fai6, 8.x-2.x, fbajs, actions by Dave Reid:
    #701010, #683648 follow-up by sun: Mollom's DB schema should not use '...