| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
For some reason we're not using the menu system for setting node titles. This may not be an immediate problem, but it sure is annoying after applying #576290: Breadcrumbs don't work for dynamic paths & local tasks. Without any title that Drupal can locate, our breadcrumbs end up leaving blanks where the node title should be. So editing a node you get the breadcrumb "Content » " instead of "Content » Node Title". When using Webform in Drupal 7, things get worse and you get breadcrumbs like "Content » » Webform", where the node title is just dropped out right from the middle of the breadcrumb.
Mysteriously more than anything else, we already have a title callback node_page_title() in Drupal 7, but we're not using it (anywhere at all). Weird weird weird.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| node_title_callback.patch | 963 bytes | Idle | FAILED: [[SimpleTest]]: [MySQL] 18,685 pass(es), 5 fail(s), and 0 exception(es). | View details |
Comments
#1
#2
FYI, using "title callback" was removed in #557292: TF #3: Convert node title to fields but not restored in #571654: Revert node titles as fields.
#3
Looks good to me.
#4
The last submitted patch, node_title_callback.patch, failed testing.
#5
Hm, we probably have to check_plain() on the returned title these days.
#6
I like.
#7
Righto, http://api.drupal.org/api/function/_menu_item_localize/7
#8
Hm, a full day an no test result? Here's the same patch again, maybe it'll work this time.
#9
HEAD was broken due to #335035: drupalPost() incorrectly submits input for disabled elements - webchick just committed that patch (just right now)
#10
without a title callback, the node title won't appear in the shortcut bar. so, this is a bad bug IMO.
#11
The last submitted patch, node_title_callback.patch, failed testing.
#12
Shortcuts still in work #686440: Shortcuts Do Not Support Dynamic Titles
#13
#8: node_title_callback.patch queued for re-testing.
#14
The last submitted patch, node_title_callback.patch, failed testing.
#15
Tests should be fixed to let this fix in.
#16
#8: node_title_callback.patch queued for re-testing.
#17
The last submitted patch, node_title_callback.patch, failed testing.
#18
So, the tests have failed for legit reasons: nodes are an exception where the page title isn't necessarily equal to the menu item title. If you have a node with title "Foo", and an associated menu item with title "Bar", and no explicit drupal_set_title($node->title), then at the theming stage drupal_get_title will get the active menu title and render "Bar" as the page title.
Some other tests failed because check_plain is in fact not needed (@quicksketch, #5), and titles were double-escaped.
Here's a patch that does an explicit drupal_set_title in the title callback. Though it feels kind of weird, I think it's the right thing to do?
#19
Actually, is there some specific reason why drupal_set_title() is not called when the title is evaluated in the menu handler? Seems weird that drupal_get_title() needs to crawl back through the menu tree to get at the title again... or am I missing something?
#20
The last submitted patch, node_title_callback_2.patch, failed testing.
#21
Different failures now: node edit pages will have the node title as the page title instead of "Edit page $title", as long as there is a title callback for node/%node. Having an extra title callback for node/%node/edit doesn't help either.
#22
My approach in #18 won't work because the title callback will also get called while constructing breadcrumbs and menu items, thus overwriting the page title. So taking the simplest approach, adding the title callback and leaving drupal_set_title() in node_page_view() intact.
#23
#24
the title callback will also get called while constructing breadcrumbs and menu items, thus overwriting the page title.
Looks strange, page title for nodes could be overridden only if node have own menu item
#25
+++ modules/node/node.module 4 Apr 2010 23:10:43 -0000@@ -1877,6 +1877,8 @@ function node_menu() {
+ 'title arguments' => array(1), ¶
Whitespaces :)
Powered by Dreditor.
Here is a new patch for this
#26
Shouldn't we remove drupal_set_title() from the page callback function?
#27
At least for me it should not be in node_view, because you can currently call node_view to display a node somewhere, not on a page
but it could be a title callback, sure
#28
I meant that this patch needs work because it fails to delete drupal_set_title() from node_page_view(). That call is no longer needed.
#29
IIRC, that drupal_set_title() is required to set the proper node title as page title on local tasks/sub-pages below node/%node/... most likely not covered by tests yet.
#30
Moshe: So, to sum up my comments (#18, #22): if a node has an explicit menu link (e.g. in the Navigation menu), the menu link could have a different title. The title callback will be called during the construction of breadcrumbs and menu items, and it will overwrite the page title with the menu title. The explicit drupal_set_title() in node_page_view() is needed to "set it in stone", so a node can have a different page title and menu title.
As far as I can see, you can't remove the drupal_set_title(). It could use some comments describing why this is though.
#31
Oh for sure you can remove that and if you create node subtabs then it's your task to fix the title.
#32
#761620: Follow-up to revert node titles as fields; allow hook_node_view() to alter page title has been marked as duplicate of this issue. That one was critical, so this one is now.
#33
OK, we leave it in. I think one could make an argument either way. Lets just commit this tiny patch and kill a critical.
#34
Ideally, we should move the explanation for why drupal_set_title() is still needed in #30 into code though.
#35
Let's make that quick adjustment, then this is good to go.
#36
How about this wording: "Set title explicitly to allow for differing menu link titles"
#37
Slightly different, with punctuation.
#38
Having this means that the title on each node view is set twice. Do we really need to set the title twice?
#39
Allow me to demonstrate. Tests will fail, with the reasons I described above. I'm not saying it isn't weird, but keeping it in makes the tests pass. I spent a few hours delving into how the menu system generates and caches titles, but couldn't make sense of it.
#40
The last submitted patch, no-drupal-set-title-for-you.patch, failed testing.
#41
#42
So, webchick in #35 just asked for a small adjustment (explaining the drupal_set_title()), the comment has been made (#37), it has been proven that drupal_set_title() is still necessary (#39). Can someone mark this RTBC?
#43
5 test failures for menu system
#44
Yes, #39 was me trying to show it will fail if drupal_set_title is removed. Let me re-upload the last working patch again.
#45
#46
#25: node_title_callback_3.patch queued for re-testing.
#47
wait for green before commit
#48
Personally, I'd still like to understand why we set the title twice. The comment "Set the title explicitly to allow for differing menu link titles." does not explain it for me. Either we need to explain it better, or we should get rid of the double drupal_set_title().
#49
So, is there anyone who actually knows how this works? I tried to grok it, but couldn't. We have four cases where a "title" is shown:
1. page head title (title meta tag)
2. page title (main title in the body)
3. breadcrumbs
4. active menu link
All of these eventually end up at _menu_item_localize(), but go at different paths at different times, through a bunch of functions (menu_tree_page_data, menu_get_active_trail, menu_get_item....), some of which statically cache, some don't... If someone could enlighten us, we could find the root cause and possibly solve the node title != menu link title case in a more elegant way.
#50
Suppose this needs pwolanin's review
#51
Okay. Here's the really funny part. Check out node_menu in d6:
<?php$items['node/%node'] = array(
'title callback' => 'node_page_title',
'title arguments' => array(1),
'page callback' => 'node_page_view',
'page arguments' => array(1),
'access callback' => 'node_access',
'access arguments' => array('view', 1),
'type' => MENU_CALLBACK);
?>
We're literally just reverting to what d6 is already doing in this patch. I think there's good reason to keep it this way as well.
Just recently in a project i ran into an issue where I needed node/%node to alter its results based on the values of an additional url param. I went with arg(3). Originally, I tried to alter the title via nodeapi but the page cache wasn't catching it because it didn't know about my query string filter. Even if i just passed through arg(3) it wasn't effecting the title. The below menu rule shows what I had to do in order to make the extra arg's titles cached correctly:
<?php
$items['node/%node/filter/%'] = array(
'page arguments' => array(1, FALSE, TRUE),
'access callback' => 'node_access',
'access arguments' => array('view', 1),
'type' => MENU_CALLBACK
/* !!!!!!!!!!! CHANGES TO MAKE TITLE WORK WHEN CACHE IS ON !!!!!!!!!!! */
'title callback' => 'my_filter_title',
// passing extra arg
'title arguments' => array(1,3),
// note "node_view" not not "node_page_view" - (originally I tried using node_page_view but the drupal set title business seemed to interfere...) not really sure..
'page callback' => 'node_view',
);
?>
Here's what I think is going on here. Cache needs "title callback", and the menu page callback needs drupal_set_title to work around something goofy the menu system is doing.
Setting it back to needs review. The primary reason being it just reverts back to what we know basically works, and the comment in #2. My gut is that there's a not so simple problem in the menu system somewhere that involes prioritizing node/%node vs node/%node/view vs node/1view. Indeed, node_page_view is not the only offender. http://api.drupal.org/api/function/drupal_set_title/7 most of the 64 functions still calling drupal_set_title appear to be page callbacks.
So in summary the original bug was caused by us removing what the patch adds back. Adding back the title callback should fix the issue.
My instinct is that its very much related to the underlying issues that http://drupal.org/node/576290 is trying to fix (maybe unintentionally). (another prayer for 576290)
#52
This patch no longer fixes the bug report from #761620: Follow-up to revert node titles as fields; allow hook_node_view() to alter page title if it ever did, despite that being marked as duplicate.
That can be fixed by either removing the drupal_set_title() altogether, or moving it, but as far as I can see leaving it in like this is completely pointless - what bug does this patch actually fix given that the title is set again anyway?
Also this isn't critical.
#53
@catch This bug becomes relevant for #686440: Shortcuts Do Not Support Dynamic Titles. In that issue, the shortcut module is relying on the menu system to retrieve the title for shortcut links. It's going to pull a blank title for a link to node/% if we don't fix this.
That call to drupal_set_title() in node_page_view(), as well as the use of the title callback, has existed prior to the now-reverted node titles as fields patch. Perhaps we should just revert to what was being done in Drupal 6 and deal with drupal_set_title() as a separate issue.
#54
subcribe
#55
I have a better explanation in this patch for drupal_set_title().
In short, the page title is determined by the active trail. The default active trail is a single item which comes from the menu router. However, if we add a menu link to node/42, for example, that link (as well as its title) is used instead to determine the active trail. This is why we must explicitly set the title.
See menu_set_active_trail() if you're really curious.
#56
Mike: thanks, that's a much better explanation. To summarize, I've just tested #55.
Before patch:
Viewing node title: Child Node Title
Viewing node breadcrumb: Home › Parent Node Menu Title
Editing node title: Edit Basic page Child Node Title
Editing node breadcrumb: Home »
After patch:
Viewing node title: Child Node Title
Viewing node breadcrumb: Home › Parent Node Menu Title
Editing node title: Edit Basic page Child Node Title
Editing node breadcrumb: Home » Child Node Title
Just added a test to check for the proper breadcrumb title. I think we've moved forward, are we ready?
#57
To summarize, this patch brings us back to what was done in Drupal 6, and there are some issues which happen if we don't do that.
Having a blank menu title for 'node/%node' is bad for several reasons:
<a href="/">Home</a> › <a href="/node/1"></a>And if you access, for example, the Edit tab for a node, because you can't access the empty link to the tab root in the breadcrumb.#686440: Shortcuts Do Not Support Dynamic Titles is relying on the menu system for shortcut titles, so it will pull a blank title if we leave the title blank instead of using the 'node_page_title' callback.
Also, if a menu link is added to the node, the link overrides the default breadcrumb. Since the breadcrumb determines the page title, the page title will become the link title, not the node title. Thus, the explicit call to drupal_set_title() on the node view page is necessary to ensure the page title is still the node title if a menu link is added to that node. This is documented in the patch.
This patch should be RTBC, but since drifter and I made some updates to the patch, neither of us can really mark it as such.
#58
+++ modules/node/node.module 25 Jun 2010 08:33:16 -0000
@@ -1876,6 +1876,8 @@ function node_menu() {
$items['node/%node'] = array(
+ 'title callback' => 'node_page_title',
+ 'title arguments' => array(1),
'page callback' => 'node_page_view',
'page arguments' => array(1),
@@ -2502,6 +2504,9 @@ function node_page_default() {
function node_page_view($node) {
+ // If there is a menu link to this node, the link becomes the last part
+ // of the active trail, and the link name becomes the page title.
+ // Thus, we must explicitly set the page title to be the node title.
drupal_set_title($node->title);
We now documented why we still have drupal_set_title(), but we do not leave any clue for as to why we are additionally invoking a title callback. Thus, the next best dev who thinks he's genius will file a patch for the unnecessary title callback. ;)
The counter-docs could be added to the node/%node definition. Ideally, we'd perhaps also append
// @see node_menu()and// @see node_page_view()to both comments.Any clarification or improvement to the comments in this patch is highly welcome. This "simple" change request led to ~60+ follow-ups to be figured out correctly.
Lastly, I wonder whether this entire special handling only applies to nodes?
Powered by Dreditor.
#59
Ok, I added a brief explanation and more importantly a do not remove notice.
Hopefully we've documented this patch enough by now :)
#60
+++ modules/node/node.module 3 Jul 2010 23:09:59 -0000@@ -1876,6 +1876,10 @@
$items['node/%node'] = array(
+ // Use the node title for the breadcrumb and modules which
+ // rely on the menu system. In short, do not remove!
"do not remove" needs to be removed. Only bogus code and unimportant comments are changed and removed over time. Let's remove this addition and make the comment important enough instead, e.g. replace the addition with:
"node_page_view() additionally invokes drupal_set_title() to not display a potential menu link title."
(or similar)
However, the more I think about this, the more I wonder why we only see this behavior on node page views? Is it because no one creates menu links to other entities?
#61
OK, I've updated the comment.
#62
Since this used to be critical, it's a regression from D6, and it affects the breadcrumbs for commonly used pages, marking as "major" would make sense.
#63
Without this patch breadcrumbs are broken for nodes. The critical issue #576290: Breadcrumbs don't work for dynamic paths & local tasks depends on this patch getting in, which makes this one critical. See quicksketch's comment: http://drupal.org/node/576290#comment-2698574
#64
#65
#61: drupal-HEAD_737792.patch queued for re-testing.
#66
Should be overridden, not overriden.
#67
same patch as #61 with fixed spelling error
#68
leaving as RTBC since all I changed was spelling error in comment
#69
Those new comments seem pretty clear to me. Thanks to drifter for the detailed before/after output examples in #56.
I really want to see us write some detailed tests around core's breadcrumb behaviour to catch these kinds of things (and also to document how the ()&@# it's supposed to work!), but it looks like #576290: Breadcrumbs don't work for dynamic paths & local tasks is probably the best place for that. This patch is pretty straight-forward; it simply re-unifies D6 and D7's method of handling node page titles, with some extra documentation so this doesn't get "fixed" again in the future.
Committed to HEAD.
#70
Automatically closed -- issue fixed for 2 weeks with no activity.