Comment body missing if comment module enabled after a content type module

Not just blog every module in core showing same problem if installed with or before comment module.

Attaching a patch to node.module which removes $is_new from node_type_set_defaults and add it to more appropriate node_type_save. This is used with

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

in comment.install which checks each content type to add comment body field to.

Updating patch try this instead....

You need to change the status.

I tried to figure out what happens here.
In a test where the forum and taxonomy is re-enabled, they test fails because taxonomy finds a field that's already created.

I wish I could do more on this, but I'm not sure what happens in more detail.

#1: comment-field-fix-890128.patch queued for re-testing.

There's a very similar bug here: http://drupal.org/node/890210 which has attracted more attention, and has a lot more details.
If there thinking in that thread turns out to be correct, this patch to the node module will become superfluous.
One could wait for the result on that one, before putting to much effort into this project.

Just my 2cents

+++ modules/node/node.module	22 Aug 2010 16:05:34 -0000
@@ -745,7 +746,6 @@ function node_type_set_defaults($info = 
-    $type->is_new = 1;

This must be not be removed.

Powered by Dreditor.

this thread has been left alone for some time...i'm having the same problem however with the latest 7.x-dev, it doesn't appear that http://drupal.org/node/890210 fixed this particular problem unless some other contributed module is causing it for me.

Easy to reproduce in HEAD. This looks like something worth fixing :)

I'm tentatively setting this to critical. Comments simply don't work if the comment module is enabled after the node-type-providing module. This seems like a serious architectural screw-up.

The blog module and the minimal profile are not specifically involved. This bug happens whenever the comment module is enabled after a content type module. Comments are then rendered useless for that content type.

This test should illustrate the problem. Setting CNR for bot.

How much does this overlap/duplicate #974712: Deleting comment_body field on sole content type breaks subsequent content types ?

I've got a patch working to fix this issue, but I had to solve another bug in the process, with node type static caching. Will cross-link once I separate out that fix and post an issue.

Filed a new issue at #993992: _node_types_build() labels content types 'is_new' when they're not, and has static caching bugs, and merged in the patch from #1 there, for testing purposes. Marked that other issue critical because it blocks this one.

This patch uses hook_flush_caches() to enumerate node types and create bundles. comment_install() hardly seems like the place for that, which causes problems when the comment module is enabled along with a string of other modules, with no dependency ordering.

hook_flush_caches() in D7 is run after groups of modules are installed/enabled/disabled, and node types are rebuilt just before the hook invocation in drupal_flush_all_caches(), so it makes sense to move it there. The original field creation stays in hook_install() though.

Merged in #4 from #993992: _node_types_build() labels content types 'is_new' when they're not, and has static caching bugs.

@yched, it looks like although #974712: Deleting comment_body field on sole content type breaks subsequent content types touches the comment_install() code, the issues are different and don't functionally overlap. I think that issue would simply just need a re-roll to move the fix to hook_flush_caches(), if/when this patch lands.

Merged #6 from #993992: _node_types_build() labels content types 'is_new' when they're not, and has static caching bugs.

hook_flush_cache??? why not hook_module_enable? hook_module_install? Something else? Why do we need something like this?

We have a number of rebuild functions, including node_types_rebuid(), running on drupal_flush_all_caches(). And I've made sure that drupal_flush_all_caches() runs after batches of modules are installed/enabled (installer, modules page, simpletest). We could move this bundle stuff to hook_modules_enabled() and call node_types_rebuild() there, but it would result in node_types_rebuild() being called multiple times for no good reason.

hook_node_type_insert?

comment_node_type_insert() already implemented. Now it seems odd that the following isn't working: admin/modules -> drupal_flush_all_caches() -> node_types_rebuild() -> node_type_save() -> module_invoke_all('node_type_insert', $type) -> comment_node_type_insert().

Might be possible that #993992: _node_types_build() labels content types 'is_new' when they're not, and has static caching bugs is all we need to fix this bug. Will investigate.

So here's what I found: blog_install() does node_types_rebuild(), so that is when hook_node_type_insert() is fired. If comment module is not enabled yet, comment_node_type_insert() never gets a chance to run on blog.

