Configuring these should be done in the same field UI we configure other content type fields, not duplicated on the content type edit form.

CommentFileSizeAuthor
#162 553306-fu-node-type-162.patch555 bytespwolanin
#147 node_type_gutting-553306-147.patch2.26 KBplach
#141 node_type_gutting_4.patch19.19 KBcatch
#139 node_type_gutting.patch22.92 KBcatch
#137 node_type_gutting.patch19.2 KBcatch
#134 node_type_gutting.patch19.5 KBcatch
#132 node_type_gutting.patch20.73 KBcatch
#129 node_type_gutting.patch20.75 KBcatch
#127 node_gutting.patch19.08 KBcatch
#122 gut_node_types.patch22.63 KBcatch
#120 gut_node_types.patch22.05 KBcatch
#114 gut_node_types.patch22.83 KBcatch
#112 gut_node_types.patch20.99 KBcatch
#105 node_type_gutting-553306-105.patch19.47 KBmradcliffe
#103 node_type_gutting-553306-103.patch19.42 KBmradcliffe
#99 node_type_gutting-553306-99.patch18.97 KBmradcliffe
#94 node_type_gutting-553306-94.patch21.1 KByched
#92 node_type_gutting-553306-92.patch17.95 KByched
#90 node_type_gutting-553306-90.patch18.03 KByched
#86 node_type_gutting-553306-86.patch23.5 KBbecw
#86 node_type_gutting-553306-86.patch23.5 KBbecw
#79 node_type_gutting-553306-79.patch26.06 KByched
#76 node_type_gutting-553306-76.patch23.8 KByched
#68 node_type_gutting-553306-68.patch26.71 KByched
#66 node_type_gutting-553306-66.patch25.11 KBbecw
#63 node_type_gutting-553306-63.patch24.62 KBbecw
#55 node_type_gutting-553306-55.patch23.49 KBbecw
#52 node_type_gutting-553306-51.patch22.62 KBzzolo
#49 node_type_gutting-553306-49.patch23.39 KBzzolo
#48 node_type_gutting-553306-44.patch22.23 KBzzolo
#44 node_type_gutting-553306-44.patch22.23 KBbecw
#39 node_type_gutting-553306-39.patch22.23 KByched
#36 node_type_gutting.patch18.87 KBwebchick
#33 node_type_gutting.patch18.87 KBcatch
#30 d7_title_body5.patch18.27 KBbecw
#28 d7_title_body4.patch18.14 KBbecw
#25 d7_title_body3.patch18.08 KBbecw
#23 d7_title_body2.patch18.52 KBcatch
#19 d7_title_body2.patch10.33 KBcatch
#17 d7_title_body2.patch17.93 KBbecw
#15 d7_title_body.patch15.96 KBbecw
#13 d7_title_body.patch15.38 KBbecw
#5 remove_old_body_ui_elements.patch1.55 KBbecw
#3 remove_old_body_ui_elements.patch1.51 KBbecw
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Title: Remove redundant Title/Body UI » Remove redundant Body UI
Component: field system » node.module

Node 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.

becw’s picture

Assigned: Unassigned » becw
becw’s picture

Status: Active » Needs review
Issue tags: +drupalconsprint
FileSize
1.51 KB

This 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
1.55 KB

I'm a core patch newbie; forgot to make the patch from drupal root. Should be fixed this time around.

yched’s picture

Thanks 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.

catch’s picture

subscribing.

Title as field is very close to RTBC by the way, so we can hopefully kill both of these.

becw’s picture

@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.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Title: Remove redundant Body UI » Remove redundant Body and Title UI

Let's blow 'em both away. I'm within seconds of committing #557292: TF #3: Convert node title to fields.

becw’s picture

Ok, 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?

webchick’s picture

Yep, that sounds good. Let's finish it all off in this one patch.

Thanks for your help, bec!

becw’s picture

Status: Needs work » Needs review
FileSize
15.38 KB

There 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
15.96 KB

In 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
17.93 KB

- 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.

Anonymous’s picture

