With the introduction of node title as a translatable field some previous behaviors of Drupal should be analyzed.
First of all since node title is a field, it can be modified by contributed modules by hook_node_view() and hook_node_build_alter(); on the other hand with #565480: TF #2: Multilingual field handling you will always need to specify which title version use: the source title (the title in the original language) or the title displayed in the current content language.
This situation may cause an inconsistency between the title version displayed at node view and the title version displayed by other modules/components, e.g. we might have the title properly translated at node view (the title value is fetched from $node->content['title']) and a wrong title in the breadcrumbs on comment preview (the title value is returned by node_get_title(), which is the equivalent of $node->title, i.e. the title in the original language).
The attached spreadsheet lists (hopefully) all the cases in which node_get_title() is used and what title version ought to be used IMO.

As you can see these are mainly administrative pages, watchdog, logs and messages, but not only.
We should primarily decide if a module should be able to alter the node title (which was not possible before, because only $node->content was alterable), then we should solve the problems related to the translatability of the title case by case.

CommentFileSizeAuthor
#116 node-title-rollback-571654-116.patch4.52 KBpeximo
#114 node-title-rollback-571654-114.patch4.51 KBpeximo
#112 node-title-rollback-571654-112.patch3.4 KBJohnAlbin
#109 node-title-rollback-571654-109.patch3.42 KBJohnAlbin
#106 node-title-rollback-571654-106.patch2.89 KBJohnAlbin
#105 node-title-rollback-571654-105.patch2.89 KBJohnAlbin
#96 571654.patch102.19 KBbabbage
#93 node_revert_title.patch94.86 KBchx
#92 node_title_sanity.patch94.93 KBchx
#91 node_title_out-571654-91.patch100.72 KBpeximo
#88 node_title_out-571654-87.patch99.81 KBplach
#82 node_title_out-571654-80.patch99.35 KBpeximo
#80 node_title_out-571654-80.patch101.32 KBpeximo
#79 node_title_out-571654-79.patch97.68 KBpeximo
#77 node_title_out.patch94.78 KBchx
#38 translatable_title-571654-38.patch51.5 KBpeximo
#36 translatable_title-571654-36.patch48.35 KBpeximo
#36 comment-preview.jpg65.22 KBpeximo
#36 comment-preview-default-display.jpg33.12 KBpeximo
#36 feed-default-display.jpg102.05 KBpeximo
#36 feed-FF.jpg45.34 KBpeximo
#36 feed-IE.jpg209.21 KBpeximo
#36 read-more.jpg176.75 KBpeximo
#36 search-results.jpg42.1 KBpeximo
#34 translatable_title-571654-33.patch47.16 KBpeximo
#32 translatable_title-571654-31.patch48.16 KBpeximo
#27 translatable_title-571654-26.patch48.1 KBpeximo
#29 translatable_title-571654-26.patch47.15 KBpeximo
#23 Title field building contexts.ods19.77 KBplach
#11 translatable_title-571654-11.patch37.57 KBpeximo
#8 translatable_title-571654-8.patch38.03 KBpeximo
#6 translatable_title-571654-6.patch35.94 KBpeximo
#2 translatable_title-571654-2.patch32.45 KBpeximo
Title field building contexts.ods19.25 KBpeximo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

I had a look through.

Menus - these currently have to be translated via i18n_menu module in contrib. I'm guessing whatever we put in there, will be handled by i18n differently anyway, since it needs to do it for generic path links too.

None of the places where we might need to do a full build seem to have any particular performance implications to me, so it might make sense to just do the build rather than try to pre-optimize with a wrapper function.

peximo’s picture

Title: Title field building contexts » Make title field translatable
Status: Active » Needs review
FileSize
32.45 KB

Hi,
this is a first attempt to make title field translatable.
I have implemented two functions, node_get_original_title(), that returns the untraslated value (the one corresponding to $node->language), and node_get_translated_title() that build the node and return right translated value.
This patch introduce some additional node_build() with full build mode, so I think we need some kind of optimization.
Attention: the patch works only with #565480: TF #2: Multilingual field handling

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’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
35.94 KB

Rerolled

moshe weitzman’s picture

I agree that it is a bit crazy to do a full node build in order to get a translated title. Is there no way to request a given field in a given language? I'm OK with skipping build_alter and just using that. We could add a title alter if necessary. Title merits a special case over other fields.

peximo’s picture

@moshe weitzman

Is there no way to request a given field in a given language?

I found a solution to avoid a full node build: _field_invoke() can operate in 'single field' mode. To implement this solution, we must modify field_attach_view() adding a parameter $field_name. With this approach in node_get_translated_title() we can call field_attach_view() only for the title and we get the node title exactly as it would normally appear.

peximo’s picture

@moshe weitzman:
Sorry I forgot to answer about

Is there no way to request a given field in a given language?

Due to language fallback rules, to get the right language for the title field in node_get_translated_title() we need to replicate most of _field_invoke() logic and code. We can be sure that the title is displayed in the right language and with the right format only using the Field API code.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
37.57 KB

rerolled

plach’s picture

Status: Needs review » Needs work

I am ok with the general approach of this patch which already looks very good. IMO using field_attach_view() in single-field mode is the cleanest solution, besides being the only one which guarantees that we get the same value when we display a node title and the full node. However we need more eyes on the single choices between raw and built values.

There are some minor code styles/string issues.

