Cannot enable Comment module after a Drupal 6 to Drupal 7 upgrade

OK, I can confirm this bug, steps taken:

1. Fresh install of the latest d6 dev tarball, no changes made.
2. Removed all files, unpacked the latest d7 tarball edited settings.php to point it to the d6 DB
3. Ran update.php, getting the following messages of updates ran:
   

   The following updates returned messages
   user module
   Update #7000
   
       * User passwords rehashed to improve security
   
   Update #7002
   
       * Migrated user time zones
   
   taxonomy module
   node module
   filter module
   comment module
   Update #7000
   
       * Comment order settings removed.
   
   block module
   system module
   Update #7007
   
       * Inserted into {role_permission} the permissions for role ID 1, Inserted into {role_permission} the permissions for role ID 2
   
   dblog module
   update module
   

   No errors were encountered during the update process.

4. Attempted to enable the comment module, resulting in the following error:
   FieldException: Attempt to create an instance of field comment_body on bundle comment_node_page that already has an instance of that field. in field_create_instance() (line 673 of /home/manuel/htdocs/d6head/modules/field/field.crud.inc).
5. Attempted to re-run update.php, but no updates were done
6. Created a new story, with comments opened (the module is enabled apparently), which resulted in the comment form having only the subject field, and no body to insert your comment, see screenshot attached.
7. Attempted to add a new comment, filling in only the subject field, and hitting preview, wich lead me to /comment/reply/1, which returns a Page not found error

Other information:

• The Add new comment link is added on teaser view, wich also ends up in a page not found obviously.
• If you dont disable the comment module prior to the update, it runs fine without any trouble.

I'm working on a different issue, that appears to be very similar.
http://drupal.org/node/890128

Sorry, re-adding the tag.

Next time someone reproduces this error, can they post the database dump so we can all be working from the same dataset?

@sfyn I worked from a fresh install of d6-dev (2010-Aug-12), and upgraded to latest d7-dev (2010-Aug-23)

For the record, I'm uploading the DB dump of the d7 database upgraded, with comment module enabled (after all my tests in my comment above).

In comment.install, comment_enable() we have:

  foreach (node_type_get_types() as $type => $info) {
    if (!isset($info->is_new) && !isset($info->disabled) && !field_info_instance('comment', 'comment_body', 'comment_node_' . $info->type)) {
      _comment_body_field_instance_create($info);
    }
  }

Since the bug report says the problem occurs while enabling the comment module, this would be a prime suspect, except the code checks whether the instance already exists. But the bug has to be that one place is creating the instance and another place is not checking before trying to create it again.

Results of repeating the process, and executing debug_backtrace(); on line 672 of field.crud.inc, like this:

if (!empty($prior_instance)) {
    debug(debug_backtrace());
    $message = t('Attempt to create an instance of field @field_name on bundle @bundle that already has an instance of that field.', array('@field_name' => $instance['field_name'], '@bundle' => $instance['bundle']));
    throw new FieldException($message);
  }

Im uploading:
d6head: fresh install of d6 database dump, with comment module disabled, ready to be upgraded.
d6head_upgradedtod7: database dump of the d7 db, just after upgrading to it, ready to enable the comment module and test the bug.

user 1: admin:admin

The results of the backtrace actualy formatted so a human can use it....

Worth noting: with devel module installed this error does no longer occur! To be continued ...

OK, after long debugging along side Sutharsan here in the drupalcon codesprint, we can have some results.

Apparently the function field_read_instance returns a positive result when called in field_create_instance(), at least in this case.

