In order to declare your own basic node type with just a title/body field, here's what I would expect to have to do:

1. Implement hook_node_info().
2. There is no step 2.

Instead, here's what I actually have to do.

1. Implement hook_node_info().
2. Implement hook_form(), copy/pasting the first bit of the documentation example. I then have to manually keep my hook_form() implementation in sync with whatever node module does between core versions.
3. Implement hook_perm(), so that I get create/edit/delete/etc. permissions. I then have to manually keep my hook_perm() implementation in sync with whatever node module does between core versions.
4. Implement hook_access(), so that the permissions defined in hook_perm() do anything. I then have to manually keep my hook_access() implementation in sync with whatever node module does between core versions.

Probably other stuff too. This is dumb.

I would of course love to have the option of implementing these hooks for more fine-grained control over my form or my permissions, but the defaults should just be "do what node module does." There's no reason I should need 100 lines of copy/pasted code to implement a basic node type.

Files: 
CommentFileSizeAuthor
#22 441148-refactor-node-type-api.patch68.48 KBDamien Tournoud
Failed: Failed to apply patch. View
#16 441148-refactor-node-type-api.patch68.2 KBDamien Tournoud
Failed: Failed to apply patch. View
#14 441148-refactor-node-type-api.patch69.17 KBDamien Tournoud
Failed: Failed to apply patch. View
#12 441148-refactor-node-type-api.patch57.84 KBDamien Tournoud
Failed: 10165 passes, 590 fails, 83 exceptions View
#10 441148-refactor-node-type-api.patch57.83 KBDamien Tournoud
Failed: Failed to apply patch. View
#8 441148-refactor-node-type-api.patch57.83 KBDamien Tournoud
Failed: Failed to apply patch. View
#6 441148-refactor-node-type-api.patch57.83 KBDamien Tournoud
Failed: 10165 passes, 590 fails, 83 exceptions View

Comments

nevets’s picture

The way I read your post you are creating a content type with no extra fields and no special functionality so why would you use a module? It would be nice if hook_node_info() allowed for more complete description, say the schema to use, perms more at the CRUD level etc and such with default function function handling the current set of hooks and the ability to say I want to use this function to load a node or what ever, kind of the way hook_menu works, with default handlers/functions for some things but with a way to specify your own.

webchick’s picture

There are a couple reasons to do this:

1. I want to ship my module with all of the content types it already needs without having to screw with CCK exports/node_type_save() or manual instructions to "go here and create a content type."
2. I want to use some of the handier node hooks, such as hook_access(), so I don't have to go all crazy with the full node access system.
3. I want my content types in version control, not in the database.

joachim’s picture

4. You want to add some sort of extra functionality and data storage that's not CCK (eg, poll module!!!), but for everything else you want the standard bits.

Definitely +1.
Having to repeat the title and body form elements is crazy. Also, it means that a lot of custom modules don't respect the 'blank body field label to omit body' feature.

caktux’s picture

big +1 from me too, really like nevets idea too

moshe weitzman’s picture

Title: The node API is a DX nightmare » Improve node API's developer experience

I haven't seen much use of custom node modules in years and i think it will get even less common in d7 now that we have fields. fields can't yet live in pure code so will a site developer really be happy with code defined content type and DB defined fields? ... But if someone wants to improve it, great.

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
57.83 KB
Failed: 10165 passes, 590 fails, 83 exceptions View

Let's try something like that (patch in the works, given in sacrifice to the test bot).

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
57.83 KB
Failed: Failed to apply patch. View

Hum. I'm unable to reproduce the bot's failures...

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
57.83 KB
Failed: Failed to apply patch. View

Again.

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
57.84 KB
Failed: 10165 passes, 590 fails, 83 exceptions View

Ok, once again (my git upstream repository is only updated every quarter of hour).

Status: Needs review » Needs work

The last submitted patch failed testing.

Damien Tournoud’s picture

Status: Needs work » Needs review
FileSize
69.17 KB
Failed: Failed to apply patch. View

Getting there, slowly.

