Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#26 | node.body_.patch | 3.54 KB | Anonymous (not verified) |
#24 | node.body_.patch | 3.6 KB | Anonymous (not verified) |
#21 | node.body_.patch | 2.62 KB | Anonymous (not verified) |
#12 | 569206_zombie_body_field-12.patch | 1.35 KB | mikey_p |
#9 | 569206_zombie_body_field-9.patch | 1.2 KB | mikey_p |
Comments
Comment #1
becw CreditAttribution: becw commentedhas_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.
Comment #2
aries CreditAttribution: aries commentedI can confirm this bug.
Comment #3
ronald_istos CreditAttribution: ronald_istos commentedWhile 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.
Comment #4
ronald_istos CreditAttribution: ronald_istos commentedComment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedthis 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
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.
Comment #6
mikey_p CreditAttribution: mikey_p commentedComment #8
mikey_p CreditAttribution: mikey_p commentedThat was easy.
Comment #9
mikey_p CreditAttribution: mikey_p commentedI 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.
Comment #10
catchLooks good, it was a silly error in the first place so I don't think we need a test for it.
Comment #11
YesCT CreditAttribution: YesCT commenteddoes 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)
Comment #12
mikey_p CreditAttribution: mikey_p commentedYeah, I'm getting tired, and this approach is more logical.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commentedthe 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():
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.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedouch, 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.
Comment #15
mikey_p CreditAttribution: mikey_p commentedSo 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.
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commentedawesome, so its broken in different ways. after installing book and blog:
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?
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedmikey_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()...).
Comment #18
mikey_p CreditAttribution: mikey_p commentedJustin, 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.
Comment #19
YesCT CreditAttribution: YesCT commented#8: 569206_zombie_body_field-7.patch queued for re-testing.
Comment #20
YesCT CreditAttribution: YesCT commentedas mentioned in #18, #8 is the one to review (so requested bot try it again), and tests are needed (so needs work too).
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedattaching a patch with a test that fails against HEAD for this bug. when this test passes, we've fixed the bug.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedreintegrated mikey_p's patch from #8, failing test now passes locally.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedoh, um, with the patch this time.
Comment #25
mradcliffeI 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?Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commented@mradcliffe - thanks for the review. attached patch makes the change suggested in #25.
Comment #27
mikey_p CreditAttribution: mikey_p commentedComment #28
webchickThis looks like a nice bug-fix, with tests to prove it works. Thanks!
Committed to HEAD.
Comment #29
asifnoor CreditAttribution: asifnoor commentedI 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.