FieldException when trying to enable blog and comment modules for first time

I'm also getting this same error again whenever I clear my caches from the Performance page.

I just did another wipe/cvs update/install. Error still occurring.

I put in a crude print statement in field_create_instance, and found that it is a collision between:

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

    [display] => Array
        (
            [full] => Array
                (
                    [label] => hidden
                    [type] => text_default
                    [settings] => Array
                        (
                        )

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

        )

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

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

    [required] => 
    [description] => 
    [weight] => 0
    [id] => 4
    [field_id] => 2
    [field_name] => comment_body
    [bundle] => comment_node_forum
    [deleted] => 0
)

and

Array
(
    [field_name] => comment_body
    [label] => Comment
    [object_type] => comment
    [bundle] => comment_node_forum
    [settings] => Array
        (
            [text_processing] => 1
        )

    [display] => Array
        (
            [full] => Array
                (
                    [label] => hidden
                )

        )

    [field_id] => 2
)

I don't know enough about that to continue debugging.

It's also interesting to me that after a while, the error messages stop happening.

The behavior I'm getting is that it happens a few times when I first install, and then I stop seeing the error message. It all seems very odd.

I just tried it (wipe everything, install from HEAD, expert profile, check all core modules except Overlay and click Submit).

I'm still getting the same error.

Correction: The error message is slightly different now:

FieldException: Attempt to create an instance of field comment_body on bundle comment_node_blog that already has an instance of that field. in field_create_instance() (line 645 of /opt/www/eclipsework/drupal7/modules/field/field.crud.inc).

Looking into this a bit more...
Places I see field_create_instance() being called with the 'comment_body' field:
1) comment_enable() in modules/comment/comment.install -- note that it's not in comment_install() but in comment_enable().
2) comment_update_7012() in modules/comment/comment.install
3) comment_node_type_insert() in comment.module

My guess is that #1 and #3 are conflicting (I don't think the update functions should be run if I'm doing initial module enable, correct?), but I'm not sure of the exact sequence of events... Let's see...

a) When I submit the modules form, system_modules_submit() calls http://api.drupal.org/api/function/module_enable/7
b) module_enable() first updates the modules db (setting status of each module to 1, indicating it's enabled). So at this time, both the blog module and the comment module will be enabled.
b) Then module_enable() calls each module's hook_enable(). Blog doesn't have one. comment_enable() at that time lists all content types with node_get_types(), finds 'blog' without a comment body field, and attaches the comment body field. Which would seem OK, except that node_types_rebuild() hasn't been called yet, so even though node_get_types() tells the comment module that the blog content type exists, it actually hasn't been run through node_type_save() yet.
c) Then module_enable calls a bunch of functions to reset caches, including node_types_rebuild(), which at this point calls node_type_save() on all the new content types (including blog), thus triggering the comment_node_type_insert() function, which doesn't bother to check to see if the comment body field already exists before attaching it. http://api.drupal.org/api/function/comment_node_type_insert/7

So. IMO, the problem is really that comment_enable() is assuming that the node types have been built, when in fact they haven't been built yet. Because comment_node_type_insert() should be able to assume it's getting a brand-new content type being saved for the first time (if it was a new content type, it should have been sent to hook_node_type_update() instead).

The fix is probably to have comment_enable() call node_types_rebuild() to ensure that the list of content types returned by node_get_types() is really accurate with what's been saved to the database. Or for node_get_types() to verify that everything is in the DB.

Minimal steps needed to reproduce this issue:

a) Create a new space (wipe database, cvs get D7 HEAD)
b) Run install.php and choose the "minimal" install profile.
c) Visit the modules page (admin/modules/list)
d) Check "Blog" and "Comment" and submit the form.

You will see the following error:

FieldException: Attempt to create an instance of field comment_body on bundle comment_node_blog that already has an instance of that field. in field_create_instance() (line 645 of /opt/www/eclipsework/drupal7/modules/field/field.crud.inc).

Thinking this over...

So the problem is that comment_enable() is assuming that node_get_types() is returning fully-created types (i.e. types that have been run through node_type_save()), because comment_node_type_insert() will take care of content types as they are created with node_type_save().

Actually, I think that in most cases, the functions that call node_get_types() and related functions will assume that the node types returned have been already run through node_type_save() at least once. So I'm going to create a patch that will assume that, but let the few cases where that isn't the case bypass that.

Ugh.
Actually, I think that maybe the best thing is just for comment_update() to call node_types_rebuild() to make sure it's getting the right information.

Here's a patch that calls node_types_rebuild() in comment_update(), and adds doc to node_type_get_types() to warn others of this issue.

