new hook_node_info() inconsistent: 'module' should be 'prefix'

This patch looks good to me - we should indeed be checking for functions, not modules.

The new 'module' attribute is identical to the old 'base' attribute - that is, it USUALLY specifies a module (which is why the name of the attribute was changed), but in some cases, it specifies some other 'function base name' (so in some ways, the old name was more accurate). But we certainly should not be assuming that 'module' specifies a module - it can specify any function base name.

I volunteer to document the changes to hook_node_info(), by the way (as well as the other changes that comprised the 'pave the way for CCK' patch).

thanks for finding this, beginner. ;) in terms of project.module itself, please update your source -- you'll find that the "project_issue" node type now lives in a whole new module, project_issue.module (http://drupal.org/project/project_issue).

however, we're just about to introduce another node type to the project.module itself for release nodes (http://drupal.org/node/75053), so even though this bug isn't *currently* going to break the project.module in HEAD, a change we're about to make will be impossible without this fix.

that said, i think renaming this key from 'base' to 'module' was a bad idea. it's not a module, it's the base of the function names you want the hooks to be called with. it just so happens that mostly it's a single module implementing all these hooks for a given node type, and that the namespaces are equivalent -- but that's not always the case, and this array key name makes it misleading and confusing. i can't imaging project.module is the *only* drupal module that defines more than 1 node type...

so, the current patch is still RTBC, but i wonder if we should roll a new patch to rename this key. comments?

"Module" in Drupal-speak means a thing that one checks on on admin/modules that provides code and functionality and other stuff. That's what the word means.

Using "module" to also refer to a function prefix that is sometimes the same as the module that defines the function and sometimes not (something that has always bugged the hell out of me, as it's more difficult to tell which functions are node hooks and which aren't) is asking for a huge set of new support issues from people who don't understand that "module" doesn't mean the same as "module", but sometimes it does, but it doesn't have to. Ick.

"Base" makes sense, or if you really want to change it "prefix" would work as well. "Module" is simply the wrong name to use.

i think renaming this key from 'base' to 'module' was a bad idea.

Changing the key from 'base' to 'module' was Dries's idea - you'll have to convince him if you want to change it back. Personally, I agree that 'base' is better - but at the time, I decided that it wasn't worth arguing over.

Committed to HEAD.

I did this mostly because last I heard, this way was marginally faster. I do not want to see 'module' abused and treated as 'base'.

Wait, so did you commit a version that uses base or uses module?

I committed the only patch here: http://drupal.org/files/issues/hook_node_info.1.diff.txt

@drumm: I do not want to see 'module' abused and treated as 'base'.

apologies for being confused about this whole situation, but that statement only further confused me. ;) the way i see it (i must be missing something) is that hook_node_info() allows a module to specify what node types that module defines, and what the prefix should be for all the node-related hooks for each node type. if we always assume a 1-to-1 relation between modules and node types, there's no point in having this hook at all. if we grant that a single module might define N node types, then i don't see how 'module' can either a) be a reasonable name for this key (since there are by assumption more than 1 node type per module) or b) be "abused and treated as 'base'". it's not a module at all, it's the prefix, basename, whatever you want to call it, that should be used for all node-related hooks for this particular node type, right?

as i said, i'm obviously full of BS with the above reasoning, since dries and drumm are both way more clueful about drupal than i am, and they seem to agree with each other that "module" is the right name for this value's key. can someone please explain the error in my logic?

thanks,
-derek

after an enlightening discussion in IRC, it's clear i'm not crazy, and that the move from 'base' to 'module' was just not well-thought out. apparently, the comment that caused it was this:

@dries: "It took me a while to figure out what 'base' was. Turns out it is the defining module? Maybe call it 'module' instead?"
(http://drupal.org/node/62340#comment-114230)

that "maybe call it 'module' instead?", given a huge patch he was trying to make sense of, was never challenged by anyone else in that thread, and that's how it happened. however, myself, drumm, chx and jaza all agree 'module' is the wrong name. the proposal drumm suggested, which we all like, is the following:

hook_node_info() will have an *optional* key called 'prefix'. the node types defined in there will have their hooks invoked as:

[module][_prefix]_hookname()

'[module]' is fixed and hard-coded -- as drumm said: "you shouldn't be able to escape your namespace". in practice, i'd bet all modules that define more than 1 node type (e.g. project.module) already do exactly this. modules that only define 1 node type, and expect their node hooks to use the identical namespace as the rest of the module, don't have to worry about prefix at all. if they don't specify one, it's not used, and all the hooks are named exactly what they should be.

the [_prefix] part is only for the modules that define more than 1 node type. in that case, whatever's in the 'prefix' value gets appended to the module namespace before the hook name.

so, for example, project.module (as soon as we commit the releases-as-nodes stuff), would look something like this:

function project_node_info() {
  return array(
    'project_project' => array(
      'name' => t('project'),
      'prefix' => 'project',
    ),
    'project_release' => array(
      'name' => t('project release'),
      'prefix' => 'release',
    ),
  );
}

i don't have time right now to roll a patch, but i agreed to document the results of our discussion. i hope this can get cleaned up in the near future, so that folks don't duplicate effort trying to port to the whole "pave the way for CCK" stuff, only to have it change again. ;)

