When hook_form_altering the node editing form, there's crazy goofy things that need to be done -- pulling a form element out of the form, checking for a special key, appending it to the form_id, and so on...

This just sets $form['#node_edit_form'] = TRUE. Check that, and you know you're on the node edit form. ;) It's not a killer patch or anything, but it would definitely make things simpler.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

moshe weitzman’s picture

oh yes, this is far too messy right now. subscribe.

catch’s picture

Status: Needs review » Needs work

no longer applies. Probably small enough it could squeeze into this version though.

moshe weitzman’s picture

anyone available to reroll and rtbc this?

Pancho’s picture

Version: 6.x-dev » 7.x-dev

Moving feature requests to D7 queue.

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
5.09 KB

Back from the dead, and ready for review. The whole test suite is passing for me.

moshe weitzman’s picture

FileSize
5.09 KB

Now, with a period at end of a comment, as suggested by Catch.

webchick’s picture

+100 for something nicer than

-  if (isset($form['type']) && isset($form['#node']) && $form['type']['#value'] . '_node_form' == $form_id) {

This patch is a really nice DX improvement.

However, why:

+  $form['#node_type'] = $node->type;

Since $form['#node'] is set on any form where $form['#node_edit_form'], you can just use $form['#node']->type same as you use $form['#node']->nid or any other node property. Then that makes one less FAPI property for people to know about.

eaton’s picture

Yeah, I concur that the #node_type property is overkill. checking for #node to ensure that it's a node edit form is risky, but once we know it's a node edit form, we can very easily check its type. Thanks to PHP eval order, the following works nicely:

if (!empty($form['#node_edit_form']) && $form['#node']->type == 'blog') {...}

webchick’s picture

Status: Needs review » Needs work

Cool. Let's remove that (doesn't seem to be used anywhere in the patch) and get correct the "// Set the id of the top-level form tag" comment so it correctly reflects what the code below is doing now.

webchick’s picture

My mistake, I was looking at an earlier patch "+ // Set the id and other useful elements." capitalize ID please.

Also, I don't see a hunk for book.module? Are there other ones we're missing too?

moshe weitzman’s picture

Status: Needs work » Needs review
FileSize
5.63 KB

Removed that extra line which set the type. I did *not* change id to ID because I refer to the HTML id attribute which is almost always lowercase.

I added the book.module hunk. I searched for more instances of this node form check but found none more in core.

sun’s picture

Re-rolled with appropriate menu.module changes.

Dries’s picture

Status: Needs review » Fixed

Looks like a small but valuable improvement and all tests continue to pass. Committed to CVS HEAD.

webchick’s picture

Status: Fixed » Needs work

Docs, please! :D

* Need an update to the upgrading docs
* Need an update to the FAPI reference for HEAD.

moshe weitzman’s picture

Added to upgrade guide http://drupal.org/node/224333#node_form ... I'm not touching that FAPI reference doc though. I proposed an alternate solution for that doc but it has not been embraced yet - #100680: [meta] Make API module generate Form API documentation

sun’s picture

Issue tags: +Needs documentation

.

sun’s picture

Status: Needs work » Fixed
Issue tags: -Needs documentation

.

Status: Fixed » Closed (fixed)

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