I've verified that with this patch, the reproduce steps in #9 do not produce the error message any more.

Just as a note, the reason I decided it wasn't a good idea to add automatic rebuild to node_type_get_types() is that it's really only an issue during module install, because node_types_rebuild() is always called during module install, and it would have added unnecessary overhead.

Yeah, it's a definite WTF. However, it does only come up at install/enable module time, since module_enable() calls node_types_rebuild() a bit farther down in the function.

er.

I guess node_types_rebuild() is called from system_modules_submit() and not from module_enable().

Hrmmph. _install_module_batch() doesn't call node_types_rebuild() either. Hopefully install.php does it at some point...

My feeling is that logically the best thing to do would be to build it into node_type_get_types() (or technically, the internal function that does the work for that and other functions), so that there would be no way to call node_type_get_types() and get the wrong answer (i.e. types that haven't been saved). This could probably be made fairly efficient with a flag saying "node types need rebuilding", which would have to be reset early in the module install process.

If that was done, then system_modules_submit() et all wouldn't even have to call node_types_rebuild(). Same thing could be done with menus, etc. if we had a hook_caches_are_all_wrong() or something like that, which is maybe what is being proposed in #651086: Cache clearing is an ineffective mess in module_enable() and system_modules_submit()?

But really, both of these seem like too big of a change for D7 at this point, which is why I proposed the patch in #13, which fixes the immediate problem with comment.module and documents the problem in the doxygen for node_type_get_types().

Dries: See #17 for my best idea of how to actually fix this... Would you support this type of change this late in D7 development? I think that's the best place to put it.

My idea in #11 of a simple fix was negated once I looked deeper into the code (see #13).

OK. I can take a look at it early next week.

Or someone else can take it on if they have time earlier -- just assign it to yourself so I know it's not my project any more. :)

OK, I'll take this on now.

Well, maybe not. I give up... The fixes I attempted did not solve the problem, and I'm not sure what to do...

Note that the easy way to reproduce the problem is in #9.

For reference, here's the patch I attempted. With this patch, the error I get when following #9 steps to reproduce is:
"FieldException: Attempt to create an instance of field body without a bundle. in field_create_instance() (line 627 of /opt/www/eclipsework/drupal7/modules/field/field.crud.inc)."

So it looks like this solves one problem but creates another. I really don't know what to do at this point, besides going back to the hackish patch in #13 that Dries didn't like.

If you comment out _comment_body_field_instance_create(), the instances on the blog node type will be created when node_type_save() is eventually called from node_types_rebuild(). That is why in #9 reproduce you get the error (without the patch): comment_enable() creates the instance, and then node_type_save() does it again. (See #8 above for complete explanation of this sequence.)

So removing that line would fix the problem, but break the intended behavior of that line: you want to add the comment field to *existing* node types that existed before this round of module enablings.

The problem is that in comment_install(), you want only the old types to be added to, not node types that have yet to be added to the database (i.e. the ones that are defined in newly-enabled modules' hook_node_info() implementations, which are not saved to the database until node_types_rebuild() is called).

Does that make sense?

The thing about having multiple instances that aren't removed is a separate issue and should probably be dealt with separately.

a) Does that fix the problem? It looks fairly reasonable...

b) If we use this tactic rather than changing how node_type_get_types() et al work, I would advocate adding doc to all of the node_type_get_* functions, as well as _node_types_build(), detailing what their return value is in the case where the db is not synchronized with the hooks, and explaining that it wouldn't be synchronized during module install/enable hooks.

I think the doc could be better...

- The field explanation needs more detail, and should explain
- Several of the functions do not give you those fields to check.
- Just having @see _node_types_build() and having the doc on node_types_build() does not seem like a very good way to warn users of those functions, who might not bother to jump to that other page/function and see the doc. So I think the warning should be on all of those node_type_get_* functions too.

I can give the doc a work-over on Monday, unless someone gets to it before me....

Here's a new patch, with more doc explanation, preserving the fix for the bug from #36.

Whoops. Try this one (a few minor typographical corrections in the doc)

be -> not

Sorry about that.

One more fix needed:

- * All new or non-modified module-defined node types are saved to the database.
+ * All new module-defined node types are saved to the database via a call to
+ * node_type_save(), and obsolete ones are deleted via a call to
+ * node_type_delete(). See _node_types_build() for an explanation of the new 
+ * obsolete types.

Last line
obsolete types -> and obsolete types.
(again, my fault, sorry)

I must run out, so cannot reroll right now, sorry.

Oh wait, I don't have to leave for an hour. Here's a revised patch.