I reviewed the patch code style and comments twice and also applied it and verified the UI changes with me own eyes.

+++ modules/node/node.install	14 Oct 2009 22:26:49 -0000
@@ -426,7 +398,7 @@
 /**
- * Convert body and teaser from node properties to fields, and migrate status/comment/promote and sticky columns to the {node_revision} table.
+ * Convert title, body, and teaser from node properties to fields, and migrate status, comment, promote, and sticky columns to the {node_revision} table.
  */

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?

catch’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
10.33 KB

Read through the patch, everything looks peachy. Re-rolled for comment wrapping. RTBC. Don't credit on commit please.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Reviewed & tested by the community

HEAD broken.

becw’s picture

Anyone have input on adding to the preexisting update function node_update_7006()? Is that ok?

catch’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
18.52 KB

My 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
18.08 KB

re-rolled to work with HEAD.

becw’s picture

Title: Remove redundant Body and Title UI » Remove redundant body and title node attributes & UI

Status: Needs review » Needs work

The last submitted patch failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
18.14 KB

Re-rolled. Now includes 'object_type' instance property from http://drupal.org/cvs?commit=275208

Status: Needs review » Needs work

The last submitted patch failed testing.

becw’s picture

Status: Needs work » Needs review
FileSize
18.27 KB

The 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Issue tags: +D7 UX freeze

Title fields are now translatable. Any chance of giving this another poke? :)

catch’s picture

Status: Needs work » Needs review
FileSize
18.87 KB

Straight re-roll to see what the bot says.

heather’s picture

FWIW - 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.

webchick’s picture

FileSize
18.87 KB

I think testing bot got stuck. Re-uploading patch from #33.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Hm. 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.

yched’s picture

Status: Needs work » Needs review
FileSize
22.23 KB