Damien Tournoud’s picture

Ok, now, a summary.

This patch implements a DrupalNodeType class, defining the methods that were previously the node module "pseudo-hooks" (ie. load, insert, update, delete, access, view, prepare, form, validate). Defining a new content type is now:

Implementing hook_node_info() (no change from today, except that you define 'class' instead of 'base')

function poll_node_info() {
   return array(
     'blog' => array(
       'name' => t('Blog entry'),
       'class' => 'DrupalBlogNodeType',
       'description' => t('A blog entry is a single post to an online journal, or blog.'),
     )
   );
}

Implement your own subclass of DrupalNodeType

At the minimum, it is an empty class (but it has no real use, then...). You only need to override the methods you want to modify. For example, the blog module only defines access and view:

class DrupalBlogNodeType {
  function view($node, $teaser) {
    if ((bool)menu_get_object()) {
      // Breadcrumb navigation.
      drupal_set_breadcrumb(array(l(t('Home'), NULL), l(t('Blogs'), 'blog'), l(t("!name's blog", array('!name' => $node->name)), 'blog/' . $node->uid)));
    }

    return parent::view($node, $teaser);
  }

  function access($op, $node, $account) {
    switch ($op) {
      case 'create':
        // Anonymous users cannot post even if they have the permission.
        return user_access('create blog content', $account) && $account->uid;
      case 'update':
        return user_access('edit any blog content', $account) || (user_access('edit own blog content', $account) && ($node->uid == $account->uid));
      case 'delete':
        return user_access('delete any blog content', $account) || (user_access('delete own blog content', $account) && ($node->uid == $account->uid));
    }
  }
}
Damien Tournoud’s picture

FileSize
68.2 KB
Failed: Failed to apply patch. View

And some fixes, following dmitri's review:

- Removed the access() implementation of poll and forum. Those were unneeded, because acting exactly as core's one... yay! more code removed
- node_update_7000() now uses update_node_type_7000()
- some comments fixed

webchick’s picture

Are you sure blog's access is the same as node's? I can't see how multiple anonymous users could maintain a blog. But yet anonymous users can create polls.

Damien Tournoud’s picture

@webchick: of course it's poll and forum, not poll and blog, which are the same as node's (edited the post).

Berdir’s picture

I don't want to comment on the whole OOP or not discussion, but imho the class names should atleast follow the naming pattern we already have with DBTNG and is also common in OOP. For example "DrupalNodeType_Blog", and not "DrupalBlogNodeType".

This would also make it possible to enforce the naming convention, by automatically adding the prefix. So it you only have to define 'class' => 'Blog'.

Damien Tournoud’s picture

I don't want to comment on the whole OOP or not discussion, but imho the class names should atleast follow the naming pattern we already have with DBTNG and is also common in OOP. For example "DrupalNodeType_Blog", and not "DrupalBlogNodeType".

Postfixing implementation is not common in OOP, prefixing is. See for example, from the Apache Solr API, the list of child classes of the Tokenizer class [1]: CharTokenizer, ChineseTokenizer, CJKTokenizer, EdgeNGramTokenizer, KeywordTokenizer, NGramTokenizer, SinkTokenizer, StandardTokenizer, WikipediaTokenizer.

[1] http://hudson.zones.apache.org/hudson/job/Lucene-trunk/javadoc//org/apac...

Damien Tournoud’s picture

FileSize
68.48 KB
Failed: Failed to apply patch. View

Here is a new version, implementing a proper DrupalNodeType interface, in addition to the base implementation DrupalBaseNodeType.

Berdir’s picture

Ok, you're right about Java, it's common to prefix there.. However, Java does have namespaces, we do not (yet). The PHP OOP Code I know/looked at (PEAR, ZF, DBTNG ;)) does use Postfixing. And you don't use prefixing either, because of the Drupal "namespace". Another thing that DBTNG isn't using, btw :)

Damien Tournoud’s picture

And you don't use prefixing either, because of the Drupal "namespace". Another thing that DBTNG isn't using, btw :)

