[drupal-devel] [bug] Multiple node types are broken

Sat Aug 27 11:06:51 UTC 2005

Issue status update for 
http://drupal.org/node/29785
Post a follow up: 
http://drupal.org/project/comments/add/29785

 Project:      Drupal
 Version:      cvs
 Component:    node system
 Category:     bug reports
 Priority:     critical
 Assigned to:  chx
 Reported by:  chx
 Updated by:   asimmonds
 Status:       patch (code needs review)

My comments were against node_basename_1.patch, with that node/add fails
to show any content types.

asimmonds

Previous comments:
------------------------------------------------------------------------

Sat, 27 Aug 2005 08:31:11 +0000 : chx

Attachment: http://drupal.org/files/issues/node_basename.patch (3.92 KB)

People come and tell me that I have broken node_get_module_name. I tell
them I fixed it. This leads nowhere. Yesterday Kjartan, jakeg and me
had finally a conversation during which we have arrived to the
conclusion that there is no need for two hooks and hook_node_name could
return either a string or an array in the form of:

<?php
array('type-a' => array('node type a', 'base')), 'type-b' => 'node type b', 'node type c');
?>

Which would define a 'type-a' , with a human readable name 'node type
a' and base_load would be called for hook_load and so forth. for
'type-b' , the difference is that modulename_load would be called. For
'node type c', the node type would be modulename.

------------------------------------------------------------------------

Sat, 27 Aug 2005 08:53:15 +0000 : jakeg

+1

Except on third example surely their shouldn't be spaces in 'node type
c' as surely that won't work?

Just to make it clear to people: this patch enables a module, such as
project.module, to define more than one node type, using different
hooks for each of those node types within the same module. Otherwise,
functions would have to be shared between the node types defined by the
function, or a different module would be needed for each node type.
Neither of which are desirable.

------------------------------------------------------------------------

Sat, 27 Aug 2005 09:20:08 +0000 : jakeg

Currently gives warnings such as:

Warning: Missing argument 1 for page_node_name() in ...something.module
on line x

Perhaps add argument $null = '' or something similar for backwards
compatability as e.g. page.module calls as thus:

function page_node_name($node) {
  return t('page');
}

... so we need the $node argument to be allowed, even if its totally
pointless.

------------------------------------------------------------------------

Sat, 27 Aug 2005 09:26:07 +0000 : chx

Attachment: http://drupal.org/files/issues/node_basename_0.patch (16.81 KB)

I thought a bit more on the issue and created something which I think is
a very clean solution. What do we need? We need a capability to get
human readable names for node names. Also, we need occasionly a
capatibility to override 'module' as the base for 'hook_foo'. Well,
then let's define hook_node_name and hook_base_name for these purposes.

hook_node_name returns either a string or an array eg. 'somemodule-1'
=> t('this is a content type');. 

If you want to use different base name, define them in hook_base_name
eg. 'somemodule-1' => 'somemodule_text' and then 'somemodule_text_load'
will be called. If you have only one node type per module ie. you
returned a string in hook_node_name then hook_base_name is not even
called, there is no reason.

Now node_list('somemodule-1') would return t('this is a content
type');. Also, node_list() as it stands is pretty useable in itself as
you'll see in the code, it elinimates a few lines of code here and
there.

The change of node_get_module_name to node_get_base_name affects only
group and og module in contrib and as for current node type modules the
output is the same (because noone has hook_base_name atm) a simple
replace will do as upgrade.

------------------------------------------------------------------------

Sat, 27 Aug 2005 09:29:43 +0000 : chx

Attachment: http://drupal.org/files/issues/node_basename_1.patch (20.71 KB)

Oh, jakeg is right, I changed hook_node_name to be parameterless.

------------------------------------------------------------------------

Sat, 27 Aug 2005 09:34:04 +0000 : robertDouglass

question: would this functionality allow us to get rid of page and story
node types in favor of a similar module that can be called whatever you
want to call it?

Instead of
function hook_node_types() {
  return array('page');
}

we could have

function hook_node_types() {
  return array(variable_get('user_defined_nodes', array('page',
'story'));
}

This would let us get rid of these two identical (yes, do a diff on
them, they're identical now) modules and let people start defining all
the module names that should be attributed to the title+body paradigm.

chx - get what I'm saying? How would your patch support this?

------------------------------------------------------------------------

Sat, 27 Aug 2005 09:35:13 +0000 : robertDouglass

the second example should be this:

function hook_node_types() {
  return variable_get('user_defined_nodes', array('page', 'story'));
}

------------------------------------------------------------------------

Sat, 27 Aug 2005 09:54:05 +0000 : jakeg

Robert:

I think that would be possible with chx's changes and code:

function simplenode_node_name() {
  return array('page' => t('page'),'story' => t('story'));
}
function simplenode_base_name() {
  return array('page' => 'simplenode','story' => 'simplenode');
}

... renaming page.module or story.module simplenode.module instead.
Then the one module can deal with two identical nodes but giving them
different names.

------------------------------------------------------------------------

Sat, 27 Aug 2005 09:55:50 +0000 : jakeg

hook_base_name should maybe be hook_node_base_name instead.

------------------------------------------------------------------------

Sat, 27 Aug 2005 10:07:52 +0000 : chx

That's not enough. You need short help, long help, an UI to list defined
types and to edit them. I have such a module -- I ripped fields stuff
flexinode. It's possible I'll redo it (not big a job) by ripping fields
from content.module instead :)

------------------------------------------------------------------------

Sat, 27 Aug 2005 10:13:07 +0000 : jakeg

in theory chx whenever the word 'page' or 'story' is mentioned you could
use node_get_module_name instead.

and in the seldom places they do deviate other than simply the word
page/story, then you can have a switch statement based on
node_get_module_name

maybe i've missed a whole chunk of discussion re. simple node types etc
in the past though

------------------------------------------------------------------------

Sat, 27 Aug 2005 10:33:18 +0000 : asimmonds

In node_list():
      else {
        $types[$module] = array($value, $module);
      }

I think should be ($node_names is a string here):
      else {
        $types[$module] = $node_names;
      }

As I'm getting a few errors as a array is being passed to other
functions instead of a string.

I've now got a working project module against HEAD, with this in
project.module and chx's patch
function project_node_name() {
  return array('project_project' => 'project',
               'project_issue' => 'issue');
}
function project_base_name() {
  return array('project_project' => 'project_project',
               'project_issue' => 'project_issue');
}

------------------------------------------------------------------------

Sat, 27 Aug 2005 10:50:03 +0000 : chx

Assimonds, your comment is not against the latest version of my patch.
Please update.