I tested to see whether field_info_instance would also return a positive result, when we are sure that the field instances do not exist (since they don't show in the comment form), and the function returns false in the situation that this bug occurs, which is what should happen.

So the attached patch does something very simple, it uses field_info_instance to check whether the instance already exists or not, instead of field_read_instance.

Ive tested creating a new content type, and debugging showed that the function did return info on field instances already there for page and story, but not for the new content type, so the new content type got the comment body field attached as expected.

We could be way off here or breaking something else, so any guidance would be very welcomed!

For the record, I've print_r'ed $prior_instance in line 670 of field.crud.inc field_read_instance($instance['entity_type'], $instance['field_name'], $instance['bundle']);

And in each case, it seems to return the result from the comment_node_page bundle.

Array
(
    [label] => Comment
    [settings] => Array
        (
            [text_processing] => 1
        )

    [required] => 1
    [display] => Array
        (
            [default] => Array
                (
                    [label] => hidden
                    [type] => text_default
                    [weight] => 0
                    [settings] => Array
                        (
                        )

                    [module] => text
                )

        )

    [widget] => Array
        (
            [type] => text_textarea
            [settings] => Array
                (
                    [rows] => 5
                )

            [weight] => 0
            [module] => text
        )

    [description] => 
    [id] => 3
    [field_id] => 2
    [field_name] => comment_body
    [entity_type] => comment
    [bundle] => comment_node_page
    [deleted] => 0
)

I am totally lost as to why field_read_instance is returning something, but field_info_instance does not.

More updates,

Looks to me that my patch is probably not the way to go, I have been doing some more digging, and upon upgrade to d7, there are two entries in table field_config_instance for comment_body, one for comment_node_page, and one for comment_node_blog.

So, next question that should be asked is, why then are the fields not being added to the comment forms?

Funther investigation on the data that field_info_instance() revealed that field_info_instance() is NOT the way to go. field_info_instance() will only return entity data of enabled modules. And at the time the function is called, the module is not yet (fully) enabled.

Changing the status back to 'active' since a totally different solution here.

I'm working on this.

The issue is that the cached field info returned by _field_info_collate_fields() does not contain the comment_body instances. Thus in comment_enable(),

    if (!isset($info->is_new) && !isset($info->disabled) && !field_info_instance('comment', 'comment_body', 'comment_node_' . $info->type)) {

the field_info_instance() test returns false and the code tries to create a new one. So why doesn't comment_body exist in the cached field info? My guess is that it is because the field and instances are created inside a comment update hook, during which the comment module (which defines the comment entity) is not enabled yet, and so the comment_body field and instances are marked inactive or something. Presumably exactly the same problem would occur for every update function that creates fields and instances for its own entity type, which led me to suspect that comment module is the LAST module to do so during the D6->D7 upgrade.

To confirm, I put a print_r($instance) statement in field_create_instance() and, sure enough, the comment_body fields are the last ones created.

So this is probably a cache clearing bug, nothing specific with comment.module. I'll look more into it later, now it is time for dinner.

I confirmed that this is not a problem with comment.module at all. The issue is that during comment.module's update functions, comment.module is not enabled yet. The update function creates the comment_body field and its instances, but it creates it on the 'comment' entity type which is not defined because comment.module is not enabled. The final call to _field_info_collate_fields() during this whole process occurs still during the update function, so the last version of field info that gets cached does not have the comment_body field in it. Apparently, the cache remains uncleared up through submitting the modules page enabling the comment module, which tries to re-create the comment_body field and its instances because its check (using Field Info API functions) for whether they already exist fails because the cache is wrong.

This will happen to whichever module creates a field and instances on its own entity last during the install process. comment.module just happens to be last.

Clearing all caches before enabling the module works around the problem.

I'm not quite sure what system is "at fault" here. One obvious fix is to clear all caches after every update function, though that might be slow, OTOH it seems pretty essential for correct functioning of arbitrary modules. However, it occurs to me that I'm surprised field_create_instance() works at all when given an entity type name that does not exist yet; it "should" just fail immediately. If it did, though, we'd have a chicken-and-egg problem because a module's entities can never exist before the module is enabled and the update functions run before the module is enabled; under this condition all modules that create a field on its own entity type, not just the last, would fail.

So I'm not sure what to do besides clearing all caches after every update function. The only alternative I can think of is to somehow force a module's entity type to exist before the module is enabled (so hook_entity_info() runs)... but that would only solve the problem for entity types, not for any other kind of hook exported by the module and cached by any API function called during an update function.

Actually I think the only *complete* solution is to declare an update function that runs after the module is enabled; it will then be up to the module author to make sure that works okay. This may be too big a change for D7 now, in which case clear caches after every update and not "fixing" field_create_instance() to be more strict is the best approach for now.

So clearly I was asleep last night when I wrote the above comment. The solution here is to move the code to create the comment_body field and instances from comment_enable() to comment_install(). That way, either the update function or the install function will create the field/instances, but not both.

Oh, this looks like fun:

  // Create comment body field.
  // @todo this should be done in comment_install, but causes exceptions
  // in testing because the list of fields types is not available in
  // hook_install(): _field_info_collate_types() returns an empty array.
  if (!field_info_field('comment_body')) {
    $field = array(
      'field_name' => 'comment_body',
      'type' => 'text_long',
      'entity_types' => array('comment'),
    );
    field_create_field($field);
  }

This patch moves comment_body field and instance creation to comment_install(). It seems to work for me but the @todo I deleted suggests it will cause test failures. We'll see.

Well, okay... now what? Should we conclude that whatever problem previously caused field and instance creation in hook install to fail has been fixed?

#891250: "View comments" permission not working after upgrade to Drupal 7 addresses the problems of comments not appearing after the upgrade. With both patches applied, comments work across the upgrade. I marked the other patch RTBC but this one is mine so I can't.

The patch looks good and is basically moving code to where it really belongs, but I didn't try it out, so I won't RTBC.

Marking as RBTC as the original reporter seems happy with the patch provided

Awesome sleuthing, folks! Committed to HEAD!

I agree, the patch makes sense.
Also thanks bjaspan for explaining what was going on, we were going nuts in the code sprint :X

reverting status (crossposted) yay fixed!