To have a working core-only solution to translate nodes through translatable fields we need to convert also the node title to fields. This has performance implication so one possible solution might be to keep the existing node.title column and the $node->title field in the node data structure, introduce an additional $title_field, keep them synchronized and use the translatable field value only on rendering.

CommentFileSizeAuthor
#100 tf3-node-title.patch771 bytesdmitrig01
#99 tf3-node-title.patch772 bytesdmitrig01
#97 node_title.patch108.61 KBcatch
#96 node_title.patch108.69 KBcatch
#93 title_as_field-557292-93.patch108.83 KBpeximo
#90 title_as_field-557292-90.patch107.6 KBpeximo
#84 title.patch108.09 KBcatch
#81 title_as_field-557292-81.patch110.24 KBpeximo
#79 title_as_field-557292-79.patch110.25 KBpeximo
#76 title_as_field-557292-76.patch110.75 KBpeximo
#73 title_as_field-557292-73.patch108.57 KBplach
#71 title_as_field-557292-71.patch109.92 KBplach
#70 title_as_field-557292-70.patch109.91 KBpeximo
#68 title_as_field-557292-68.patch110.04 KBpeximo
#66 node-view.jpg64.27 KBpeximo
#66 comment-preview.jpg62.37 KBpeximo
#61 title_as_field-557292-61.patch111.34 KBpeximo
#59 title_as_field-557292-59.patch108.5 KBpeximo
#57 title_as_field-557292-57.patch108.5 KBpeximo
#55 title_as_field-557292-55.patch108.5 KBpeximo
#53 title_as_field-557292-53.patch107.37 KBpeximo
#49 title_as_field-557292-49.patch106.9 KBpeximo
#47 title_as_field-557292-47.patch106.94 KBpeximo
#46 title_as_field-557292-46.patch105.17 KBpeximo
#39 title_as_field-557292-39.patch103.4 KBpeximo
#35 title_as_field-557292-35.patch104.15 KBpeximo
#33 title_as_field-557292-32.patch97.29 KBpeximo
#31 title_as_field-557292-31.patch96.08 KBpeximo
#26 title_as_field-557292-26.patch96.1 KBpeximo
#18 title_as_field-557292-18.patch63.98 KBpeximo
#9 title_as_field-557292-9.patch6.53 KBplach
#1 title_as_field-557292-1.patch4.93 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Active » Needs review
FileSize
4.93 KB

Here is a draft patch, I didn't fix any test so it's likely to fail. My main worry for now is about performance, so any help with benchmarking is greatly appreciated.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
Issue tags: +i18n sprint

Tagging.

sun’s picture

Title: Convert node title to fields » TF #2: Convert node title to fields
yched’s picture

#553372: Convert node->title to a field is a bit older, but this one has a patch, so i marked the other duplicate.

From catch over there : "The only reason I can think of to keep $node->title in the {node} table is for alphabetical ordering of nodes, which is relatively unusual, and direct query listings of node titles in blocks/views - which is less necessary now that we have node_load_multiple(). So, we should look at converting title to a field. This would help with Field UI consistency, field translation and a lot of other big patches which otherwise have to special-case node titles.

I won't have time to work on this in the next two weeks, just opening the issue."

yched’s picture

-1 on 'title_field', BTW. Why not just 'title' (we did name the body field 'body'...)

moshe weitzman’s picture

FYI, book module is a use case for ordering by node title. But anyway, this is a good idea.

catch’s picture

subscribing - would be great to get an upgrade path here, would make benchmarks much easier.

plach’s picture

I modified the node update 7006 to migrate also titles to fields. I couldn't test the upgrade path due to #558778: Error while upgrading from Drupal 6 to Drupal 7 but it should be ok.
If someone could test this and benchmark on D7 it would be great.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

10 nodes, front page, ab -c1 -n1000

HEAD:

Document Path:          /
Document Length:        11240 bytes

