When you remove the body field from a node type via admin/structure/node-type/my-type/fields, and then you go to admin/structure/node-type/test-type and re-save the settings, the body field gets added back to the node.

CommentFileSizeAuthor
#26 node.body_.patch3.54 KBAnonymous (not verified)
#24 node.body_.patch3.6 KBAnonymous (not verified)
#21 node.body_.patch2.62 KBAnonymous (not verified)
#12 569206_zombie_body_field-12.patch1.35 KBmikey_p
#9 569206_zombie_body_field-9.patch1.2 KBmikey_p
#8 569206_zombie_body_field-7.patch762 bytesmikey_p
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

becw’s picture

has_body is still stored in the {node_type} table; is it still considered a fundamental node property? -- I think it should not be, but that a 'body' field should still automatically be added on new content types.

aries’s picture

I can confirm this bug.

ronald_istos’s picture

While looking at this - http://drupal.org/node/822128 - came across this bug as well.

Setting this to critical because it sounds like it is pretty serious.

ronald_istos’s picture

Priority: Normal » Critical
Anonymous’s picture

this seems like an easy fix, but there looks to be a broader bug here.

in node_type_form_submit() there's an unconditional call to

  node_add_body_field($type);

so we need to make that conditional on whether the type is new or not. however, looking at $type->is_new, i see it set to 1 even when editing an existing type. so either i'm completely misunderstanding $type->is_new, or we need to fix that so we can reliably use $type->is_new to figure out if we should be adding a body field or not.

mikey_p’s picture

Assigned: Unassigned » mikey_p
mikey_p’s picture

Status: Active » Needs review
FileSize
762 bytes

That was easy.

mikey_p’s picture

I don't see where new $type->is_new is used at all in content_types.inc, but I suppose that since it's passed through hook_node_type_update(), this should be fixed in node_type_save().

Ignore #8 and use this instead.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, it was a silly error in the first place so I don't think we need a test for it.

YesCT’s picture

Status: Reviewed & tested by the community » Needs review
+++ modules/node/node.module
@@ -527,6 +527,7 @@ function node_type_save($info) {
+    $type->is_new = 0;

does this set it to zero to mean that it is "not new"?

is_new is true and false??

67 critical left. Go review some!

(wish Dreditor would let me click on a file name to see the whole file)

mikey_p’s picture

Yeah, I'm getting tired, and this approach is more logical.

Anonymous’s picture

Status: Needs review » Needs work

the patch fixes the problem for me, but we need a test, because we only found out when a user clicked around to find a broken bit :-(

also, the $type->is_new stuff is icky. i think making node_type_set_defaults() set this to '0' will cause new module content types to not be saved properly in node_types_rebuild():

   foreach (node_type_get_types() as $type => $info) {
    if (!empty($info->is_new)) {
      node_type_save($info);
    }
    if (!empty($info->disabled)) {
      node_type_delete($info->type);
    }
  }

we've got an overloaded term here - $type->is_new currently means "is a new definition from a module we need to add to the database".

so if you guess, like i did, that its a useful flag for "this is a new type being saved to the database directly from a form", you'd be wrong. sad panda.

Anonymous’s picture

ouch, yeah, we don't have a test for admin/structure/types/add that i can find.

also, i can't find a test that creates a module that implements hook_node_info(), which explains why tests pass while i suspect content type creation from newly installed modules will be broken with the patch.

mikey_p’s picture

So Drupal 7 copies defaults defined in hook_node_info() into the DB? This is new to me, and another reason why #535122: Integrate CTools export.inc into core for declaring instances of objects had gotten in and that hook_node_info had been re-written to use it.

Maybe the simplest way would be for _node_types_build() to add the check for is_new to all types from the hook, and the overwrite that when merged with DB types.

Anonymous’s picture

awesome, so its broken in different ways. after installing book and blog:

mysql> select type, name from node_type;
+---------+------------+
| type    | name       |
+---------+------------+
| article | Article    |
| book    | Book page  |
| foo     | foo        |
| page    | Basic page |
+---------+------------+
4 rows in set (0.00 sec)

because book_install() explicitly calls node_type_save(), so we have a {node_type} row, whereas blog_install() doesn't, so we don't.

this close to the end of the cycle, is it a bad idea to open tickets to clean this stuff up?

Anonymous’s picture

mikey_p: i think maybe your first patch is the way to go, just using the return value from node_type_save(), but maybe take out the line that sets $type->is_new to zero.

that is_new flag is meant (currently) for something else, so we should probably just leave it as a smelly non-obvious don't-touch-coz-its-not-what-you-think-and-stuff-may-break piece of code for now.

that ctools stuff looks interesting. personally, i'd like to see us move away from the info hook being used at any time other than module install for D8. (and this goes for other stuff, like hook_permissions()...).

mikey_p’s picture

Status: Needs work » Needs review

Justin, yeah I started down the road to trying to clean this up by adding a form value in the node_type_form (to be used in node_type_form_submit()), and then setting is_new explicitly in _node_type_build() for the items fetched from hook_node_info... The problem also seems to stem from that fact that we have two sets of places where 'new' is described, type->is_new and then the flags in node_type_save. Maybe a better followup patch would rename is_new to is_default or something like that?

But yeah I think just moving the node_add_body_field($type); in node_type_form_submit() is the best way to go. That patch is actually posted in #8 so please review that.

Edit: And yes this still needs tests, we have a few basic ones in NodeTypeTestCase::testNodeTypeEditing() but adding a default hook implementation to node_test module would be good as well, I'll try to wrap that up today.

YesCT’s picture

#8: 569206_zombie_body_field-7.patch queued for re-testing.

YesCT’s picture

as mentioned in #18, #8 is the one to review (so requested bot try it again), and tests are needed (so needs work too).

Anonymous’s picture

FileSize
2.62 KB

attaching a patch with a test that fails against HEAD for this bug. when this test passes, we've fixed the bug.

Status: Needs review » Needs work

The last submitted patch, node.body_.patch, failed testing.

Anonymous’s picture

Status: Needs work » Needs review

reintegrated mikey_p's patch from #8, failing test now passes locally.

Anonymous’s picture

FileSize
3.6 KB

oh, um, with the patch this time.

mradcliffe’s picture

I like this. Should the + $this->assertNoRaw('Body', t('Body field was found.')); read "Body field was not found" or "Body field does not exist" or "Body field remains deleted" instead?

Anonymous’s picture

FileSize
3.54 KB

@mradcliffe - thanks for the review. attached patch makes the change suggested in #25.

mikey_p’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Fixed

This looks like a nice bug-fix, with tests to prove it works. Thanks!

Committed to HEAD.

asifnoor’s picture

I have placed the fix and it is working fine. The code in elseif ($status == SAVED_NEW) block is being executed only when a content type is created initially.

Status: Fixed » Closed (fixed)

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