I'm wondering if it would be possible to ignore fields that do not expose DB columns when building table schemas.

There's a problem I've seen when the field is shared, or allows multiple values, then a table with only the primary key will be created, but doesn't make much sense.

No problem when the field storage is per type, and the type has more fields. But when the field uses per field table, we end up with a table that's completely useless.

Fields that would benefit from this: Computed field when not instructeds to store computed value, and maybe other modules.

CommentFileSizeAuthor
#6 content_field_no_columns-524314-6.patch4.54 KBmarkus_petrux
#4 content_field_no_columns-524314-4.patch1.79 KBmarkus_petrux

Comments

markus_petrux’s picture

markus_petrux commented

Related issue with markup CCK field:

http://drupal.org/node/83787#comment-1835238

cyu’s picture

cyu commented

One problem comes from this code in content_alter_db(), 

    if ($new_field['multiple']) {
      db_query('INSERT INTO {'. $new_table .'} (vid, nid, delta, '. implode(', ', $columns) .') '.
        ' SELECT vid, nid, 0, '. implode(', ', $columns) .' FROM {'. $previous_table .'}');
    }

because it assumes that $columns is not an empty array and will throw a SQL syntax error if it is.

cyu’s picture

cyu commented

The other issue I see comes in the insert/update $op of content_storage() when you have a field which is multiple but also has no db columns,

if (count($record) || empty($schema['content fields'])) {

will call content_write_record() for the markup field mentioned above since empty($schema['content fields']). This will insert with delta 0.

But then

// Handle multiple fields.
      foreach ($type['fields'] as $field) {
        if ($field['multiple'] && isset($node->$field['field_name'])) {
       /* stuff
      */

          // If there was no non-empty item, insert delta 0 with NULL values.
          if (empty($records)) {
            $record = array();
            foreach ($db_info['columns'] as $column => $attributes) {
              $record[$attributes['column']] = NULL;
            }
            $record['nid'] = $node->nid;
            $record['vid'] = $node->vid;
            $record['delta'] = 0;
            $records[] = $record;
          }

will call content_write_record() and try to write again, also with delta 0, which will throw an error, "user warning: Duplicate entry '%nid-0' for key 1 query"

markus_petrux’s picture

markus_petrux commented
Status: Active » Needs review
StatusFileSize
new content_field_no_columns-524314-4.patch1.79 KB

The issue in #2 can be easily fixed, but the issue in #3 is a bit more complex because when we have a field using per field storage, but that field does not have columns defined, then the per field table is not necessary at all.

I would be open to fix #3 as well, but it would be nice to get some input from yched/KarenS here.

The attached patch fixes #2.

cyu’s picture

cyu commented

The error in #3 can be fixed in a pretty straightforward manner by checking for columns as you've done in your patch for #2, but I agree that a deeper problem is that there is no need for the table when no columns are being stored. I did notice that removing the table will end up causing more complex issues though, such as failed joins...so perhaps the straightforward fix would be more suitable for now.

markus_petrux’s picture

markus_petrux commented
StatusFileSize
new content_field_no_columns-524314-6.patch4.54 KB

Here's another take to fix the data migration issues. This time, I'm also preventing row updates when migrating from per-field storage to per-content-type storage AND the field has no columns.

Since fields that do not expose DB columns are not exposed to Views either, I think this change won't impact existing installations.

The patch is against CCK2, but it probably works for CCK3 as well.

If this works, I think we could commit and then proceed with the operations performed in nodeapi(update/insert). Once we get to the point no data is inserted to per-field tables with no DB columns, then we could also prevent DELETE in these cases, and finally try to see if we can get rid of per-field tables with no columns.

Testing and feedback is more than welcome. :)

[EDIT]
Hint to review the patch: While the patch seems to affect several lines, I'm only adding the !empty($migrate_columns) condition prior to proceed with data migration steps.

brad.bulger’s picture

brad.bulger commented

I'm still getting this error in 6.x-2.9 - is there another issue or other change in 3.x that supersedes this?