Per discussion on the developer list:

Concept:

Remove all special processing for $node->body and $node->teaser. Create two fields, body and teaser, both text fields. Assign both fields to every existing content type with a "body field" using the textarea widget. For existing nodes, as part of the upgrade path, initialize the teaser field value with whatever teaser would normally be generated automatically from the body.

Random thoughts:

- We will get to remove a bunch of code, much of it quite ugly, from node.module.

- Sites that really want an auto-generated teaser can remove the teaser field from a content type and use the text field's teaser Display Formatter in the teaser context.

- This will mean removing the body field from the node and node_revision tables, and creating a field_data_body table for it instead. Don't worry about database query efficiency; that is a solved problem (see http://drupal.org/node/368674).

- With $node->body and $node->teaser being normal fields, they will not be available for overloaded use as the complete rendered output of the node. Hurray! We can use $node->content = drupal_render($node) or something like that.

CommentFileSizeAuthor
#209 body-as-field-372743-209.patch111.19 KByched
#207 body-as-field-372743-207.patch110.53 KByched
#203 body-as-field-372743-203.patch111.37 KBbjaspan
#200 body-as-field-372743-200.patch106.52 KByched
#199 body-as-field-372743-199.patch103.36 KBcatch
#188 body-as-field-372743-187.patch99.26 KByched
#186 body-as-field-372743-186.patch99.2 KByched
#184 body-as-field-372743-184.patch103.81 KBtstoeckler
#180 body-as-field-372743-180.patch103.81 KBtstoeckler
#177 body-as-field-372743-177.patch103.51 KBtstoeckler
#173 body-as-field-372743-173.patch103.52 KBtstoeckler
#171 body-as-field-372743-171.patch111.08 KByched
#169 body-as-field-372743-169.patch109.2 KByched
#166 body-as-field-372743-165.patch109.92 KByched
#161 updates.txt6.54 KBcatch
#159 body-as-field-372743-158.patch109.69 KByched
#150 body-as-field-372743-150.patch467.73 KByched
#146 body-as-field-372743-146.patch177.76 KByched
#144 body-as-field-372743-144.patch130.1 KByched
#142 body-as-field-372743-142.patch130.1 KByched
#141 body-as-field-372743-141.patch117.34 KByched
#140 body-as-field-372743-140.patch118.05 KByched
#137 body-as-field-372743-137.patch108.8 KByched
#134 body-as-field-372743-134.patch106.1 KByched
#132 body-as-field-372743-131.patch104.19 KByched
#130 body-as-field-372743-130.patch104.22 KByched
#124 body-as-field-372743-124.patch101.88 KByched
#123 benchmarks.txt5.7 KBcatch
#123 benchmarks.txt5.7 KBcatch
#120 body-as-field-372743-120.patch102.63 KByched
#118 body-as-field-372743-118.patch100.54 KByched
#113 body-as-field-372743-113.patch88.72 KByched
#108 body-as-field-372743-108.patch78.73 KByched
#105 body-as-field-372743-104.patch61.59 KBbjaspan
#101 body-as-field-372743-101.patch54.45 KBbjaspan
#98 body-as-field-372743-95.patch50.32 KBbjaspan
#92 body-as-field-372743-92.patch48.71 KBbjaspan
#82 body-as-field-372743-82.patch46.32 KByched
#83 body-as-field-372743-83.patch46.2 KByched
#80 body-as-field-372743-78.patch54.67 KBbjaspan
#79 body-as-field-372743-77.patch54.66 KBbjaspan
#78 body-as-field-372743-77.patch45.76 KByched
#75 body-as-field-372743-75.patch54.04 KBbjaspan
#70 body-as-field-372743-70.patch54.21 KBbjaspan
#63 body-as-field-372743-63.patch51.87 KBbjaspan
#49 body-as-field.patch49.38 KBKarenS
#48 body-as-field.patch50.35 KBKarenS
#42 body-as-field.patch46.19 KBKarenS
#41 body-as-field-372743-41.patch41.35 KBbjaspan
#26 body-as-field-372743-26.patch35.03 KBbjaspan
#16 body-as-field-372743-16.patch31.43 KBbjaspan
#13 body-as-field-372743-12.patch31.12 KBbjaspan
#9 body-as-field-372743-9.patch30.59 KBbjaspan
#1 body-as-field-372743-1.patch29.79 KBbjaspan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bjaspan’s picture

Status: Active » Needs work
FileSize
29.79 KB

Here is the first stab at changing body to be a field. Some notes:

- min word count

node supports min word counts for body per content type. text fields have a max_length setting but no min_length or min_word_count. If we want to preserve this feature, we can add a min word count setting to text fields; it would not be hard.

Actually, once #369964: Refactor field validation and error reporting is committed, we can arrange a way for node.module to check min word count itself and set an error on the field if there is a problem.

- $node->body

$node->body starts life as a value read from the body column from the node_revisions table. In node_build_content(), the

tag is removed if present. In node_prepare() it converted to the output of check_markup() on itself. During search and RSS feed processing, it is set to drupal_render($node->content), i.e. the rendered HTML for the entire node, and then has comment and taxonomy content appended. node_body_field() generates the Form API elements for the body field and uses $node->body as the default value if it is exists (sometimes after prepending the teaser). etc. etc. etc. In short, it's a mess and there is no telling what code where depends on it having some particular kind of value when.

Rather than try to fix all uses of $node->body at once, I'm declaring the new body field to be named 'body_field'. It's a poor and temporary choice but better than a huge flag day. Perhaps before code freeze enough will have been cleaned up that we can switch back to 'body'. However, since the body field will no longer be special any way, it does not really matter much what it is called, so 'body_field' will do fine for now.

- $node->teaser

$node->teaser is set up automatically based on $node->body using a complicated set of rules that I do not even want to try to figure out. I have two suggestions for replacing it:

1. Every content type gets a separate teaser and body field. Existing node bodies with

are converted on update and we never auto-trim a body again. The disadvantage is that for someone that wants a simple blog site, this makes it "harder" because they have to enter a body and teaser separately.

2. Every content type gets just a body field and uses a display formatter to show an auto-shortened teaser on teaser views. The display formatter supports

for existing nodes but it is deprecated. We remove the JS splitter entirely. If you want something more powerful, install the CCK UI and create a teaser field, or a contrib module, or whatever.

The second approach is less work because it does not require a per-node upgrade action.

- $node->readmore

node_prepare() used to set $node->readmore to TRUE if the body was longer than the teaser, and node_link() added a 'Read more' link if it was set. This already made no sense because many sites don't use body at all and the CCK display settings can specify all kinds of additional content for the full view vs. the teaser view of a node. With body (and teaser) as a field, $node->readmore makes equally/even less sense. I therefore removed $node->readmore and made node_link() always add a 'Read more' link.

Note, however, hook_link() no longer seemed to be invoked anywhere. In D6 it came from node_view() but no longer does. Was this an accidental change, or did I miss something?

bjaspan’s picture

My patch takes the #2 approach for $node->teaser. There is no teaser field; body is always used, automatically trimmed in the teaser context. In other words, this is a "regression" to D5 behavior in that the JS splitter is removed. However, anyone that wants something better for a teaser can simply add an explicit teaser field (this perhaps makes it important that we get a Field UI into core).

yched’s picture

Oh. My. God. So cool.

Small remarks for now :
- I favor approach #2 as well. But besides the 'regression' of teaser splitter *UI* being moved away, which it seems many/most agree is not a too serious loss, it also raises the issue of existing D6 nodes that used the feature to store a teaser completely different from the body. How can we provide an automated path to not lose this data ?

- 'anyone that wants something better for a teaser can simply add an explicit teaser field - this perhaps makes it important that we get a Field UI into core' : Well, the addition of a teaser field might have its own UI (just an additional 'Teaser field title' text input on the node type definition form) that uses field CRUD in the background, just like your node_configure_fields() does for the 'body_field' field.
Of course, the more existing stuff we end up moving to being a field calls for a unified UI louder, but the teaser itself is not a dealbreaker IMO.

- name 'body_field' (ok, bikeshedding already) : how about 'node_body' ? We recommend modules to prefix their fields with the module name to avoid namespace issue, so we could just go with that rule ?

yched’s picture

Side note : the $size param of text_teaser() would be perfect as a formatter setting. Can be done in a followup patch, though.

David_Rothstein’s picture

I read through this patch, and yeah, this looks great :) It seems like it leads to a good cleanup of node.module.

A couple things I noticed:

1. The blog and blogapi modules might unfortunately need some serious reworking as part of this patch. Right now they basically assume/force blog nodes to always have a body field, and do "interesting" things like calling node_prepare() and node_submit() directly. I think this patch will break them. I know this from bad experiences writing other patches :)

2. hook_link() doesn't really exist any more (at least for nodes), due to #339929: Move node links into $node->content, so the "read more" links are totally broken now anyway (presumably it was just an oversight that they didn't get moved somewhere else). As for what to do about them, one possibility is to look at #16161: Move Read More link to end of node content, which aims to put the "read more" link inline with the teaser. If the node body isn't going to be special anymore, then perhaps that's something that should happen with any field that has a teaser: If the teaser is shorter than that field's original content, that field gets a "read more" link inline with it? So in other words, a node might theoretically have more than one "read more" link in different places in the node.

3. In theory it makes a lot of sense to only create the body field in the default install profile, but right now, that doesn't work. If you install with the expert/minimal profile, this patch breaks things completely :( I'm guessing that for now it would be easier just to create this field in system.install, rather than leaving it as a choice for install profiles (especially since there is all the residual "body field label" stuff in the content types UI)? I guess that would mean that text.module becomes required, but that doesn't seem so bad.

bjaspan’s picture

I like the idea of a "read more" link on the field itself. With my patch the text module implements the "trimmed" formatter. It can easily provide a "trimmed, with read more link" formatter.

bjaspan’s picture

At first I disagreed with David's #3 but now I think he is right. node.module supports the "has_body" property of node types and assumes that NODE_BODY_FIELD exists if has_body is true. Therefore, we can't have NODE_BODY_FIELD's existence depend on the install profile.

When we have a CCK UI in core, the has_body property of node types can do away, and then node module will not have to know about the body field at all.

catch’s picture

There's a separate issue for removing the teaser splitter, no patch yet. We can justify removing it for many different reasons. Seems like this patch is a better place for it to happen - but if anyone wants it left in, I suggest they comment there rather than here #274947: Remaining bits from teaser splitter.

bjaspan’s picture

FileSize
30.59 KB

New patch. This moves creation of the body field from default.profile to node.module. The fact that node.module assumes the existence of a text field means that text.module must be required, and in fact the Field API team and Dries have all agreed that all core field types will be required so as to be always available, so the patch now makes all the core field modules required.

bjaspan’s picture

I still need to address items 1 and 2 from #5.

bjaspan’s picture

Regarding Read More:

1. I do like David's idea of a display formatter that adds a read more link.
2. We have to decide what the markup for that link should look like.
3. I now realize a bit of a problem because a field module does not have any way to know how to make a link to an arbitrary object type. We'll need a new hook for that.
4. We also probably need some way to encode "this field has more to read" so that sites that (for example) want a D6-like "read more" link can change their theme to discover that a read more link should be displayed wherever they want it.

yched’s picture

1. I guess 'display read more' would be a formatter setting rather than a hardcoded behavior of some formatters ?
The actual text to display could be a setting too (would have to be localizable, though, so not sure about this)
3. pattern for 'url to object's page' : new entry in fieldable_info ?
4. D6-like single node-wide 'read more' : something that ORs together the 'has more' values for individual fields (for each field value, actually) ? I'm not sure how we can avoid using a global here :-(

bjaspan’s picture

Actually, I almost completely changed my mind about read more while talking to people at a post-Drupalcon dinner.

"Read more" is a property of nodes. Field API currently knows something about "build modes" but we have TODOs in the code because it is wrong. I do not think Field API should be trying to figure out when to show a read more link. Also, the fact that the Field API would have to figure out how to generate a link to the object being fielded indicates that the link just shouldn't be generated there.

The right thing here is for node.module to put a read more link on EVERY teaser. A contrib module or site-custom module can do something more clever if desired. New patch attached. This solves most of the issues from my #11.

I do recognize that I'm saying "Field API should not know about read more" while in the same patch moving the node_teaser() function into text.module to implement the "trimmed" display formatter.

