Now that we got rid of $op paramenters from hook_nodeapi() it's time to make node APIs more consistent, and take it to the next level. Currently we have:

Node data API (CRUD):

node_load()
node_validate()
node_save()
node_delete()
....

Hooks for modules that implement node types:

hook_load()
hook_validate()
hook_insert()
hook_update()
hook_delete()
...

Hooks for modules that not necessarily implement node types:

hook_nodeapi_load()
hook_nodeapi_validate()
hook_nodeapi_insert()
hook_nodeapi_update()
hook_nodeapi_delete()
...

These hooks doesn't have a consistent namespace, i.e. they don't tell what object type they act on.

What does this patch changes?

Provide a consistent naming for node hooks.
Completely remove hook_invoke_nodeapi().
Provides consistent function signatures within hooks.

Currently we have e.g.:

node_save()
hook_save() -> for modules which implement custom node types.
hook_nodeapi_save() -> for modules which implement don't custom node types.

With the this patch we'll have:

node_save()
hook_node_save() -> for modules which implement custom node types and for modules that don't.
hook_node_save_NODETYPE() -> for modules which implement custom node types (when hook is actually node type base name not necessarily the same as the module name).

Also this patch is a step in the direction for consistent data APIs.

Overview of the new node hooks:

node.module

hook_node_access(&$node, $op, $account)
hook_node_load(&$node)
hook_node_validate(&$node, $form)
hook_node_presave(&$node)
hook_node_insert(&$node)
hook_node_update(&$node)
hook_node_delete(&$node)
hook_node_alter(&$node, $teaser, $page)
hook_node_view_prepare(&$node, $teaser, $page) *
hook_node_view(&$node, $teaser, $page)
hook_node_search_result(&$node)
hook_node_rss_item(&$node)
hook_node_update_index(&$node)
hook_node_prepare(&$node)
hook_node_form(&$node, $form_state)**
hook_node_delete_revision($node_revision)

* I have to rename hook_view() to hook_node_view_prepare() due to the way it's called in node_build_content().
** I tried to make hook_node_form() pass $form array as reference but didn't come with a good sollution. Anyway, this is another issue.

blogapi.module

hook_node_blogapi_new(&$node)
hook_node_blogapi_edit(&$node)

translation.module

hook_node_prepare_translation(&$node)

Still to do:

Update function for actions hook nodeapi -> node (?)

All related tests seems to pass.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

subscribing, will review in the next couple of days. might be worth a post to the dev list?

recidive’s picture

FileSize
60.24 KB

Updated patch. Fixing errors in book.module.

Anonymous’s picture

Status: Needs review » Needs work

The last submitted patch failed testing.

recidive’s picture

Status: Needs work » Needs review
FileSize
59.66 KB

Re-rolling the patch.

Leaving hook_access() alone for now.

All tests pass.

merlinofchaos’s picture

I just want to comment that you should be careful about removing the distinction of "I am the module that owns this node" and "I am some other module operating on this node". The owning module should always go first, which is why the distinction between hook_load and hook_nodeapi_load is good. Please do not remove that, the problems that will be caused will be subtle and annoying. Module weight is not a good solution to this, because module weight affects *all* hooks and there are many reasons why you would not want to force your module to always go first just because you own a node type.

catch’s picture

I think we should consider removing hook_load() altogether:

1. It's not a hook, it's a 'node type specific callback'
2. Apart from operating only on one node type (which is easy with quick check anyway), it doesn't do anything special compared to hook_nodeapi)
3. The only two modules I was able to find which use it were poll and image (actually sun found image). Forum module uses it but it's completely redundant and removed in #324313: Load multiple nodes and terms at once
4. image.module is likely to be using cck fields by the time Drupal 7 is released
5. Having a whole hook just for poll module to do something it could do very easily in hook_nodeapi_load() seems a bit much.

Otherwise this looks good :)

merlinofchaos’s picture

Fanciful scenario:

I'm a module doing some spiffy operation to poll nodes. I use hook_nodeapi_load() to do it.

Except I named my module abracadabra.module. I'm kind of screwed because poll.module hasn't had an opportunity to load its data into the node yet.

I shouldn't have to use module weight to correct this.

recidive’s picture

@merlinofchaos: yes, the patch takes care of that, modules that implement node types are always called first, if the node in the context was defined by that module. The order of calls in the new node_invoke() is:

1- Call node type specific hook on module that implement node of this type. E.g. poll_node_load_poll().

2- Call hook on module that implement multiple node types with only one hook implementation for all types. E.g. node_content_node_form().

3- Call on all modules allowing them to act on nodes of all types. E.g. book_node_load().

@catch: The problem with loading node specific things using hook_nodeapi_load() is the one merlinofchaos described above. E.g., forum module load it's specific bits, then other modules which extends forums may depend on this data to work, and if forum module uses hook_nodeapi_load(), its data may not be available for the forum extension module to work with, unless this module has a lower weight.

merlinofchaos’s picture

recidive: Excellent. Your description didn't cover that, glad you did!

recidive’s picture

New patch. Uncommenting some code I've commented out for debugging.

cburschka’s picture

2- Call hook on module that implement multiple node types with only one hook implementation for all types. E.g. node_content_node_form().

Sorry, I don't understand the semantics of that hook implementation... is it node.module implementing hook_content_node_form? What is the "content" part for? As it is, I'm not sure what separates this second hook from the one after it; this is likely due to my faulty understanding of it.

recidive’s picture

@Arancaytar: this is for custom node types, (e.g. article and page). The 'contet ' part is set on hook_node_info() as 'base', this was node_content_form(). This was done this way to don't conflict node_form(). By the way, with that patch we will not need to set 'base' anymore, so this function can call just as it where before.

beginner’s picture

subscribe

Status: Needs review » Needs work

The last submitted patch failed testing.

recidive’s picture

Status: Needs work » Needs review
FileSize
59.49 KB

Re-rolling.

catch’s picture

Since hook_load() only ever gets called by the node type providing module, and afaik only forum, poll and image module actually use hook_load(), providing the extra hook call (#2 from post #8) seems like over-engineering things. In that case we could also use hook_node_load_type() instead of hook_node_load_$type() as well.

This would be better DX I think, since 1. it's not a hook, it's a node type specific callback and 2. therefore it doesn't work like the dynamic hook_form_alters do - which hook_node_load_poll() suggests it does (unless I'm completely misunderstanding). Of course I'm also being a bit selfish here in that I'd like this and the multiple load patch to sit happily together, and because we may need to add a new (not persistently cached) hook if caching node load - and that'll be easier if it's a straight rename/cleanup here.

Additionally we should should eventually get rid of node_invoke_nodeapi() and node_invoke() altogether, and just use module_implements() then $function($node) in situ - since we get free by-ref for objects in PHP5, and can just let hook implementations operate on the node directly. This would then match the recent changes to taxonomy and vocabularies - where all hooks get an object and there's no need to do all the messing about with return values that node module currently does. node->foo = $foo;

Status: Needs review » Needs work

The last submitted patch failed testing.

Xano’s picture

Subscribing.

Xano’s picture

Status: Needs work » Closed (duplicate)

This has been fixed (Can't find the issue). See http://api.drupal.org/api/search/7/hook_node for more details.

recidive’s picture

This is the issue:

#383066: hook_nodeapi_X => hook_node_X

But this didn't fix "pseudo hooks" hook_form, hook_load, etc.

Anyway, I'll not work on this any time soon. I'm felling like crap in this community.

Xano’s picture

Pseudo hooks are now in #400694: Not all hooks are hooks.

recidive’s picture

Assigned: recidive » Unassigned

Thanks.