Concurrency Level:      1
Time taken for tests:   78.385 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      11705000 bytes
HTML transferred:       11240000 bytes
Requests per second:    12.76 [#/sec] (mean)
Time per request:       78.385 [ms] (mean)
Time per request:       78.385 [ms] (mean, across all concurrent requests)
Transfer rate:          145.83 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    68   78  10.1     76     126
Waiting:       62   72   9.6     70     117
Total:         68   78  10.1     76     126

Patch:

Document Path:          /
Document Length:        11237 bytes

Concurrency Level:      1
Time taken for tests:   77.587 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      11702000 bytes
HTML transferred:       11237000 bytes
Requests per second:    12.89 [#/sec] (mean)
Time per request:       77.587 [ms] (mean)
Time per request:       77.587 [ms] (mean, across all concurrent requests)
Transfer rate:          147.29 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    69   77   5.6     76     121
Waiting:       64   71   5.4     70     113
Total:         69   78   5.6     76     121

So there's zero performance impace here in straight node load / node view. This will cause some performance issues when wanting to sort node titles alphabetically, but no different to any other field, and I think having title as a field (widget settings, translatable, consistency) is a big win compared to that.

One issue I noticed - the title field itself is weighted just above vertical tabs - not sure what the current state of weights is for fields, but if possible we should weight it very low so it's at the top of them form when creating the instances.

Note I didn't test the upgrade path for these benchmarks, just created one node, then duplicated it 10 times in devel php block in two clean installs of HEAD.

yched’s picture

+    $instance = array(
+      'field_name' => 'title_field',
+      'bundle' => $type->type,
+      'label' => $type->title_label,
+      'widget_type' => 'text',
+      'weight' => -5,

weight is per context: $instance['widget']['weight'] and $instance['display'][$build_mode]['weight']

+      // With no UI in core, we have to define default
+      // formatters for the teaser and full view.

Deprecated comment, there is now a UI in core. Obviously the original comment this was copied from (for body field) should be removed as well.

-  if (!isset($form['title']['#weight'])) {
+  if (!isset($form['title_field']['#weight'])) {
     $form['title']['#weight'] = -5;
   }

I'm not sure why this is needed and if we want to keep this at all, but if we do, it looks like the code inside the if branch needs to be updated as well.

I was about to say 'I'm still -1 on field name 'title_field', but I realized the patch keeps {node}.title db column and the $node->title property ? Why do we need to do this exactly ? And even if we still keep {node}.title to optimize some requests, does this imply we keep $node->title as well ? Duplicate properties seems like an odd precedent, and is bound to cause confusion (which one should I change ?)

moshe weitzman’s picture

Subscribe. This seems pretty crucial for localized sites.

catch’s picture

The reason to keep node.title is so you can still do queries like:

SELECT nid, title FROM {node} WHERE type = article AND status = 1 ORDER BY created LIMIT 0, 5;

And also:

SELECT nid, title FROM {node} WHERE type = article AND status = 1 ORDER BY title ASC LIMIT 0, 5

If we rely only on field storage then the first requires a node_load_multiple() to get the title, and the latter requires views.

Allowing people to do direct queries like that in contrib modules will meant that i18n sites will have to do more work (replacing a block so it actually loads the title field value so that the translated version can be used), however it should remove most performance concerns for monolingual sites.

yched’s picture

So {node}.title and $node->title will contain the title in the node's original language (kind of 'the human readable name of the multilingual node - mainly for admin purposes, I guess ?), while the 'title field' will contain all translations ? Sounds fine to me.

I'm still wondering (as in: you guys probably know much better than me ;-) ) whether we really need the $node->title property, though. It probably saves a lot of changes to legacy code, but how much of these places (including contrib) currently using $node->title should actually use the translated title to be correct ? How do we make this not confusing for contrib authors or other people (themers ?) needing to read from / write to the $node object ?

plach’s picture

@yched:

Probably your point about $node->title is right, AAMOF we are trying to skip $node->title_field and just convert $node->title to fields (obviously keeping the {node}.title column).

catch’s picture

Yes we should not load $node->title into the node object at all, that won't help anyone - can be done easily enough in NodeController.

peximo’s picture

Hi, I worked on this one with plach.
The patch does the same work as the "body as field patch" (#372743: Body and teaser as fields): it transforms $node->title into a (required) field. Moreover it preserves the {node}.title column to avoid performance issues.
The patch introduces a new API function node_get_title (actually it renames the existing function node_page_title) which performs all the needed language negotiation and returns the proper title translation.

As today the head was broken I'm not sure all the tests will pass.

plach’s picture

Assigned: plach » Unassigned
Status: Needs work » Needs review

The patch looks good, but we still need to know if tests pass. However any review is welcome.

moshe weitzman’s picture

Looks good to me too.

yched’s picture

Great work, peximo.

+++ modules/field/field.multilingual.inc	31 Aug 2009 14:12:17 -0000
@@ -82,9 +85,9 @@ function field_multilingual_content_lang
-  return isset($obj_info['translation_handlers'][$handler]);
+  return isset($handler) ? isset($obj_info['translation_handlers'][$handler]) : !empty($obj_info['translation_handlers']);

Are the changes around translation handlers really related, or did they sneak in from a different patch ? (just wondering)

+++ modules/node/node.install	31 Aug 2009 13:59:45 -0000
@@ -510,6 +510,7 @@ function node_update_7006(&$context) {
+          $node->title[FIELD_LANGUAGE_NONE][0]['value'] = $revision->title;

This will be a little more complicated, because that whole branch is inside a if ($node_types[$revision->type]->has_body) {. That loop needs to account for the various combinations of has_body TRUE/FALSE and has_title TRUE/FALSE

+++ modules/node/node.module	31 Aug 2009 15:09:57 -0000
@@ -206,19 +206,7 @@ function node_field_build_modes($obj_typ
 function node_field_extra_fields($bundle) {
-  $extra = array();
-
-  if ($type = node_type_get_type($bundle)) {
-    if ($type->has_title) {
-      $extra['title'] = array(
-        'label' => $type->title_label,
-        'description' => t('Node module element.'),
-        'weight' => -5,
-      );
-    }
-  }
-
-  return $extra;
+  return array();
 }

Then the hook implementation can simply go - yay !

+++ modules/node/node.module	31 Aug 2009 15:09:57 -0000
@@ -552,6 +540,48 @@ function node_configure_fields($type) {
+   // Add the title field if not present.
+  $field = field_info_field('title');
+  $instance = field_info_instance('title', $type->type);

(and the rest of this section) Shouldn't this be wrapped in if ($type->has_title) { ?
Actually I'm not clear on whether we need to those has_body / has_title flags anymore ?
Related question : do we still allow node types without titles (whatever this currently means) ?
If no, we should make the field 'locked' so that it can't be removed in the Field UI.
If yes, a couple places should probably be tuned to avoid warnings if no $node->title entry.

+++ modules/node/node.module	31 Aug 2009 15:09:57 -0000
@@ -870,6 +900,15 @@ function node_save($node) {
+  // When converting $node-title to fields we preserved the {node}.title db 

Typo: $node->title. Or 'the title property' ?

+++ modules/node/node.module	31 Aug 2009 15:09:57 -0000
@@ -2752,23 +2795,7 @@ function _node_access_rebuild_batch_fini
 function node_content_form($node, $form_state) {
-
-  $type = node_type_get_type($node);
-
-  $form = array();
-
-  if ($type->has_title) {
-    $form['title'] = array(
-      '#type' => 'textfield',
-      '#title' => check_plain($type->title_label),
-      '#required' => TRUE,
-      '#default_value' => $node->title,
-      '#maxlength' => 255,
-      '#weight' => -5,
-    );
-  }
-
-  return $form;
+  return array();
 }

Hook implementation could go as well ?

+++ modules/simpletest/drupal_web_test_case.php	31 Aug 2009 14:51:08 -0000
@@ -725,6 +725,13 @@ class DrupalWebTestCase extends DrupalTe
+    // Merge title field value and format separately.
+    $title = array(
+      'value' => $this->randomName(8),
+      'format' => FILTER_FORMAT_DEFAULT
+    );
+    $settings['title'][FIELD_LANGUAGE_NONE][0] += $title;

Too quick copy-paste ;-), titles have no format.

I'm on crack. Are you, too?

catch’s picture

+++ modules/field/field.attach.inc	31 Aug 2009 13:59:45 -0000
@@ -1079,8 +1079,11 @@ function field_attach_query_revisions($f
+  // If no language is provided use the current language or FIELD_LANGUAGE_NONE.
+  if (empty($langcode)) {
+    $langcode = field_multilingual_get_current_language($obj_type);
+  }
+  $options = array('language' => $langcode);
+++ modules/field/field.multilingual.inc	31 Aug 2009 14:12:17 -0000
@@ -74,7 +74,10 @@ function field_multilingual_content_lang
- *
+ * 

Why would langcode be empty?

+++ modules/field/field.multilingual.inc	31 Aug 2009 14:12:17 -0000
@@ -74,7 +74,10 @@ function field_multilingual_content_lang
- *
+ * 

Trailing whitespace.

+++ modules/field/field.multilingual.inc	31 Aug 2009 14:12:17 -0000
@@ -82,9 +85,9 @@ function field_multilingual_content_lang
-function field_multilingual_check_translation_handler($obj_type, $handler) {
+function field_multilingual_check_translation_handlers($obj_type, $handler = NULL) {
   $obj_info = field_info_fieldable_types($obj_type);
-  return isset($obj_info['translation_handlers'][$handler]);
+  return isset($handler) ? isset($obj_info['translation_handlers'][$handler]) : !empty($obj_info['translation_handlers']);
 }

Again, not really sure why this is needed here, or what the change is - maybe a comment would help?

+++ modules/field/field.multilingual.inc	31 Aug 2009 14:12:17 -0000
@@ -121,3 +124,16 @@ function field_multilingual_valid_langua
+/**
+ * Return the appropriate field language according to the entity info.
+ * 
+ * @return
+ *   The current language if the given entity has translation handlers,
+ *   FIELD_LANGUAGE_NONE otherwise.

Needs a @param to go with the @return.

+++ modules/node/node.module	31 Aug 2009 15:09:57 -0000
@@ -552,6 +540,48 @@ function node_configure_fields($type) {
+   // Add the title field if not present.
+  $field = field_info_field('title');

Extra indentation here.

+++ modules/node/node.module	31 Aug 2009 15:09:57 -0000
@@ -870,6 +900,15 @@ function node_save($node) {
+  // When converting $node-title to fields we preserved the {node}.title db 
+  // column, hence we need to temporarily set a string value into $node->title 
+  // to make drupal_write_record behave properly.

This should say what we do rather than referencing what we used to. i.e. $node->title is a field, but we denormalize the default language into {node}.title as well. Also a typo on $node-title.

+++ modules/node/node.module	31 Aug 2009 15:09:57 -0000
@@ -1090,7 +1132,7 @@ function node_build_content($node, $buil
+    drupal_set_title(t('Revision of %title from %date', array('%title' => node_get_title($node), '%date' => format_date($node->revision_timestamp))), PASS_THROUGH);

Why can't we access the field data structure directly here? This suggests that field.multilingual.inc will be loaded on every time we call node->show? which is a lot.

I also don't see any hunks for NodeController here - I think we should be unsetting node.title from node_load() here so we never have a $node->title returned from node_load() - otherwise themers are going to get very confused about what to print.

Otherwise this looks fine, and we confirmed with benchmarks that there's no performance impact for monolingual sites already.

This review is powered by Dreditor.

catch’s picture

Crossposted with yched, see that's sprintin' for yer.

yched’s picture

Yup - although I'm now out of tonight's sprint, unfortunately :-(

catch’s picture

Status: Needs review » Needs work

Marking CNW for those two reviews, otherwise this looks RTBC to me once that's cleaned up.

Quoting kika from a duplicate issue quoting barry:

For background: http://drupal.org/node/372743#comment-1350144 from Barry:

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.

So that's sign-off from both Field API maintainers it looks like.

peximo’s picture

Title: TF #2: Convert node title to fields » TF #3: Convert node title to fields
Status: Needs work » Needs review
FileSize
96.1 KB

The attached patch implements all the suggestions from #24 and #26 except:

@ yched

Are the changes around translation handlers really related, or did they sneak in from a different patch ? (just wondering)

Removed, plach is working on an other patch.

(and the rest of this section) Shouldn't this be wrapped in if ($type->has_title) { ?
Actually I'm not clear on whether we need to those has_body / has_title flags anymore ?
Related question : do we still allow node types without titles (whatever this currently means) ?
If no, we should make the field 'locked' so that it can't be removed in the Field UI.
If yes, a couple places should probably be tuned to avoid warnings if no $node->title entry.

Currently the title field is locked and required and there is control for the has_title flag.

Hook implementation could go as well ?

node_overview_types() checks if a type implements the hook_form, if I remove the hook from node.module the list breaks.

@ catch

Why can't we access the field data structure directly here? This suggests that field.multilingual.inc will be loaded on every time we call node->show? which is a lot.

Actually field.multingual.inc is included in field.module everytime it is loaded, so this is causing no additional include.

I also don't see any hunks for NodeController here - I think we should be unsetting node.title from node_load() here so we never have a $node->title returned from node_load() - otherwise themers are going to get very confused about what to print.

In the current approach $node->title is overridden by field_attach_load before returning from entity_load so it seems there is no possibility for themers to find in $node->title something different than a field data structure.

I runned the test on my computer but due to problem with drupal_send_mail() I'm not sure all the tests are ok.

plach’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Related question : do we still allow node types without titles (whatever this currently means) ?
If no, we should make the field 'locked' so that it can't be removed in the Field UI.
If yes, a couple places should probably be tuned to avoid warnings if no $node->title entry.

Currently the title field is locked and required and there is control for the has_title flag.

Well, I'm not sure what has_title = FALSE currently means, but if the D6 API supports it, I guess we have to assume that there are such node types out there. That would be module-defined node types, since you can't create such node types in the 'Create custom content type UI'.
Those node types would suddenly wake up from D6->D7 upgrade with a 'title' field showing up ?

I also don't see any hunks for NodeController here - I think we should be unsetting node.title from node_load() here so we never have a $node->title returned from node_load() - otherwise themers are going to get very confused about what to print.

In the current approach $node->title is overridden by field_attach_load before returning from entity_load so it seems there is no possibility for themers to find in $node->title something different than a field data structure.

Hm, if for whatever reason field_attach_load() does not override title (for instance if we chose to support has_title = FALSE at some point), it would be cleaner to do as catch suggested and unset $node->title instead of relying on the overwrite.

yched’s picture

node_overview_types() checks if a type implements the hook_form, if I remove the hook from node.module the list breaks.

OMG, that looks screwy. Not for this patch, but this would need a cleanup. With body + title as field, this hook is not needed anymore in many cases, so node_overview_types() shouldn't rely on this...

peximo’s picture

Status: Needs work » Needs review
FileSize
96.08 KB

Re-rolled patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
97.29 KB

Ops, re re-rolled.

plach’s picture

Status: Needs review » Needs work

Great, tests are fixed! The suggestions in #29 are still pending, though.
Marking CNW.

peximo’s picture

FileSize
104.15 KB

For types with flag $has_title = FALSE it should be all right, the patch respects the condition and does not create a title field.
We still have that in core it is impossible to unset a title for an existing content type because there is no UI for this and the title label field is required.
I made the unset of the title field in NodeController, I hope the right way; I verified that a node without a title does not have errors or warnings when viewed.
In the content list a node without title of course has no display link as the title is not shown.
I could not test the upgrade path because on my box it does not work (even without the patch applied).

peximo’s picture

Status: Needs work » Needs review
yched’s picture

+++ modules/node/node.module	2 Sep 2009 09:14:16 -0000
@@ -3118,5 +3140,13 @@ class NodeController extends DrupalDefau
     }
     $this->hookLoadArguments[] = array_keys($typed_nodes);
     parent::attachLoad($nodes);
+
+    // For content types that have $has_title = FALSE we need to unset the
+    // $node->title property.
+    foreach ($nodes as $node) {
+      if (!is_array($node->title)) {
+        unset($node->title);
+      }
+    }
   }
 }

Works for me, but I'll leave catch feedback wheter that's what he intended in #22.

+++ modules/node/node.test	2 Sep 2009 08:38:17 -0000
@@ -445,19 +448,22 @@ class NodeTitleXSSTestCase extends Drupa
     $edit = array(
-      'title' => $xss . $this->randomName(),
+		"title[$langcode][0][value]" => $title,
     );

indentation is off (is that a tab ? you might want to check if there are others in the patch).

+++ modules/simpletest/drupal_web_test_case.php	2 Sep 2009 08:38:17 -0000
@@ -694,7 +694,7 @@ class DrupalWebTestCase extends DrupalTe
+      'title'     => array(FIELD_LANGUAGE_NONE => array(array())),
@@ -725,6 +725,12 @@ class DrupalWebTestCase extends DrupalTe
+    // Merge title field value and format separately.
+    $title = array(
+      'value' => $this->randomName(8)
+    );
+    $settings['title'][FIELD_LANGUAGE_NONE][0] += $title;
+

I don't think the 2nd part is actually needed. Could be done as
+ 'title' => array(FIELD_LANGUAGE_NONE => array(array('value' => $this->randomName(8)))), in the 1st part. Body receives a special treatment because of the format.

Other than that, looks ready to me.

catch’s picture

+++ modules/node/node.module 2 Sep 2009 09:14:16 -0000
@@ -3118,5 +3140,13 @@ class NodeController extends DrupalDefau
     }
     $this->hookLoadArguments[] = array_keys($typed_nodes);
     parent::attachLoad($nodes);
+
+    // For content types that have $has_title = FALSE we need to unset the
+    // $node->title property.
+    foreach ($nodes as $node) {
+      if (!is_array($node->title)) {
+        unset($node->title);
+      }
+    }
   }
}

I actually meant doing this in the original database query generated by buildQuery() - however I just realised the node specific stuff we have already is in entity.inc, not NodeController - so this needs a general cleanup issue, which I just opened #566940: Move node specific code out of entity.inc

I don't think removing the node title from the node object is critical here, since we're not sure we should even support $has_title and node->title gets the field structure before anyone can access it anyway - so IMO we can just skip the loading of the title from the database in the first place in a followup-issue.

peximo’s picture

yched:

Works for me, but I'll leave catch feedback wheter that's what he intended in #22.

catch:

I don't think removing the node title from the node object is critical here, since we're not sure we should even support $has_title and node->title gets the field structure before anyone can access it anyway - so IMO we can just skip the loading of the title from the database in the first place in a followup-issue.

Code Removed.

indentation is off (is that a tab ? you might want to check if there are others in the patch).

Checked and cleaned.

I don't think the 2nd part is actually needed. Could be done as
+ 'title' => array(FIELD_LANGUAGE_NONE => array(array('value' => $this->randomName(8)))), in the 1st part. Body receives a special treatment because of the format.

Fixed.

catch’s picture

This looks more or less ready to me - last remaining question is about the node_get_title() function - can't we just access $node->title[$langcode][0]['value'] directly?

plach’s picture

@catch: In most cases that would override any language negotiation/fallback rule applied by field_attach_view. In node_get_title we'll add the language negotiation/fallback code.

catch’s picture

But why would we add language negotiation/fallback code to node.module at all - if it's done in field_attach_view?

plach’s picture

If you know which language you wish to pick you can simply access $node->title[$langcode][0]['value'] directly.
In the cases we used node_get_title, you wish to get the title as it will be displayed, so you have to use the same logic that field_attach_view applies but you can't rely on its result, as it will return a structured array ready for rendering. Moreover calling drupal_attach_view every time you need the title as displayed might be expensive. Even with static caching there would be a lot of unecessary work as all the node field would be processed.

Edit: the function which will be called in node_get_title is field_multilingual_get_items (see #565480-5: TF #2: Multilingual field handling).

catch’s picture

OK. So field_multilingual_get_items() doesn't load any field values - so we still need to go and fetch the title field for nodes when it's loaded - which means presumably a field_attach_load(). You do a full node_load(), then do the fallback handling just for the title field because it's special cased, and this is less expensive than a field_attach_view() which has to run on the full node object for every field. i see why this has been done but it makes me a bit uneasy. Hopefully the following summarizes the differences:

HEAD:

  $node = node_load($nid);
  print $node->title;

Patch:

  $node = node_load($nid);
   print node_get_title($node);

Patch without special casing node.title:

  $node = node_load($nid);
  node_build_content($node);
  print $node->title;

Note that other modules can run after field_attach_view to affect the contents of the node title - in hook_node_view() and hook_node_build_alter() - so the only way to verify you have the correct title, is to run node_build_content() anyway. This is the case in HEAD, but is ignored - since we treat those modules messing with title as somewhat misbehaving, whereas now it's as much messable with as $node->content.

I think for people who are really worried about performance on monolingual sites, they should do direct queries for {node}.title. Where we're worried about getting the correct title, we should do node_build_content($node); I'm not sure we need that extra layer in the middle - since node_get_title($node) isn't any more readable than node_build_content($node) - and there aren't all that many situations where you do a full node_load() but then only show the title (apart from maybe admin listings, and extra work rendering on those isn't a big deal IMO).

For simplicity though, node_load($nid); $node->title wins - but that means putting something like node_get_title() into NodeController:attachLoad() - and I'm not sure about the implications of that - seems like it could lead to the same themer confusion we were discussing having the database value there.

I'd rather this was kept internal to node_load() OR we force a node_build_content(), I think. Could do with some feedback from others on this I reckon.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
105.17 KB

I introduced a change in theming: in addition to the changes suggested by catch for node_page_view(), I renamed the variable set in template_preprocess_node() $title in $node_title and modified node.tpl.php both in node.module and garland. This because field_attach_preprocess(), called in template_preprocess_node(), overrides the value set for $variables['title'] because now there is a field in $node named title.

<?php
$variables[$field_name] = isset($object->{$field_name}[$langcode]) ? $object->{$field_name}[$langcode] : NULL; // field_attach_preprocess(), line 1128
?>

Other solutions are possible but IMO it's better to explicitly distinguish the variable $title used in page.tpl.php from the one used in node.tpl.php.
The title as a field also introduces another set of problems on which we should reflect: I opened another issue #571654: Revert node titles as fields in which I explain what I find working on this patch.

peximo’s picture

FileSize
106.94 KB

This patch adds some simpletests which test if the node title has the correct value in the title tag, in the page breadcrumb and node display on comment preview.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
106.9 KB

Re-rolled

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

Status: Needs work » Needs review

Can't reproduce any error on my box. Re-testing.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
107.37 KB

Re-rolled

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
108.5 KB

Re-rolled

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
108.5 KB

Re-rolled

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

rerolled

peximo’s picture

Status: Needs work » Needs review
peximo’s picture

FileSize
111.34 KB

catch expressed some concerns in IRC about the need of node_get_title(), here we followed the BAF approach: first make title a field and use $node->title[FIELD_ LANGUAGE_NONE][0]['value'] where needed, then address translatability in a following patch (probably #571654: Revert node titles as fields)

plach’s picture

One of the main implications of the approach taken here is that we'll lose the ability to access the title's raw value with $node->title, we'll have to use $node->title[$language][0]['value'].

Should we somehow retain the value from {node}.title in the node's data structure (you can look at it as human-readable identifier)?
And if we go this way, should we keep $node->title and introduce a new $node->title_field or put the field data structure in $node->title and have a new $node->title_raw field in which store the {node}.title value (which equals to $node->title[$node->language || FIELD_LANGAUGE_NONE][0]['value'])?

Suggestions are welcome.

peximo’s picture

I reiterate my post #571654: Revert node titles as fields to answer the raised question.

In D7 we have various access modes for the title value:

  1. From DB via an SQL query, e.g. node.admin.inc, line 450.
  2. From a node loaded but not built, e.g. comment.pages.inc, line 32
  3. a node built, e.g. node.module, line 1896.

The value returned by the three methods above is always the same unless it gets altered by hook_node_load().

With the title as a field we introduce another level of variability as the title value may be altered also through hook_node_view() and hook_node_build_alter(), since the title field is part of $node->content.

This ability to alter the title implies that we may have some mismatches between the values of the title (the "raw" value from DB and the value(s) in $node->title after load may be different from the actual value after build).
For example you can have a node title altered while viewing the node and a non-altered title in breadcrumbs and node preview while inserting a comment for the same node (see the attached images).

Now keeping a $node->title that holds the "raw" value and a $node->title_field alterable with field data structure does not change the situation by any means: we would still have the possibility of misalignments, because the main problem is identifying when we can use the raw/loaded value and when we have to use the built value (a review of these use contexts is reported in #571654: Revert node titles as fields).
IMO it would only introduce two fields almost identical that would create confusion for developers and themers.

IMO the best way to bring on the title as field work is:

  1. Straightforwardly convert node title to an (untranslatable) field, just like we did for the node body.
  2. Adjust the use context in #571654: Revert node titles as fields (it shouldn't be a great amount of work).
  3. Make title translatable as part of #565480: TF #2: Multilingual field handling.
catch’s picture

I think a straight conversion of node title to a field is a good first step - it makes sense with or without translatable fields.

plach’s picture

Ok, the current patch implements exactly step 1.

peximo’s picture

FileSize
62.37 KB
64.27 KB

Sorry i forgot the images.

catch’s picture

Status: Needs review » Needs work

So without node_get_title(), this patch suddenly looks a lot more straightforward. The only changes really are the upgrade path and updating to the new field structure.

Found some code style issues / questions when reading through the patch. Would also be interested to see basic benchmarks, although I think I did some above without finding any huge regressions so we ought to still be fine there.

+++ modules/forum/forum.module	18 Sep 2009 08:23:00 -0000
@@ -539,12 +539,12 @@ function forum_block_view($delta = '') {
+   // Cache based on the altered query. Enables us to cache with node access enabled.
+   $query->preExecute();
+   $cache_keys[] = md5(serialize(array((string) $query, $query->getArguments())));

The comment exceeds 80 chars, and indentation also seems to be off here and below.

+++ modules/forum/forum.module	18 Sep 2009 08:23:00 -0000
@@ -578,7 +578,6 @@ function forum_block_view_pre_render($el
 function forum_form($node, $form_state) {
   $type = node_type_get_type($node);
-  $form['title'] = array('#type' => 'textfield', '#title' => check_plain($type->title_label), '#default_value' => !empty($node->title) ? $node->title : '', '#required' => TRUE, '#weight' => -5);

Is it OK to just remove this?

+++ modules/node/node.install	18 Sep 2009 08:23:00 -0000
@@ -450,9 +450,9 @@ function node_update_7006(&$context) {
-      if ($info->has_body) {
+      if ($info->has_title || $info->has_body) {

I vaguely remember seeing a mention of has_title being a real node type property. Ideally we should be removing both of these. I'm pretty sure node.title can't be NULL. So probably this should just re-save all node types, without that check. If it's 5 or 20 node types it's really not going to take that much longer in the scheme of things.

+++ modules/node/node.module	18 Sep 2009 13:12:09 -0000
@@ -897,6 +922,14 @@ function node_save($node) {
+  // db column for performance, setting the default language value into this 
+  // column. After this we restore the field data structure to the previous node 

Trailing white space.

+++ modules/node/node.module	18 Sep 2009 13:12:09 -0000
@@ -897,6 +922,14 @@ function node_save($node) {
@@ -921,6 +954,9 @@ function node_save($node) {
@@ -1752,7 +1790,7 @@ function node_menu() {
-
+ 

Trailing whitespace again.

+++ modules/node/node.module	18 Sep 2009 13:12:09 -0000
@@ -2719,20 +2760,8 @@ function _node_access_rebuild_batch_fini
  * Implement hook_form().
  */
 function node_content_form($node, $form_state) {
-  $type = node_type_get_type($node);
-
-  if ($type->has_title) {
-    $form['title'] = array(
-      '#type' => 'textfield',
-      '#title' => check_plain($type->title_label),
-      '#required' => TRUE,
-      '#default_value' => $node->title,
-      '#maxlength' => 255,
-      '#weight' => -5,
-    );
-  }
-
-  return $form;
+  // node_overview_types() list content types only if they implement this hook.
+  return array();
 }

I think the explanation for the empty hook needs to go in the header documentation, had to read that comment twice to work out what was going on. This looks like a bug in node_overview_types() though really, but not the fault of this patch. Mind opening an issue for that bug though?

+++ modules/node/node.test	18 Sep 2009 13:18:39 -0000
@@ -445,19 +448,23 @@ class NodeTitleXSSTestCase extends Drupa
+    $xss = '<script>alert("xss")</script>';   
+     

More trailing whitespace (sorry).

This review is powered by Dreditor.

peximo’s picture

Status: Needs work » Needs review
FileSize
110.04 KB

Ok, I fixed the code style issues. For others questions:

Is it OK to just remove this?

Yes, I removed the element also in poll_form() because the form element is replaced by the field api form element.

I vaguely remember seeing a mention of has_title being a real node type property. Ideally we should be removing both of these. I'm pretty sure node.title can't be NULL. So probably this should just re-save all node types, without that check. If it's 5 or 20 node types it's really not going to take that much longer in the scheme of things.

In D6 you can have a node without a title (I tested this), so IMO in the upgrade we must consider a node without a title.

I think the explanation for the empty hook needs to go in the header documentation, had to read that comment twice to work out what was going on. This looks like a bug in node_overview_types() though really, but not the fault of this patch. Mind opening an issue for that bug though?

Ok I'll open the issue as soon as possible.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
109.91 KB

rerolled

plach’s picture

FileSize
109.92 KB

The attached patch fixes a wrong indentation in:

+   <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $node_title; ?></a></h2>

Aside from this looks good to me.

Status: Needs review » Needs work

The last submitted patch failed testing.

plach’s picture

FileSize
108.57 KB

Rerolled

plach’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
110.75 KB

Rerolled and fix field_attach_load() after #443422: 'per field' storage engine.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Why is the field_attach_load() change needed ?

-          if (!isset($queried_objects[$id]->{$field_name})) {
+          if (!is_array($queried_objects[$id]->{$field_name})) {
             $queried_objects[$id]->{$field_name} = array();
           }
peximo’s picture

Status: Needs work » Needs review
FileSize
110.25 KB

The previous change in field_attach_load() is not the right approach.
After a debug session I've modified NodeController:attachLoad().
The change is necessary after changes introduced by #443422: 'per field' storage engine: we need an array (CCK field) structure on attachLoad() but, because title is also a {node} table field, we retrived a string.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
110.24 KB

Rerolled

catch’s picture

Status: Needs review » Reviewed & tested by the community

I've reviewed this several times now, actually making the node title translatable isn't done here yet - but regardless we need to make the title a field for API consistency. Marking RTBC. This has only had review from myself and yched, so really hoping this important patch gets a bit more attention in the green queue than the yellow one.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
108.09 KB

Untested re-roll.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

While the re-roll is pending, here's a code review. Mostly questions and some minor things. I haven't had a chance to read the issue yet. Will do so in a moment.

-  $title = array('%title' => $node->title);
+  $title = array('%title' => $node->title[FIELD_LANGUAGE_NONE][0]['value']);

Marg. It's really too bad that this is the new syntax. The old syntax was /so/ much more readable. But the nice thing about this approach is that once you learn it, it applies consistently to *all* fields. I just got done teaching a theming course, and this was a major point of contention; themers never knowing where to locate input, and how it's inconsistently referenced between node module ($node->FOO) and CCK ($node->field_FOO[0]['something']).

+++ modules/node/node.module	9 Oct 2009 04:35:42 -0000
@@ -2744,20 +2785,7 @@ function _node_access_rebuild_batch_fini
 function node_content_form($node, $form_state) {
-  $type = node_type_get_type($node);
-
-  if ($type->has_title) {
-    $form['title'] = array(
-      '#type' => 'textfield',
-      '#title' => check_plain($type->title_label),
-      '#required' => TRUE,
-      '#default_value' => $node->title,
-      '#maxlength' => 255,
-      '#weight' => -5,
-    );
-  }
-
-  return $form;
+  return array();
 }

What is the point of this function if it's just returning an empty array?

+++ modules/node/node.pages.inc	9 Oct 2009 04:35:42 -0000
@@ -150,9 +150,6 @@ function node_form($form, &$form_state, 
-  if (!isset($form['title']['#weight'])) {
-    $form['title']['#weight'] = -5;
-  }

Hm. Making the title field optional is going to have interesting effects on listing pages, etc. I guess this is just Drupal's code falling inline with its APIs?

+  // When converting the title property to fields we preserved the {node}.title
+  // db column for performance, setting the default language value into this
+  // column. After this we restore the field data structure to the previous node
+  // title field.
+  $title_field = $node->title;
+  $langcode = FIELD_LANGUAGE_NONE;
+  $node->title = $title_field[$langcode][0]['value'];

Interesting trick. :) Do themes have access to that property, too?

+++ modules/node/node.test	9 Oct 2009 04:35:42 -0000
@@ -986,7 +992,54 @@ class NodeAdminTestCase extends DrupalWe
+  /*
+   *  Create one node and test if the node title has the correct value in the
+   *  <title> tag, in the page breadcrumb and node display on comment preview.
+   */

Needs to start with /** , should have an initial summary comment that's only one line.

+++ modules/node/node.test	9 Oct 2009 04:35:42 -0000
@@ -986,7 +992,54 @@ class NodeAdminTestCase extends DrupalWe
+    // Test <title> tag
...
+    // Test breadcrumb in comment preview
...
+    // Test node title in comment preview

End sentence in period, please.

+++ themes/garland/node.tpl.php	9 Oct 2009 04:35:46 -0000
@@ -6,7 +6,7 @@
-    <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $title; ?></a></h2>
+    <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $node_title; ?></a></h2>

Although I like the new name better, what's the reason for this change?

Also, I do not see an update path in this patch? Did I miss it?

I'm on crack. Are you, too?

catch’s picture

Update function is at node_update_7006() - re-using the body as field one.

Just a note I'm no longer re-rolling this patch, because i'm re-rolling #282191: TF #1: Allow different interface language for the same path so anyone else feel free to have at it.

webchick’s picture

Ok, after reading the issue, a lot of these were addressed above already.

I would add the comment back to node_content_form() explaining why we're doing that, since this confused both catch and I, along with a TODO to clean that crap up sometime in the future.

Also fix the comment oddities in the .test. I'll need to give this a UI run-through as well, once I can apply the patch, but that looks like about it from a code perspective.

I also share yched's musing about whether we even need $node->has_title anymore. I guess it's tricky because of that code in node_configure_fields() that forces it to re-create the title field unless that flag is set. And I guess it's not possible to treat title field as "just another text field" because of the special-casing with saving its value to node.title, and whatnot. We did finally find an actual use case for that flag when we built Twitter in Drupal at DIWD. :D

catch’s picture

I think we can make title completely required for nodes, however plach was concerned about breaking this API feature for content types as part of an 'innocent' conversion patch for title.

peximo’s picture

Status: Needs work » Needs review
FileSize
107.6 KB

Rerolled.

Although I like the new name better, what's the reason for this change?

$node_title is no longer required, it's an error from a previous patch. I'd forgotten it. Now I removed it from the patch.

catch’s picture

This still needs the explanatory comment in node_content_form().

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
108.83 KB

Sorry, I'm very tired today. I apply a wrong patch; $node_title is needed for the node comment preview. I named the variable $node_title instead of $title to distinguish the node title from the page title.
I don't understand what to do for the comment in node_content_form().

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

I so very much would like to commit this sucker. :) I can has re-roll?

catch’s picture

Status: Needs work » Needs review
FileSize
108.69 KB

Fixes those test failures locally for me, and adds the explanatory comment to node_content_form().

catch’s picture

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

Also fixed remaining issues from #86. Back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Spent some more time with this tonight, and can't really find anything else to hold it up.

I cringe at how verbose it is now to refer to the simple $node->title property, but I went back and found the original node body as field issue -- #372743: Body and teaser as fields -- (thanks to Albright in IRC!) and see that exactly the same thing was done there, with the exception of the langauge code stuff which wasn't there yet. That makes me feel slightly better.

I'm still pretty worried about the DX WTF we're imposing on every. single. Drupal developer and themer. by pushing the language system directly in their face wherever they turn. I have no real alternatives, however, and understand why it needs to be this way. At least once you learn this weird Drupalism rule, it's consistent across all fields, I guess... :\

Anyway, none of that should take away from the AWESOMENESS that is this patch, for finally getting node title to be consistent with other field types! :D Let's make sure #553306: Make nodes have no body field by default. Remove deprecated APIs for body field gets taken care of during "polish" phase.

Committed to HEAD! This change will need to be documented in both the module and theme upgrade guide.

dmitrig01’s picture

Category: task » bug
Priority: Normal » Critical
FileSize
772 bytes

Introduced a pretty serious bug, you can't click on node titles anymore

dmitrig01’s picture

FileSize
771 bytes

Whoops

catch’s picture

Status: Needs work » Needs review
webchick’s picture

Status: Needs review » Needs work

Whoops. :(

Committed to HEAD. Looks like our tests don't cover clicking on the node title to get to the node. I created #601308: Test clicking on node title from teaser list as a follow-up issue.

catch’s picture

Priority: Critical » Normal
Status: Needs work » Fixed

I'm marking this as fixed since the API change is being actively discussed in #571654: Revert node titles as fields and we should avoid duplcating what happens there.

Status: Fixed » Closed (fixed)

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

plach’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » feature
Status: Closed (fixed) » Closed (duplicate)
sfyn’s picture

@D7csmtl

Regarding this test, introduced in the patch from #93:

// Test node title in comment preview.
    $xpath = '//div[@id="node-'. $node->nid .'"]/h2/a';
    $this->assertEqual(current($this->xpath($xpath)), $node->title, 'Node preview title is equal to node title.', 'Node');

Can you explain why this was done this way? This effectively binds node titles to being displayed inside an h2 tag, which kinda blows away any theming possible.

Why not just use an assertText($node->title) to detect the presence of the node title in comment previews?

JacobSingh’s picture

While I agree that many functional tests in Drupal are really brittle by relying on markup to validate APIs, your custom theme shouldn't effect tests, since tests should be run only with the default core theme AFAIK.

bgogoi’s picture

Status: Closed (duplicate) » Needs review

#1: title_as_field-557292-1.patch queued for re-testing.

plach’s picture

Status: Needs review » Closed (duplicate)

This is is closed. Please don't reopen it. See #1188394-46: Make title behave as a configurable, translatable field (again) and following.