right: we'd remove 'module' entirely (since that's hard-coded), add an optional 'prefix' (described above), and there'd be nothing called 'base'...

That well explains how "module" got in there, and the proposed fix makes sense. +1 on the prefix concept.

The other option is to make the node's internal name the prefix by default, so that all node hooks would be modulename_nodename_hook(), which I suspect is what will happen in most cases anyway. (And it separates off the node hooks from module hooks, something I've wanted for a while. :-) )

Attached patch changes the behaviour of the new node type system. Changes:

• The 'node_type' table has a new 'prefix' column. The 'module' column has NOT been removed: it is still used.
• Modules can no longer define 'module' through hook_node_info(): the 'module' field is determined automatically for each node type, by the _node_types_build() function.
• Modules can now optionally define 'prefix' through hook_node_info().
• All node hooks are now invoked by concatenating the 'module' and the 'prefix' fields of the relevant node type (with an underscore '_' character automatically put in between).
• node_get_types() now also accepts 'prefix' as its first argument.
• The 'module' field has been removed from all implementations of hook_node_info() in core.
• Node types that are user-defined through node.module now have their 'prefix' field set to 'content' (this was previously being done in a hard-coded, hackish way - the old hardcoding of the string 'node_content' has been removed).
• node_type_save() updated to reflect the new DB schema.
• node_type_delete() introduced, to separate out the node type deletion query from the deletion form.
• Install and update scripts updated for new DB schema.

Note: this patch provides an upgrade path from 4.7 only. It will not add a new update_x function for migration from previous revisions of HEAD.

To test this patch on HEAD, you will need to manually add the following field to 'node_type' (preferably after the 'module' field - same for MySQL as for PgSQL):

prefix varchar(255) NOT NULL

And you will need to change all user-defined node types (e.g. page, story) to have a prefix of 'content', using a query such as this:

UPDATE node_type SET prefix = 'content' WHERE module = 'node'

Testing and review is most welcome.

Oops - the update script still needs to explicitly specify 'node' for the 'module' field, when creating the 'page' and 'story' node types.

Updated patch fixes this. Otherwise, all the same.

Well, I for one didn't find any bugs in this patch. Testing by other people is welcome. This is a critically needed update for the new core CCK system. Would be great if we could get this in soon.

I'm not a fan of the prefix-stuff. We need a much simpler solution.

New patch against latest HEAD. No changes, except for fixing up some offsets and conflicts. Tested for CRUD'ing nodes and node types, and it still works fine.

Dries, I urge you to please reconsider the merit of this patch. This IS a simpler solution than what is currently in core. This patch makes makes hook_node_info() simpler, NOT more complex:

1. Modules no longer have to define 'name_of_module_foo' as the prefix for each node type, because it's done automatically.
2. Module authors don't have to worry about namespace conflicts, because they're forced to stay within the namespace of their own module (which most, if not all, contrib modules do already - this patch just makes it automatic).
3. Module authors can define a custom prefix if they want to, but they don't have to - this means that most hook_node_info() implementations have one less attribute that they need to define.
4. All this is done at the small price of having a few lines of code that concatenate 'module' and 'prefix' in various places. Really, I think that this price is well worth the benefit. We're making node.module a bit more complex under the hood (because it has to do some concatenation to work out hook names - not a big deal IMO), but we're making hook_node_info() easier and more logical to implement, which is what really matters for module developers. Really, we're moving the complexity out of sight, away from a public hook, and into the behind-the-scenes code.

I remind you that this patch has strong support from myself, drumm, dww, chx, augustin, and others. Drumm, perhaps you could review this patch as well? I know that the code freeze is now in, and that this patch changes the API somewhat, but I am still firmly classing this as a bug fix, because the current system is just wrong, and this is a minor change that will result in it making sense again.

Add me to the list of people +1ing this idea. "module" is simply the wrong name to use there.