About DBTNG, "Database" and "Query" are probably two of the worst names you can choose for a class.

I admit that the prefixing / postfixing mix is not that great. I'll change the names on the next reroll.

sun’s picture

@webchick: Is turning custom node types into OOP really what you imagined? Based on your initial/original post in this issue, I would guess: "no."

@Damien: This patch is incredible in terms of completeness. That Rocks(tm). However, I'm not sure whether it really *simplifies* creation of new, custom node types. ...most likely, "yes", if one takes the assumption that everyone knows how to deal with and code OOP...

@webchick: After all, how much do we really need to focus on custom node types these days? Considering CCK-being-almost-completely-in-core and field API around...?

@all: What's the purpose of custom node types considering field api - and - considering that even poll.module could be turned into a field only?

@anyone: I mean, considering THE custom node type module - Image module - even that will (probably) vanish with D7. Really. I know many contrib modules, but I don't know one that would still need a custom node type in D7.

@core-blessers and @vbulletin-haters: Sure, you have aggregator.module and forum.module in core. But, how many other modules fit into this (limited) scheme?

I know this hurts being at #25. But. Let's re-consider.

I'll do as well.

joachim’s picture

Custom node types can still be useful for glue modules. Eg, I've had to make custom node types that interact with CiviCRM, so when a node is created a matching record is made in Civi. That would be possible with hook_nodeapi and hook_form_alter, but the custom node type hooks are simpler to read as there's one hook per operation rather than big switch statements.
Basically, custom stuff that probably doesn't ever see the light of day on drupal.org because it's all far too site-specific :)

Status: Needs review » Needs work

The last submitted patch failed testing.

Crell’s picture

There are things now that you cannot possibly do without a custom node type; most notably have any sort of more complex access control than what node module does by default. Rather than change the way that one does logic that has to be bound to a custom node type, I'd rather get rid of the need to have a custom node type in code in the first place.

joachim’s picture

I thought of a custom node type that I don't reckon can be replaced with cck: station module's schedule nodes. That needs to maintain its own tables to store programs in timeslots, and has an extra tab to edit them.

Having implemented a few custom node types myself, here's how I reckon the process could be simpler:

1. Implement hook_node_info().
2. Implement hook_form(). Chances are if you want a custom module you want to customize the form. We could do two things here:
- add a short-circuit so if this hook is missing, node module uses its own thing.
- for modules that do want to implement hook_form, add a function that can be called to get the standard title and body fields, as copying these from node module just feels silly. So you'd just say in your hook_form: node_get_title_and_body().
3. Implement hook_perm(). Same here. There would be heaps of benefits to being able to say node_content_permissions('my_type_name') in hook_perm. This would return an array of the standard core perms: create own foo / edit own foo / edit all foo / etc that you could merge in with any other permissions your module needs. (Incidentally, would make this module's job easier: http://drupal.org/project/node_permissions_grid)
4. hook_access() -- if the permissions are standard, then hook_access can probably be skipped -- maybe leave it as a hook in case some more ecccentric modules need it. But like Crell says, complex access requirements shouldn't need to be bound to a content type.

Finally:
5. Documentation. Even if we simplify the system, the effort is only worthwhile is combined with clear, authoritative documentation on how the system is meant to be used. Last time I looked the API example module and pages in the documentation here didn't implement all the same hooks and didn't explain which were essential and which optional

sun’s picture

Version: 7.x-dev » 8.x-dev
joachim’s picture

Points 3 and part of 4 in my comment #29 have since been done in D7.

Point 5 is possibly still relevant to D7.

sun’s picture

Given where we're heading with the entity system, bundle system, and configuration system for D8, it seems to make more sense to drop and remove hook_node_info() entirely.

I quickly searched the core queue, but wasn't able to find an existing issue for that possible direction. Do we want to re-purpose this issue for that or create a new one?

alexpott’s picture

Status: Needs work » Closed (won't fix)

To create a node type plus any fields in Drupal 8 now all you need to do is have the requisite config entity files in the modules config directory. For an example see the standard install profile or the forum module.