Attached patch should fix the field_ui tests. locale related failures are above me :-(.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review

Other than that:

+++ modules/node/node.install	21 Nov 2009 21:13:54 -0000
@@ -443,10 +414,26 @@ function node_update_7006(&$context) {
+      if ($info->has_title) {
..
+      if ($info->has_body) {

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)

+++ modules/node/node.install	21 Nov 2009 21:13:54 -0000
@@ -541,9 +528,15 @@ function node_update_7006(&$context) {
+      // Remove node properties that descrbe titles and bodies.

typo 'descrbe'

+++ modules/node/node.module	21 Nov 2009 21:13:54 -0000
@@ -512,7 +507,13 @@ function node_type_save($info) {
+    $default_instances = node_type_default_fields($type);

if the function returns *instance* arrays, it should be named node_type_default_field_instances() ?

+++ modules/node/node.module	21 Nov 2009 21:13:54 -0000
@@ -749,19 +643,102 @@ function node_type_set_defaults($info = 
+      'label' => t('Title'),
...
+      'label' => t('Body'),

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.

yched’s picture

DrupalWebTestCase::drupalCreateContentType() also has

      'title_label' => 'Title',
      'body_label' => 'Body',
      'has_title' => 1,
      'has_body' => 1,

Those can be removed. The patch already takes care of all tests that actually call drupalCreateContentType() with those options.

becw’s picture

Using 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.

becw’s picture

Here'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.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

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.

True 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.

the base test class method drupalCreateContentType() still relies on these node properties

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.

drupalCreateNode() expects title and body fields

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.

yched’s picture

Patch 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 ?

zzolo’s picture

I have rerolled the patch:

* Updated use of strtolower to drupal_strtolower in tests.

zzolo’s picture

upload wrong patch. :)

zzolo’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

zzolo’s picture

Status: Needs work » Needs review
FileSize
22.62 KB

Ugh. My other patch was in there too. New patch; this time should be good. Maybe its time for me to go to bed. :)

Status: Needs review » Needs work

The last submitted patch failed testing.

zzolo’s picture

Well, 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:

    *  Notice: Undefined offset: 0 in node_save() (line 965 of /Users/alanpalazzolo/Sites/localhost/core-dev-7/modules/node/node.module).
    * Notice: Undefined offset: 0 in node_form_submit() (line 420 of /Users/alanpalazzolo/Sites/localhost/core-dev-7/modules/node/node.pages.inc).
    * Notice: Undefined offset: 0 in node_form_submit() (line 421 of /Users/alanpalazzolo/Sites/localhost/core-dev-7/modules/node/node.pages.inc).

I believe it has something to do with the following structure:

$node->title['lang'][0]['value'] = 'some title';
$node->body['lang'][0]['value'] = 'some title';

But I do have to go to bed now.

becw’s picture

re-rolled, changed use of node_type_get_types() in node_update_7006(). I guess now I have zzolo's patch to look at...

yched’s picture

Status: Needs work » Needs review
zzolo’s picture

FYI, I just re-rolled your patch. The only thing I changed was using drupal_strtolower() instead of strtolower()

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review

Differences 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:

+++ modules/node/node.install	23 Nov 2009 00:56:50 -0000
@@ -431,22 +401,42 @@ function node_update_7005() {
+    // Create title and body field instances for each node type.
+    $default_instances = node_type_default_field_instances();

Comment is misleading. We don't create them yet.

+++ modules/node/node.install	23 Nov 2009 00:56:50 -0000
@@ -431,22 +401,42 @@ function node_update_7005() {
+    // Get node type info, specifically the fields that this update will deprecate.
+    $result = db_select('node_type', 'node_type')
+      ->fields('node_type', array('type', 'name', 'base', 'description', 'help', 'has_title', 'title_label', 'has_body', 'body_label', 'custom', 'modified', 'locked', 'orig_type'))
+      ->execute();

yay ;-) Though it would be more concise to just say ->fields('node_type')

+++ modules/node/node.install	23 Nov 2009 00:56:50 -0000
@@ -431,22 +401,42 @@ function node_update_7005() {
+      if ($node_type->has_title) {
@@ -461,6 +451,10 @@ function node_update_7006(&$context) {
+    // Get node type info.
+    drupal_static_reset('_node_types_build');
+    $node_types = node_type_get_types();
+

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:

foreach ($result as $node_type) {
  if ($node_type->has_title) { ... }
  if ($node_type->has_body) { ... }
  $context['node_types_info'][$node_type->type] = array(
    'has_title' => $node_type->has_title, 
    'has_body' => $node_type->has_body,
  );
} 

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.

+++ modules/node/node.install	23 Nov 2009 00:56:50 -0000
@@ -533,7 +527,7 @@ function node_update_7006(&$context) {
-    if (!$found) {
+    if ($found === FALSE) {

$found is either TRUE or FALSE, so I don't think that change is needed ?

This review is powered by Dreditor.

yched’s picture

And thanks for sticking with this, bec, that's really appreciated !

becw’s picture

The 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;

yched’s picture

Hm, 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.

becw’s picture

Status: Needs review » Needs work
FileSize
24.62 KB

oops, 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...

yched’s picture

A couple fixes related to API changes in #470242: Namespacing for bundle names:

+++ modules/field/field.api.php	23 Nov 2009 02:28:37 -0000
@@ -35,7 +35,7 @@ function hook_field_extra_fields($bundle
-     if ($type->has_title) {
+    if (field_info_instance('title', $type->type) === NULL) {

should be field_info_instance('node', 'title', $type->type)

+++ modules/node/node.install	23 Nov 2009 02:28:58 -0000
@@ -431,38 +401,63 @@ function node_update_7005() {
+        $instance = field_info_instance('title', $node_type->type);

should be field_info_instance('node', 'title', $node_type->type)

+++ modules/node/node.install	23 Nov 2009 02:28:58 -0000
@@ -431,38 +401,63 @@ function node_update_7005() {
+        $instance = field_info_instance('body', $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?

yched’s picture

[fixed - edited out for readability's sake]

becw’s picture

OK, 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.

yched’s picture

FWIW, 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.

yched’s picture

Status: Needs work » Needs review
FileSize
26.71 KB

DOH. 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.

Status: Needs review » Needs work
Issue tags: -drupalconsprint, -D7 UX freeze

The last submitted patch failed testing.

Status: Needs work » Needs review

bec requested that failed test be re-tested.

Status: Needs review » Needs work
Issue tags: +drupalconsprint, +D7 UX freeze

The last submitted patch failed testing.

becw’s picture

That's funny, #68 passes all the locale tests on my local machine (in 36 minutes and 7 seconds, haha).

webchick’s picture

Hm. 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?

yched’s picture

@webchick: the body field is created as translateble in node_configure_fields() (HEAD) and in node_type_default_field_instances() (patch)

yched’s picture

I marked #504564: Make summary length behave with fields as dependant on that one.

yched’s picture

Remaining 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:

  drupal_static_reset('entity_get_info');
  cache_clear_all('entity_info', 'cache');

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)

yched’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
26.06 KB

Bleh, forgot some hunks while updating field_ui.test.

yched’s picture

OMG. 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 ?

catch’s picture

Sounds good to me.

webchick’s picture

Yeah, we definitely need to retain the programmatic ability to create nodes without bodies/titles.

AND OMG IT'S GREEN!!!!!!!!!!!!!!!! :D So exciting.

yched’s picture

bec, can I leave that to you ? ;-)

yched’s picture

Status: Needs review » Needs work

#642612: No clean way to reset entity_info cache has been committed, so we can replace the

  drupal_static_reset('entity_get_info');
  cache_clear_all('entity_info', 'cache');

in node_type_save() by

entity_info_cache_clear();

+ back to CNW.

becw’s picture

fyi, just checked this and am now working on it.

becw’s picture

Status: Needs work » Needs review
FileSize
23.5 KB
23.5 KB

- 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

Status: Needs review » Needs work

The last submitted patch failed testing.

moshe weitzman’s picture

Issue tags: +D7 upgrade path

Add tag. This patch will remove a major error in upgrade path.

yched’s picture

This 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.

yched’s picture

Status: Needs work » Needs review
FileSize
18.03 KB

Still needs a little refactor, but let's see where tests stand.

Status: Needs review » Needs work

The last submitted patch, node_type_gutting-553306-90.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
17.95 KB

True. What about that one ?

Dave Reid’s picture

Status: Needs review » Needs work

Needs to have the body title summary removed from modules/node/content_types.js

yched’s picture

Status: Needs work » Needs review
FileSize
21.1 KB

VT 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.

gopherspidey’s picture

sub

mcrittenden’s picture

Sub.

sun.core’s picture

Status: Needs review » Needs work
Issue tags: +drupalconsprint, +D7 UX freeze, +D7 upgrade path

The last submitted patch, node_type_gutting-553306-94.patch, failed testing.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
18.97 KB

Found 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.

Status: Needs review » Needs work

The last submitted patch, node_type_gutting-553306-99.patch, failed testing.

mradcliffe’s picture

Awesome, 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.

Array ( [und] => Array ( [0] => Array ( [value] => eJyyrluZi1HVhTew [summary] => [format] => 2 [safe_value] => <p>eJyyrluZi1HVhTew</p>  [safe_summary] => ) ) ) 

edit: edited out some assumptions.

mradcliffe’s picture

Further 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 to node_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.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
19.42 KB

Okay, 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.

mradcliffe’s picture

Status: Needs review » Needs work

Err, whoops. Too tunnel visioned I guess. :)

back to #99

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
19.47 KB

Let'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.

mradcliffe’s picture

Note: 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.

catch’s picture

Status: Needs review » Needs work

I 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?

yched’s picture

re catch : see #80 and #82.

catch’s picture

OK 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?

catch’s picture

Actually 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.

yched’s picture

Note that has_body doesn't appear in node_field_extra_fields(), it's just an optical illusion from adjacent patch hunks:

+++ modules/node/node.module	25 Mar 2010 20:42:48 -0000
@@ -544,64 +565,6 @@ function node_type_save($info) {
  * Implements hook_field_extra_fields().
  */
 function node_field_extra_fields() {
@@ -730,14 +693,15 @@ function node_type_set_defaults($info = 
     $type->base = '';
     $type->description = '';
     $type->help = '';
-    $type->has_title = 1;
-    $type->has_body = 1;
-    $type->title_label = t('Title');
-    $type->body_label = t('Body');
     $type->custom = 0;
     $type->modified = 0;
     $type->locked = 1;
     $type->is_new = 1;
+
+    $type->has_title = 1;
+    $type->has_body = 1;
+    $type->title_label = 'Title';
+    $type->body_label = 'Body';

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.

catch’s picture

Status: Needs work » Needs review
FileSize
20.99 KB

Re-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.

Status: Needs review » Needs work

The last submitted patch, gut_node_types.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
22.83 KB

Fixed 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.

Status: Needs review » Needs work

The last submitted patch, gut_node_types.patch, failed testing.

catch’s picture

Status: Needs work » Needs review

Spoke 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.

mradcliffe’s picture

Over 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.

catch’s picture

Fully 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.

plach’s picture

#114: gut_node_types.patch queued for re-testing.

#632172: Node language and field languages may differ has been committed, let's retest this.

catch’s picture

FileSize
22.05 KB

Re-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.

Status: Needs review » Needs work

The last submitted patch, gut_node_types.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
22.63 KB

Last patch was missing blog.install, no other changes.

mradcliffe’s picture

Ah. 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.

mradcliffe’s picture

Is node_add_body_field() redundant when looking at node_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.

catch’s picture

Status: Needs review » Needs work

The 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.

tutumlum’s picture

sub

catch’s picture

Status: Needs work » Needs review
FileSize
19.08 KB

Removed node_add_body_field().

Status: Needs review » Needs work

The last submitted patch, node_gutting.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
20.75 KB

Looking 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.

yched’s picture

+ 1 on the latest approach - sorry, that's all the amount of reviewing I can deliver currently :-/.

Status: Needs review » Needs work

The last submitted patch, node_type_gutting.patch, failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
20.73 KB

blog.install was missing the change.

plach’s picture

+++ modules/field_ui/field_ui.test	26 Apr 2010 00:34:37 -0000
@@ -26,15 +26,15 @@ class FieldUITestCase extends DrupalWebT
+    $this->type_name = drupal_strtolower($this->randomName(8)) . '_' .'test';

Weird string concatenation.

+++ modules/node/content_types.js	26 Apr 2010 00:34:37 -0000
@@ -7,7 +7,6 @@ Drupal.behaviors.contentTypes = {
       vals.push(Drupal.checkPlain($('#edit-title-label', context).val()) || Drupal.t('Requires a title'));

(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.

catch’s picture

FileSize
19.5 KB

Those 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.

plach’s picture

Status: Needs review » Needs work

The js issue, I have no idea what that's supposed to do and don't fancy trying to fix it here either.

I 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.

+++ modules/node/node.install	26 Apr 2010 00:34:38 -0000
@@ -496,30 +481,39 @@ function node_update_7005() {
+        node_add_body_field($node_type->type);

I think we want also a $label parameter here, otherwise we'll lose any label different from 'Body' during the upgrade path.

+++ modules/node/node.module	26 Apr 2010 00:34:39 -0000
@@ -537,60 +534,43 @@ function node_type_save($info) {
+function node_add_body_field($type) {

Missing $label parameter.

+++ modules/node/node.module	26 Apr 2010 00:34:39 -0000
@@ -537,60 +534,43 @@ function node_type_save($info) {
+      'label' => 'Body',

'Body' should be only the default value.

Powered by Dreditor.

plach’s picture

catch’s picture

Status: Needs work » Needs review
FileSize
19.2 KB

Should address #135.

plach’s picture

Status: Needs review » Needs work
+++ modules/node/node.install	2 May 2010 11:39:17 -0000
@@ -496,30 +481,39 @@ function node_update_7005() {
+        node_add_body_field($node_type->type, $node_type->title_label);

I guess this should be $node_type->body_label :)

Feel free to set RTBC after fixing that if the bot is happy.

Powered by Dreditor.

catch’s picture

Status: Needs work » Needs review
FileSize
22.92 KB

ouch, fixed.

plach’s picture

Status: Needs review » Needs work

nope, it seems the same patch with some extra stuff on the top :(

catch’s picture

Status: Needs work » Needs review
FileSize
19.19 KB

Arggh, sorry.

plach’s picture

Status: Needs review » Reviewed & tested by the community

The patch looks good and behave as expected, has yched's approval on the Field API side and tests pass.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This looks both yummy and delicious, but unfortunately no longer applies.

asimmonds’s picture

Looks like some of #141 was committed in http://drupal.org/cvs?commit=363156 and catch's patch in #144 is broken.

catch’s picture

Title: Remove redundant body and title node attributes & UI » HEAD broken: Remove redundant body and title node attributes & UI

Whoops, 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.

plach’s picture

Status: Needs work » Needs review
FileSize
2.26 KB

This should be what was left out of #141: just blog.install and parts of node_update_7006().

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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.

plach’s picture

Yes, head is broken due to the missing blog.install.

webchick’s picture

Title: HEAD broken: Remove redundant body and title node attributes & UI » Remove redundant body and title node attributes & UI
Status: Reviewed & tested by the community » Fixed

Oh, 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!

plach’s picture

@webchick:

Oh, crap. :( I'm sorry! This will teach me to commit patches after 3am... ;P

It's not you, it's the evil Curse of the Redundant Body UI (tm)

webchick’s picture

That's right, damn it! ;)

webchick’s picture

Left a note over at #771922-6: Strings cleanup to help some poor schmuck trying to figure out where this code actually came from.

webchick’s picture

Status: Fixed » Needs work
Issue tags: +Needs documentation, +API change

Oops. 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.

webchick’s picture

Hm. I guess, since there's no longer a way to "opt out" otherwise...

catch’s picture

Status: Needs work » Fixed

@webchick #155, yes that's why.

Documented at http://drupal.org/update/modules/6/7#body_field

rfay’s picture

Trying 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:

Error message
PDOException: SQLSTATE[HY000]: General error: 1364 Field 'has_body' doesn't have a default value: INSERT INTO {node_type} (type, name, base, has_title, title_label, description, help, custom, modified, locked, orig_type) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10); Array ( [:db_insert_placeholder_0] => simpletest_example [:db_insert_placeholder_1] => Simpletest Example Node Type [:db_insert_placeholder_2] => simpletest_example [:db_insert_placeholder_3] => 1 [:db_insert_placeholder_4] => Title [:db_insert_placeholder_5] => simpletest_example page node type. [:db_insert_placeholder_6] => [:db_insert_placeholder_7] => 0 [:db_insert_placeholder_8] => 0 [:db_insert_placeholder_9] => 1 [:db_insert_placeholder_10] => simpletest_example ) in node_type_save() (line 522 of /home/rfay/workspace/d7git/modules/node/node.module).

With a reinstall all is well. Shucks. I'm looking forward to not having the reinstall flag played any more.

catch’s picture

@rfay: you might be interested in http://drupal.org/project/head2head, although no update for this yet.

rfay’s picture

In 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.

rfay’s picture

Title: Remove redundant body and title node attributes & UI » Make nodes have no body field by default. Remove deprecated APIs for body field

Changing the title. If you can say it better, please do.

pwolanin’s picture

pwolanin’s picture

Status: Fixed » Needs review
FileSize
555 bytes

I 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

Status: Needs review » Needs work

The last submitted patch, 553306-fu-node-type-162.patch, failed testing.

catch’s picture

Status: Needs work » Fixed

This was found and fixed in another issue.

Status: Fixed » Closed (fixed)
Issue tags: -Needs documentation, -API change, -drupalconsprint, -D7 UX freeze, -D7 upgrade path

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