The simpler solution would be to just flat out rename module to prefix, but then that doesn't do anything about namespace issues. I just read through the code, and with one exception everything looked good to me, and not too complex. If one is allergic to the ternary operator (I used to be, I admit), the same thing could be achieved in a slightly more verbose manner if desired. I don't see that as necessary, though, as this is more descriptive of what the code is actually doing.

(That one exception is in _node_types_build(), where I worry about the performance of all those is_array() calls.)

This is an API bugfix, and should be accepted as such.

what you want is postponed but i have not yet given up on this

- This seems to be missing an implementation of hook_update_N().
- Something unrelated, node_type_delete() is included.
- (!empty($type->prefix) ? $type->prefix .'_' : '') is repeated quite a bit. That /may/ want some way of not being repeated so much.

What happens if a node module specifies 'module' (was converted using the older instructions)? Is it possible to multiple node types in one module without this change?

@drumm:

- This seems to be missing an implementation of hook_update_N().

This patch modifies system_update_1005(), which contains the original upgrade path for the configurable content types functionality. As I understand, it is not our policy to provide upgrade paths between revisions of HEAD - we only provide them between official releases, and that is what this patch does. Unless we break this policy (which will make the update code messier), users will have to update manually from previous revisions of HEAD (not the first time that they'll have had to do this).

- Something unrelated, node_type_delete() is included.

Sorry. My fault. That has been moved to a separate issue (which it would be great if you could review and commit as well).

- (!empty($type->prefix) ? $type->prefix .'_' : '') is repeated quite a bit. That /may/ want some way of not being repeated so much.

A new _node_type_format_prefix() function has been created, and all instances of that line have been replaced with calls to this function. Code repetition is now significantly reduced because of this.

What happens if a node module specifies 'module' (was converted using the older instructions)? Is it possible to multiple node types in one module without this change?

Yes, it is possible, but it's not as intuitive. For example, here's how module 'foo' would define node types 'bar1' and 'bar2' using the old system:

function foo_node_info() {
  return array(
    'bar1' => array(
      'name' => t('Bar 1'),
      'module' => 'foo_bar1',
      'description' => t("A bar 1 is cool."),
    ),
    'bar2' => array(
      'name' => t('Bar 2'),
      'module' => 'foo_bar2',
      'description' => t("A bar 2 is also cool."),
    )
  );
}

And here's how it would be done using the new system (i.e. after applying this patch):

function foo_node_info() {
  return array(
    'bar1' => array(
      'name' => t('Bar 1'),
      'prefix' => 'bar1',
      'description' => t("A bar 1 is cool."),
    ),
    'bar2' => array(
      'name' => t('Bar 2'),
      'prefix' => 'bar2',
      'description' => t("A bar 2 is also cool."),
    )
  );
}

As you can see, at the moment, 'module' doesn't mean 'module' at all (if the prefix is anything other than just the module name). The new system, on the other hand, is more sensible and actually makes sense.

I asked the last question because it is past freeze time and people might have converted using the old API already. What happens if they do that and run this code?

If people run this code without adding the 'prefix' field to the 'node_type' table (and then rebuilding their node types), then anything node-related will be completely broken. For example, even the default front page '/node' will give a blank page (although '/admin' will work ok).

Suggestions? Should the update code be moved to a new update_N function? Should we introduce a new special case in update.php? Or is it OK as is?

I am building a module that definies two node types in 5.0, and the term "module" rather than "prefix" confuses me as well.

If some people don't like "prefix" then surely even "sub-module" or "node-name" would be more preferable to "module"???

What about 'submodule' whenever this is discussed again?

Oops, that was just suggested.

I know it's a bit late in the game, but is there any chance that this patch can still get in for 5.0?

Here's an updated patch against latest HEAD. I've added a dedicated DB update function (as opposed to just modifying the existing update function - which the patch still does as well), which I figure is essential if the patch is to get in during beta. Apart from that, the patch is pretty much the same.

Tested for install of 5.0 beta and for upgrade. Tested for creating / editing / resetting / deleting node types. All working well AFAICT.

$underscore_before? Document that function.

if ($column_added) is a weird concept. We usually assume that the column was added.

Is the only module change needed removing the 'module' key from hook_node_info? What would would happen if that were left in?

bump? anyone have time to work on this before the API freeze?

+1

Is this getting into 6?

Bumping this to the top of my issues list. Whatever else gets done to nodes this version, this is a critical API bug and we *cannot* let it last for another version.

for D7 we made it "base" already. http://api.drupal.org/api/function/hook_node_info/7 it's unfortunate that "module" was released in D6, but too late now...