This patch still has the problem that node_revisions.teaser and node_revisions.body both exist and the teaser might contain a summary unrelated to the body. I've talked through this several times and I do not think there is a good solution. I have yet to decide on the least bad solution.

I am actually becoming inclined to suspect this patch until we have a CCK UI in core, because here I am adding some code to handle the has_body property of node types which will have to come out when the UI arrives. However, I also do not want to see all use of fields in core held up until we have a UI. So I'm kinda torn.

catch’s picture

$has_body should be a TODO for further cleanup rather than holding this up waiting for a UI.

For teasers/summaries, my initial thought is to add the extra 'teaser' field to all node types and copy the data from teaser to there. That gives people an upgrade path for 'summaries' in terms of their actual content.People who don't need the two fields can delete the teaser field (assuming there's a UI), those who want 'hard' teasers or the summary behaviour can keep it around. That covers a lot of sites.

Then we've got the issue that there's currently a per-node setting for 'show summary on full view' - if we count that setting as user data, then we'd need to add the checkbox back to that teaser field, and store a boolean somewhere. Extra column on that particular text field?

Then you can do the same thing as D6 minus the JavaScript. Upgrade path could compare the teaser and body to determine if it's dealing with a summary or a teaser - just a strpos would do. Then we've got an upgrade path for the data, for the display behaviour, and we have a simple two-field UI (+ checkbox if necessary) instead of the widget.

bjaspan’s picture

The problem with adding an extra teaser field is that then we make life harder for people who want the "standard" Drupal behavior of automatically-generated teasers. Now, simple blog sites will have to manually enter a teaser and a body for all nodes, instead of just a body that results in an automatically-generated teaser. I think this will get a lot of complaints.

The obvious reply is to say: "So, have a teaser and body field, and if they do not provide a teaser value, we just automatically initialize the teaser from the body." But now we're adding back the very special-case knowledge of body and teaser that we are trying to remove! And maybe the special case logic will be simpler than the mess we have now, but (I fear) not by a lot.

The bottom line is that with D6 we painted ourselves into a nasty corner and no one has thought of a complete, decent way out yet.

My best idea so far, pre-CCK UI, is for node types only to have the has_body option and corresponding field, and the teaser is automatically generated, period. On upgrade, we also create a teaser field and initialize it with the 'teaser' column of node_revisions (before we drop the column), but we do not attach that field to any content types so it is never shown. Users can install CCK UI to start showing it if they want, and someone can write a contrib module to handle some of the various more complicated upgrade cases for people who have used it is as a teaser, summary, summary-to-be-shown, or all of the above (!) on different nodes.

Thoughts?

bjaspan’s picture

Patch re-rolled for hook_nodeapi => hook_node.

kika’s picture

bjaspan’s picture

I do not believe that #396006: Addition of a node--teaser.tpl.php by default and #372743: Body and teaser as fields have anything to do with each other. A node has various fields and other module-provided content that render the same or differently on a teaser or full view. A site admin can change what renders how. A theme developer can build the node template (including a separate node teaser template) to show whatever subset of the available data that is desired.

Whether node's body and teaser come from Field API or from a special case inside node.module seems unrelated.

jstoller’s picture

Currently, when I create a CCK field for a content type there are a wide variety of settings I can control for that field. For instance, picking which content types can be referenced by a node reference field. Couldn't the "teaser" field type similarly allow users to chose which other field they are a teaser of and auto generate content based on that field? A standard Drupal install would then include a default "body" field and a default "teaser" field which was preset to pull its content from that "body" field. No UI would be required to setup this default behavior. Am I missing something?

jstoller’s picture

Forgive my naivete, but why wouldn't "Title" also be a field?

bjaspan’s picture

re #19: In fact, that is exactly the solution I am leaning towards.

re #20: I'd love for title to be a field but it will involve changes all over the place in Drupal, lots of code like l($node->title, 'node/'.$node->nid) would have to change, etc. So I'm doing body first, title later.

jstoller’s picture

Cool! I expect removing content fields from the node and node_revisions tables will remove a substantial barrier toward implementing features like #120967: Separate revisions from node.module and #218755: Support revisions in different states.

bjaspan’s picture

The first roadblock with the "teaser field uses data from body field if it is empty" feature is deciding when the teaser field should grab the body's value.

It could happen on field_attach_insert/update. In that case, the teaser field will generally be initialized when the object is first created and then not subsequently re-initialized (because after the first save it won't be empty). This is only slightly better than simply always requiring a separate teaser field without automatic initialization.

It could happen on field_attach_load, but whenever an existing object is re-saved, this reduces to the previous problem.

It could happen on field_attach_view. If the teaser only sets its displayable field data properties (e.g. 'safe', but not 'value') we won't get the saving problem above, but then the teaser field won't contain a 'value' property which violates the promised contract of a text field. If the teaser does set its 'value' property, then a load, view, and update will again save a fixed value in the field.

I think I've convinced myself that if we do want a text field to know how to grab a value from another text field, it also has to support a checkbox controlling whether it *should* grab the other value. At the UI level, the widget would know not to show the text input field if the box is checked, at the Field Type API level it would validate that no text data is provided if the box is checked.

Thoughts?

bjaspan’s picture

Here's a thought. Instead of telling the teaser field that it should initialize itself if empty from the body field, I'm thinking we can tell the body field to display (with a "trimmed" display formatter) if the teaser is empty. So we'd have a teaser and body field. Teaser would display in teaser mode only. Body would display in full mode, and trimmed in teaser mode only if the teaser field is empty.

This completely avoids all the complication with trying to avoid saving the automatically trimmed version into the teaser field.

catch’s picture

#24 sounds much better. Are you planning to keep the

tag around for arbitrary length teasers which also show on the page? For people who want a separate teaser field which always shows up in both listings and the page view, I guess they can just remove the special teaser field and add their own?

bjaspan’s picture

Here's the first patch using the technique from #24. It turned out to be straightforward. I created a new text field instance setting, 'teaser_field', and a new display formatter, text_trimmed_if_teaser_empty. I then create a node_teaser and node_body field, setting node_body's 'teaser_field' property to node_teaser, using the new formatter in teaser mode. So in a teaser view, node_body will render trimmed if node_teaser is empty, otherwise it will render as the empty string.

The formatter is simple:

/**
 * Theme function for 'trimmed' text field formatter if the referenced
 * teaser field is empty.
 */
function theme_field_formatter_text_trimmed_if_teaser_empty($element) {
  $field = field_info_field($element['#field_name']);
  $instance = field_info_instance($element['#field_name'], $element['#bundle']);

  $object = $element['#object'];
  if (!empty($instance['settings']['teaser_field'])) {
    $teaser = $instance['settings']['teaser_field'];
    if (!empty($object->{$teaser}[$element['#item']['#delta']]['safe'])) {
      return '';
    }
  }

  return text_teaser($element['#item']['safe'], $instance['settings']['text_processing'] ? $element['#item']['format'] : NULL);
}

Assuming no one discovers a fundamental flaw in this approach, I think it is now possible to write an upgrade path.

KarenS’s picture

This is a very interesting and creative solution. The only thing that bothers me a little is that we have created fields that 'know' about each other, which is something very unusual. In all other cases, fields are totally independent and unaware of each other. Maybe it doesn't matter, not sure, and it's only possible because we have given them names that no other fields can use.

Actually do we need to do anything anywhere to be sure that no other module, script, or UI tries to create fields with these names or do we just assume they should know better?

KarenS’s picture

Another approach to this would be to add one more column to the long text field type to create a three-column field to hold body, teaser, and format in a single field. Then the teaser (empty or not) is a part of the same field and there are no issues about one field 'knowing' anything about another. It means there would be one more column in the database for every node, but that is actually the same number of columns we have in the current schema. We can use the same logic -- the trimmed node can be used for the teaser when the teaser is empty. Simple textfields wouldn't have the extra column, only long text fields.

This also means that any text area field will have an automatic place for an excerpt or summary that can either be different text or a subset of the regular text. That could have interesting uses. Or if it seems too heavy-handed to add that column to all text areas, it could be an additional text field type (so we would have three text field types: textfield, simple textarea, and textarea with teaser).

Did you already explore that idea? Sorry if you did and I missed a reason why it wouldn't work.

jstoller’s picture

Having "text area with teaser" as a separate field type sounds like an excellent idea. It definitely solidifies the connection between the body and teaser, with no room for error, and I can see possibilities where someone may want to display several of these on the same node. Perhaps a user's role could determine whether they saw all the content on a page or just a page full of brief content summaries.

I would generalize the name of the field type to "text area with summary," recognizing that the summary won't always be used as a page teaser.

bjaspan’s picture

re #27: There is nothing specifically hard-coded about the field names. node.module creates node_teaser, then it creates node_body and sets is 'teaser_field' instance setting to 'node_teaser'. I'm not to worried about having fields relate to each other. They do so indirectly, by name, and if the referenced field ever goes away, nothing bad happens.

I definitely do not like adding a summary column to *every* text field because for most it is just a confusing waste of space. I could probably be happy with a "text + summary" field type, though it will make the patch bigger. Perhaps I considered and rejected it as too much of a "special case" but then what I did with the "teaser_field" setting is a similar special case (though much less code).

What I really want, though, is to get this issue finished and closed. Perhaps I'll try the text+summary type tomorrow (it is Community Gardening Day at Acquia) and see how it comes out.

jstoller’s picture

I would recommend against doing anything that makes the default body and teaser fields special, or privileged, when compared to other custom fields. The only special thing about them should be that they happen to be installed by default, just as the Page content type happens to be installed by default. Deleting any of these and building your own should have no negative ramifications.

I find the "text + summary" field type appealing because it makes this a decidedly un-special case. It makes your default Body field just a run-of-the-mill field type that anyone can put in any content type they want, as many times as they want, or not at all if they so choose. This has a clean, straight forward feeling to it, with no messy configuration options for a user to break. And it seems to add a great deal of flexibility to the system. I expect that in no time people will be using text+summary fields in ways you never imagined.

KarenS’s picture

Barry, sorry for ringing in so late in the game on this, but I also like the new field type idea the best, and I agree with jstoller that this is a type that could have lots of interesting new uses. If you need any help with this or want a review, ping me.

David_Rothstein’s picture

It seems to me like Barry's approach in #26 might actually have the most use cases, at least once/if it's generalized a bit and given a UI (someday). The reason is that at that point, there is no longer anything about the approach that's specific to text fields. The idea of being able to tell a field "only display yourself when some other field is X" seems extremely interesting...

Also, for the 98%** of sites that never actually used the 'separate teaser' feature in D6, this approach might make the upgrade path cleaner. For those sites, the upgrade path probably doesn't even need to add a separate teaser field at all? This would keep the UI simpler, and not require that a separate textarea show up on the form every time a node is created or edited. ** I am making up the 98% number of course, but I bet it's something like that :)

jstoller’s picture

@David_Rothstein

While in theory I do find that an interesting prospect, in practice I think it will be unnecessarily complex, from a user experience standpoint. I just can't picture a simple, intuitive UI that would allow me to make the display of any custom field dependent on the state of any other custom field. The current Manage Fields page is confusing enough as is. And if the current implementation of this feature relies on some fancy behind the scenes programming that I cannot duplicate with my own custom text fields and the provided GUI, then I think it should be avoided.

For what its worth, perhaps few sites use the separate teaser feature in D6 because it is unintuitive and lacks any instruction on the content creation page. I know the majority of content I deal with would likely benefit from custom teaser text.

I see two "simple" approaches that would be intuitive to a user:

  1. Add the aforementioned "text+summary" field type. You could even provide a mechanism to hide the summary field for those who don't use it. This keeps everything nice and clean, all bundled together in one package.
  2. You could create a special "teaser" field type. When you defined a field instance there would be a drop down selector that let you choose the field within that content type that this field is a summary of. There would then be a checkbox next to the text box on content edit pages that lets you choose to "automatically generate summary" from the chosen body field. If a user checks that box then every time the node is saved, the teaser text is regenerated from the body text and saved to the database. If they un-check the box, it will behave like a custom teaser field.

Either of these would work for me, and either should provide a workable upgrade path.

bjaspan’s picture

re #34: The "simple" option #2 you listed is what we started with and it turns out to be a nightmare to implement. However, you basically just described UI which earlier in your comment you couldn't picture, only in reverse. "When you define a text field instance there would be a drop-down selector that let you choose the field that contains the summary of this field, if there is one."