+++ modules/field/field.attach.inc	21 Oct 2009 07:41:58 -0000
@@ -1183,9 +1183,12 @@ function field_attach_prepare_view($obj_
+  if ($field_name) {

We might want to add an inline comment here, e.g. "Build a single field.", and (possibly) a blank line.

+++ modules/locale/locale.field.inc	21 Oct 2009 07:41:58 -0000
@@ -52,7 +52,7 @@ function locale_field_fallback_view(&$ou
-    if (empty($output[$field_name]['items'])) {
+    if (isset($output[$field_name]) && empty($output[$field_name]['items'])) {

This is already addressed in #608424: Error with body display set to hidden.

+++ modules/locale/locale.test	21 Oct 2009 07:41:58 -0000
@@ -1394,11 +1394,15 @@ class LocaleContentFunctionalTest extend
+  

Trailing whitespaces.

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -220,6 +220,7 @@ function node_field_build_modes($obj_typ
+      'title' => t('Title'),

Why do we need this?

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+ * Get the title value in the node language.

IMO the documentation should not focus only on translatability. We might have something like "Get the title raw value in the node language".

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+ * 

Trailing whitespaces.

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+ *   The $node on wich get the title.

Should be "The node whose title has to be returned."

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+ *   The title value in the node language.

We might want to change this into "The title raw value in the node language. It needs to be sanitized before rendering.". Moreover there should be a blank line between parameters and return values in the PHPdocs.

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+function node_get_original_title($node) {

We might want to call this node_get_title_raw(), to stress on the fact that the returned value is not sanitized.

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+ * Get the builded (translated) title value.

Should be "Get the built...".

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+ * 

Trailing whitespaces.

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+ *   The $node on wich get the title.

See above.

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+ *   The builded (translated) title value .

Should be "The built (translated) title value." (watch out for the space before the period).
There should be a blank line between parameters and return values in the PHPdocs.

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+function node_get_translated_title($node) {

We might want to call this node_get_title_built().

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -3204,6 +3203,42 @@ function node_requirements($phase) {
+  return (isset($content['title'])) ? $content['title']['items'][0]['#item']['value'] : NULL;

This should be the sanitized version, right?

This review is powered by Dreditor.

plach’s picture

I forgot:

+++ modules/node/node.module	21 Oct 2009 07:41:58 -0000
@@ -1307,7 +1308,7 @@ function template_preprocess_node(&$vari
+  $variables['node_title'] = check_plain(node_get_translated_title($node));

We shouldn't be using check_plain() here, as node_get_translated_title() should return a fully-built (thus sanitized) value.

This review is powered by Dreditor.

yched’s picture

Quickly browsed through the patch for now.

Viewing a single field is the whole purpose of the existing field_view_field() function.
The function needs to be switched to use _field_invoke() in single-field mode, and also most probably needs to be updated for translatable fields. Note that this code tends to stay behind because core doesn't have a use case for this yet (the D6 equivalent was mainly used in Panels integration)

plach’s picture

I am sure I am missing something but I really can't understand why we need two functions when in other places we have node_load() which calls node_load_multiple(). IMHO if we made field_attach_view() work in single-field mode we could make field_view_field() a simple wrapper as currently node_load() is.

yched’s picture

OK, I was a little fast, and also a little wrong.

I opened #612894: field_format() is just a pile of code that doesn't work to clarify the situation. This outlines that :
- field_attach_view() is not the right function here, even with a 'single field' mode, because we don't want the result to depend on the display settings the admins have set for 'full' or 'teaser' - nor do we want a separate 'title' build mode like the patch adds.
- field_display_field() is not right either, because we know we won't want the label or multiple values.
- field_format() is actually more like what we need : just format a single value using a given formatter.

Drawback : this means the language logic cannot be shared with field_attach_view()...

peximo’s picture

because we don't want the result to depend on the display settings the admins have set for 'full' or 'teaser'

Are we sure? Eg. if you want the title to be not shown for a content type is not the right way set the title display to hidden? It's the same behavior of the others fields.

nor do we want a separate 'title' build mode like the patch adds.

Sorry, the title build mode is a mistake from a previous patch, it is not part of this patch.

yched’s picture

Eg. if you want the title to be not shown for a content type is not the right way set the title display to hidden? It's the same behavior of the others fields.

That's a different issue. When you do drupal_set_title(node_get_translated_title($node));, you don't want that to depend on display settings for some build mode.

Besides, the 'title' display is handled separately in node.tpl.php, and the 'title' field is created as 'hidden' in 'full node' and 'teaser', so it's taken out of the default $content display in node.tpl.php.
(this promises to be a bit confusing, BTW : The UI says it's hidden, yet I 'see' it when viewing a full node or teaser...)

peximo’s picture

Sorry, but I did not understand: when drupal is installed the title display for 'full node' and 'teaser' is set to 'default' not 'hidden'. The title is removed from the node content:

  if (isset($variables['content']['title'])) {
    unset($variables['content']['title']);
  }

The previous patch #557292: TF #3: Convert node title to fields has renamed $title to $node_title to distinguish between the common page title and the node title viewed in node preview.
So why not use the build mode/display system as other field to decide how to see the title in the page?
IMO the only problem with this approach is that some modules uses the node title for administration, like book: in this case a title set to hidden can be a problem.

plach’s picture

That's a different issue. When you do drupal_set_title(node_get_translated_title($node));, you don't want that to depend on display settings for some build mode.

I tend to agree with this but

Drawback : this means the language logic cannot be shared with field_attach_view()...

is not just a drawback, it simply invalidates the main reason we introduced node_get_translated_title($node) (which should actually be called node_get_built_title($node)): having the title value as it would appear without having to worry about the actual language. OTOH we need to have a reference build mode otherwise the "as it would appear" sentence has no meaning.

I still think making field_attach_view() work in single-field mode has no evident drawback and would render field_display_field() useless.

yched’s picture

field_attach_view() displays a field including its label and HTML wrapping defined in field.tpl.php. I don't see how this can be what we need here ?

Which part of the code currently holds the language fallback logic that takes place during field_attach_view() ? Couldn't it be abstracted out and called both during f_a_view() and node_get_built_title() ? Something like 'given the current language settings, the requested langage and the following languages available for the field, give me the language to use'.
That part of the code is largely unknown to me, so this is very possibly more easily said than done ;-). But having this logic hidden inside f_a_view() execution instead of publicly available and reusable seems like a bad thing. I'm pretty sure other use cases than node_get_translated_title() will pop up.

yched’s picture

re my #21:

I'm pretty sure other use cases than node_get_translated_title() will pop up.

And, as plach noted in #609118: node.tokens.inc: $node->body issues, another use case is tokens.

plach’s picture

field_attach_view() displays a field including its label and HTML wrapping defined in field.tpl.php. I don't see how this can be what we need here ?

Well, it depends on the use cases: peximo, who probably is the one who knows them best, is telling me that there are many situations where this is exactly what we need. Probably there isn't a single solution, we have to address them case by case.

Which part of the code currently holds the language fallback logic that takes place during field_attach_view() ? Couldn't it be abstracted out and called both during f_a_view() and node_get_built_title() ?

All the core fallback logic is in locale_field_fallback_view() (locale.field.inc) but any contrib module could alter/replace its result, so directly calling it might end in a different output. Our point is we want the same logic executed while displaying a node.

But having this logic hidden inside f_a_view() execution instead of publicly available and reusable seems like a bad thing. I'm pretty sure other use cases than node_get_translated_title() will pop up.

Our main problem is hook_field_attach_view_alter(): if we skip it, there will always be the chance to get a different result. We might want to isolate language negotiation in field_attach_view() so it is immune from hook_field_attach_view_alter() but probably this too is easier to say than to do. In this scenario we might try to impose a best practice: use hook_language_fallback_candidates_alter() to change fallback rules, never use hook_field_attach_view_alter() to mess with field languages. The main problem here is that theorically fields might have different languages, so we can't apply a fallback logic without having the actual field values. A possible alternative version of f_a_v():

<?php
function field_multilingual_fallback($obj_type, $object, $langcode) {
  $fallback_candidates = language_fallback_get_candidates();
  $cloned_object = clone($object);
  // Apply fallback rules directly on every $cloned_object->{$field_name}.
  // In $cloned_object each field might have only one langauge value, i.e. 
  // $langcode.
  drupal_alter('field_multilingual_fallback', $cloned_object, $obj_type, $fallback_candidates, $langcode);
  return $cloned_object;
}

function field_attach_view($obj_type, $object, $build_mode = 'full', $langcode = NULL) {
  // If no language is provided use the current UI language.
  $langcode = field_multilingual_valid_language($langcode, FALSE);
  $options = array('language' => $langcode);

  // Apply language fallback rules.
  $fallback_object = field_multilingual_fallback($obj_type, $object, $langcode);

  // Let field modules sanitize their data for output.
  $null = NULL;
  _field_invoke('sanitize', $obj_type, $fallback_object, $null, $null, $options);

  // field_default_view() does not need 
  $output = _field_invoke_default('view', $obj_type, $fallback_object, $build_mode, $null, $options);

  // Add custom weight handling.
  list($id, $vid, $bundle) = entity_extract_ids($obj_type, $fallback_object);
  $output['#attached']['css'][] = drupal_get_path('module', 'field') . '/theme/field.css';
  $output['#pre_render'][] = '_field_extra_weights_pre_render';
  $output['#extra_fields'] = field_extra_fields($bundle);

  // Let other modules make changes after rendering the view.
  $context = array(
    'obj_type' => $obj_type,
    'object' => $fallback_object,
    'build_mode' => $build_mode,
    'langcode' => $langcode,
  );
  drupal_alter('field_attach_view', $output, $context);

  return $output;
}
?>

With this approach we might cleanly re-use fallback logic to select the $items to be passed to field_format() but we still have the problem of hook_field_attach_view_alter().

webchick’s picture

Status: Needs work » Fixed

Committed to HEAD. Thanks!

webchick’s picture

Status: Fixed » Needs work

Wow, that was totally on the wrong issue! Sorry!

peximo’s picture

I analyzed all the use cases and tried to come up with a summary and a solution, the discussion on how to re-use the language fallback code probably belongs to #612894: field_format() is just a pile of code that doesn't work.

The core use of the title can be divided into 3 cases:
1. the value stored in {node}.title: This value is used by node, poll, forum and tracker to generate lists of content available on the site.
2. the raw value, inserted at node creation: This value is used as page title in administration context, for example when editing a node or managing the revisions and for messages and watchdog. As these case are related to the "original" node and not to that showed, I think it's right to use a non translated value.
3. the value for display in the required language: this value is displayed to the user, and I think that it is correct to use the rendered value, with all display settings (label and format). This value is used by node in node visualization (node_page_view ()), by book when a node is displayed for printing (book_node_export()) and by poll to display a poll both as a page than as a block (template_preprocess_poll_vote (), poll_view_results () , poll_votes (), poll_results ()).
If the idea of using the rendered title with all its display settings will be accepted , the title could also be hidden: this setting in the previous cases should not be a problem.

But there are particular use cases of this value that may be affected by an hidden title, mainly when the title is not printed on the screen as a heading, but used in other contexts:
a. comment in comment_reply() uses the title as the value of the breadcrumbs showed in the preview of a comment.
b. node in node_build_content() uses it as the value of the title attribute associated for 'read more' link.
c. node in node_feed() uses it as the title of a feed.
d. node in node_search_execute() uses it as title in the list of contents found by a search.

In all these cases, the absence of the title may cause problems. IMO there are two approaches:
1. We consider that if a user hides the title knows what he is doing and therefore the anomalous result (not an error) is correct
2. We introduce a fourth case, in which it is required a node identifier.
Function node_get_identifier() implements this case, returning an identifier formed by type + nid when the title is hidden.

I introduced a raw formatter that prints the title without HTML; this formatter should preserve the default behavior of D6.

I think having the ability to set the display of the title as all other fields can bring great benefits: like displaying a label that explain the title value (eg codes), or the ability to hide the title for some content type (eg codes for internal use, not for display).

Finally, I disabled a test, the one concerning the display of the breadcrumb in the preview of comment because of the bug reported in #623314: Fields displayed in a wrong way on comment preview.

The last submitted patch failed testing.

peximo’s picture

rerolled

peximo’s picture

Status: Needs work » Needs review
sun’s picture

+++ modules/book/book.admin.inc	14 Nov 2009 11:21:54 -0000
@@ -78,7 +78,7 @@ function book_admin_settings_validate($f
-  drupal_set_title($node->title[FIELD_LANGUAGE_NONE][0]['value']);
+  drupal_set_title(node_title_get_raw($node));

+++ modules/book/book.module	14 Nov 2009 11:21:54 -0000
@@ -1088,11 +1088,12 @@ function book_export_traverse($tree, $vi
+  $title = node_title_get_rendered($build);

+++ modules/comment/comment.pages.inc	14 Nov 2009 11:21:54 -0000
@@ -38,6 +36,9 @@ function comment_reply($node, $pid = NUL
+        // Set the breadcrumb trail.
+        $title = node_title_get_identifier($build['comment_form']['comment_output_below'], TRUE);
+        drupal_set_breadcrumb(array(l(t('Home'), NULL), l($title, 'node/' . $node->nid)));

+++ modules/node/node.module	14 Nov 2009 11:21:54 -0000
@@ -1307,11 +1308,11 @@ function template_preprocess_node(&$vari
+  $variables['node_title'] = drupal_render($variables['elements']['title']);

My first thought and reaction was: WTF?

Especially the "identifier" function confuses me.

+++ modules/node/node.module	14 Nov 2009 11:21:54 -0000
@@ -3201,6 +3204,41 @@ function node_requirements($phase) {
+function node_title_get_raw($node, $localized = FALSE) {
+  if (isset($node->language)) {
+    $langcode = field_multilingual_valid_language($node->language);
+  }
+  else {
+    $langcode = FIELD_LANGUAGE_NONE;
+  }
+  return $node->title[$langcode][0]['value'];
+}

It makes no sense to limit this to node titles.

function field_get_value($object, $field_name, $language = NULL)

Do we still store $node->language after all or is that more or less obsolete support for locale.module?

+++ modules/node/node.module	14 Nov 2009 11:21:54 -0000
@@ -3201,6 +3204,41 @@ function node_requirements($phase) {
+function node_title_get_rendered($element, $strip_tags  = FALSE) {
+  $title = drupal_render($element['title']);
+  if ($strip_tags) {
+    return strip_tags($title);
+  }
+  return trim($title);
+}

How often is this function used? This looks fairly special to me, especially the included strip_tags() and trim() logic...

+++ modules/node/node.module	14 Nov 2009 11:21:54 -0000
@@ -3201,6 +3204,41 @@ function node_requirements($phase) {
+function node_title_get_identifier($element, $strip_tags  = FALSE) {
+  $title = node_title_get_rendered($element);
+  if (empty($title)) {
+    return "{$element['#entity']->type}:{$element['#entity']->nid}";
+  }
+  return $title;
+}

I can't make any sense out of this function.

This review is powered by Dreditor.

peximo’s picture

Because the raw title value is always present, I have substituted the identifier (node type + nid) with this value in node_title_get_identifier().
I have fixed other minor bugs that I have found.

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
47.16 KB

rerolled

yched’s picture

re #31:

function node_title_get_raw
It makes no sense to limit this to node titles.
function field_get_value($object, $field_name, $language = NULL)

Slippery slope, because
a) fields can be multivalued
b) a 'single field value' is a collection of 'column' => 'value' pairs, and
A generic function cannot make assumptions about a single-valued field with a single-column-named-'value' field schema (as is the case for node title field)

So I see value in a field_get_value() function that embeds some language logic, *if* there is such an entity-agnostic property as $object->language, which to be fair, I don't remember exactly.
But then $return = field_get_value($node, 'title', $langcode)<:code> needs to return the array of multiple values, and node module needs to do the additional <code>$result[0]['value'] when it know it deals with a node title.

peximo’s picture

How often is this function used? This looks fairly special to me, especially the included strip_tags() and trim() logic...

While reviewing the patch I found that trim() was needed only in the simpletest code and strip_tags() only when node_title_get_rendered() was called by node_title_get_identifier(), so in the new patch I have fixed the tests and removed the entire function.

I can't make any sense out of this function.

As I said in #31, we can simply remove this function and use drupal_render() as in the other cases. But we will have some unexpected behaviors (see the attached screenshots), if title has display set to hidden:
1. No node title breadcrumb in comment preview, but there should be another way to return to the original commented node, like a “return to node” link.
2. Empty href title attribute when viewing node teaser, but if node doesn't have a title displayed also the related attribute should be empty.
3. A search results list without link to each node, but 1) if a user has set the title display to hidden for the “search” build mode he should know what he's doing and 2) there should be another way to view the node, like a “read more” link.
4. Feed without title: but 1) if a user has set the title display to hidden for the “RSS” build mode he should know what he's doing and 2) the link element is enough to view the node.
IMO these cases are not really problems.
But if we want to always have an identifier in these cases a possibly solution is to use the rendered title if visible, else use the raw value as identifier (I realized that we can use the raw title value instead of node type + nid because the title is required).
The new node_title_get_identifier() function would be:

<?php
  function node_title_get_identifier($node, $element, $strip_tags  = FALSE) {
    $title = drupal_render($element['title']);
    if (empty($title)) {
      $title = node_title_get_raw($node);
    }
    if ($strip_tags) {
      return strip_tags($title);
    }
    return $title;
  }
?>

About the strip_tags() function, it was used by the comment preview breadcrumbs and in node_feed() because in these two contexts it's not possible to have HTML tags; at the moment with the raw format set for title there is no problem, but if a user changes the display format to default or enable the label we have unwanted HTML (see comment-preview-default-display.jpg and feed-default-display.jpg attached).

Status: Needs review » Needs work

The last submitted patch failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
51.5 KB

I have missed some test. Rerolled.

yched’s picture

Going through a render() process to get the title for admin links, breadcrumbs etc is plain insane.
This runs the theme layer, field.tpl.php, is affected by its overrides, is dependant on the display settings specified on build modes, etc - just to run a strip_tags() on it to get rid of all the extraneous stuff that we didn't need to compute in the first place ?

sun’s picture

I agree with yched.

+++ modules/book/book.module	30 Nov 2009 15:20:40 -0000
@@ -1114,11 +1114,12 @@ function book_export_traverse($tree, $vi
 function book_node_export($node, $children = '') {
...
+  $title = drupal_render($build['title']);
...
-  return theme('book_node_export_html', array('node' => $node, 'children' => $children));
+  return theme('book_node_export_html', array('node' => $node, 'title' => $title, 'children' => $children));

@@ -1132,7 +1133,6 @@ function book_node_export($node, $childr
 function template_preprocess_book_node_export_html(&$variables) {
...
-  $variables['title'] = check_plain($variables['node']->title[FIELD_LANGUAGE_NONE][0]['value']);

Additionally, this is not the same.

This review is powered by Dreditor.

peximo’s picture

for admin links, breadcrumbs etc

ok, so I think the problems are limited to breadcrumbs and feed, because there aren't other cases besides this two using strip_tags().

This runs the theme layer, field.tpl.php, is affected by its overrides, is dependant on the display settings specified on build modes, etc - just to run a strip_tags() on it to get rid of all the extraneous stuff that we didn't need to compute in the first place ?

For comment breadcrumbs, except one case, and RSS title tag we just use the node's rendered content which is already available: you don't have the rendered content if the comment is a reply to another comment.

As you said, it may seem insane to theme a value and after remove all the HTML added and print a raw value; but with this process we have not only the HTML etc. but also the content itself that may have been altered by other modules. I don't know another way to get the same result.
In core however we have a strip_tags() call in template_preprocess_html():

<?php
  // Construct page title.
  if (drupal_get_title()) {
    $head_title = array(strip_tags(drupal_get_title()), variable_get('site_name', 'Drupal'));
  }
?>

This value is used in html.tpl.php:

  <title><?php print $head_title; ?></title>

This seems to me a similar use case.

peximo’s picture

@sun

Additionally, this is not the same.

I'm not sure wath you mean: if the problem is that there isn't the check_plain(), drupal_render() already returns a safe value.

chx’s picture

Title: Make title field translatable » Absolute release blocker: node title is the most horrible DX I ever saw
Category: task » bug
Priority: Normal » Critical
sun’s picture

Title: Absolute release blocker: node title is the most horrible DX I ever saw » Make title field translatable
Category: bug » task
Priority: Critical » Normal
+++ modules/book/book.admin.inc	30 Nov 2009 15:20:39 -0000
@@ -77,7 +77,7 @@ function book_admin_settings_validate($f
+  drupal_set_title(node_title_get_raw($node));

@@ -130,7 +130,7 @@ function book_admin_edit_submit($form, &
+        $langcode = field_multilingual_valid_language($node->language);

These functions will be used a lot, so we want to name them short and concise:

node_title()

field_language()

The latter, I don't even know what it has to do with fields.

I'm on crack. Are you, too?

sun’s picture

Title: Make title field translatable » Absolute release blocker: node title is the most horrible DX ever
Category: task » bug
Priority: Normal » Critical
Issue tags: +Release blocker

I agree with those issue property changes. Additionally, tagging.

plach’s picture

Indeed having to write $node->title[$wtf_language][0]['value'] to get the node's title is horrible DX.
Moreover in many contexts it is simply wrong.

Now that title is a field, using its raw value as stored in {node}.title (i.e. $node->title[$node->language][0]['value']) or a raw translation value might leave us with a value that differs from the one displayed on a plain node view.
Using a raw value simply skips any language negotiation/fallback rule applied and also any alteration performed during the build/render workflow. Let's keep in mind that these alterations might touch the title value itself, not only the HTML (or whatever) wrapping it.

Peximo and I tried to analyze all the contexts in which the node title is used (in core) and found that we have 3 main scenarios:

  • human-readable identifier, the title is used only to identify a node; it's probably ok to use the raw (untranslated) value here;
  • rendered value, the title must be printed exactly as it appears on a plain node view;
  • plaintext rendered value, the title's textual content has to be printed exactly as it appears on a plain node view, but in these contexts no HTML tag is allowed.

The main questions we have to answer now, even before deciding function names, are:

  1. Do we want node titles to behave as any other field?
    This means they can have labels, HTML wrapping them, display settings and so on
  2. No matter what we answer above, how do we get plaintext rendered values?
    (provided that we want them)

While ansewring to these questions we should keep in mind that here we are setting some kind of best practice for the use of fields outside of the Field API scope; if we can put together some guidelines contrib modules will be able to use in other cases, we'll have achieved a positive side-effect.

Do we still store $node->language after all or is that more or less obsolete support for locale.module?

Yes, we store it and it has an important role: entity translation use node language to determine the original content's language (for nodes).

So I see value in a field_get_value() function that embeds some language logic, *if* there is such an entity-agnostic property as $object->language, which to be fair, I don't remember exactly.

There is no generalized $object->language property, I proposed a way to re-use language fallback rules outside field_attach_view() in #23 and I still think it's the right way, although it's out of the scope of this patch (probably it fits better in #612894: field_format() is just a pile of code that doesn't work).

field_language()
The latter, I don't even know what it has to do with fields.

It is used in field_attach_view() and field_attach_form() to provide a valid language when none is passed. ATM it incorporates the code that converts from Node's language neutral language code to Field's one.
When #635094: Unify "language neutral" language codes goes in, we can safely rename it to language_valid() (or whatever) and move it to common.inc.

mattyoung’s picture

~

peximo’s picture

To avoid possible misunderstandings I'd want to point out that every solution tried here is experimental, and has the sole purpose to explore the possible solutions to this issue.
Plach and I decided to go this way because we don't think that such a complex matter can be addressed with our limited experience with the Drupal core. We absolutely need a discussion and a shared solution before starting to code a commit candidate.
I intentionally used ugly solutions (e.g. $build_copy), instead of trying to solve the problems I found, to make evident every tricky situation.

mcrittenden’s picture

Subscribe.

chx’s picture

Here are my suggestions:

  1. make node_load prepare a sane ->title for you.
  2. add a function that adds translated titles to objects node_get_titles($objects) foreach ($objects as $nid => $object) $object->title = ...... approimately.
  3. add a function that gets a dtbng select object and joins in field_data_title and adds the database field title_value so that your query can have a (translated) title that can be sorted on.
catch’s picture

I'm not keen on putting language-specific logic into node load itself, because that can be awkward for things like http://drupal.org/project/entitycache

catch’s picture

Actually I'll retract that - entitycache already handles uncacheable stuff in node_load() by adding an additional hook after the cache_set() (which is currently implemented on behalf of book and poll modules), so we could just do the same here.

peximo’s picture

Plach and I had a long discussion on the solution proposed by chx and on this issue in general: our conclusion is that after making the title a field we can't keep using it as a plain string as we did before. The conversion to field exposed the fact that previously we used $node->title in different contexts with different semantics. Now that we have a field we simply can't do that anymore.

To try make our thinking more general we started from this (absurd) scenario: what if our title was an image field instead of a text field? We would never try to access its data structure properties directly to visualize them: $node->title[$langcode][0]['fid'] would make no sense. The only way do display it would be passing through a build/render process.

The analisys of the core use-cases seems to support our idea that in core we actually don't need to access $node->title anymore. In some situations we already have a $build array to start from. In other situations a field_attach_view() (or whatever) acting in single-field mode would be enough, but in some of these contexts we need a plaintext value. What is the plaintext value of an image? It seems we need something like a toString() function, as even the (ugly) solution of rendering the field and then stripping away its tags wouldn't provide us a usable textual value - with an image field we would get an empty string. A possible solution to this problem would be to require any field to be able to provide a string representation of its data structure - in our fictious scenario it could be the image URL. We could trigger this plaintext building through a specific build mode called, say, 'plain' with associated a 'plaintext' formatter. This would avoid the rendering process in the cases we woudn't need it, which are almost all the cases in which we directly access $node->title at the moment.

This solution would let us improve the current DX by having a single function handling node title, e.g.:

<?php
function node_title($data, $translated = TRUE, $build_mode = 'full') {
  // We can have either a $node object or renderable array. 
  if (is_object($data)) {
    // If we need the translated value we can use NULL: the current language will be used,
    // otherwise the node's original language will be used.
    $langcode = $translated ? NULL : $data->language;
    // Act in single-field mode.
    $options = array('field_name' => 'title');
    $build = field_attach_view('node', $data, $build_mode, $options, $langcode);
    // We avoid to run the theme layer if not necessary.
    return $build_mode == 'plain' ? $build['title'] : drupal_render($build);
  }
  else {
    return drupal_render($data['title']);
  }
}
?>

By going this way we'll make title behave exactly as any other field: we might have a title label if needed and, much more important, we could change the title field type to a textarea and allow HTML in it.

plach’s picture

Title: Absolute release blocker: node title is the most horrible DX ever » Title field does not behave as a field

DX is only one of the problems we have to solve here.

yched’s picture

Sorry, #53 IMO builds up on wrong assumptions.
Making $node->title a field just meant "instead of the hardcoded and manually stored 'title' property, the title will be a Text Field, that is defined and created at node install time".
It never meant: "admins or coders should be able to specify which arbitrary field of any arbitrary field type they want to act as 'node title'".
I'm not saying this wouldn't be interesting (autonodetitle in core ?), but it's a whole different story, and is definitely not for D7. The 'title is an imagefield' scenario is not absurd, it's just off topic.

As I said earlier in the thread:
- A 'plain' build mode makes no sense. This is not what build modes are. Build modes are a display "flavor" for a full object.
- Triggering a formatter / render process when we want a plain text value for node title as links in admin lists, because we don't know whats in the 'title' value is insane. Node title is an unformatted text on which you know you can run check_plain(). Simple.

This does relate to the question of "what's a generic, cross-field-type way to display a field value in a link", for which we have no good answer right now (this is still the business of each individual formatter). This is a different issue, which does *not* need to be addressed here nor is critical for D7. Node titles are predictable.

chx’s picture

If we can't work this out then we need to roll back to old node titles.

sun’s picture

After reading some more discussion in IRC, chx's suggestion in #50 makes perfectly sense.

I don't see any reason why we shouldn't or couldn't have $node->title always be in the content language of the current page. We can prepare this on load. Also simplifies a lot of test code.

If you need the title in a different language, your fault. You can prepare yourself. But very unlikely anyway.

plach’s picture

Making $node->title a field just meant "instead of the hardcoded and manually stored 'title' property, the title will be a Text Field, that is defined and created at node install time".

Let's rollback title as field, then. It overcomplicates title handling without giving any advantage.

sun’s picture

@plach: What's wrong with the suggestion above?

plach’s picture

*If* title is a field it has to work as a field. Any attempt to make it work differently is a hack and is hack legalized by core. We'll have zillions of contrib modules using hackish techniques to handle their fields instead of properly exploiting the Field API.

yched’s picture

Hm, doesn't 'title as field' kind of legitimates the whole TF approach ?

plach’s picture

Entity translation can live in contrib. In contrib we'll have titles behaving as fields.

chx’s picture

I am of strong opinion to roll back to node title strings because node titles are unique in that most of the time we do need a string to "grab" a node. Should your site be different to this you can disable node titles and add a field. It was possible to disable node titles since eternity.

plach’s picture

I think we should have Dries' feedback at this point, we might not have time to wrap our minds on this in D7 and TF-UI is officially in contrib, so having title as field is less important now.

However I still think title is an example of the problems every contrib module will face when dealing with fields, so coming up with an "officially-approved" solution would be definetly better.

plach’s picture

FYI I am working on making the field language fallback actually reusable outside field_attach_view() so #50 might be feasible.

chx’s picture

See http://drupal.org/files/issues/337947-grndlvl-144.patch for a terrible example of the current situation we load all those nodes to just have a blatantly simple listing.

yched’s picture

re #65: "FYI I am working on making the field language fallback actually reusable outside field_attach_view() so #50 might be feasible."
Double yay. We really need this, and IMO it should really simplify this issue.

catch’s picture

Been thinking about this, since we don't have field translation in core now, I'd suggest something like the following:

Rollback title as field - this issue suggests we need to keep 'entity labels' as properties of the object rather than field API fields - i.e. do we also apply the same pattern to term name etc.?

In field translation module, create 'title field' fields which override the node title similar to how autonodetitle does in D6. This may take some workarounds, but at the moment, trying to keep $node->title intact we're adding a lot more complexity just to get back where we were in the first place.

Then we can see how painful those workarounds are in field translation module, and in Drupal 8 come up with a decision on whether entity labels are properties or fields, and then apply this consistently to nodes and taxonomy terms (maybe others) either way.

yched’s picture

#68 works for me :-(.

We do need the patch plach mentioned in #65, though.

sun’s picture

#68 does not work for me. auto_nodetitle works on save, not load.

plach’s picture

@yched:

Still working on it, didn't have much time.

I'd try #50 before giving up.

catch’s picture

@sun, yes I meant like autonodetitle not what it actually does. It'd involve preprocess hacks and other nastiness, but it's possibly a better option than leaving things how they are. I also want to see what #50 looks like in practice though if we can do that.

plach’s picture

chx’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » feature
Priority: Critical » Normal

Too much work left to do and one of those things that make D7 too complex. With TF in contrib, one can disable core node title and use a translatable field instead. We can do better in D8 but for sake of node listing performance and in general simpler coding I recommend rolling this back for now.

catch’s picture

Version: 8.x-dev » 7.x-dev
Category: feature » bug
Priority: Normal » Critical

chx - if it needs a rollback, then this need to stay as a critical bug report against D7, not be moved to D8. Unless there's an open rollback issue somewhere else (and if there is I haven't seen it).

webchick’s picture

Title: Title field does not behave as a field » Revert node titles as fields
Issue tags: +webchick's D7 alpha hit list

Ok. I spent a long time with chx discussing this issue and reading over the recent replies.

The complaints basically come down to the following:

1. The new syntax for dealing with titles is horribly obtuse. Compare l($node->title, 'url'); with l($node->field_title[LANGUAGE_NONE][0]['value'], 'url'); (though, on the other hand, this is consistent with all other node fields...)

2. node_load()s are required in order to pull the proper language for the field. Making a block of node titles, for example, gets much more obtuse.

3. We are not currently leveraging any benefits of title being a field in core. Contrib does have the ability to allow this (though maybe not easily...) through the new hooks added in D7.

4. There is a patch for #538164: Comment body as field, but not one for comment subject. This would create yet another discrepancy between comment and node APIs, and we did a lot this release to reconcile those.

5. TFs are too new; we don't yet know how we're going to use that functionality and hard-coding the current way this works is risky, and also exposes translatable properties to the vast majority of people who will never use it.

6. A rollback seems to have support of the major i18n contributors who worked on this patch. I know that sun raised a concern about using the auto_nodetitles approach, but catch seems to have countered that.

Therefore, I think it probably does make sense to roll this back. Tagging as something to get into core before the alpha on 1/15.

chx’s picture

FileSize
94.78 KB

This does not work. However, it's a start. Quite a start, I must say.

Edit: please work on this more.

Status: Needs review » Needs work

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

peximo’s picture

Status: Needs work » Needs review
FileSize
97.68 KB

This patch should be complete.

peximo’s picture

FileSize
101.32 KB

I forgot hook_field_extra_fields() to allow users re-order also the node title.

Status: Needs review » Needs work

The last submitted patch, node_title_out-571654-80.patch, failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
99.35 KB

sorry

chx’s picture

Status: Needs review » Reviewed & tested by the community

Nice job! thanks.

Dries’s picture

OK, let's roll this back. Before I do, let's re-run the tests to make sure.

Status: Reviewed & tested by the community » Needs review
Issue tags: -Release blocker, -translatable fields, -webchick's D7 alpha hit list

Re-test of node_title_out-571654-80.patch from comment #82 was requested by Dries.

Status: Needs review » Needs work
Issue tags: +Release blocker, +translatable fields, +webchick's D7 alpha hit list

The last submitted patch, node_title_out-571654-80.patch, failed testing.

webchick’s picture

Now that this has Dries's blessing as well, I'd like this to be my "big ass Drupal 7 patch of the day" this evening, so if someone could get around to re-rolling this in the next 4-6 hours, that would be wonderful!

plach’s picture

Status: Needs work » Needs review
FileSize
99.81 KB

Rerolled: search.test (testSearchResultsComment) was outdated.

sun’s picture

I'm still against rolling this back, but seems I have been outvoted.

+++ modules/forum/forum.module	8 Jan 2010 18:44:56 -0000
@@ -677,6 +677,12 @@ function forum_block_view_pre_render($el
 function forum_form($node, $form_state) {
...
+  $form['title'] = array(
+  	'#type' => 'textfield', 
+  	'#title' => check_plain($type->title_label), 
+  	'#default_value' => !empty($node->title) ? $node->title : '', 
+  	'#required' => TRUE, '#weight' => -5
+  );

1) Do we really still add node titles manually?

2) Wrong indentation.

3) Trailing white-space.

+++ modules/node/node.module	8 Jan 2010 18:44:57 -0000
@@ -597,52 +597,27 @@ function node_configure_fields($type) {
+/**
+* Implement hook_field_extra_fields().
+*/
+function node_field_extra_fields() {
+  $extra = array();
+  

Wrong indentation and s/Implement/Implements/ and trailing white-space.

+++ modules/node/node.module	8 Jan 2010 18:44:57 -0000
@@ -1898,6 +1858,8 @@ function node_menu() {
   $items['node/%node'] = array(
+    'title callback' => 'node_page_title',
+    'title arguments' => array(1),
     'page callback' => 'node_page_view',

@@ -2224,19 +2186,12 @@ function node_page_default() {
 function node_page_view($node) {
...
+  drupal_set_title($node->title);

One of both.

+++ modules/node/node.module	8 Jan 2010 18:44:57 -0000
@@ -2986,10 +2941,20 @@ function _node_access_rebuild_batch_fini
 function node_content_form($node, $form_state) {
...
+  
...
+  

Trailing white-space.

+++ modules/node/node.module	8 Jan 2010 18:44:57 -0000
@@ -2986,10 +2941,20 @@ function _node_access_rebuild_batch_fini
-  // It is impossible to define a content type without implementing hook_form()
-  // so simply return an empty array().
-  // @todo: remove this requirement.

We should keep this note.

+++ modules/node/node.test	8 Jan 2010 18:44:57 -0000
@@ -484,20 +484,17 @@ class NodeTitleXSSTestCase extends Drupa
+    $edit = array("title" => $title,);

Bogus comma.

6 days to code freeze. Better review yourself.

Status: Needs review » Needs work

The last submitted patch, node_title_out-571654-87.patch, failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
100.72 KB

Rerolled and fixed sun suggestions.

chx’s picture

FileSize
94.93 KB
chx’s picture

FileSize
94.86 KB

Included sun's stuff.

Status: Needs review » Needs work

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

eojthebrave’s picture

I think problem is here. See sun's comments in #89 for reference.

+++ modules/node/node.module	8 Jan 2010 18:44:57 -0000
@@ -1898,6 +1858,8 @@ function node_menu() {
   $items['node/%node'] = array(
+    'title callback' => 'node_page_title',
+    'title arguments' => array(1),
     'page callback' => 'node_page_view',

@@ -2224,19 +2186,12 @@ function node_page_default() {
function node_page_view($node) {
...
+  drupal_set_title($node->title);

For some reason only drupal_set_title() (which was removed from the latest patch) works correctly in my testing. Bug?

babbage’s picture

Status: Needs work » Needs review
FileSize
102.19 KB

Both eojthebrave and I confirmed in IRC that we thought the above was the problem. Sure enough, removing A from his code example and putting B back in resulted in all tests passing again. (Actually, having both A and B also resulted in all tests passing.)

Attached patch is the same as #93 excepting this change and also correcting one extraneous comma. (Also my first core patch if it is committed, albeit hardly a feat of coding on my part!)

babbage’s picture

Status: Needs review » Reviewed & tested by the community

Yep, so let's call that RTBC shall we and make webchick happy. :)

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yay!! Thanks a lot, dbabbage and eojthebrave, for taking this one home.

*gulp* Ok. Committed to HEAD. This will probably make a few module/theme porters angry. :\ But not nearly as many as would be had we done this any later than now, or not done it at all, IMO.

plach’s picture

Title: Revert node titles as fields » Make title behave as a field
Version: 7.x-dev » 8.x-dev
Category: bug » feature
Priority: Critical » Normal
Status: Fixed » Active

Now that titles as strings are back we can safely keep discussing this for D8. I'd keep this issue as most discussion is here.

plach’s picture

jherencia’s picture

Subscribe.

mcrittenden’s picture

Let's keep it tagged for webchick even though it's fixed.

webchick’s picture

Ah if this is moving to D8, then we can go ahead and remove the tag.

I highly recommend not starting a D8 initiative 100+ comments into an issue though, from personal experience. :P

babbage’s picture

Title: Make title behave as a field » Revert node titles as fields
Version: 8.x-dev » 7.x-dev
Category: feature » task
Status: Active » Fixed
Issue tags: -webchick's D7 alpha hit list

OK... changes as per #103.

I'll let someone else create a new D8 issue for "Make title behave as a field", but I think we should heed webchick's advice as few people know like her what it is like to approach these from a maintainer's point of view. Also, re-reading this thread the vast majority of it isn't about making titles behave as fields anyway, so it wouldn't actually make sense to keep this thread for that issue... let's just link back. Someone who's actually able to outline that need well, or to work on a D8 patch, should feel free to open that issue. :)

JohnAlbin’s picture

Status: Fixed » Needs review
FileSize
2.89 KB

I know this may seem minor to non-themers, but the biggest part of the title-as-field patches that was annoying the crap out of me was the fact that node.tpl's $title got renamed to $node_title and the class="title" got removed. Using consistent variable names across templates makes learning theming easier.

This patch reverts that part as well, since it wasn't included in the previous patch.

JohnAlbin’s picture

Fixed extremely minor white space inconsistency in template_preprocess_node.

Status: Needs review » Needs work

The last submitted patch, node-title-rollback-571654-106.patch, failed testing.

sun’s picture

You didn't update Garland's node.tpl.php?

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
3.42 KB

Whoops! You're right, sun. Good catch.

sun’s picture

+++ modules/node/node.tpl.php	2010-01-15 07:43:02 +0000
@@ -82,7 +84,7 @@
-    <h2<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $node_title; ?></a></h2>
+    <h2 class="title"<?php print $title_attributes; ?>><a href="<?php print $node_url; ?>"><?php print $title; ?></a></h2>

Are you sure this works? $title_attributes should or may already contain a 'class' attribute... so you'd get two class attributes?

Powered by Dreditor.

Status: Needs review » Needs work

The last submitted patch, node-title-rollback-571654-109.patch, failed testing.

JohnAlbin’s picture

Status: Needs work » Needs review
FileSize
3.4 KB

Ok. $title_attributes doesn't have any classes in it, but contrib could add some. So I can live without the class="title".

Status: Needs review » Needs work

The last submitted patch, node-title-rollback-571654-112.patch, failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

The problem seemed to be in the RDF module, that uses the node variables; it should now be fixed.

Status: Needs review » Needs work

The last submitted patch, node-title-rollback-571654-114.patch, failed testing.

peximo’s picture

Status: Needs work » Needs review
FileSize
4.52 KB

Probably an encoding problem. Rerolled.

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

I'm at a loss as to why the patch in #112 failed saying "Failed on MySQL 5.0 InnoDB" when the only diff between it and #116 is a missing RDF ref to node_title. :-\

Regardless, #116 is RTBC. :-)

arhak’s picture

@#104 please, if a new issue is opened, please drop the link over here to follow up
(I currently have contrib code depending on node titles)

Morbus Iff’s picture

Category: task » bug
Priority: Normal » Critical
Status: Reviewed & tested by the community » Needs work

I think this patch might have reverted a previously working behavior too: the ability for a hook_node_view() to change $node->title at a page level. If you take a look at the diff at http://drupalcode.org/viewvc/drupal/drupal/modules/node/node.module?r1=1..., you'll see that node_page_view() has been changed. Prior to the reversion, the workflow was a) call node_show(), b) set the title. In the reverted/commited code, that's reversed: we a) set the title, then b) call node_show(). Since node_show() calls all the 'view' hooks, the reverted patch now makes it impossible to set the node title at the page level (both in the HTML [title] and in the page.tpl.php $title).

Per chx: "Morbus|kids: if this is so that would be a tad bit surprising and of course mandating a critical bugreport" (though, instead of making a bug report, I added it here; splice it out if you think it should).

sun.core’s picture

Status: Needs work » Needs review
sun.core’s picture

@Morbus: Are you sure that the most recent patch doesn't resolve the bug you were experiencing, too?

sun’s picture

sun’s picture

Re-testing.

However, I'm not sure about this additional revert of $node_title to $title... Doesn't that introduce an inconsistency with other template files? Did we check that?

Status: Needs review » Needs work

The last submitted patch, node-title-rollback-571654-116.patch, failed testing.

quicksketch’s picture

A missing piece that was not restored was using "title callback" in node_menu(). See #737792: Use a "title callback" for node/%node instead of drupal_set_title() for a followup patch.

catch’s picture

I'm not clear what's left to do here. Can we split any followups into new issues and close this one?

moshe weitzman’s picture

Priority: Critical » Normal
Status: Needs work » Fixed

Apparently nothing more to do.

Status: Fixed » Closed (fixed)

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

JohnAlbin’s picture

However, I'm not sure about this additional revert of $node_title to $title... Doesn't that introduce an inconsistency with other template files? Did we check that?

It was the conversion of $title to $node_title that introduced an inconsistency with other templates. The #116 patch restores the consistency.

Ok. I've moved my patch to another issue #761616: Follow-up to revert node titles as fields; revert $node_title to $title. I also split Morbus's issue to #761620: Follow-up to revert node titles as fields; allow hook_node_view() to alter page title. So, including quicksketch's, that's at least 3 follow-ups to this follow-up.

Apparently, there's lots left to do. :-)

Gábor Hojtsy’s picture