#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.

  1. The docblock is wrong. The sole purpose of the function is to return a node submission form.
  2. There's no need to check if the content type exists. node_menu() already does that.
  3. 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.

CommentFileSizeAuthor
#9 node-node_add-unnecessarily-complex-802524-3.patch1.39 KBcrookednumber
#6 802524-node-add-simplified.patch2.82 KBaaronbauman
#4 802524-node-add-simplified.patch2.1 KBaaronbauman
#3 node-node_add-unnecessarily-complex-802524-2.patch1.39 KBcrookednumber
#1 node-node_add-unnecessarily-complex-802524.patch1.39 KBcrookednumber

Comments

crookednumber’s picture

crookednumber commented
Assigned: Unassigned » crookednumber
Status: Active » Needs review
StatusFileSize
new node-node_add-unnecessarily-complex-802524.patch1.39 KB
aspilicious’s picture

aspilicious commented

Return => Returns

crookednumber’s picture

crookednumber commented
StatusFileSize
new node-node_add-unnecessarily-complex-802524-2.patch1.39 KB

Thanks. Re-roll attached.

aaronbauman’s picture

aaronbauman
he/him
commented
StatusFileSize
new 802524-node-add-simplified.patch2.1 KB

We 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.

Status: Needs review » Needs work

The last submitted patch, 802524-node-add-simplified.patch, failed testing.

aaronbauman’s picture

aaronbauman
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new 802524-node-add-simplified.patch2.82 KB

This patch had the side effect of renaming links on "node/add" from "@type" to "Create @type", since node_add_page uses system_admin_menu_block to display its content.

revised patch:

  • includes the simplifications from OP and the simplification from comment #4: removed call to drupal_set_title from node_add.
  • creates a menu callback for "node/add/%" menu items node_add_page_title, which returns the translated "Create @name" title
  • updates theme_node_add_list to 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.

Status: Needs review » Needs work

The last submitted patch, 802524-node-add-simplified.patch, failed testing.

aaronbauman’s picture

aaronbauman
he/him
commented
Status: Needs work » Needs review

Looks 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.

crookednumber’s picture

crookednumber commented
StatusFileSize
new node-node_add-unnecessarily-complex-802524-3.patch1.39 KB

No 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.

betz’s picture

betz commented
Status: Needs review » Reviewed & tested by the community

Patch tested and works.

node/add/nonexistingtype gives me the node/add page, which is a good thing ^^

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -D7DX

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

Issue tags: +D7DX

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.