That said, I did try to create the text_with_summary field type today. It seemed straightforward until I got to the widget. I do not really grok the widget creation code and I ran out of time.

I'm attaching a new patch. This one contains *both* the text_trimmed_if_teaser_empty display formatter previously discussed and my partial implementation of the text_with_summary field type. Presumably only one will survive. For the new code, look for:

field type: text_with_summary
widget type: text_textarea_with_summary
formatter: text_summary_or_trimmed
theme function: field_formatter_text_summary_or_trimmed

The function which is definitely broken is text_textarea_with_summary_process.

If Karen or yched wants to figure out this out, great. Otherwise I'm going to lean heavily back to the code that works.

KarenS’s picture

If you attach a patch, I'll take a look :)

jstoller’s picture

@bjaspan

A teaser field that "knows" where its body field is, and can automatically generate itself based on said body, provides a strait forward and intuitive user experience. Though perhaps similar in concept, a body field that "knows" where its teaser field is and will jump in and act like a teaser if that field happens to be empty, is decidedly unintuitive. If this is abstracted, as has been suggested, so that any field can jump in and act like any other field if the other field is empty, then it will be even less intuitive. It is this UI that I can't quite picture.

As for the relative difficulty of implementing all these solutions, that is beyond my expertise. I'm just looking out for the user experience.

David_Rothstein’s picture

For the UI, I was thinking something along the lines of this might work (similar to what @bjaspan said in #35, basically)?

Give this field an explicit teaser: [DROPDOWN MENU]

If you do not choose one, or if the chosen field does not contain any content, then a teaser will be automatically generated.

Of course, this patch isn't going to have a UI because one doesn't exist yet :) But it does make sense to think about it, to make sure we aren't backing ourselves into a corner or something.

Also, just to clarify, by saying that I thought Barry's idea was interesting because it could be generalized, I did not mean that it would be generalized any time soon... for this patch, the text fields are all that is needed. I just meant that it seems to be a good path to follow because this kind of approach could eventually be reused in other contexts. Presumably, if it is ever generalized, that would only be in the underlying code, not the UI. Different types of fields could provide their own user interfaces for different kinds of situations, so the end user would never need to really "know" that it's all the same, highly generalized idea underneath...

KarenS’s picture

I think generalizing Barry's idea is well worth considering, but there are all kinds of issues that we just don't know the answer to because it is untried and untested. It might work fine or things might break. The other idea, a field designed explicitly to hold two textfields, one for a summary and one for the text is simple, totally in line with things that have been already tried and tested, and is also potentially useful as a field that might be used in other ways.

I don't think it's even that we need to choose one or the other, I think we could benefit from both. But for the immediate task of getting existing body and teaser fields updated, I think the simplest answer is probably the one that makes the most sense -- and the text_with_summary field is the one that most closely matches the current data structure.

I'm curious what kind of problems Barry ran into. If he posts the code I'd be glad to dig into it and see. If there are knarly problems doing it that way I totally agree that we should fall back and use the original code. And I'm completely ready to back down on this whole train of thought if you guys feel strongly enough about going that direction because we need to get this behind us and move on to other things, I'm just making the case for this option.

BTW we *do* have a UI, albeit not a pretty one and not in core, the code in the HEAD of CCK. It will probably need some adaptations either way we go to provide a way to choose and explain what will happen with the teaser, but it is useful as a platform for thinking about how a UI will interact with our fields.

jstoller’s picture

I just think that any feature implementation, especially one so integral to basic content management, needs to take into consideration its impact on the user. And by user I don't mean Drupal Core maintainer. I mean the first time Drupal user who's never touched a line of PHP code in his life, doesn't have the slightest idea how a database works, and is now trying to create some content types for his new website.

From this perspective, choosing between "text field" and "text field with summary" makes perfect sense. How it is implemented behind the scenes makes little difference, as long as the user is confronted with this one, intuitive choice. The body and teaser must appear to be inextricably linked, so even an untrained eye will automatically recognize the relationship.

bjaspan’s picture

Here is the patch I promised in #35.

KarenS’s picture

FileSize
46.19 KB

This needs a little more polishing but is mostly working. I haven't yet incorporated any tests into it (yched has some tests on #375907: Make text field use D7 '#text_format' selector that could be used).

KarenS’s picture

I'm sure Barry would have had a heck of time getting anything to work because the filters were broken completely by the changes in the way filters work in D7, they needed yched's patch at #375907: Make text field use D7 '#text_format' selector. But I incorporated the changes into this patch (other than the tests).

And I updated the HEAD of CCK to work as a UI for this new field, to make it easier to see how it works.

yched’s picture

function text_field_is_empty() doesn't look right : all non-'text with summary' fields are empty ? :-)

I'd vote to keep the format column named 'format' as in D6 test fields. Shorter is better, + would allow easier upgrade.

KarenS’s picture

I used 'value_format' to match what the filter wants to call it, which avoids any need to re-name that field after the filter gets done with it. So that omits a need for the function you have in your patch to save the filter with a different name. We can change it back to 'format', but adds more code.

I didn't even look at text_field_is_empty() yet :)

yched’s picture

I'm aware it saves some code, but I don't think we should let a stupid (ok, really cool and not stupid) form element decide the name of our column for us :-)

KarenS’s picture

I'm incorporating your changes from the other patch at #375907: Make text field use D7 '#text_format' selector, fixed the text_field_is_empty() (you're right, makes no sense that way), and found some more code that could be ripped out to simplify things, some leftovers from D6 that don't have any purpose in the current code:

    // The following values were set by the field module and need
    // to be passed down to the nested element.
    ...    
    '#field_name' => $element['#field_name'],
    '#bundle' => $element['#bundle'],
    '#delta' => $element['#delta'],
    '#columns' => $element['#columns'],

I'm doing a little more testing and will post it soon.

KarenS’s picture

FileSize
50.35 KB

OK, generally working now. You can use it out of the box to create body and teaser and they work, and you can use the CCK UI to add the text_with_summary fields to your content types and they work. You can even use the HEAD version of Devel Generate to generate content for all these fields if you comment out the place in Devel Generate where it tries to create body and teaser fields.

There are two exceptions in the testing, both related to the filter handling, so something may still need work there, but visually the filters are behaving correctly.

KarenS’s picture

FileSize
49.38 KB

Oops, I left in some changes I made in the Filter module while trying to get the filters working that shouldn't be there, so here we go again.

mfer’s picture

subscribe. Looking forward to following this one up with #396006: Addition of a node--teaser.tpl.php by default

bjaspan’s picture

This patch removes body and teaser columns from the node_revisions table. Today I noticed that it does not remove the format column, so I removed it. Then, I discovered that node_access() contains this code:

  // If the node is in a restricted format, disallow editing.
  if ($op == 'update' && !filter_access($node->format)) {
    return FALSE;
  }

This should be removed along with other assumptions about the body field. However, this made me wonder how Fields/D6 CCK handles editing text fields when the editing user does not have permission for the selected text format. AFAICT, the answer seems to be that if you do not have permission to use the selected format, text.module's widgets change it to the default input format. This is different than node's current behavior for body.

Should we preserve node's current behavior in text.module or replace it with text.module's current behavior?

bjaspan’s picture

Oh, by the way: Nice work, Karen! :-)

catch’s picture

There's a patch somewhere, which I can't find right now, which disables the textarea instead of denying access to edit the node - we should definitely do that instead of hiding the edit tab though, and it'd allow for consistent treatment.

KarenS’s picture

Hm, I don't think I realized the Text module was handling filter permission differently from core. I imagine we should match what the node module does. There is another patch around somewhere to allow the user to edit the rest of the node if they don't have permission to use the filter (the old behavior has been that they cannot edit the node at all). So with that change they would be able to see the contents of the field but not edit it if they don't have filter permission, which makes sense to me.

KarenS’s picture

Crosspost with catch, but we had the same idea :)

KarenS’s picture

KarenS’s picture

Another related issue is #366416: Field Permissions UI, which has not even been started.

bjaspan’s picture

Using Karen's most recent patch, though modified to remove the format column from node_revisions and to remove the code I referenced in #51:

* Create a node as uid 1 with text format Full HTML
* Grant anon users permission to edit that node type, but not to use Full HTML
* Log out
* Edit the node as anonymous
* Get the error message: "Notice: Undefined index: value_format in text_field_widget_validate() (line 647 of /Users/bjaspan/drupal/head/modules/field/modules/text/text.module)."

KarenS’s picture

That's a leftover from changing the code to match Yve's filter patch in #45-#47, I apparently missed a place where my value needed to be changed back to his value. It should be 'format' everywhere now instead of 'value_format'.

No time to re-roll now, but I can do it later or you can grep for other places that were missed, if any.

yched’s picture

#58 might be a bug in #375907: Make text field use D7 '#text_format' selector. Will check that case.

KarenS’s picture

@yched, I don't have time to check this now, but you also might be sure that the filter doesn't get goofed up in previews.

yched’s picture

Latest patch in #375907: Make text field use D7 '#text_format' selector fixes #58 (#411596: filter format form when only 1 format available is needed as well).

re Karen #61: OK, checked, previews work fine.

bjaspan’s picture

Here is a new patch incorporating the changes I mentioned in #51:

* node_configure_fields sets the display_summary instance setting to TRUE. It isn't clear to me why we wouldn't want this on by default.
* Remove $node->format check from node_access()
* Remove format column from node_revisions schema

I'm not up to speed on #375907. Should we just wait for that to be committed and re-roll this patch when it is done?

I still feel like this patch needs an upgrade path, though the new text+summary field type makes it even easier (mostly just a copy). Other than that, what else do we need here?

KarenS’s picture

I set display_summary setting to FALSE to mimic the current behavior -- you don't ordinarily see the teaser field in addition to the full text field, especially if you are just populating teasers automatically. I can go either way on that, but I thought the upgrade path would feel more normal that way. On the other hand, without a UI there is no way to set it on now, so that might be the reason to do so.

I'm not totally sure either which parts of #375907 belong in here -- that issue got pretty convoluted. yched, ideas??

An upgrade path should be fairly easy, maybe we should go ahead and add it in, but it could also be a followup patch.

bjaspan’s picture

The current "when to display the summary field" logic is:

  $display = !empty($element['#value'][$field_key]) || !empty($instance['settings']['display_summary']);

That is, if there is text in the 'body' field, show the body field's summary box. This means that to create a node with a body and distinct teaser, you have to create the node then edit it to add the teaser. That doesn't seem right, which is why node.module now always sets display_summary to true.

I'm wondering if the condition on $field_key should really exist at all. When would we want a summary field to appear only after the object is saved with a value in the text field?

KarenS’s picture

This was an attempt to workaround situations caused by the teaser splitter -- some summaries have specific values different than the body and must be shown because they already have content while most others just use the trimmed values. I figured we didn't want to show the summary for new nodes or new content types or any other time the teasers don't have different content so they will just default to use the trimmed value as we have always done in the past.

None of this would be necessary if we had a UI so that it is configurable, I was trying to make it default to something that would make sense for those old values. Considering the confusion this is causing, probably the best default is to always show it, which is not what I did. We do need a setting for this so that it is possible to let it be configurable because it works very nicely in the CCK UI where the user can decide whether or not to show the summary, it's just awkward figuring out what to do during the update when we have to set a default that will work whether or not the teasers have their own content.

yched’s picture

