Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Configuring these should be done in the same field UI we configure other content type fields, not duplicated on the content type edit form.
Comment | File | Size | Author |
---|---|---|---|
#162 | 553306-fu-node-type-162.patch | 555 bytes | pwolanin |
#147 | node_type_gutting-553306-147.patch | 2.26 KB | plach |
#141 | node_type_gutting_4.patch | 19.19 KB | catch |
#139 | node_type_gutting.patch | 22.92 KB | catch |
#137 | node_type_gutting.patch | 19.2 KB | catch |
Comments
Comment #1
yched CreditAttribution: yched commentedNode title is not a Field yet -
and probably won't be within the current freeze deadline.[edit: let's not be negative ;-) catch just opened a candidate thread on #553372: Convert node->title to a field]Also, this is for node.module, not Field API / UI.
Comment #2
becw CreditAttribution: becw commentedComment #3
becw CreditAttribution: becw commentedThis patch removes the body UI element on the node type's settings page.
Something I noticed while looking at this code is that the node module code still talks about $type->has_body. Also, when you remove the body field via the "manage fields" tab, and then go back to the node type settings page and re-save the settings, the body field gets re-added. Clearly, this should not happen.
Comment #5
becw CreditAttribution: becw commentedI'm a core patch newbie; forgot to make the patch from drupal root. Should be fixed this time around.
Comment #6
yched CreditAttribution: yched commentedThanks for stepping in, bec !
This probably extends a bit further:
- the has_body / body_label properties on node types can now disappear completely
- the corresponding columns on the {node_type} table can be removed from the schema, and be dropped at the end of node_update_7006()
node_configure_fields() can go away:
- the 'body' $field should be created when node module is installed (it seems currently system_install() is preferred over node_install() for this kind of stuff, not sure why), and at the beginning of node_update_7006()
- the instances should be created in node_type_save() when saving a new type, and in node_update_7006() instead of the current code under
// Re-save node types to create body field instances.
. I guess a helper function to build and return the $instance array could be helpful here.Comment #7
catchsubscribing.
Title as field is very close to RTBC by the way, so we can hopefully kill both of these.
Comment #8
becw CreditAttribution: becw commented@yched, I was looking at those canges as I made this patch, but I held back because the scope is pretty far beyond 'body field UI' :) Maybe when 'title as field' is in, we can kill the old body and title UI, the node object properties, and the schema changes in one fell swoop.
Comment #10
webchickLet's blow 'em both away. I'm within seconds of committing #557292: TF #3: Convert node title to fields.
Comment #11
becw CreditAttribution: becw commentedOk, I'm ready to jump back in on this unless someone else is already in motion. Should I include all the items from #6 in a patch here?
Comment #12
webchickYep, that sounds good. Let's finish it all off in this one patch.
Thanks for your help, bec!
Comment #13
becw CreditAttribution: becw commentedThere are a couple questions I had while making this patch, but it's ready for a review.
- updates went into the node_update_7006(), which leads to some funkyness
- tests fail, I think because they expect to set title and body labels through the old UI -- should I try to update these?
- updates to docs on hook_node_form() need a second set of eyes
- title and body fields are automatically added to content types in node_type_form_submit(), rather than in node_type_save()--I feel like automatically adding title/body fields is a usability thing (give users something to poke at right away on content types created through the UI) rather than an API feature (if I'm generating a content type, don't make me delete the body field afterward). On the other hand, this makes it more complicated for developers to get the default title field.
Comment #15
becw CreditAttribution: becw commentedIn the last patch I forgot to add title/body fields to article/page content types in the default install profile. This is necessary because of bullet point #4 above.
Comment #17
becw CreditAttribution: becw commented- since everything expects a title and body by default, give it to them (code now matches #6, NOT #13)
- update fn 7006 now uses original title & body labels (oops)
- removed node.tests that examined title_label and body_label node properties.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedI reviewed the patch code style and comments twice and also applied it and verified the UI changes with me own eyes.
I know that the original wasn't wrapped at 80 characters, but can the change be wrapped at 80?
Other than this one point, I find no other issues with this patch.
I'm on crack. Are you, too?
Comment #19
catchRead through the patch, everything looks peachy. Re-rolled for comment wrapping. RTBC. Don't credit on commit please.
Comment #21
catchHEAD broken.
Comment #22
becw CreditAttribution: becw commentedAnyone have input on adding to the preexisting update function node_update_7006()? Is that ok?
Comment #23
catchMy patch was actually malformed somehow, just happened to see 3-4 issues with HEAD not installing in the same five minutes. This one should work, if it doesn't, let's commit #17 since I'm doing more harm than good at this point trying to fix a comment :(
Re-using the existing node_update_* is fine, and in general a lot better than adding a new update right up until beta/rc stage.
Comment #25
becw CreditAttribution: becw commentedre-rolled to work with HEAD.
Comment #26
becw CreditAttribution: becw commentedComment #28
becw CreditAttribution: becw commentedRe-rolled. Now includes 'object_type' instance property from http://drupal.org/cvs?commit=275208
Comment #30
becw CreditAttribution: becw commentedThe last patch failed translation tests; on the surface, this is fixable by giving the 'translatable' flag to the title and body fields (which I forgot). But then I run into an issue in node_save(): the title is expected in $node->title[FIELD_LANGUAGE_NONE][0]['value'], but they aren't always there when dealing with translations. So now this patch depends on #571654, Make title field translatable.
This latest patch flags title and body fields as translatable, but it won't pass testing until #571654 is in.
Comment #32
webchickTitle fields are now translatable. Any chance of giving this another poke? :)
Comment #33
catchStraight re-roll to see what the bot says.
Comment #35
heather CreditAttribution: heather commentedFWIW - i just tested this patch with a ui-text change and it worked fine #614316: improvements to body field label and submission guidelines.
In fact, seemed like a big improvement, getting rid of body field.
Comment #36
webchickI think testing bot got stuck. Re-uploading patch from #33.
Comment #38
webchickHm. At least one of those test failure hunks looks legit. Not sure about all the translation ones. But in any case, looks like this still needs work.
Comment #39
yched CreditAttribution: yched commentedAttached patch should fix the field_ui tests. locale related failures are above me :-(.
Comment #41
yched CreditAttribution: yched commentedOther than that:
Hm, 'has_title' and 'has_body' columns have been removed from the schema, and node_type_get_types() is not supposed to return this data anymore, so I'm not sure it's safe to use them here. We might need to query the {node_type} table directly (since the values are still in there at this point)
typo 'descrbe'
if the function returns *instance* arrays, it should be named node_type_default_field_instances() ?
Should be without t(), we can't have the saved label depend on the language that happens to be used when the node type is created. Translation of field labels is still not addressed - see #549698: Prepare field label and description for DDT (translatable queries). For now, with 'body and title as field', we've lost the translatability of the 'Body' and 'Title' labels...
This review is powered by Dreditor.
Comment #42
yched CreditAttribution: yched commentedDrupalWebTestCase::drupalCreateContentType() also has
Those can be removed. The patch already takes care of all tests that actually call drupalCreateContentType() with those options.
Comment #43
becw CreditAttribution: becw commentedUsing has_title and has_body properties in node_update_7006() is ok (and necessary), since they aren't removed from the {node_type} table until the end of that update function; they're present in the node type arrays that get loaded initially, since that data is loaded with whatever fields are present in the {node_type} table.
Comment #44
becw CreditAttribution: becw commentedHere's a version of the patch with the changes suggested above. Some things still need fixing--tests and API use by at least forum.module and poll.module. The failures shown by the test bots are mostly because of reliance on the title_label & friends node properties. Also, in the simpletest module itself, the base test class method drupalCreateContentType() still relies on these node properties, and drupalCreateNode() expects title and body fields (which might be a good thing, I don't know...?)
I can come back to this tomorrow night, but I will need some help on the tests.
Comment #46
yched CreditAttribution: yched commentedTrue in the current state of the node_type_get_types() function. My worry is that since returning 'has_title' etc. is not part of the 'contract' anymore, this might very well be inadvertently changed by some internal refactoring in the future, say in D7.12 when we all have forgotten about this update, and then the update breaks. API functions in update hooks can be tricky, sometimes you just can't use them.
Ah, true. Currently it's a feature of drupalCreateContentType() to be able to create a node type without title or body field.
As I wrote in #42, few tests use that function currently - actually, only one: the one that precisely tests the UI we are removing, and the patch rightfully removes those asserts already.
Not sure we really need to keep that feature for drupalCreateContentType() - however, this outlines the fact node_type_save() *will* create body and title instance no matter what. UI-created node types will automatically have a title and body, that's cool, but we probably need more flexibility at the API level.
So maybe : keep 'has_[body|title]', '[body|title]_label' in drupalCreateContentType() and node_type_save(). Those properties are not saved in the database, they just control what node_type_save() does around node_type_default_field_instances(). Means we also keep the hunk in node_type_set_defaults() - just remove the t() around 'Body' and 'Title' labels.
We're good IMO. It fills in default values for body and title if not provided, and saves the node. If the node type happens not to have title or body, they just won't be saved. Then if a test expects to find a body a title back later on, it fails, but that means the test is buggy to begin with.
Comment #47
yched CreditAttribution: yched commentedPatch doesn't apply because the context below the
function node_type_default_field_instances() {
hunk is incorrect. No recent commits in that area AFAICT, so I'm not sure what happened and I'd prefer not to reroll myself and mess something up - could you reroll, bec ?Comment #48
zzolo CreditAttribution: zzolo commentedI have rerolled the patch:
* Updated use of strtolower to drupal_strtolower in tests.
Comment #49
zzolo CreditAttribution: zzolo commentedupload wrong patch. :)
Comment #50
zzolo CreditAttribution: zzolo commentedComment #52
zzolo CreditAttribution: zzolo commentedUgh. My other patch was in there too. New patch; this time should be good. Maybe its time for me to go to bed. :)
Comment #54
zzolo CreditAttribution: zzolo commentedWell, there are a number of failures, but the patch applies now. I believe it is in the node_save() function. For instance when then the Locale modules tries to test saving a node after changing the language, we get a failure. And performing it manually, I get the following errors:
I believe it has something to do with the following structure:
But I do have to go to bed now.
Comment #55
becw CreditAttribution: becw commentedre-rolled, changed use of node_type_get_types() in node_update_7006(). I guess now I have zzolo's patch to look at...
Comment #56
yched CreditAttribution: yched commentedComment #57
zzolo CreditAttribution: zzolo commentedFYI, I just re-rolled your patch. The only thing I changed was using drupal_strtolower() instead of strtolower()
Comment #59
yched CreditAttribution: yched commentedDifferences between #52 and #55 are the update_7006 fixes in #55 and strtolower() -> drupal_strtolower() in #52.
So let's continue with #55, the strtolower changes can be added back when the patch is rerolled (although I don't think it makes a real difference for 'az_' strings.
Notes on #55:
Comment is misleading. We don't create them yet.
yay ;-) Though it would be more concise to just say
->fields('node_type')
For the same reason, we don't want this here; below we'll use ->has_body (we'll also need ->has_title, bug in the current code. Not strictly related to this issue, so I'll fix that in a followup)
I'd suggest storing 'has_body' and 'has_label' properties for each node type in $context while we loop over the query results. Something like:
Then we can reuse that information in the rest of the function.
BTW, strictly speaking, in the whole function, the $context param should really be named $sandbox ($context is for generic batch operations, but update hooks only receive the 'sandbox' part of it). Error was not introduced by this patch, so it can be fixed in a followup if you prefer.
$found is either TRUE or FALSE, so I don't think that change is needed ?
This review is powered by Dreditor.
Comment #60
yched CreditAttribution: yched commentedAnd thanks for sticking with this, bec, that's really appreciated !
Comment #61
becw CreditAttribution: becw commentedThe change to checking $found change IS needed, because on the first run of the update, $found is not set (=> false-y), but we're not done so we don't want $context['#finished'] = 1;
Comment #62
yched CreditAttribution: yched commentedHm, if I read correctly, that
if (!$found) {
is in the code branch run only on 'Subsequent invocations'. We'll always go through$found = FALSE;
before running it.Comment #63
becw CreditAttribution: becw commentedoops, my bad. Thank YOU for your persistent reviews :)
Here's a patch with the latest round of comments fixed, I'm about to dive into fixing the tests that are failing. I also included zzolo's drupal_strlower changes, though they aren't specifically related to these node type properties...
Comment #64
yched CreditAttribution: yched commentedA couple fixes related to API changes in #470242: Namespacing for bundle names:
should be field_info_instance('node', 'title', $type->type)
should be field_info_instance('node', 'title', $node_type->type)
should be field_info_instance('node', 'title', $node_type->type)
+ it seems your latest patch was not rolled correctly, node.module and node.test have strange incorrect hunks:
[fixed - edited out for readability's sake]
Maybe you should reapply #55 on a fresh HEAD but keep node_update_7006 from #63 ?
I'm on crack. Are you, too?
Comment #65
yched CreditAttribution: yched commented[fixed - edited out for readability's sake]
Comment #66
becw CreditAttribution: becw commentedOK, I'm so sorry I fail at patches tonight. This one is re-rolled from a fresh repo, with the field_info_instance() usage changes mentioned in #64. *Now* I'll address those failing tests.
Comment #67
yched CreditAttribution: yched commentedFWIW, the LocaleContentFunctionalTest fail seems caused by the fact that field_multilingual_available_languages() doesn't return the same array with and without the patch. Trying to track this down.
Comment #68
yched CreditAttribution: yched commentedDOH. Answer was *so* simple :-).
node_type_default_field_instances() sets 'translatable' to TRUE for the title field. Current HEAD doesn't, title is not translatable yet.
Let's see how this one does.
Comment #72
becw CreditAttribution: becw commentedThat's funny, #68 passes all the locale tests on my local machine (in 36 minutes and 7 seconds, haha).
Comment #73
webchickHm. I can reproduce here, (un)fortunately.
I can't really figure out why these tests succeed without this patch; it doesn't look like anything ever goes to the body field settings and toggles on its translatable property. Was this taken care of automatically in the old way by the node type's translatable property?
Comment #74
yched CreditAttribution: yched commented@webchick: the body field is created as translateble in node_configure_fields() (HEAD) and in node_type_default_field_instances() (patch)
Comment #75
yched CreditAttribution: yched commentedI marked #504564: Make summary length behave with fields as dependant on that one.
Comment #76
yched CreditAttribution: yched commentedRemaining failures were by stale entity info.
- Translatable field handling depends on information stored in the cached entity_info.
- In current HEAD, node_type_save() always updates the body and label instances to update the labels. field_update_instance() triggers a refresh of the entity cache.
- With the patch, node_type_save() doesn't call field_update_instance() when updating an existing node type. No cache rebuild, test runs with stale entity info and fails.
Attached patch forces a clear of the entity cache info at the end of node_type_save(). Only makes sense.
Entity API offers no clean way to clear that info, unfortunately. RDF module, field module, system_modules_submit() currently do:
Ugh. Not for this patch to change, though.
+ rerolled after #619230: Update 'default field value' UI code for D7 Field API (field_ui.test changes)
Comment #77
yched CreditAttribution: yched commentedSide issues:
#642612: No clean way to reset entity_info cache
#642614: locale_add_language() should reset entity_info cache
Comment #79
yched CreditAttribution: yched commentedBleh, forgot some hunks while updating field_ui.test.
Comment #80
yched CreditAttribution: yched commentedOMG. Green.
So, last point as far as I'm concerned - from my #46 above:
With the removal of 'has_body' / 'has title', node_type_save($new_node_type) *will* create a type with body and title instances no matter what. UI-created node types will automatically have a title and body, that's cool, but we probably need more flexibility at the API level.
So maybe : keep 'has_[body|title]', '[body|title]_label' in node_type_save() (and drupalCreateContentType() ?). Unlike current HEAD, those properties are *not* saved in the database, they just control what node_type_save() does around node_type_default_field_instances(). Means we also keep the hunk in node_type_set_defaults() - just remove the t() around 'Body' and 'Title' labels.
Opinions anyone ?
Comment #81
catchSounds good to me.
Comment #82
webchickYeah, we definitely need to retain the programmatic ability to create nodes without bodies/titles.
AND OMG IT'S GREEN!!!!!!!!!!!!!!!! :D So exciting.
Comment #83
yched CreditAttribution: yched commentedbec, can I leave that to you ? ;-)
Comment #84
yched CreditAttribution: yched commented#642612: No clean way to reset entity_info cache has been committed, so we can replace the
in node_type_save() by
+ back to CNW.
Comment #85
becw CreditAttribution: becw commentedfyi, just checked this and am now working on it.
Comment #86
becw CreditAttribution: becw commented- kept 'has_[body|title]' and '[body|title]_label' in node_type_set_defaults(), and use them in node_type_save()
- reset entity_info cache the new way
Comment #88
moshe weitzman CreditAttribution: moshe weitzman commentedAdd tag. This patch will remove a major error in upgrade path.
Comment #89
yched CreditAttribution: yched commentedThis issue is doomed, obviously.
Now that node titles are back to non fields, retitling.
Will try to update the patch with the new restricted scope ('body' only) asap.
Comment #90
yched CreditAttribution: yched commentedStill needs a little refactor, but let's see where tests stand.
Comment #92
yched CreditAttribution: yched commentedTrue. What about that one ?
Comment #93
Dave ReidNeeds to have the body title summary removed from modules/node/content_types.js
Comment #94
yched CreditAttribution: yched commentedVT summary in modules/node/content_types.js: true, done (although it doesn't seem to be included right now ?)
This should fix the tests. CNR for the bot, but still needs minor internal refactoring.
Comment #95
gopherspidey CreditAttribution: gopherspidey commentedsub
Comment #96
mcrittenden CreditAttribution: mcrittenden commentedSub.
Comment #97
sun.core CreditAttribution: sun.core commented#94: node_type_gutting-553306-94.patch queued for re-testing.
Comment #99
mradcliffeFound this issue deep in the critical issues list, and noticed it had a couple of testing bumps. I modified the patch so it should patch now. I don't know what the refactoring in #94 refers to so it's pretty much the same patch as #94.
field_ui.test: there's no deleteField anymore as it got refactored into testDeleteField.
I hope this helps.
Comment #101
mradcliffeAwesome, I love it when testing works.
Anyway the locale test on 1841 seems to be failing because the index is 'und' (LANGUAGE_NONE) instead of 'en'. The test sends in
$language['en']
when posting, but sends the $body as$edit['body'][LANGUAGE_NONE][0][value]
. This seems to be appropriate though as if it's changed to 'en' instead of LANGUAGE_NONE it explodes.edit: edited out some assumptions.
Comment #102
mradcliffeFurther debugging reveals that it's the changes in node.module that's causing the problem. Most likely due to the change from
node_configure_fields
tonode_type_default_field_instances
(and code surrounding that method call).If I reverse just the node.module part of the patch (commenting out the has_body, etc... fields) it passes all tests.
Comment #103
mradcliffeOkay, this should pass the locale test by adding the same logic when creating a field for existing fields, and instead uses the update field instance (part of field CRUD).
I'm not sure if this is correct. In #3 there was a plan to get rid of
node_configure_fields
, which is essentially doing the above without the duplicate code blocks. It would be easier and more efficient to put this function back in if that's the direction that we need to move in.Otherwise we need to investigate why we need to update field instance to get language to work correctly as noted in my debugging in #102.
Comment #104
mradcliffeErr, whoops. Too tunnel visioned I guess. :)
back to #99
Comment #105
mradcliffeLet's try again. I'm having an issue with color.test on my local machine, but I can't see how anything in this patch could be causing it.
incoming edit: Okay, color thing was just on my end. We're back to the duplication I mentioned in #103 about whether to revert back to node_configure_fields.
Comment #106
mradcliffeNote: the changes so far cause the patch in #13 of #682552: FieldException when trying to enable blog and comment modules for first time that calls node_types_rebuild() to not work as a solution ofr that issue.
Comment #107
catchI think I'd prefer to keep the instance handling self contained in a (possibly renamed) node_configure_fields() as opposed to doing it directly in node_type_save(). Note there's tabs in node_type_save() in the latest patch.
I'm a bit confused why there are still lines for ->has_body in this patch too - shouldn't that be completely removed?
Comment #108
yched CreditAttribution: yched commentedre catch : see #80 and #82.
Comment #109
catchOK that makes sense for adding the instances, but not for has_body and friends still appearing in node_field_extra_fields(), looks like they should be dropped from there no?
Comment #110
catchActually no.
Why do we need to create instances automatically in node_type_save() at all? We can just create the instances in the new content types submit form instead, then we don't have to track $has_body as well. Any code adding new content types programmatically should have to add the instance itself, which we have a nice API function for in field_create_instance(). We could add a wrapper around that - something like node_create_body_instance($type); for additional convenience if we like.
Comment #111
yched CreditAttribution: yched commentedNote that has_body doesn't appear in node_field_extra_fields(), it's just an optical illusion from adjacent patch hunks:
Other than that, suggestion in #110 sounds good. We might want to have simpletest's drupalCreateContentType() method support optional automatic creation of the instance, though.
Powered by Dreditor.
Comment #112
catchRe-rolled with changes suggested in #110. Added it to drupalCreateContentType() with no option, since if we really needed a type with no body there's always node_type_save() itself.
Comment #114
catchFixed most tests except locale, nearly all down to the uselessness which is hook_node_types(). Didn't see a way around that except for either stopping blog and forum from using it altogether and moving them to node_type_save(), or the hacks I added to $module_install().
Locale test I'm not entirely sure what's supposed to happen there, so leaving for now. Back to CNR to check that's the last failure though.
Comment #116
catchSpoke to plach, if you also apply #632172: Node language and field languages may differ then this one passes locale tests. So CNR for a code review.
Comment #117
mradcliffeOver in #682552: FieldException when trying to enable blog and comment modules for first time there was discussion about calling node_types_rebuild() in hook_install being ugly.
I'm going to try to take a deeper look into this when I can. Thanks.
Comment #118
catchFully agreed it's ugly. With the node_access() changes in D7, I'm not sure there are many valid use cases remaining for hook_node_type(), so I'd be quite into getting blog and forum to switch to node_type_save(), which would remove the need for node_types_rebuild(), however I didn't yet look to see if anything in particular would be broken by that.
Comment #119
plach#114: gut_node_types.patch queued for re-testing.
#632172: Node language and field languages may differ has been committed, let's retest this.
Comment #120
catchRe-rolled for some js changes.
Question remaining is whether we swap the node_types_rebuild() for creating a content type with node_type_save() for blog and forum modules.
Comment #122
catchLast patch was missing blog.install, no other changes.
Comment #123
mradcliffeAh. I was wondering if we needed to add blog.install on Sunday. I keep not having a chance to sit down for several hours to work on thinking about how not to run node_types_rebuild in install files for this case.
Things to test: enable forum, blog, comment, book, etc... at the same time.
Comment #124
mradcliffeIs
node_add_body_field()
redundant when looking atnode_type_default_field_instances()
? The only place it's used is in node.install and in node_update_7006. It would make sense to eliminate one of these. Personally I'd air on the side of the latter as it could be extensible later on in D8 (adding more default fields & instances via a hook).I think we should implement whatever in
node_node_type_insert
to add default field instances. And then when new node types are built the default fields are added. We wouldn't need to add .install files for d7 this late. node_types_rebuild would run anyway for now.Trade-off: no default field instances on install time.
Edit: Okay, theory is possible. But a bit more work then I thought. Book module doesn't have a hook_node_info and /only/ declares in .install. Ugh.
Comment #125
catchThe main idea here is not to add default field instances for people creating custom node types, only for those created via the UI - so you don't get into a situation where you have to remove instances created by core. But yeah agreed those two functions are largely duplicated, and lets keep node_type_default_field_instances() out of the two.
Comment #126
tutumlum CreditAttribution: tutumlum commentedsub
Comment #127
catchRemoved node_add_body_field().
Comment #129
catchLooking at the two, node_add_body_field() is actually what we want, so I removed node_type_default_instances() instead. If and when we add another default field, we can decide what to do, but for now that's the most descriptive name.
Comment #130
yched CreditAttribution: yched commented+ 1 on the latest approach - sorry, that's all the amount of reviewing I can deliver currently :-/.
Comment #132
catchblog.install was missing the change.
Comment #133
plachWeird string concatenation.
(OT) Maybe I'm missing something but I can't see this behavior neither with the patch applied nor without.
I tested the patch and found that it works as expected. I cannot give a feedback on design, but it has yched's ok.
Aside from the minor string issue above it looks RTBC.
Powered by Dreditor.
Comment #134
catchThose hunks don't seem to have anything at all to do with this patch, so I just removed them.
The js issue, I have no idea what that's supposed to do and don't fancy trying to fix it here either.
chx pointed out in irc that the $sandbox/$context rename is killing kittens, agreed, but that's also been in the patch for some time and I don't see a particular reason to take it out given it's the same hunks.
Comment #135
plachI think it's the code providing the summary for the vertical tab. I can't see any summary appearing. I'll give a look if there is an open issue for this.
I think we want also a
$label
parameter here, otherwise we'll lose any label different from 'Body' during the upgrade path.Missing
$label
parameter.'Body'
should be only the default value.Powered by Dreditor.
Comment #136
plachBtw while reviewing I discovered #782846: Body field language is not properly set during the upgrade path.
Comment #137
catchShould address #135.
Comment #138
plachI guess this should be
$node_type->body_label
:)Feel free to set RTBC after fixing that if the bot is happy.
Powered by Dreditor.
Comment #139
catchouch, fixed.
Comment #140
plachnope, it seems the same patch with some extra stuff on the top :(
Comment #141
catchArggh, sorry.
Comment #142
plachThe patch looks good and behave as expected, has yched's approval on the Field API side and tests pass.
Comment #143
webchickThis looks both yummy and delicious, but unfortunately no longer applies.
Comment #145
asimmonds CreditAttribution: asimmonds commentedLooks like some of #141 was committed in http://drupal.org/cvs?commit=363156 and catch's patch in #144 is broken.
Comment #146
catchWhoops, I deleted #144 because that was clearly just the wrong patch, this explains why it was coming up weird though.
The conflict is in node.install, blog.install is a new file which was missing from that other commit.
Either we need a rollback of the partial commit so that #141 can be re-rolled properly, or someone needs to figure out what's left.
Comment #147
plachThis should be what was left out of #141: just
blog.install
and parts ofnode_update_7006()
.Comment #148
catchLooks good, similar hunks to my aborted patch except this one's not broken. I'd say wait for green before commit but without blog.install tests should be broken.
Comment #149
plachYes, head is broken due to the missing blog.install.
Comment #150
webchickOh, crap. :( I'm sorry! This will teach me to commit patches after 3am... ;P
OK, hopefully the testbot will talk to us again now. :)
Thanks so much for everyone's work on this!!! YAY!
Comment #151
plach@webchick:
It's not you, it's the evil Curse of the Redundant Body UI (tm)
Comment #152
webchickThat's right, damn it! ;)
Comment #153
webchickLeft a note over at #771922-6: Strings cleanup to help some poor schmuck trying to figure out where this code actually came from.
Comment #154
webchickOops. Now that I actually read this patch, thanks to rfay reporting that it broke Examples for Developers module, there's quite a bit here that needs documenting in the module upgrade guide:
- no more has_body/body_label attributes
- now have to call node_add_body_field() to get the body field to show up
I'm curious why that second one is required. That seems like a very invasive change to make this late in the cycle.
Comment #155
webchickHm. I guess, since there's no longer a way to "opt out" otherwise...
Comment #156
catch@webchick #155, yes that's why.
Documented at http://drupal.org/update/modules/6/7#body_field
Comment #157
rfayTrying to sort this out to improve the docs (and fix Examples module).
Unfortunately, this patch also seems to require a Drupal reinstall. Shucks.
Without the reinstall... I get this error about has_body not having a default value on module install:
With a reinstall all is well. Shucks. I'm looking forward to not having the reinstall flag played any more.
Comment #158
catch@rfay: you might be interested in http://drupal.org/project/head2head, although no update for this yet.
Comment #159
rfayIn case it's useful to anybody, the report on what I had to do (or at least think I had to do) to get node_example.module and simpletest_example.module going again is in #755640-17: HEAD Broken on node_example module - core shift?.
I'm certainly hoping that one of you participants will write an issue summary here and an official "here's how you change your code after this patch". I will post it to the Development list. Otherwise, they're going to get my limited view of it.
Comment #160
rfayChanging the title. If you can say it better, please do.
Comment #161
pwolanin CreditAttribution: pwolanin commentedhead2head issue: #799188: update for node body field API changes at #553306
Comment #162
pwolanin CreditAttribution: pwolanin commentedI think there is #fail in the API call to node_add_body_field().
see: http://api.drupal.org/api/function/node_add_body_field/7
Comment #164
catchThis was found and fixed in another issue.