Using arg() is for checking and coping node_build_content() is useless because:

1) menu_get_object() returns loaded node with access check - modules' blocks are only viewable on node-page

2) node_build_content() analyzes a build mode

3) current implementation passes $teaser and $page which undefined

CommentFileSizeAuthor
#9 746056-newline-d6.patch652 bytesandypost
#6 746056-cleanup-d6.patch4.28 KBandypost
cck_blocks-optimize.patch2.85 KBandypost

Comments

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

I love clean-ups. I hope, that simplification doesn't bring up any compatibility issues.

Is there any chance that menu_get_object() doesn't return a node and our module is asked to display a block? If not, we could remove isset($node->nid).

It would be great to display the block when the user is requesting an older revision, so we should check for (!arg(2) || arg(2) == 'revisions'). As far as I understand menu_get_object(), it returns the node in the revision that is given, if not the latest revision is requested, right?

Unfortunately, menu_get_object() doesn't work with node_breadcrumbs, as it modifies the path of the currently displayed node. Do you see another way to geht the currently displayed node or at least the real path of that node?

andypost’s picture

andypost
he/him
Primary language Russian
commented

menu_get_object() only depends on hook_menu() declaration which is node/%node and so by default so node_load() is called and then node_access on loaded $node object, I never test this with revisions :(

So it's only one chance to have this code broken when some contrib module hook_menu_alter to unset() node/%node path!

If you are planing to support node revisions so better to test menu_get_object() ELSE you need to copy/paste code from node.module to load node's revision then check node_access()

Next, isset($node->nid) is required to check that menu_get_object() returns a node-object, then we should check rightmost part of arg(2) and so. I think it's good idea to include a 'revision'

But anyway no reason to copy/paste node_build_content()

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

I filed a new bug for node_breadcrumbs #746344: Don't modify cached items.

(!arg(2) || arg(2) == 'revisions' && is_numeric(arg(3))) doe's what I intended, but something tells me that there is a better way to determine the way a node is being displayed (view, revision, etc.). The code is not very readable, but it solves the issue.

andypost’s picture

andypost
he/him
Primary language Russian
commented

The change seems reasonable, I dont know other method to determine "is this page a node page"
I use this in my imagecache_profiles module and there's no problems.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Fixed

Fixed in the next -dev.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Fixed » Needs review
StatusFileSize
new 746056-cleanup-d6.patch4.28 KB

here is a follow up patch, the condition was not clear

-    $display_nodecontent = (!arg(2) || arg(2) == 'revisions' && is_numeric(arg(3)));
+    $display_nodecontent = !arg(2) || (arg(2) == 'revisions' && is_numeric(arg(3)));

Also a lot of trailing whitespace fixes and indention of code block :) Let's make it perfect

There's The Drupal coding standards or coder module gives a good review of code but this is for another issue :)

Anonymous’s picture

Anonymous (not verified) commented

committed. I like it when it's perfect :-)

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Fixed
andypost’s picture

andypost
he/him
Primary language Russian
commented
StatusFileSize
new 746056-newline-d6.patch652 bytes

Great! So only a question: why you ignore new line at the end of file?

Anonymous’s picture

Anonymous (not verified) commented

Hm, I acted like it is stated at the end of every patch: "No newline at end of file". But I just realized that this is not an instruction but the system complaining about a missing new line :-)

Will be fixed in the future.

Status: Fixed » Closed (fixed)

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