@@ -94,9 +136,11 @@ function text_field_validate($obj_type, 
(...)
  foreach (array('value', 'summary') as $column) {
+        if (!empty($item[$column])) {
+          if (!empty($field['settings']['max_length']) && drupal_strlen($item[$column]) > $field['settings']['max_length']) {
+            form_set_error($error_element, t('%name: the value may not be longer than %max characters.', array('%name' => $instance['label'], '%max' => $field['settings']['max_length'])));
+          }

We should probably state the exact column (value or summary) having the error in the error message - and make sure the right form element gets highlighted. The latter should be much easier when #369964: Refactor field validation and error reporting (finally) lands, so maybe just a TODO note for now ?

About #375907: Make text field use D7 '#text_format' selector (sigh...). I really wish it would be committed quick now. *If* the 'body as field' patch can work the "old school" way (direct call to filter_form() like current text widgets) - and off-hand I don't see any reason why it couldn't -, then maybe the easiest way is to stick to it right now. When one of the patches gets in, the other one will need a reroll - shouldn't be that much of a change either way.

KarenS’s picture

Or rename the format column to value_format, which is what I did originally. That takes care of any need to rename it and it did seem to be working.

yched’s picture

Yes, I know :-). I hate to be a pain, but I really think having our column name enforced on us by purely incidental 3rd party code is not right. If #text_format doesn't let us store our data the way we want, then #text_format is bad. Then again, that's only my personal feeling...

bjaspan’s picture

For your experimental pleasure, here is a first pass at the upgrade path. See node_update_7003() in node.install. I tested this by creating nodes in D6 with devel.generate and updating to D7. Hint: Disable devel.module before trying the update or the update will fail. You also can't have admin_menu enabled or even in your sites/all directory or the update will fail.

This update function needs to use the batch api to break itself up into multiple calls since on large sites it will take while.

bjaspan’s picture

I just added this bit too, not bothering with a new patch yet:

        if (!empty($revision->teaser) && $revision->teaser != text_teaser($revision->body)) {
          $node->{NODE_BODY_FIELD}[0]['summary'] = $revision->teaser;
        }
KarenS’s picture

Love the comment:

+  // OMG, do not frickin' tell me that in D7 I *still* can't assume
+  // that enabled core modules are fully loaded at this point...

Welcome to our world, this is the kind of stuff we have wrestled with in every CCK upgrade :)

But the upgrade path looks very very clean. Nice!

I don't have time to test yet, but will try to get to it in the next couple days. In particular we should create a variety of teaser values, some using the teaser splitter in the D6 version, and then be sure they all make their way into D7 properly.

KarenS’s picture

@yched, re #69, my point was that we can change the name of the column to de-couple this patch from the other one, then if that gets in we can fix this one to match. IMO that is cleaner than adding in the old filter form. That will give us a fully functional patch for this issue that is not dependent on any other patches.

yched’s picture

About batch update : the nested foreach (foreach node type / foreach node in the type) make this not completely trivial. I think http://drupal.org/node/407446#comment-1378500 can provide an example structure for a similar case.

re Karen #73 : changing db column names now and reverting them later sounds heavier and fragile to me. The rule is to not bother about HEAD -> HEAD upgrade paths, but if we can avoid breaking everyone's test setup... I might be missing something, but it seems sticking to the current 'filter_form() / non-#text_format' code would work just the same. #text_format is 'just' a hint to attach rich text editors, nothing actually relies on this in D7 yet.

bjaspan’s picture

New patch with a batchified upgrade path and the "don't set teaser if it is just the trimmed body" fix from #71.

The field validation patch got committed (yay!) so some merge conflicts occurred in text.module. Karen, can you make sure I restored the text_with_summary behavior correctly?

To test, use devel generate to make 1000 nodes and mysqldump the database. Apply the patch, run the update, and then you can restore the db to try again when the update blows up on you. :-)

Getting close!

yched’s picture

Since the 'field validation' patch got in, we should address the 1st point in #67. Will try later today if no-one beats me to it.

Update : Shouldn't the select query use a sort ? I've always considered that SQL didn't ensure results order when no ORDER BY clause was specified, so theoretically you might end up treating a given row several times. All existing multipass batch code use a 'SELECT * FROM {table_foo} WHERE id > $last_treated_id ORDER BY id ASC" pattern.
+ details :
- the query selects node.type but doesn't seem to actually use $revision->type ?
- no need to prefix $sandbox keys with 'node_7003_', batch API ensures that the sandbox is internal to a given update.

yched’s picture

Attached patch cleans error reporting and messages about 'max_length', as mentioned in #67.
Also removes the 'node_7003_' prefixes in node_update_7003(). Didn't do anything about my other remarks in #76 yet.

Additional remarks :
- Technically, the update should use field_attach_update() instead of field_attach_insert(). The fact that we are indeed *inserting* new field values is not relevant : we are updating existing nodes.
- I think the update could be faster if we didn't do a node_load, but rather built a stub $node object with only nid, vid, type, and the NODE_BODY_FIELD values.
- As a side bonus : both points above would make the update work even when you already have fields on your node types (granted, this not supposed to happen at this point in a true D6 -> D7 upgrade)

- If I run the update straight after applying the patch, the batch fails with error 500, caused by an uncaught exception 'Attempt to add a field type that doesn't exist'. I had to revisit the modules page *and* clear all caches before I was able to make the update run - it seems the cached field type info wasn't refreshed. Not sure that would be a problem in a 'real world' straight from D6 upgrade.

yched’s picture

er, patch.

bjaspan’s picture

New patch. Addresses yched's query comments in #76 and implements full text/summary field validation details from #67 (the new field validation patch did make this very easy).

bjaspan’s picture

I did not translate the new full/summary validation error message. Is this an okay way to do it?

    foreach (array('value' => t('full text'), 'summary' => t('summary')) as $column => $desc) {
            'message' => t('%name: the @desc may not be longer than %max characters.', array('%name' => $instance['label'], '@desc' => $desc, '%max' => $field['settings']['max_length'])),
yched’s picture

Patch crosspost :-) - we can keep on with Barry's latest patch.

I think, however; that '%name: the @desc may not be longer than %max characters' is not good in terms of translatability : depending on the gender of @desc, 'the' will need to be translated differently in French. Since we're dealing with a fixed, hardcoded set of possible values ('value', 'summary'), we should provide two distinct messages IMO.

PS : I committed a fix in CCK HEAD for PHP notices during the update.

yched’s picture

Updated patch :

- uses field_attach_update() and a stub $node in the update, as mentioned in #77. From completely empiric tests, update is about 2x faster.

- Provides 2 separate error messages for 'max_length' on summary and full text, as mentioned in #81. Since the message for an error on 'value' will also be used when there is no 'summary' field, I used 'The text may not be longer than...' instead of 'The full text may not be longer than...'. WIth FAPI error highlightling, this should be clear enough in both cases (w or w/o summary field).

- Additionally, uses two different error codes : the $error['column'] introduced by #80 is not 'standard' and not documented. From hook_field_validate() PHPdoc :
"Each error is an associative array, with the following keys and values:
'error': an error code (should be a string, prefixed with the module name)
'message': the human readable message to be displayed."
Granted, the 'standard' is brand new ;-) and can definitely be amended if needed. But keeping the $errors array standard is important, since in the future we want them consumed by other stuff than hook_field_widget_error() (for exception handling on programmatic saves), so it's best if every field type doesn't come with its own variants.

yched’s picture

Oops, typo in the previous patch.

yched’s picture

Additional remarks after a test with devel_generate - numbered for easier discussion :

1- We could probably make the update a tad faster if we restricted the SELECT to node types where has_body is TRUE.

