The name of hook_form() is a hangover from the days when nodes were the main thing, and it would make a lot more sense for this hook to be namespaced as hook_node_form().

CommentFileSizeAuthor
#17 1390716-kill-hook-form.patch10.68 KBbleen
#16 1390716-kill-hook-form.patch10.96 KBbleen
#13 1390716-kill-hook-form.patch10.96 KBbleen
#11 1390716-kill-hook-form.patch9.21 KBbleen
#9 1390716-kill-hook-form.patch9.21 KBbleen
#6 1390716-kill-hook-form.patch9.21 KBbleen
#3 1390716-kill-hook-form.patch9.85 KBbleen

Comments

bleen’s picture

bleen commented
Title: rename hook_form to hook_node_form » kill hook_form() with fire

There is actually a TODO in the node_form() function to get rid of hook_form ... I feel like that would be even more useful than renaming it

  // @todo hook_form() implementations are unable to add #validate or #submit
  // handlers to the form buttons below. Remove hook_form() entirely.
joachim’s picture

joachim commented

It's a magic callback rather than a true hook, so I suppose the simplest way to fix this would be to require the callback to be declared in hook_node_info().

Though, are node types getting converted to plugins?

bleen’s picture

bleen commented
Status: Active » Needs review
StatusFileSize
new 1390716-kill-hook-form.patch9.85 KB

Attached is a first pass at killing hook_form ... I'm sure testbot will gripe about the node_content_form() function in node.module but I'm at a good testing point.

Testbot, please do your thing...

bleen’s picture

bleen commented

re #2: there is a desire to kill hook_node_info() as well: #1376884: Use configuration for entity types

Status: Needs review » Needs work

The last submitted patch, 1390716-kill-hook-form.patch, failed testing.

bleen’s picture

bleen commented
Status: Needs work » Needs review
StatusFileSize
new 1390716-kill-hook-form.patch9.21 KB

I think this solves the main gripes ... but there will still be many remaining.

Status: Needs review » Needs work

The last submitted patch, 1390716-kill-hook-form.patch, failed testing.

joachim’s picture

joachim commented

I think doing this with an alter hook is the right way. It feels like weaker DX. hook_form() feels like you, the node type providing module is *making* the form. Going to a mere form alter hook feels a bit uncomfortable.

bleen’s picture

bleen commented
Status: Needs work » Needs review
StatusFileSize
new 1390716-kill-hook-form.patch9.21 KB

and now we see about fixing these poll issues by naming the function poll_form_node_form_alter instead of forum_form_node_form_alter ... Oh cut & paste

bleen’s picture

bleen commented

re#8: Hmmmm ... my thought is that this should be no different a process than any other form that you are manipulating with a module. In my mind this adds a level of consistency.

It sounds like we could do with a few more opinions. Ill see what I can drum up in IRC

bleen’s picture

bleen commented
StatusFileSize
new 1390716-kill-hook-form.patch9.21 KB

Doh! Swentel just pointed out that I uploaded the wrong patch in #9. This is the correct one

Status: Needs review » Needs work

The last submitted patch, 1390716-kill-hook-form.patch, failed testing.

bleen’s picture

bleen commented
Status: Needs work » Needs review
StatusFileSize
new 1390716-kill-hook-form.patch10.96 KB

This removes a bit more code from node.pages.inc and it should fix the simpletest fail... though admittedly I'm not sure why the shortcut test was failing in the first place - hmmmm

Status: Needs review » Needs work

The last submitted patch, 1390716-kill-hook-form.patch, failed testing.

joachim’s picture

joachim commented

The hook_form_alter way is going to cause problems with contrib modules that want to alter the forms of particular node types.

I think that a node type module needs to be lower down and to have a conceptual responsibility for the form.

bleen’s picture

bleen commented
Status: Needs work » Needs review
StatusFileSize
new 1390716-kill-hook-form.patch10.96 KB

I see your point in #15, but I still think this can be a viable solution. Ill marinate some more on it

In the mean time this patch should pass tests ...

bleen’s picture

bleen commented
StatusFileSize
new 1390716-kill-hook-form.patch10.68 KB

friggety frak!!! I did it again

alansaviolobo queued 17: 1390716-kill-hook-form.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 17: 1390716-kill-hook-form.patch, failed testing.

alansaviolobo’s picture

alansaviolobo commented
Issue summary: View changes
Issue tags: +Needs reroll
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Closed (duplicate)
Issue tags: -Needs reroll

Duplicate of #2018375: Get rid of node_hook and node_invoke