Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.#484060: node_add needs a sanity check successfully removed the extra access check. But, as agentrickard pointed out in http://drupal.org/node/484060#comment-1675134, the function is still overly complex and, hence, confusing.
- The docblock is wrong. The sole purpose of the function is to return a node submission form.
- There's no need to check if the content type exists. node_menu() already does that.
- At the very least, $output should be defined before being returned. Although
isset($types[$type]will always evaluate to TRUE and thus $output will always be set. (But that of course just proves that you don't need the if() at all.)
I'm happy to roll a patch if this is a valid (albeit minor) bug.
Comments
Comment #1
crookednumber commentedComment #2
aspilicious commentedReturn => Returns
Comment #3
crookednumber commentedThanks. Re-roll attached.
Comment #4
aaronbaumanWe can simplify this further:
If the sole purpose of node_add is to return the node form, it shouldn't set the page title either.
This also alleviates the side effect of developers having to reset the page title whenever they call node_add to get the node form.
Is there a historical reason that node_menu's node/add/* doesn't just set the final title?
Here is a re-roll with the suggestion.
Comment #6
aaronbaumanThis patch had the side effect of renaming links on "node/add" from "@type" to "Create @type", since node_add_page uses
system_admin_menu_blockto display its content.revised patch:
drupal_set_titlefromnode_add.node_add_page_title, which returns the translated "Create @name" titletheme_node_add_listto use the menu item's $item['link_title'] (the human readable type name) instead of $item['title'] (the translated menu title "Create @name") in order to address the side effect mentioned at the beginning of this comment.Comment #8
aaronbaumanLooks like the reason for using drupal_set_title in node_add, rather than declaring it in the menu callback, is to make the menus cleaner all around. Please ignore comments #4 - #7 and see patch #3, which works for me.
sorry for the WOB.
Comment #9
crookednumber commentedNo problem. I was wondering about that, too. Now I learned something.
Re-rolling patch from #3 (vs latest HEAD), so folks scanning this list see green at the bottom.
Comment #10
betz commentedPatch tested and works.
node/add/nonexistingtypegives me thenode/addpage, which is a good thing ^^Comment #11
dries commentedCommitted to CVS HEAD. Thanks.