book_install() does something a little different -- it provides a content type with node_type_save() rather than with hooks. But that is also when hook_node_type_insert() fires, and again comment can't react to it.

poll.module has no hook_install() and simply uses hook_node_info(). Yet I was still able to reproduce the missing comment body in HEAD with this type, possibly due to #993992: _node_types_build() labels content types 'is_new' when they're not, and has static caching bugs and the type getting cached as 'is_new'.

To solve all of the above, and contrib profiles which may enable comment/content modules in any order, and contrib modules which will surely be doing weird things in hook_install(), we could create comment bundles in hook_flush_caches(), as in #23. Or we could change comment_node_type_update() to create the bundle if it doesn't exist, and hope it fires. I think hook_flush_caches() is the safest way to go though, since I can say with certainty that when it fires, modules are completely installed, node types are rebuilt, and node_type_get_types() returns what's expected.

Discussed with @chx in IRC, a better solution would be a new hook to perform rebuilds, since putting this in hook_flush_caches() is not semantically or functionally very good. (but new hooks are no longer possible in RC stage)

I think the main problem lies here: node_types_rebuild() or node_type_save() called in hook_install() doesn't allow other modules to react via hook_node_type_insert(). So instead we might use hook_modules_installed()/enabled(), and check if our module is in_array(). By then the module list is complete, so reaction via hook_node_type_insert() should not be a problem.

I decided to merge this with #974712: Deleting comment_body field on sole content type breaks subsequent content types, since it contains some good cleanups for comment bundle creation, and it's easier to work on top of that.

The idea in this new patch is to use comment_modules_enabled() instead of comment_install() to create the comment body field instances. That should be much more robust, and allows the comment module to be disabled, re-enabled somewhere down the line, and missing instances will be correctly filled in. comment_node_type_update() has also been enhanced to insert a field instance if it doesn't exist, in case comment module wasn't able to react to the original firing of hook_node_type_insert().

The tests are also now improved to use a functional method of module installation, i.e. admin/modules. That's because I found that the bug was still manually reproducible with #23, so DWTC::setUp() is not enough to simulate what it's like to enable blog,book,poll on the modules page, and then enable comment in a different request.

Let's try that again...

use comment_modules_enabled() instead of comment_install() to create the comment body field instances. [...] allows the comment module to be disabled, re-enabled somewhere down the line, and missing instances will be correctly filled in

I must be missing something : if content types are created while comment.module is off, then comment_install() can create the corresponding comment field instances when comment module is re-enabled (what HEAD does). comment's hook_modules_enabled() precisely cannot, since comment module is asleep when the hook is invoked.

hook_modules_enabled($modules) only receives $modules that *are being* enabled, right ? Or is it *all* enabled modules ?

[...] then comment_install() can create the corresponding comment field instances when comment module is re-enabled (what HEAD does).

@yched, you might be confusing hook_install() with hook_enable(). hook_install() only ever fires once, unless you fully uninstall the module and re-enable it.

So, this is true and reproducible in HEAD:

1. Install with standard profile
2. Disable comment module
3. Enable book module
4. Enable comment module (hook_install() does not fire)
5. Add a book node
6. Have fun with your comment form consisting of only a "Subject" field :)

See module_enable(), which is the relevant function for all of this.

[...] comment's hook_modules_enabled() precisely cannot, since comment module is asleep when the hook is invoked.

Comment instance creation doesn't need to happen when comment module is asleep. It needs to happen when the comment module is (re)enabled, which is when comment_modules_enabled() fires. My usage of hook_modules_enabled() is similar to the traditional usage of hook_enable(), but it has this important difference:

hook_enable() fires in the midst of a loop in module_enable(), the module list is not complete and node_type_get_types() may return an incomplete list of types.

So the idea of using hook_modules_enabled() is that it's a hook that runs after the batch of modules enabled completes -- i.e. each module in $modules passed to module_enable() is processed and the module list is all up-to-date.

$modules is an array of modules being enabled at the time (if you want all enabled modules, you can use module_list()), but since all we care about is iterating node_type_get_types(), that's not relevant to our task :)

Fixed the field CRUD tests by using field_read_field('comment_body', array('include_inactive' => TRUE)) instead of field_info_instance(). Reason being field_info_instance() returns NULL if the field is disabled (apparently?), so can lead to an exception trying to create a field that exists (but is disabled).