2- New field type and widget type human names are a bit long. IMO we could have
'Long text with a summary field' -> 'Long text with summary'
'Textarea with summary' -> 'Textarea' (I'm not sure we really need to differentiate this from the 'Textarea' widget used by a regular text field)

3- I noticed that some summaries (trimmed body) end up shorter than their pre-update version. Will try to reproduce and isolate.

4- at the end of the update, nodes that had a custom summary 'not shown on full text' end up with their body_value starting with a <!--break-->. Probably the teaser splitter's fault, but we might want to fix that while updating ?

yched’s picture

#84-3 is caused by devel generate creating texts with <br>'s while the tag is not allowed in 'Filtered HTML'.

This actually raises an interesting point :
Old : $node->teaser was generated by smart truncation of the *unfiltered* body, then filtered at display time : truncate then filter.
New : theme_field_formatter_text_trimmed() and theme_field_formatter_text_summary_or_trimmed() take the filtered ('safe') value and truncate it : filter then truncate.

Also raises a performance issue : previously, truncation was done at save time, now it's done at display time, and is *not* cached.

Since we now have a 'text with summary' field with a dedicated 'summary' column, should we move the truncation logic to text_field_presave() and always populate the 'summary' column ?

yched’s picture

Nitpicking : At the end of the update, what gets displayed is

The following queries were executed
node module
Update #7003
- ALTER TABLE {node_revision} DROP body
- ALTER TABLE {node_revision} DROP teaser
- ALTER TABLE {node_revision} DROP format

That's a little scary :-) Maybe we should add a 'fake' result before - 'body and teaser properties migrated to 'body' field'

bjaspan’s picture

Since we now have a 'text with summary' field with a dedicated 'summary' column, should we move the truncation logic to text_field_presave() and always populate the 'summary' column ?

No, that approach was already rejected. For sites that want to preserve the automatically-generated teasers, this would be a regression; they'd have to edit both the full text and summary. I'm assuming there would be riots.

catch’s picture

If they want automatically generated teasers can't they remove the input for the 'summary' field and use the old (Drupal 5) logic to populate it in the database?

If doing truncation on view turns out not to be expensive, then this'd be a useful thing though - i.e. it'd mean changing teaser settings would actually affect old posts. Should benchmark it though.

bjaspan’s picture

Sure, they could regenerate a new teaser by erasing the old one, but that is still one more step than is currently required to get the current behavior.

I'm not morally opposed to it, I just do not think it will fly with the community.

catch’s picture

Don't think I explained myself well. I meant allowing sites to have the 'summary' field as #access = FALSE and populate it based on the old logic from body each node_save() to save doing it on render. So not actually deleting the field, just not exposing it in the UI, Drupal 5 style.

frankcarey’s picture

If anyone would like to discuss extensions to this idea where user 'mail' and 'name' are core fields, please join the conversation in the fields in core group. http://groups.drupal.org/node/20656

bjaspan’s picture

Status: Needs work » Needs review
FileSize
48.71 KB

New patch attached.

re #82: I'd like to change the field validation specs so that error arrays can contain arbitary data (e.g. like 'column'). I don't think there is any reason not to and it will probably make life a lot easier for some widgets. That's a separate issue, though.

re #84.1: I added code to filter on node types with bodies. I'm not convinced it is worth the complexity.

re #81.2: 'Text area with a summary' is the same length as 'Text area (multiple rows)'. Shrug.

re #81.4: New patch strips <!--break--> at the beginning of node bodies.

re #85: You are right that we are now trimming the filtered value instead of the raw value. It's a change. Does it matter? In the mos common case, plain text gets marked up with paragraph and line break tags, so the teasers will end up shorter. If people don't like the shorter teasers, they can increase the teaser_length variable; see #88, doing that will now affect all posts, not just new ones.

re #86: Added a reassuring message about body and teaser fields being migrated.

re trimmed teasers not being cached: Yes, that's true, and it is change. Does it matter? The downside is a potential performance hit, though it is CPU not DB, and it is a fairly simple text operation. The upside is that teaser_length changes take effect everywhere. Do we need to do a benchmark before this issue can proceed? 'Cause otherwise, I think we're done.

Status: Needs review » Needs work

The last submitted patch failed testing.

bjaspan’s picture

Wow! That's a lot of failures. :-)

bjaspan’s picture

This patch fixes the test failures in the field module. One was simply because the field info tests were assuming there were no core fields, and now there is one. The other involved check_markup. In text_field_sanitize(), we were calling:

      check_markup($item['value'], $format, isset($object->language) ? $object->language : $language, $check)

$object was a field test entity so has no language, thus we used the global $language, but that is an object and check_markup() expects a string langcode. So now we pass $language->language in that case. I'm not familiar with the current language-handling logic; is what I did the correct fix?

The other failures presumably identify other places in the code or tests that assume $node->body is what it always has been. We'll just have to fix them one at a time. whee.

I'm thinking it will be easier to review this patch if the current functional patch stays as is, and we have a separate patch file containing all the fixes to tests and code throughout the rest of Drupal. If I post two patch files on a single issue comment, will the testbot apply both before running tests?

yched’s picture

Patch missing ?

About $language : right, that's a bug in text.module, was also fixed in the #text_format patch. $language->language should be fine AFAIK.

About the non-field test failures : ouch. Do you want to go on a team fix ?
We can list the test sections with fails, X says 'I'll look into the 'forum' fails', then posts a '.patch.txt' just for those fixes, which then gets (manually, I think) merged in the main patch ?

David_Rothstein’s picture

Actually, that bug with check_markup() has now been "fixed" at least three different times by people writing larger patches. I remember fixing it as part of a patch I was writing a while ago, @yched also fixed it in the #text_format patch, and now it's showing up again here...

So I split it off to its own separate issue instead: #420904: text_field_sanitize() does not handle languages correctly. Should be possible to get that one committed quickly :)

bjaspan’s picture

Sorry, here is the patch for #95.

re #96: Yeah, we probably want a team effort to fix the body issues everywhere in core. However, I wonder if we should first get a review/approval for the approach from Dries and/or Angie, to make sure the way we are planning to do the body field is okay before we go to all the work of fixing all these other body assumptions.

moshe weitzman’s picture

Took a quick look at the update function:

- comment mentions field_attach_insert() but we changed to field_attach_update()
- the code below was state of the art in D5.

  $context['total'] = db_result(db_query("SELECT COUNT(*) FROM {node} n INNER JOIN {node_revision} nr ON n.vid=nr.vid WHERE n.type IN (" . implode(',', array_fill(0, count($body_types), '%s')) . ")", $body_types));

In D6, we added db_placeholders(). In D7, a count query looks something like(from database_test.inc)


  $query = db_select('node');
  $query->join('node_revisions', 'nr', 'n.vid = nr.vid');
  $query->condition('n.type', $body_types, 'IN');
  $count = $query->countQuery()->execute()->fetchField();
Dries’s picture

1. This approach looks good but I was somewhat confused by the difference between text_with_summary and text_summary_or_trimmed. These names are not self-explanatory at the moment. The difference is not clear from the label definitions either. I think the terminology and naming could use some more thought.

2. 'label' => t('Summary or Trimmed'). Trimmed should be trimmed.

3. text_with_summary and text_summary_or_trimmed are slightly inconsistent in naming. One has 'with', the other omits 'with' in the name.

4. The name text_summary_or_trimmed confused me. I initially thought the summary was equivalent with trimmed so I didn't get the 'or' part. It is not properly explained, and I wonder if we can find a better name (see also 2).

5. A couple of spacing issues; e.g. no spaces at n.vid=nr.vid.

6. The terms 'teaser' and 'summary' are used inconsistently, it seems.

bjaspan’s picture

Attached patch addresses comments from #100 and #101.

What I would most like at this stage is approval for the basic approach of the patch before we go updating code all over core to stop assuming that $node->body exists:

* Create a new text field type called text_with_summary. Normal text fields have a value column for the text. Text with summary fields have a value and a summary column. (Both have a format column.) The new field type makes it easy to migrate the body, teaser, and format column from node_revision into a single new field.

* The new field is called node_body, e.g. $node->node_body[0]['value', 'summary', 'format']. The fact that "node_body" is longer to type than "body" is irrelevant because almost nobody should ever type it, because it should no longer be a special case (*).

* On node forms, we show all text_with_summary fields (including Body) as a fieldset containing two text areas, Summary and Full. If the Summary is blank, on view the field is rendered as a trimmed version of Full.

* Summary splitting off of the full version occurs during VIEW, not during save, and is not saved in the database (if you specify an actual Summary field, that text is saved, of course). This means that displaying a summary view might take slightly longer because we are performing the trim operation every time instead of caching it, but it also means that changed teaser settings apply to all existing fieldable objects, not just one ones.

* The teaser splitter is completely yanked, no saving throw.

(*) There are still going to be places that have to know about node bodies in some way. For example, blogapi, because blog posts are assumed to have bodies. Perhaps the solution here is that blog.module creates a custom node type with a custom blog_body field, and blogapi knows it can use that. This is an example of the kind of fixing we are going to have to do all over core, and is why I want approval of the approach of this patch before we do that work.

KarenS’s picture

I think part of the confusion Dries mentions in #100 is that we have a field name 'text_with_summary' and a similar but different formatter name 'text_summary_or_trimmed'. They are describing two different things. The field describes the fact that we have a field that has both a text field and a summary field. The formatter describes a specific way of dealing with the summary field, displaying its value if it is not empty, otherwise displaying a trimmed version of the full text field.

The names are pretty good descriptions of what these elements do, I'm not sure we can do better. I think Barry has done a good job explaining that better in the latest patch.

The node module is still creating a 'teaser' in the latest patch, using the text_with_summary field and the text_summary_or_trimmed formatter. I suppose there is a question about whether the node module should now be creating a 'summary' to eliminate all references to 'teaser'. That may be the remaining confusion. I'm not sure there is anything wrong with continuing to call the summary view the Node module creates a 'teaser', but I think that's the remaining issue.

I interpret #100 to mean the basic concept is OK, is that right Dries? If so, we need to resolve the question about using 'teaser' in the Node module and then start rooting out all the core dependencies on $node->body.

catch’s picture

Since the current CCK UI allows us to define which fields and formatter we want in the teaser view, I think it'd be good if the specific behaviour of the body field was renamed to 'summary' consistently to differentiate it from that. While the summary might be the full content of the 'teaser' out of the box, it won't always be. Note I didn't look at how this impacts the patch yet.

yched’s picture

+1 for settling on 'summary' for the 'short version of a specific text_with_summary field', and dedicating 'teaser' to the display mode (as opposed to 'full node').
The patch does a good job at that, with two exceptions :
- 'This field stores long text in the database along with optional summary/teaser text' would then become '...optional summary text'.
- variable_get('teaser_length', 600);
"What used to be called 'teaser' is now called 'summary', but the variable 'teaser_length' is preserved for backwards compatibility."
Couldn't we rename the variable in the update function, and notify the change in the D7 upgrade page ?

bjaspan’s picture

re #102: I specifically avoided explaining to Dries that text_with_summary is a field type and text_summary_or_trimmed is a formatter #101 because I wanted to see if the comments I added to the code spoke for themselves.

re #103: I think catch identifies an important distinction. "Teaser" is a concept (actually just a build mode) defined by node.module. "Summary" is (now) a feature of the text.module via a field type and formatter. A node teaser might contain a text field summary, or it might contain three summaries, two images, and a PIAPT(*). So, we should name things accordingly: the word "summary" should not appear in node module, and the word "teaser" should not appear in text.module. And the word "teaser" should only appear in node module as a build mode at most.

Granted, node module could decide to rename its "teaser" feature to be something else, but if we decide to name it "summary" then we should rename the feature that text module provides just so we keep the distinction clear.

I'm not sure how it happened but some of the node teaser JS splitter code stopped getting yanked out by this patch, so I rerolled the patch again to kill it.

KarenS’s picture

I think it's kind of confusing to tell where we are drawing the line between times we use 'teaser' and times we use 'summary'. We still use $teaser in all our hooks and in the display build mode. Would it make sense to just replace all references to 'teaser' with 'summary'? That might not be that hard to fix, just a find and replace. And then document in the changes that this is the new terminology.

KarenS’s picture

Oops, cross post. OK, the explanation in #105 makes sense to me, that 'teaser' is a concept used by Node module only.

yched’s picture

Status: Needs work » Needs review
FileSize
78.73 KB

- Rerolled
- Updated the '#text_format' part to be inline with #375907: Make text field use D7 '#text_format' selector (handled with #value_callback instead of #element_validate / form_set_value)
- Started to fix a few tests. Would've set to 'needs review' to get an update of where we stand in terms of test failures, but it looks like the bot is on forced vacation.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

A s this patch currently stands it's going to require an extra cache_get() for every node loaded on a page (edit, assuming vanilla core) - i.e. an extra 10-30 queries on the default front page. I just installed an ran the upgrade path to see if it would be benchmarkable but update.php wasn't happy at all, however posting this as a marker that we need to benchmark this before commit. While I'm 100% in favour of it, I don't want to see out-of-the-box Drupal get slower, and the field cache is being touted as a reason to not bother with caching node load on #111127: Cache node_load, whereas in this specific case it's likely to slow things down.

yched’s picture

catch : those additional cache_get()s would be alleviated by the cache_get_multiple() part of #111127: Cache node_load, right ?.
*If* node cache is eventually deemed unnecessary, at least cache_get_multiple() is still precious to have.

catch’s picture

Yeah that's currently the only other place in core where there's a clear use case for cache_get_multiple(), although with the very long cids used in _field_attach_load() it'd be quite messy to implement as it stands now.

yched’s picture

Rerolled, with a few more test fixes.

- Some fails were caused by node_configure_fields() setting a max_length of 50 chars on the body field.
That seems a little short, and I don't think node.module currently enforces a limitation, so I removed that setting.

- 'Module API' tests were failing because they assumed that sorting default modules by filename is equivalent to sorting them by module name. Field type modules break that pattern, since they are located in a subfolder of field.

Investigated, but not fixed :-) :

- Blog functionality
The module still relies on node_prepare(), node_body_field(), which are removed by the patch.

- DBLog functionality
Relies on blog

- Blog API functionality
Relies on blog (?)

- Filter administration functionality
filter_admin_delete_submit() does :
db_query("UPDATE {node_revision} SET format = %d WHERE format = %d", $default, $form_state['values']['format']);
Obviously this line will need to go away, but we won't be able to do an equivalent mass update for text fields.
I need to check what happens in CCK D6 when an input format is removed. Until then, It's safer to leace that line in as a reminder

- Help functionality
The test assumes that all enabled modules come with an admin/help/$module page. Fails because text, number, list, and option modules (now required) do not - they currently don't implement hook_help() at all.
Not sure exactly what's the rule for required modules wrt hook_help, but the test looks a bit blunt...

- Node teaser
unit tests for node_teaser() - should move to text.test, I guess ?

yched’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

David_Rothstein’s picture

db_query("UPDATE {node_revision} SET format = %d WHERE format = %d", $default, $form_state['values']['format']);
Obviously this line will need to go away, but we won't be able to do an equivalent mass update for text fields.
I need to check what happens in CCK D6 when an input format is removed. Until then, It's safer to leace that line in as a reminder

I believe the answer is that CCK does nothing when an input format is removed :) This was one of the reasons for SA-2008-073, in fact. The problem isn't with CCK, but rather with core, since core never gives you a good way to find out when an input format has been deleted.

I have an issue at #428296: Filter system doesn't communicate with modules about altered text formats for dealing with this mess properly in D7 (and it's marked critical), since the underlying problem is that filter.module should not be hardcoding queries to other modules' database tables. So for the current issue, I'd recommend ignoring this problem as much as possible.

yched’s picture

Status: Needs work » Needs review

Yes, obviously CCK keeps deleted formats :-). I meant 'check what happens if you try to display / edit data that uses a non existent format'. Probably nothing good.
Agreed about dealing with this in a separate issue.

Other that than, we went from 434 fails to 266, which is good. New exceptions, though. One recurrent exception is 'Undefined index: #id in theme_fieldset()', which I find intriguing. I don't think we had those in the previous iteration.

yched’s picture

FileSize
100.54 KB

Updated patch.

- Filter administration functionality:
Removed the UPDATE {node_revision} query in filter_admin_delete_submit(), as discussed in #116 / #117
- Help functionality:
Added a check to test the admin/help/$module path only for modules that actually implement hook_help().
- Node teaser:
moved tests for teaser/summary generation from node.test to text.test, and uses the new text_summary() function.
- Fixed the numerous warnings caused by a change in the theme_fieldset() function from the 'vertical tabs' patch (the function now requires the incoming $element to have an #id)
- Added a system update function, to enable text, number, list, option modules.

Investigated 'Book functionality':
Tricky. The fails are caused by the body_field instance being marked as widget_active = FALSE, because of module install workflow at setUp.
The 'book' content type (and thus the body instance for 'book') is created in book_install(). By this time text.module is installed, but not 'active', because drupal_install_modules($module_list) installs all modules, *then* marks them as active and refreshes module_list().
So $widget_active = module_exists($widget_module); in _field_write_instance() returns FALSE.
All tests pass if I replace this line with $widget_active = drupal_function_exists($widget_module . '_field_widget');
Not sure that this is the desired fix, nor of the exact consequences of this change. This area has always been touchy in CCK.
The patch leaves _field_write_instance() untouched for now.

I'll leave the bot confirm, but the remaining failures should be:
- Book functionality: see above
- Blog functionality: blog type still relies on node_prepare(), node_body_field()
- Forum functionality: forum type still relies on node_prepare(), node_body_field()
- DBLog functionality : relies on Blog node type
- Blog API functionality : relies on Blog node type, and blogapi.module still hardcodes behavior on $node->body / $node->format

Also :
- node.api.php: hook_view() PHPdoc and sample code still refer to node_prepare()
- search.api.php: hook_update_index() sample code still refer to node_prepare()

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
102.63 KB

Oops, previous patch left out the help.test fixes.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Just ran the update with an empty node table, and got:
Warning: Division by zero in node_update_7003() (line 500 of /home/catch/www/7/modules/node/node.install).

It's unlikely that'll happen in real life but probably worth fixing.

I then did a clean install and manually created 10 nodes, then inspected with devel:

It looks like check_markup() is being called for both body and summary on the front page - this means 20 calls to cache_get() from {cache_filter} instead of 10 currently. The 10 additional cache_get() calls for the field cache on a default install means we add a total of 20 queries to the out of the box front page - i.e. from around 40 to 60.

We should be able to save 9 of those with #333171: Optimize the field cache, and the other 10 with something like #369011: Performance: do check_markup() in text_field_load() - but needs to be borne in mind.

catch’s picture

FileSize
5.7 KB
5.7 KB

Quick benchmarks:
HEAD - 10 nodes on front page:
10.50 requests / second

Patched:
7.40 requests / second

yched’s picture

FileSize
101.88 KB

Attached patch should fix the 'Division by zero' error mentioned in #122, and another bunch of test exceptions.

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review

re #122:
check_markup() is run on both summary and full text no matter what because of the way hook_field_sanitize() currently works. I gave a few details in #369011-6: Performance: do check_markup() in text_field_load()

catch’s picture

Since #333171: Optimize the field cache only clawed back < 5% of around a 40% slowdown I did some initial before/after profiling.

Doesn't have anything specific to do with this patch (or most of it looks not to anyway), so opened #438570: Field API conversion leads to lots of expensive function calls.

Dries’s picture

The performance tests that catch shared in #123 are worrisome and I'd like to see us spend time on that -- or at least have the fields API maintainers comment on it.

catch’s picture

Note I've now posted more benchmarks and cachegrind.out files on #438570: Field API conversion leads to lots of expensive function calls.

yched’s picture

FileSize
104.22 KB

Rerolled after recent commits.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
104.19 KB

Forgot to bump the node update number.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Rerolled after this weekend's commit spree.

catch’s picture

Status: Needs work » Needs review

nudging the test bot.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Rerolled.

ugerhard’s picture

Status: Needs work » Needs review

For the Bot.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

FileSize
118.05 KB

Reroll, just so that it can be used as a base for profiling work in #438570: Field API conversion leads to lots of expensive function calls. No need to have test bot run for now.

yched’s picture

FileSize
117.34 KB

Oops, wrong patch.

yched’s picture

Status: Needs work » Needs review
FileSize
130.1 KB

Reroll. Let's see where we stand in terms of test failures.
Note that the failures in book (investigated in #118 above) are fixed by #472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
130.1 KB

A classic - forgot to bump system update number.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
177.76 KB

Rerolled after #369011: Performance: do check_markup() in text_field_load() + did some more work on the remaining failures.

This patch *includes* #472684-16: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks for now, to fix creation of the 'book' body instance in simpletest's setUp (see comment #118 above)
It also adds a missing node_types_rebuild() in setUp(), to fix creation of the 'blog' body instance in simpletest's setUp (blog type is defined in hook_node_info(), simpletest currently doesn't trigger node_type_save() for those). I'll submit that one in a separate patch.

For now, I'll let the bot clarify where we stand - tests tend to timeout on my box.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

OMG ! *2* fails ! W00t !

bjaspan’s picture

Wow!

yched’s picture

Status: Needs work » Needs review
FileSize
467.73 KB

This should take care of the last fails / exceptions.

yched’s picture

Er, all green is way cool, but 467Kb seems a lot compared to 177Kb in #146. Two thirds of the patch seems to be binary cruft inside text_summary(), something strange happened during the patch generation I guess.
It possibly doesn't affect the tests, though, but I'll reroll tomorrow to be sure.

Dries’s picture

Great work, yched. You're rocking it.

- It sounds like there is no consensus yet on #472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks. Should we try to resolve #472684 first?

- Now the tests pass, I'd love to see us spend some more time on performance. The results in #123 showed a 30%-40% performance regression. See you guys in #438570: Field API conversion leads to lots of expensive function calls?

catch’s picture

On the performance issue, while this patch highlights it, I'm pretty sure it's just the cost of having a field attached to a node rather than anything this patch does specifically. Having this patch in core will make it much, much easier to work on optimizing the field API -with no fields converted in core, it's a pain to generate content for benchmarking and profiling

Having said that, while the tests pass, the upgrade path is hosed:

Message: FieldException: Attempt to create an instance of a field that doesn't exist. in field_create_instance() (line 422 of /home/catch/www/7/modules/field/field.crud.inc).
bjaspan’s picture

re #153 point 1: I agree with catch. Unless we are going to ditch the Field API, we know that basically every significant site is going to have some fields on most/all of their node types (just like in D6). Thus, if Field API is causing a slowdown, it will cause that slowdown regardless of whether the body field is converted, because the other fields will cause it too. We absolutely have to do the best we can to fix performance problems caused by Field API, but I do not see why holding up this patch is necessary for that reason.

re #153 point 2: I cannot reproduce the upgrade problem. I created a new D7 db without the patch, devel generated 50 nodes, installed the patch, ran update.php, and it worked. What did you do?

@dries and/or webchick: How should we handle reviewing/RTBC'ing this patch? I wrote the functionality and yched slammed through the test failures caused by changing $node->body. So we're both heavily involved. In the past Angie has said this means neither of us can mark it RTBC. Around #100 we laid out the design and (we hope) got approval for it. So do we need to get someone other than bjaspan and yched to do a full review?

yched’s picture

Not where I can reroll without the binary cruft for now, just a quick note :
- Agreed 100% with catch and bjaspan about performance considerations.
- I do think, however, #472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks needs to be fixed first. We shouldn't bundle this fix with the 'Body as field' patch without being sure of the proper fix - and I personally have little expertise in this area to decide much on this specific issue.
I also have another patch for simpletest setUp() that I need to roll separately.

Dries’s picture

Ok, agreed. I've already bumped the priority of #472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks to critical.

As for the patch review -- I think this is one of those patches that must land, and that benefit from an early commit. Incremental improvements are the magic words. You've both stepped up to provide incremental improvements so I trust this will work. As such, I'll probably "time box" my review to 2 hours, provide feedback, and commit the next version at a specified date and time. That is what I did last time, and while not perfect, I think it worked reasonably well.

If there is API documentation that describes the architecture (highly recommended), I'd use that as a starting point for my patch review. I'll review this patch after #472684 landed, and this patch is rerolled for that.

catch’s picture

@bjaspan - I tried to upgrade from Drupal 6 directly.

Then I did a 6-7 update, applied the patch, and ran update.php - slight variation this time but essentially the same error.

An error occurred.
Path: http://d7.7/update.php?id=8&op=do
Message: FieldException: Attempt to create a field of unknown type text_with_summary. in field_create_field() (line 224 of /home/catch/www/7/modules/field/field.crud.inc).

bjaspan’s picture

That means text.module is not enabled, so either system_update_7024() did not run, or it ran after the node update function that creates the field, or some other module install or caching problem is happening. I'll try look into it later tonight.

yched’s picture

FileSize
109.69 KB

Patch rerolled without the binary cruft. Looks like an UTF-8 encoding error that accumulated during my last few patches, because fixing it brings the size of the patch down to 110Kb (equivalent to #136, which had far fewer test fixes)
The line involved was $break_points[] = array('. ' => 1, '! ' => 1, '? ' => 1, '。' => 0, '؟ ' => 1);, moved from node_teaser() in node.module to text_summary() in text.module.
I replaced the buggy line with a fresh copy from the old code, and checked that the hex versions matched.

Additionally :
- Dries, I should probably mention that (since comment #43 way above) this patch includes #375907: Make text field use D7 '#text_format' selector, which you still had reservations about ;-). Back then, we thought #375907 would be long fixed before 'body as field' comes to a commitable state...
I'll update #375907 with the fix for the notices I mentioned in my last followup over there.

- We still have a design issue about the 'trim' feature - Back in #85 I wrote :

Old : $node->teaser was generated by smart truncation of the *unfiltered* body, then filtered at display time : truncate then filter.
New : theme_field_formatter_text_trimmed() and theme_field_formatter_text_summary_or_trimmed() take the filtered ('safe') value and truncate it : filter then truncate.

The performance implications (smart truncation is not cached) were deemed negligible, and the profiling work done in #438570: Field API conversion leads to lots of expensive function calls confirms that the impact is indeed minor.
The other issue, that I realized recently, is that the htmlcorrector filter doesn't run on truncated text anymore, and thus tags left open by the truncate (when the truncate <strong>happens ->*<- here</strong>) do not get fixed anymore.

So, leaving to CNR to confirm tests after the patch cleanup, but there are still a couple points to address.

bjaspan’s picture

@catch: I started from D6 and am still not seeing that error. What info does update.php give you about the updates performed before the error?

catch’s picture

FileSize
6.54 KB

Attached update output.

edit: just did a cvs update and using yched's latest patch - I still get the error, but if I run update.php again it starts on the node update and I got through it successfully at least once.

catch’s picture

Fresh benchmarks. note the 7-7 upgrade just worked again for me. So for now going to put upgrade issues down to something with my install rather than the patch - could do with a couple more people checking it though really just in case.

Front page, 30 nodes:
HEAD:
Executed 101 queries in 72.98 milliseconds (NB, the field cache gets hit for nodes even if there's no fields enabled).
3.42 [#/sec]

Patch:
Executed 73 queries in 71.71ms
2.86 [#/sec]

Front page, 10 nodes.
HEAD
6.43 [#/sec]

Patch:
5.72 [#/sec]

Single node with no comments (node/n)
HEAD:
12.56 [#/sec]

Patch:
9.66 [#/sec]

yched’s picture

Status: Needs review » Needs work

D6 -> D7 upgrade issues sound like text.module not being marked as active yet by the time the node update is first run.

Back to CNW for this and for the truncate / htmlcorrector issue mentioned in #159.

yched’s picture

I uploaded the 'other simpletest patch' mentioned at the end of #155: #479200: missing node_rebuild_types() in setUp()

yched’s picture

Rerolled after #479200: missing node_rebuild_types() in setUp().

About the truncate / htmlcorrector issue :
text_summary() (formerly node_teaser() ) currently checks the $format for the presence of the PHP filter. It could also check for the htmlcorrector filter and run the truncated text through _filter_htmlcorrector() accordingly ?
_filter_htmlcorrector() will then run twice on a generated summary : once during check_markup() on the full text (cached), then once on the truncated text at display time. _filter_htmlcorrector() is reentrant, so this shouldn't be a problem.
Thoughts ?

yched’s picture

FileSize
109.92 KB

Sigh, forgot the patch...

webchick’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
109.2 KB

Rerolled + bumped system update number.

brianV’s picture

Minor coding style issue - not much harm to leave it...

Missing a space after the first dot operator.

+    $edit[NODE_BODY_FIELD .'[0][value]'] = 'SimpleTest test body ' . $this->randomName(32) . ' ' . $this->randomName(32);

Also, although this isn't part of the patch, we may as well nail this line from node.module since we are in the file anyways. Line 2158 in node.module:

     drupal_set_title(t('Welcome to @site-name', array('@site-name' => variable_get('site_name', 'Drupal'))));

should use the PASS_THROUGH variable in D7 (http://drupal.org/node/224333#drupal_set_title). Therefore, it should be:

     drupal_set_title(t('Welcome to @site-name', array('@site-name' => variable_get('site_name', 'Drupal'))), PASS_THROUGH);
yched’s picture

FileSize
111.08 KB

Updated :
- as proposed in #165, changed text_summary() to apply _filter_htmlcorrector() after truncation if the format includes the htmlcorrector filter. Changed TextSummaryTestCase tests accordingly.

- tested D6->D7 upgrade, went smoothly for me too.
However, the upgrade path relies on the fact that you can create a bundle on a node type defined by a disabled contrib module in its hook_node_info(). field_create_instance still had 'check that $bundle exists' as a TODO :-). I removed it and added a note about why we explicitly do not check this. Disabled node types should be OK, although I did not specifically test it.

- fixed the string concatenation spacing error mentioned by brianV in #170. The drupal_set_title() is really unrelated, so unless webchick or Dries want it fixed it here, I see no reason to include it. It deserves its own patch IMO.

So, I think we're ready for the review Dries mentioned in #156 :-)
(#472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks still needs fixing, though)

Status: Needs review » Needs work

The last submitted patch failed testing.

tstoeckler’s picture

FileSize
103.52 KB

Rerolled.

tstoeckler’s picture

Status: Needs work » Needs review
catch’s picture

$a = 1; what?

+    // Verify that no unexpected instances exist
+    $core_fields = field_info_fields();

missing period.

+   * Tests an edge case where if the first sentence is a question and
+   * subsequent sentences are not. This is edge case is documented at
+   * http://drupal.org/node/180425.

remove 'if' and 'is'.

+  // OMG, do not frickin' tell me that in D7 I *still* can't assume
+  // that enabled core modules are fully loaded at this point...
+  field_init();

lol but can we have a @TODO instead/

+      // TODO: Actually, we probably can assume default field storage
+      // since this is a core update and no contrib modules are
+      // enabled yet, and it would be WAY faster.

Do we want to try implementing this TODO - followup issue is fine but let's not forget it.

None of these are showstoppers so leaving at CNR, only got half way through the patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
103.51 KB

Rerolled for changed in Node type API and adressed 2. and 3. in #175 (coding style issues).

bjaspan’s picture

Actually, I think the "OMG" comment and call to field_init() can be removed now that the file-loading operations that used to be in field_init() are now loose code in field.module. field_init() is just

function field_init() {
  drupal_add_css(drupal_get_path('module', 'field') . '/theme/field.css');
}

which can't matter for update.

Status: Needs review » Needs work

The last submitted patch failed testing.

tstoeckler’s picture

FileSize
103.81 KB

One last try.

tstoeckler’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

@tstoeckler : it's because the node update number needs to be bumped to 7005; 7004 has just been taken by a recent commit.

tstoeckler’s picture

Status: Needs work » Needs review
FileSize
103.81 KB

Gee, that makes sense...

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
99.2 KB

Fixed for the remaining comments in catch's #175, including the "TODO: Actually, we probably can assume default field storage" part.
Fixed a typo in the node_type_*() API reroll: $node_type_clear() -> node_type_clear()

Still get test failures I can't figure out right now. Looks like the node_type_*() API changes had a strange result of breaking stuff like

function blog_perm() {
  return node_list_permissions('blog');
}

$info = node_type_get_type('blog'); in node_list_permissions() returns FALSE. We don't operate in this area, so this is a little puzzling...

Sigh. I can't wait for this to be committed and it becomes the job of other patches not to break it :-/

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
99.26 KB

Attached patch should fix the remaining warnings - except that simpletest currently seems to issue warnings of its own on a vanilla HEAD. Setting to CNR anyway.

The warnings were caused by a combination of the new 'admin role' and #472684: Install non-default modules one at a time to ensure proper module list is maintained during installation hooks (included in this patch to prevent other failures): when default modules are enabled, the node_type_cache gets initialized (no field type). The static cache is then not reinitialized for the installation of other modules (enabled by the specific test) and default_profile_tasks().
The patch fixes this by adding a node_type_clear() call after default modules are installed.

Contrary to what I said in #155 above, I now don't think we should wait for #472684 to be resolved. That thread is currently going nowhere, and keeping the 'Body as field' patch to date and with 0 failures is an incredibly painful task.
The patch has been ready for about a month, and is now struggling with Simpletest false positives. It includes fixes to make all tests pass. If the fixes are deemed less-than-perfect, they can be polished later on by the competent and interested parties, which currently do not seem to care too much (also because the bug can only be reproduced by patching HEAD right now). If needed, the 'right' fix will be easier to fine-tune with the BAF patch in, since any 'wrong' fix will simply cause test failures.

So I think we should commit this asap (before some other HEAD change breaks tests again), so that work on #409750: Overhaul and extend node build modes can start.

bjaspan’s picture

I really wanted to mark this RTBC, and I waive my temporary hold on Field API patches for this one, but the first two comments below prevented me from doing so:

- filter_admin_delete_submit() changes format to default format when a format is deleted. Where is that code now? Don't we eed to do the same thing in text.module?

- Since we do not have translatable fields yet, I think this patch makes Drupal node bodies not translatable at present. Right? Are we willing to trust that translatable fields will get in?

- In drupalCreateNode(), the old default body was randomName(32) and the new default body is the empty string. No biggie, since the tests all pass.

- I'm not familiar with the module.test changes and did not take the time to figure them out.

yched’s picture

- text format deletion : see comment #116-#117 (basically : moved over to #428296: Filter system doesn't communicate with modules about altered text formats)

- I don't think we should blindly trust 'translatable fields' to get in, but this patch shouldn't change anything for node translatability : currently, translated nodes are separate nodes, each with their own value for the 'body' field. The only thing we lose, since we didn't fully implement field_attach_translation_prepare() so far, is the 'body' textarea prefilled with source language when creating a new translation.
Since it doesn't break any test, I suggest we move on with #362021: field_attach_prepare_translation needs to be updated for D7 API in a followup patch - should be pretty straightforward.

- drupalCreateNode() / default body was randomName(32) : don't remember if I did that intentionally. I'll quickly ponder and reroll if needed.

yched’s picture

Actually, drupalCreateNode() still uses $this->randomName(32) as a default value for body. It's just that the default values for body and format are merged separately a few lines below.

yched’s picture

About [module].test changes: those are mainly
- changing from $node->body / $node->format to $node->NODE_BODY_FIELD[0]['value'] / ['format']
- changing from $edit['body'] / $edit['format'] to $edit[NODE_BODY_FIELD . '[0][value]'] for drupalPost() forms.
Additionally :
- help.test: stop assuming every core module comes with a help page
- module.test: fix the 'modules sort' tests to account for the nested folder structure below /modules/field

catch’s picture

Since we do not have translatable fields yet, I think this patch makes Drupal node bodies not translatable at present. Right? Are we willing to trust that translatable fields will get in?

Just to back up yched - currently the body field isn't translatable, only entire nodes are. If translatable fields gets in, then the body field gets to take advantage of it, but there's no regression if not.

The filter code needs to be fixed generally for Field API (and any other subsystem) and shouldn't hold this up - in fact it's important to get patches like this in so that we can make better progress on issues either with Field API itself, or exposed by it, which would otherwise fall by the wayside without any actual fields in core.

And I think this should go in before the name/numeric ID patch as well - looks like there's unanswered questions over there whereas this is just in re-roll hell now. I haven't reviewed the most recent patch though so not actually marking rtbc.

bjaspan’s picture

Status: Needs review » Reviewed & tested by the community

My concerns are addressed.

bjaspan’s picture

Brief architecture summary as requested in #156:

This patch basically removes all the assumptions about $node->body. The body and teaser columns are removed from the node_revisions table. All the code for dealing with body and teaser, including the splitter (yay), is removed from node.module.

In its place, we create a new field whose name is the defined constant NODE_BODY_FIELD (currently 'node_body'). The field is of type text_with_summary. It stores body and summary as two separate database columns. On an edit form it renders as two text areas. On view, it renders as body in full view, and summary (or trimmed body if summary is empty) in teaser view.

$node->body used to be used for two complete different purposes. If $node->body was being used in "field mode" (e.g. just the contents of the body column), the code now uses $node->{NODE_BODY_FIELD}. If $node->body was being used as "the fully rendered node" (for example, for search indexing, RSS output, etc.), the code now uses $node->rendered. In other words, $node->content += field_attach_view('node', $node): add in the structured content array for all fields on the node, including NODE_BODY_FIELD as a non-special case; $node->rendered = drupal_render($node->content): get me the rendered HTML so I can index it, etc.

A lot of test cases made assumptions about $node->body too, and those have been updated as described in the previous paragraph.

The content type UI remains the same: node types have a text field for "Body field name." If you fill one in, an instance of NODE_BODY_FIELD is created for that node content type (which Field API calls a bundle). If you empty it, the instance is removed. This is handed by node_configure_fields() which is pretty much the only code in node.module that knows about the body field at all. If/when we have a UI in core, NODE_BODY_FIELD may go away entirely, to be created by the profile and managed with the Field UI. A lot of tests assume nodes have bodies, so they would have to be updated in that case. blogapi.module would have to ensure a body field because the API it implements assumes one. etc. We may find that keeping a hard-coded body field is just simpler.

An upgrade path is included.

I think the patch is primarily the fault of four people: I wrote the initial code, Karen (IIRC) thought of and then implemented the text_with_summary field type, yched fixed all the tests, and catch did a ton of performance testing. If I'm missing anyone, speak up so you can be blamed when it all goes to hell, too! :-)

moshe weitzman’s picture

This is RTBC from me too. We need folks using the field system ASAP so that we can deliver a highly functional fields api by september.

Dries’s picture

Happy to commit this later today -- want to give it a good look.

I think it is slightly odd and seemingly unnecessary that we use a define NODE_BODY_FIELD -- we don't do that for other fields.

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
FileSize
103.36 KB

Re-rolled for system update conflict only.

yched’s picture

FileSize
106.52 KB

Patch hunk that removes node_teaser(), node_teaser_js(), node_teaser_include_verify() was lost in latest reroll (that hunk often fails to apply because of UTF8 encoding, I think).

Note that catch's patches are probably more reviewer friendly, because my Eclipse can't diff with -p.

bjaspan’s picture

Dries:

I think it is slightly odd and seemingly unnecessary that we use a define NODE_BODY_FIELD -- we don't do that for other fields.

I do not think there are any other "core fields" yet (fields created and managed by core modules that need a globally constant field name) so there is no other field that logically would have a defined name yet. We could certainly just use a string constant like 'node_body' everywhere but, well, ... go read http://jaspan.com/dx-files-improving-drupal-developer-experience. :-)

As for why it is called 'node_body' and not 'body', there are two reasons: (1) We haven't resolved the issue of field and bundle namespacing, but ultimately the answer is going to be "prefix everything with the name of the module that creates it," and (2) I specifically did not want to use $node->body so as to ensure that any code referencing that property gets an "undefined property" error.

Dries’s picture

Awesome patch, but I already mentioned that. Good work all! :)

Some comments:

  • I'm not going to obsess about it, but for me, NODE_BODY_FIELD actually makes things harder. In other words, I disagree that it is a DX improvement in this particular case.

    1) There now is a disconnect between the generated HTML code that uses 'node_body' and the source code that uses NODE_BODY_FIELD.
    2) It is asymmetric with all of the other node properties, making it feel sloppy instead of clean, especially in array listings.

    To me, 'body' can remain 'body'. I don't see how it would clash (it is defined by core).

  • When on the subject of DX, it would also be great if, at some point, we could clean up the many '[0][value]'s a bit. I believe there is a patch for that in the queue somewhere (e.g. input format filter clean-up). We can worry about that later but it is certainly a DX issue.
  • The PHPdoc of node_configure_fields() is lacking. The function name wasn't self-explanatory so I had to read the code. Adding some PHPdoc would be helpful.
  • With this patch, we started to mix 'teaser' and 'summary'. I'm OK with that but could be a DX issue.
  • I found text_field_widget_error() to be a little cryptic at first. The old error handling was easier.
  • I don't understand this change to hook_update_index and as a result, I might not understand the impact on search:
    -  $result = db_query_range('SELECT n.nid, c.last_comment_timestamp FROM {node} n LEFT JOIN {node_comment_statistics} c ON n.nid = c.nid WHERE n.status = 1 AND n.moderate = 0 AND (n.created > %d OR n.changed > %d OR c.last_comment_timestamp > %d) ORDER BY GREATEST(n.created, n.changed, c.last_comment_timestamp) ASC', $last, $last, $last, 0, $limit);
    +  $result = db_query_range("SELECT n.nid FROM {node} n LEFT JOIN {search_dataset} d ON d.type = 'node' AND d.sid = n.nid WHERE d.sid IS NULL OR d.reindex <> 0 ORDER BY d.reindex ASC, n.nid ASC", 0, $limit);
  • You forgot to update the CHANGELOG.txt
bjaspan’s picture

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

New patch attached. I made the following changes:

* Replaced NODE_BODY_FIELD with 'body', using these commands (order is important):

find modules -type f | xargs perl -pi -e "s/\[NODE_BODY_FIELD\s*\.\s*'/['body/g"
find modules -type f | xargs perl -pi -e "s/->NODE_BODY_FIELD/->body/g"
find modules -type f | xargs perl -pi -e "s/->{NODE_BODY_FIELD}/->body/g"
find modules -type f | xargs perl -pi -e "s/NODE_BODY_FIELD\s*\.\s*'/'body/g"
find modules -type f | xargs perl -pi -e "s/NODE_BODY_FIELD/'body'/g"

NOTE: It is possible these changes will new failures. I saw some code like "$node->NODE_BODY_FIELD" which is not the same as "$node->{NODE_BODY_FIELD}" and so might not have been doing what we expect. We'll see what the testbot says.

* Added better PHPdoc to node_configure_fields().

* Updated CHANGELOG.txt.

* text.module defines a field with body and summary. node.module chooses to present "summary" as "teaser" via the UI. Other modules may do differently. If summary is used inconsistently within text.module, or teaser within node.module, that should be fixed. I actually saw one inconsistent use in text.module and fixed it: "This field stores long text in the database along with optional summary/teaser text." (I removed "/teaser".)

Addition replies to your comments:

* The old error handling might have been easier, but it was also wrong, and made field validation on save impossible.

* I don't understand the change to the update_index query either.

I am setting this patch batch to CNR so the testbot will go at it, but in fact its CNW because it looks like some of the hunks removing (e.g.) node_body_field() got lost again. I'll see about looking into this later if no one else does.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

:-( tests (but agreed on the NODE_FIELD_BODY changes)

- clean up '[0][value]'s: I agree that $node->node_body[0]['value'] is not as cool as $node->body, but I'm not sure what can be done about it. I couldn't think of any good way to add shorthands / sugar around Field API data structure so far. I don't think I see what patch ongoing Dries refers to.

- teaser / summary: #103-#105 settles an excellent distinction (summary = truncated version of a longer text, teaser = display mode for a node or another fieldable object). I found the patch fairly consistent with that, except for legacy db variables ('teaser_length', 'aggregator_teaser_length', 'feed_item_length' = 'teaser'), which I think should be messed with in a followup patch if at all.

- It seems noone understands the hook_update_index change, which is strange :-D.
[edit: I see Barry removed the change in his latest patch]

plach’s picture

And if translatable fields land (and I hope they will :), this is going to be $node->node_body[$language][0]['value']...
If one day we will be able to do $node->get_node_body(), where the accessor method is defined as function get_node_body($language = FIELD_LANGUAGE_DEFAULT, $delta = 0, $column = 'value'), I'll be a happy man :)

yched’s picture

Status: Needs work » Needs review
FileSize
110.53 KB

- removed node_body_field() (again)
- redeleted some lines in node_submit() and node_preview()
It seems several hunks got lost between #171 and #177 (~10Kb drop in file size). I made sure every bit of #171 was added back in.
[edit: another proof that this patch is a major pain to reroll ;-)
Note that this patch is slightly smaller than #203 while it has additional chunks, because #203 has "diff -p" labels]

Let's see what the bot says.

The last submitted patch failed testing.

yched’s picture

FileSize
111.19 KB

That's better :-). Attached patch should fix the last exception.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Let's rock.

Dries ? :-)

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Committed to CVS HEAD. A-w-e-s-o-m-e. :)

Marking 'code needs work' until we updated the upgrade documentation in the handbook.

Thanks for the hard work and persistence.

catch’s picture

bjaspan’s picture

It should have been obvious to me but wasn't, so: the upgrade documentation Dries mentions that we need is for http://drupal.org/node/224333 (does [#224333] work for non-issues?). It's not 100% clear to me what we should in the upgrade docs, so let's plan it here.

My thoughts of things to say:

* $node->teaser is gone completely, so modifying or setting it will no longer have any effect.

* $node->body still exists, but it no longer corresponds in any way to "the content of the node." Modules should not ever assume it exists or treat it specially in any way. It's existence and nature is now no more than a configuration option.

* If you *really* want to screw with $node->body, it is a single-value Field API field of type text_or_summary. So, $node->body[0]['value'] and ['summary'].

* If you want to create a new content type with a textarea with summary like body, use the text_or_summary field type, the text_textarea_with_summary widget, and the text_summary_or_trimmed display formatter. Link to http://drupal.org/node/443536 ([#443536]) to which we should add a HOWTO for this.

What else?

bjaspan’s picture

Priority: Normal » Critical

With the code committed, the docs are critical.

bjaspan’s picture

Title: Body and teaser as fields » Upgrade docs for body and teaser as fields
Component: node system » documentation
yched’s picture

I guess we should list removed functons:
- some functions were internal, tied to the JS splitter- should we mention that feature is gone ? I don't think it affects developpers...
- node_prepare() is gone because $node->body and $node->teaser are not "special" anymore and don't have to be massaged.
- node_body_field() is gone because the 'body' form element is generated by Field API
- node_teaser() was renamed to text_summary() + the function now runs htmlcorrector if applicable
I think the 'teaser vs. summary' nomenclature will be re-enforced and probably better explained when #409750: Overhaul and extend node build modes lands and the upgrade doc is updated.

+ One big win of this patch is the end of $node->body being several things at various time in the load / display workflow, but I'm not sure if or how it should be stated. Is $node->rendered worth talking about ?

moshe weitzman’s picture

Correct me if I am wrong, but $node->rendered is only present when we are indexing and maybe node preview. I am not a fan of $node->rendered. I want rendering to happen as late as possible - during drupal_render_page(). I recognize that earlier rendering is needed during these special cases. But I see no need to advertise this property.

Stefan Nagtegaal’s picture

I'm so sorry if I'm missing everything here... (And I am probably)
But... What is so awesome about this patch? What does it give us?
I really tried to get things sorted out myzelf, but couldn't find out (yet)...

yched’s picture

yched’s picture

Bojhan’s picture

Stefan Nagtegaal it removes teaser splitter.

Lets remove the body thing surrounding it, seems rather useless and ugly.

catch’s picture

kika’s picture

emmajane’s picture

Documentation update:

I've added a draft for the module upgrade text to http://drupal.org/node/546608
Please edit that page and leave a comment here to say if (1) more work is needed or (2) the text is fine as written. Once that text is correct I will incorporate it into the main D6->D7 upgrade page.

@bjaspan mentioned in #214 that a longer HOWTO may also be required? I've added a link to the Field API handbook in preparation for this HOWTO. Draft text would be much appreciated. I can help edit it once the core information is available.

dale42’s picture

Will review for documentation team.

Any and all comments/suggestions still appreciated.

bjaspan’s picture

Comment on http://drupal.org/node/546608:

Body and teaser and not separate fields. They are now a single field, managed by Field API, of type text_or_summary.

dale42’s picture

I've updated http://drupal.org/node/546608

I've included info abut the UI changes because I know there are modules that modify data entry form behavior.

I'm not entirely sure of naming, where do I look to find the list of field/formatter/widget types?

@bjaspan, it's now possible for a node not to have a $node->content['body'] field, correct? I think it's mentioned, but I wanted to confirm. If that's the case, will $node->body still exist? I'm thinking a bullet point should be added about it.

In the issue discussion, there was a comment about retaining $node->body so specific error messages would be thrown by code that hadn't been updated. Do we want a note about that to explain why $node->body was eliminated?

jhodgdon’s picture

I took a look at http://drupal.org/node/546608

A couple of comments:
- I found this section very confusing -- no idea what it means:

The $node->body field still exists but no longer represents the full content of the node, it is now a display-only field containing only the body field contents. Additionally, its data-type changes from text to array. Changes made to this field will not be rendered so it is recommended developers simply ignore it.

- I need a bit more description of what "Field API best practice" is in the next bullet point after that one. Or maybe a link?

bjaspan’s picture


@bjaspan, it's now possible for a node not to have a $node->content['body'] field, correct? I think it's mentioned, but I wanted to confirm. If that's the case, will $node->body still exist? I'm thinking a bullet point should be added about it.

$node->body will exist after node_load() if the node has a body field, and will not if the node does not have a body field.

$node->content['body'] will exist after $node->content is created (e.g. when the node is rendered into the content structured array, I've lost track of which hook does this) if and only if $node->body exists.

In the issue discussion, there was a comment about retaining $node->body so specific error messages would be thrown by code that hadn't been updated. Do we want a note about that to explain why $node->body was eliminated?

I feel like the previous explanation covers this.

Changes made to this field will not be rendered so it is recommended developers simply ignore it.

Changes made to $node->body before the node is rendered will have the same effect as changes made to any other part of $node before the node is rendered.

I'm too far behind on D7 changes to specify exactly how this works, but the bottom line is "$node->body is no longer a special case."

webchick’s picture

Title: Upgrade docs for body and teaser as fields » Body and teaser as fields
Issue tags: +Needs documentation

I spent about 10 minutes tonight trying to find this issue.

Re-titling back to original and using tags to indicate that documentation is needed.

catch’s picture

Priority: Critical » Normal
willmoy’s picture

This affects the processes for making nodes programmatically, and so the doxygen of drupal_form_submit() [1].

I managed to get the $node = new stdClass; method working using $node->title = array(LANGUAGE_NONE => array(0 => array('value' => $title))); but that's obviously a dirty hack. I still can't work out what the proper way would be... calling a field function to generate $node->title, perhaps?

The drupal_form_submit() method has eluded me, it's all to do with needing langcodes in the right places, I think.

@jhodgdon, #229, I think what would be really helpful for me to clarify the oblique reference to 'Field API best practice' is a fields_example.module. api.drupal.org is actually quite impressive on Field API [2], but it's pretty abstract, and an example module might make it a lot easier to grok. It's a big step from just being able to shove stuff in $node->title to where we are now.

[1] http://api.drupal.org/api/function/drupal_form_submit/7
[2] http://api.drupal.org/api/group/field/7

jhodgdon’s picture

The correct place for an example module would be in the relatively new Examples for Developers project http://drupal.org/project/examples (which collects together the examples that were previously in the main Drupal contrib repository). Please feel free to file an issue there. In fact, please file an issue there.

jensimmons’s picture

subscribe

willmoy’s picture

jherencia’s picture

Subscribe.

jhodgdon’s picture

Component: documentation » node system

What's the status of this -- is http://drupal.org/node/546608 ready to go? What is now correct and incorrect on that page?

Also, moving back to the correct issue queue. It should be node system, needs work, tagged "needs documentation".

Aren Cambre’s picture

If anyone feels inclined: #745614: Move teasers to contrib is partly about this issue.

BenK’s picture

Keeping track of this thread...

thamas’s picture

Just wanted you to know that I added a relevant issue: #881018: Body field "Show summary in full view" option

jhodgdon’s picture

changing tag to new tagging scheme

asb’s picture

sub

Heihachi88’s picture

Status: Needs work » Needs review

#16: body-as-field-372743-16.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Needs work

I took another look at http://drupal.org/node/546608

My comments in #229 above still stand. Can someone please clarify? See also comments #230 and #233. If someone can clarify, either by editing the page or commenting here, that would be helpful.

asb’s picture

Re:245: I re-read #546608 a couple of times, and couldn't make any sense of it, either. It would be nice to get some simple explanations for non-devs, as well.

jhodgdon’s picture

For non-devs? I don't think it affects non-devs really. We're talking about documentation for the guides on how to update your modules from D6 to D7.

Speaking of which, does this also need documenting for theme developers?

asb’s picture

According to #293803, the Teaser splitter is no longer existing in Drupal 7.. That, and my (personal) experience with totally different behaving teasers of migrated sites in D7 would suggest, that "Body and teaser as fields" (this issue) might affect non-devs as well. Your milage might vary.

jhodgdon’s picture

If you think some on-line documentation on Drupal.org needs to be written about it, it would be great if you could figure out where it should go (start at http://drupal.org/documentation). Or at least you could file an issue in the Documentation project suggesting that this change be documented somewhere, and someone in the Documentation team can figure out where to put it. I am not clear yet on what changed between 6 and 7 from a user interface perspective, so if someone else is clearer on it, it would be better if they could file the issue.

KarenS’s picture

Status: Needs work » Reviewed & tested by the community

I made some updates to http://drupal.org/node/546608 to try to clarify it. I also deleted the comments and incorporated them into the text. I think the 'DRAFT' in the title could be removed now and it can be moved wherever it belongs in the documentation. I'm not sure where it belongs so I didn't do that.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks Karen! Much clearer.

I've moved this into
http://drupal.org/update/modules/6/7#body_teaser_fields
and archived the old page.

Status: Fixed » Closed (fixed)
Issue tags: -Fields in Core, -Needs documentation updates

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