The upgrade tests are not looking good, however :( There is a problem with:

/**
 * Enable field and field_ui modules.
 */
function system_update_7020() {
  $module_list = array('field_sql_storage', 'field', 'field_ui');
  module_enable($module_list, FALSE);
}

In the patch, this update function triggers comment_modules_enabled(), which tries to create comment body fields, which triggers some module_invoke_all()'s that the partially-upgraded system chokes on, one being that text.module isn't installed yet (so we can't create the comment body), another that the taxonomy 'machine_name' field doesn't exist yet.

The taxonomy 'machine_name' is created in taxonomy_update_7002(), but that update is scheduled to run after all the system updates though hook_update_dependencies(), and I was not successful in making system_update_7020() depend on taxo_7002(). Probably a circular update dependency somewhere.

So using hook_modules_enabled(), while being more preferred than hook_flush_caches(), is going to require some really messy update dependency stuff. Help needed!

Took some trial and error, but I found that the original taxonomy_update_7002() dependencies were put into place because 7002 used to do field CRUD. That is no longer the case. 7004 does CRUD though, so I substituted that, then added 7002 as a dependency of system_update_7020(), so we can get the 'machine_name' schema update. I think it works!

Still needs manual testing though.

I haven't had time to review this in depth, but it looks like comment module would now re-add a field instance to a node type even if it had been explicitly removed under certain circumstances. If that only happens if you disable then re-enable a module then it's probably a better trade-off than not adding the comment body per the original bug report though.

Test bot could be stalled, re-uploading #38.

re catch #39: the correct way to avoid that would be to recreate the 'comment_body' instance only on comment bundles that have no other field instances. If other instances are present and body_field is absent, you can assume it was removed intentionally.

That would require some more refactoring on _comment_body_field_create(), though, and that's enough of an edge case that I think it should be treated as a 'normal' / 'minor' followup bugfix, that shouldn't hold this patch.

The patch looks pretty reasonable to me. Maybe it was my imagination, but I thought I overheard someone saying on IRC that one way to help prevent this code from running on node types that it shouldn't would be for comment_modules_enabled() to check that 'comment' is among the passed in $modules; that way at least it only runs when comment.module itself is being enabled, not any old module. That sounded pretty reasonable on a first glance :)

Either way, comment_modules_enabled() should get a code comment explaining why that hook is the one being used.

The changes to the update dependencies look correct, and are a good step down the road of untangling that whole mess in general -- see #717834: The dependencies declared in core's hook_update_dependencies() implementations aren't actually correct :)

Hm... what's up with this?

   protected function resetAll() {
     // Rebuild caches.
+    system_list_reset();
+    module_list(TRUE);
+    module_implements('', FALSE, TRUE);
+    entity_info_cache_clear();
     drupal_static_reset();
     drupal_flush_all_caches();

With the possible exception of module_list(TRUE) (which is kind of its own beast), doesn't the combination of drupal_static_reset() and drupal_flush_all_caches() do all of that already?

@yched - yeah I can live with this as a followup, and the proposed fix sounds reasonable as well.

@David, the additions to resetAll() are just trying to mirror what is in module_enable(), since if the actual enable happens in a drupalPost, we need to reset those caches as if module_enable() was called in the test class. I'm not sure that drupal_flush_all_caches() covers entity cache, system_list, definitely not module_list, so I'll check if any of that is superfluous (although better to clear *more* caches there, than less!)

I'll also try to do if (in_array('comment', $modules))... in comment_modules_enabled(), to see if that's viable. I think as it is though, it's what is intended: "whenever the module list changes, react by creating fields, if needed". Doing it only when 'comment' is in the list might mean the update dependency stuff can be put off to #717834: The dependencies declared in core's hook_update_dependencies() implementations aren't actually correct though.

I added the if (in_array('comment', $modules)) idea, and now this patch is looking squeaky clean! I was able to simplify things, removing the update dependencies, the text.module check, and the resetAll() changes except module_list(TRUE). Now we're left with a patch that does exactly what it's supposed to do, with no hacks. I also added documentation to comment_modules_enabled() to explain why we use that instead of hook_enable().

Keep in mind that this patch has absorbed #974712: Deleting comment_body field on sole content type breaks subsequent content types, so see that issue for background. There are tests included and everything seems to be in order -- the tests alone cause an exception as that issue describes.

Great ! Happy to see that the code could be simplified. Looks completely reasonable now.
Minor nit : the testCommentEnable() test method could completely be part of the CommentFields test class.
I won't RTBC myself since I wrote part of this (the easy part...) in #974712: Deleting comment_body field on sole content type breaks subsequent content types

Note to reviewers / committers : the @todo note is intentional, and denotes code that can be removed once #915906: Deleting node type with only instance of a field leaves the field in a strange zombie state gets in.

@yched the testCommentEnable() test is better as it is (in a separate class), since it depends on $profile = 'testing' and has a custom setUp() method that bypasses parent::setUp().

@carlos8f: OK - I'm not sure the testCommentDefaultFields actually depends on CommentHelperCase::setUp(), I'm also not sure whether it could use the 'testing' profile.
Since the two tests actually test two aspects of the same feature (create the default 'comment_body' fields), they would ideally be more coupled. As I said, nitpick at this point.

I reviewed this pretty carefully and I think it's good.

To summarize how I see it:

• Essentially the main (initial) bug this fixes is that the previous code in comment_install() checked this condition before deciding to add a comment body field:

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

  That basically never worked. For content types like 'blog' and 'poll' $info->is_new is always erroneously set to TRUE (a bug to be fixed by #993992: _node_types_build() labels content types 'is_new' when they're not, and has static caching bugs), so the condition was never triggered. Meanwhile, for content types like 'book', $info->disabled is set to 0 (rather than not being set at all), so the check there was wrong also. By getting rid of those checks and doing a node_types_rebuild() beforehand so we don't have to worry about the new/disabled issue, the patch fixes that bug.

• Additionally, doing this all in comment_install() fails to create the comment body field for content types in a number of cases, so the code was moved to comment_modules_enabled() to catch all of them, as explained in the code comments.

***

The only possible issues I saw with this patch were in the tests, and were relatively minor:

+    // Try to post a comment on each node. An failure will be triggered if the
+    // comment body is missing on one of these forms.
+    $this->web_user = $this->drupalCreateUser(array('access content', 'access comments', 'post comments', 'skip comment approval'));
+    $this->drupalLogin($this->web_user);
+    $this->postComment($blog_node, $this->randomName(), $this->randomName());
+    $this->postComment($book_node, $this->randomName(), $this->randomName());
+    $this->postComment($poll_node, $this->randomName(), $this->randomName());

I'd feel more confident about this if instead of relying on a failure being triggered somewhere else, we explicitly asserted something we expected to be true (e.g. view the node and check that the comment body is displayed). The helper function might trigger a failure now and cover us that way, but that doesn't mean it always will.

Also, in the code comment "An failure" should be "A failure".

+/**
+ * Test fields on comments.
+ */
+class CommentFields extends CommentHelperCase {

Shouldn't the class name have the word "Test" (or even "TestCase") in it?

Oh, I think I take back some of what I said above. It looks like the postComment() helper function is pretty comprehensive and already does things like check for the comment body on the page. So we don't have to do it again here. Well, maybe the code comment in this patch should be a little more explicit about why we don't need to do that, then :)

This should address the reviews above.

- I merged the test classes, and rewrote the enable test to test for a slightly different scenario -- use the standard profile, disable comment module, then reproduce the bug from there (details in #35). Now we no longer need the 'testing' profile or custom setUp() method.
- In rewriting the tests, I needed to move the schema cache reset to *before* drupal_flush_all_caches() in resetAll(), which makes sense and I added comments to explain that.
- Improved comments for the assertions nitpicked in #49 :)

Yay for merged tests class, thanks :-)

#915906: Deleting node type with only instance of a field leaves the field in a strange zombie state got in, so the

+    // @todo simulate http://drupal.org/node/915906
+    field_delete_field('comment_body');

lines can go away.

Thanks, rerolled without those lines.

Thks - I'll leave David Rothstein RTBC

Yup :)

Reviewed this patch and I agree that it looks good. Glad we could leverage one of the many existing hooks. Committed to CVS HEAD. Thanks.