Pave the way for CCK

I like the approach this is taking.

And the code looks good.

Didn't I review this patch elsewhere? The suggestions have not been incorporated yet, it seems.

This starts to look better and better! The patch doesn't apply though. :)

1. I'd make _node_types() non-caching. It's not like it saves you a SQL query. Premature optimization, IMO.
2. Also, I'd remove the duplicate functionality. Creating a new content type take 30 seconds. The duplicate is just not worth it, and it's not something we offer elsewhere.
3. Titles are not always properly escaped, making them prone to XSS injection attacks.
4. node_clear_type_cache() serves what purpose?
5. I like 'name' better than 'label'.
6. The node_types / node_names stuff is somewhat ugly. There are APIs but they are being hijacked. Looks like we might be able to simplify some of this, or at least clean it up some more ...

How do you create a new content type? the only menu item it created is admin/node/types/'. arg(3) .'/delete'.

I like Dries's reedback, except #2-

We should have duplicate, at some point, it saves time. But, I DO think it should be removed from the initial patch, since I like to see one thing done at a time and not having it isn't hurtaing anything as far as I can see. A new button will be needed initially.

Additionally- what exactly does content module do now? It looks like more code can be folded into node.module since it make a menu item under node's tree. I'm still thinking this can be done most cleanly with no new files.

i couldn't get this to apply either.

Cool! Glad to see the patch works again. My review (comment 6) still stands though.

I encourage everyone to test this patch, and to review its usability. It is extremely important to get this right. Thanks!

1. The node caching is for CCK, see module_invoke('field', 'clear_type_cache'); there.
4. node_type_cache_clear() clears the cache you did not like in #1.

Please remove it for now.

5. We have name and label too, so can't help you.

What do you mean with 'we have name and label too'?

no time to test now.. but am adding my mark so that it's in my queue for the weekend.

We need more people to look at this. We gotta make sure it is dead easy to use and that no confusion can arise.

Dries: re:

5. We have name and label too, so can't help you.

chx means that there is both a 'name' field (for internal name like 'blog') and a 'label' field (for user-viewable names like 'blog entry'). Therefore, node_get_name and node_get_label have to be named as they are -- they're both referring to different data.

Ok, so here comes a webchick review. Stand back! ;)

First thing I did was blow away my db entirely and start w/ a fresh copy of HEAD and this patch. I got an error about the node_type table being missing and noticed the table definition was not included. Attached patch adds that in.

No content types are enabled by default, since page and story were nuked. So the only options upon logging in as uid 1 are:

- my account
- administer
- log out

We should probably include at least one content type enabled by default.

When I go to admin/node, I receieve the error:

warning: Invalid argument supplied for foreach() in D:\xampplite\htdocs\head\includes\form.inc on line 642.

When I enable content.module I receive the error:

warning: Invalid argument supplied for foreach() in D:\xampplite\htdocs\head\modules\content.module on line 43.

Not sure if administer >> settings >> content types is the most intuitive place to put the "add" ability... I'd maybe make it a top-level menu item underneath administer, so it's right in peoples' faces, since this will be 'core' functionality like categories or menus.

Underneath "Descrption" in the add screen reads:
"A brief description of the content type." -> I'd also mention that this will be displayed in the "create content" menu.

When attempting to add a new content type I received:

* user warning: Column count doesn't match value count at row 1 query: INSERT INTO node_type (base, name, label, description, help, title_label, body_label) VALUES ('content', 'content-page', 'Page', 'Create a page for your spiffy site.', 'Help!', 'Title') in D:\xampplite\htdocs\head\includes\database.mysql.inc on line 120.
* warning: Invalid argument supplied for foreach() in D:\xampplite\htdocs\head\modules\content.module on line 43.
* warning: Invalid argument supplied for foreach() in D:\xampplite\htdocs\head\modules\content.module on line 43.
* warning: Invalid argument supplied for foreach() in D:\xampplite\htdocs\head\modules\content.module on line 43.

I'm on PHP 5.1, not sure if that's causing drama or not.

Quote from Webchick: Not sure if administer >> settings >> content types is the most intuitive place to put the "add" ability... I'd maybe make it a top-level menu item underneath administer, so it's right in peoples' faces, since this will be 'core' functionality like categories or menus.

I agree for the most part. I heard somewhere that an admin menu item goes under settings if it's something you set when you create the site and leave alone, and this falls under that category for the most part. But on the other hand, I think this is a huge feature that needs to be right under admin.

Just my $.02.

Hi,

Just a short post to say this patch is great and I'd love to see it in next Drupal, and to get posts to this issue by email from now on :-)

Btw, I'm about to contribute a stand alone module that will implement this very same feature -and most of this code- and I hope it may be kind of 'transitional' module from 4.7 to 4.8...

5. We have name and label too, so can't help you.

chx means that there is both a 'name' field (for internal name like 'blog') and a 'label' field (for user-viewable names like 'blog entry'). Therefore, node_get_name and node_get_label have to be named as they are -- they're both referring to different data.

How about 'type' and 'name'? So 'name' becomes 'type' (as in 'node type') and 'label' becomes 'name'?

This seems to remove all the major issues that we found during our debugging session this afternoon. Changing back to "needs review."

When reviewing this patch, bear in mind that the "create content" menu won't show up until:

a) you enable the content module under administer >> modules
b) you create at least one content type under administer >> settings >> content types

Nice going, chx! I have issues the menu locations, but as we discussed I'll open a separate issue about that. :)

I tried out this patch, and I think it's a good start, but it still needs plenty of work, and it also still (IMO) needs plenty of thought. My review of the patch:

1. The table name for the 'CREATE TABLE' statements (in updates.inc) needs to be wrapped in curly brackets. Currently not working for sites that use table prefixing.
2. In the content_perm() function, foreach ($node_get_type as $type) should be foreach (node_get_types() as $type).
3. I agree with Dries's suggestion in #6: 'name' should be 'type', and 'label' should be 'name'. This would make it consistent with hook_node_info(), and would be a better naming system IMPO.
4. Speaking of which: why are you removing hook_node_info()? Obviously, content.module and CCK do not need this hook, but what about modules that need to define nodes in the traditional way? Are such modules going to be expected to maintain entries in the 'node_type' database table? If so, where are the API functions to help them do this? I think that the best solution is to make it so that:
   • the 'node_type' table becomes the new 'source of truth' to find all node types;
   • BUT we keep hook_node_info();
   • node types defined through hook_node_info() get added to the 'node_type' table automatically, whenever the 'node type cache' is cleared (i.e. analogous to the way that menu items are defined through hook_menu(), and are automatically added to the 'menu' table whenever the menu cache is cleared).
5. Honestly, I'm quite unimpressed with the whole arrangement that this patch sets up in the 'admin/settings/content-types' area of a site. At the very least, the 'add new' fieldset should be removed from this page, and the form inside it should be moved to a separate 'add' tab (i.e. 'admin/settings/content-types/add' - then the default tab becomes 'list'). Ideally, all of this should be moved to the 'admin/node/types' page, and should be made more consistent with the CCK UI, which is IMO much better in terms of usability.
6. Following on from the 'keep hook_node_info()' point, I also think that we have a lot to learn from the 'admin/menu' page, in terms of designing a user-friendly way to distinguish module-defined entities from user-defined entities, and also a user-friendly way to manage both types of entities on one page. The current design doesn't even cater to module-defined node types at all, and this is just crazy.
7. Upgrade path needed from the old 'page' and 'story' types. Upon install, the 'content' module should create these two content types in the 'node_type' table (or, even better, define them through the improved hook_menu()-like hook_node_info() that I envisaged above). The upgrade script should change all existing 'page' or 'story' nodes to 'content-page' or 'content-story'. ATM, existing page and story nodes become 'typeless' when this patch is applied, and their title and body fields do not appear in the node editing form.
8. Why is the 'has_title' field needed? Titles are currently compulsory for all node types, and I think it should stay this way. If titles become optional, then managing nodes on pages such as 'admin/node' becomes a nightmare.

I'd be happy to work on some of the issues that I discussed above - otherwise I guess you can implement them, chx. ;-)

Jaza: thanks for the excellent review.

chx: thanks for working on the patch.

Jaza: do you think you have to work on the patch some more? For example, the UI and upgrade path? That would be super. :)

I don't like the following piece of code:

+  $type = trim($form_values['name']);
+  $type = drupal_strtolower($type);
+  $type = str_replace(array(' ', '-'), '_', $type);
+  $type = preg_replace('/[^a-z0-9_]/', '', $type);
+  $type = 'content-'. $type;
+  $type = substr($type, 0, 32);

We need to figure out a way to get rid of that. I'd rather ask the user to provide a 'type' in the submission form (and to validate it properly) than Drupal trying to extract it using the above code.

It's good to have control over the type name; it might get used in the URLs, for CSS class names, etc.

Also, what happens when someone changes $name? Does that break existing content? Maybe we need a numeric ID to map nodes onto their types? That would prevent breakage when things get renamed.

All the code surrounding $type is a little bid odd. I'm hoping we can clean that up a little. Still wrapping my head around it some more.

Also, I'm still not convinced that we need the database caching in _node_types. Drupal's database cache is not super-fast, neither is serialization. Not to mention Rasmus adviced us not to use serialization becuase it is memory hungry.

I don't like this piece of code:

+function node_get_name($node, $check_plain = TRUE) {
+  $name = _node_types('name', $node);
+  return $check_plain ? check_plain($name) : $name;
 }

It's a construct that we don't use elsewhere. The check_plain() should be removed from that function, and should be done on output. get-functions shouldn't do check_plain()s.

I think the code to set up $type which Dries commented on is okay. Machine-readable strings aren't something the average user shouldn't have to see or care about. If we can make a good string programmatically, then we should do it. Every unnecessary file removed is one step to getting the user to their goal.

Numeric things (flexinode-N and field-N) caused a few headaches in flexinode. Once you get into custom programming you need to constantly look up what all the numbers mean with no obvious listing (look directly at the database tables or the URLs of configuration pages) until you remember them. Numeric ids are also not portable from site to site if they appear in templates.

The string processing that happens looks okay. If it is ever used as a HTML class or id, then any numbers on the front should be chopped off.

For renaming- my initial thought is that all name changes should be propegated. If that affects a URL, then a 302 redirect should be set up. However that does break some things, so maybe it should always remain the same. I'd be curious to know what thought has gone into this decision previously from the CCK team.

Just FYI, people: I'm working on a big change to this patch, which will:

- allow for clear distinguishing between module-defined and user-defined node types
- allow module-defined node types to be modifiable or locked
- allow any node types to be enabled or disabled

This will bring much of the functionality and UI goodness of menu items to node types. Will post an updated patch when one is ready.

I'm working on a UI cleanup one.
- Validate the add content type form (probably some required fields)
- Put the form in a tab instead of a form_group.
- Provide an appropriate view when there are no content types.
- Probably remove the extra operations column for edit fields. I'm thinking of doing a mockup encompassing a bit of redesign of the existing edit content type form and incorperate the field editing fields on the same page (in a separate issue, of course).
- Whatever else I see along the way.

I'd be happy to pass off the first few items since they are relatively straightforward, and I haven't gotten a lot done just yet. In fact, here is what I got done:

    if (count($rows) == 0) {
      $rows[] = array(array('data' => '<em>'. t('No content types have been defined. You can <a href="%url">add a new content type</a>.', array('%url' => 'admin/settings/content-types/')) .'</em>', 'colspan' => 4));
    }

(Change %url as needed once that exists as a separate page.)

Preparing this patch is taking a bit longer than I anticipated. The patch is NOT yet complete, but I have posted what I've done so far. What you'll notice is that I seem to be copying lots of ideas and code from the menu system. This is because I believe that we have a lot to learn from the menu system, and that node types would be better off if they behaved more like menu items. Major changes to the node type system:

- node types have a 'status' field, which consists of a number of binary flags, such as NODE_TYPE_ENABLED and NODE_TYPE_MODIFIABLE_BY_ADMIN. These flags are based on those used in the menu system (for menu items).
- the node types admin page is at 'admin/node/types' (consistent with CCK in contrib), and has a UI similar to the 'admin/menu' page. That is, there are options for enabling/disabling node types, and editing/deleting/resetting node types. Node types can be locked, and appear as such in the UI.
- The whole UI for managing node types (at 'admin/node/types') is defined in content.module. If content.module is disabled, then there is no front-end for managing node types, and module-defined node types get handled behind the scenes. This is similar to the way that menu.inc handles background stuff for the menu system, and menu.module provides an optional UI for admin mgmt.
- module-defined node types are still defined by hook_node_info(), and hook_node_info() is fully compatible with its 4.7 version; but additional fields such as 'description' and 'status' can be set with it. The 'node_type' table is now the 'source' of all node types, and nodes defined through hook_node_info() are collected and put into this table when node_types_rebuild() is called - similar to menu_rebuild() behaviour.

Major todo items:
- Get the add/enable/disable/reset UI functionality built - currently only the 'edit' functionality in the UI works.
- Get rid of 'admin/settings/content-types' altogether, and move all those settings to 'admin/node/types/type-blah/edit'.
- Upgrade path.

This is a great time for interested people to give this patch a try, and to see if you think it's going in the right direction. If there are things in this patch that you really don't like, then let me know sooner rather than later. Reviews welcome, even at this work-in-progress stage of things.

Another todo item:
- more comments

Looks like this could use a new review ... updated status.

Nice patch, badly needed.

Some notes though:

Permissions:

"create $name content";  "edit own $name content"; "edit $name content";

Why not go for the usual CRUD (create read update delete) support by default. It has always nagged me a lot that we did not have CRUD support like any other CMS has. The patch for CCK is not that hard to merge into this project. The benefits are HUGE. We could consider dropping the "view $name content", since that is the hard part that requires hook_db_rewrite_sql(). The rest is VERY trivial. But a huge benefit IMO. http://drupal.org/node/53772

Menu location:
I despise (that is: far worse then not liking) the place where CCK and Flexinode tuck away their Admin interfaces. Can someone (CivicSpace) not do a small usability interview and compare/test the following concepts?
/admin/settings/content-types (integrated with the current content-type settings)
/admin/node/types (like it is in the patch)
I guess doing that investigation *before* the patch lands is better then fixing usability afterwards!

Page vs. Story:
I still fail to see why we need both. Yes, for flexibility they can be nice. But since they don't differ at all it is very misleading! I would very much like to drop "pages" alltogether and *communicate* to the users that books are meant for static content! (they actually DO serve that purpose, while pages == stories both don't). It makes the patch simpler too!

LOCKED:
I am no fan of «making hardcoded choices to "protect" users». If you tell users clear enough that they should not delete something because it could break their site, that should do. For developers or people who want complex sites, such hardcoded "drupalisms", (where Drupal has hardcoded behaviour just to protect users), are a nightmare! You will always find a situation where you want to remove "that locked menu item" or rename that "locked" content type!
I vote for not locking anything, just warning people about dataloss when they don't respect Drupals 'guidelines'. Its up to them to decide. Not up to us!
I vote for dropping the NODE_TYPE_MODIFIABLE_BY_ADMIN flag alltogether, or else just treat it as advice.

I love the fact that one can hand-set the internal (machine-readable) name! Very good feature!

All in all a very nice patch.

I agree with you Bèr, Drupal needs CRUD and both page n story is unnecessary...

All in all a very nice patch.

How can we do Ber's CRUD in a way that does not conflict with node-level permissions?

As I've said before, I'd actually go a step farther and point out that RUD has two versions: Own node and any node. So a full breakdown would in fact have 7 permissions: Create, View Own, View, Edit Own, Edit, Delete Own, Delete.

As for how to not conflict with node-level permissions, I'm not sure I understand. The logical way to do a complete breakdown would be as permissions on nodes via hook_access(). Instead of the current de facto standard of "create foo", "administer foo", "view foo", we'd have 8 permissions per type: administer (all perms) and the 7 above. That's node-level permissions right there, per node type.

Or am I misunderstanding you?

subscribing and try to fix html

Crud² = crud own + crud all.

Acctually I don't like todays permission system. I think we should able to set permissions on user, role AND group basis. Especially larger sites/communitys/intranets needs often 'more' permissions than the ones that we already have. I have a site with two users, and I miss something like this. I want my co-editor to be able to edit his menu items but not mine...

Thanks for the feedback, people (especially Ber). I have reviewed the recent comments, and will be implementing many of the suggestions soon.

Regarding the CRUD issue, I am all for more permissions, and I think that it is worth adding the 'full' suite of permissions to this patch. However, as for forngren's suggestion that permissions be available on a "group basis", I assume that this is referring to groups in the OG sense? If so, then this is something that the HEAD/4.8 version of OG will have to implement separately. In fact, there's no reason (AFAIK) why OG, or any other custom-node-type module, can't do this now in 4.7.

I haven't looked at the OG but I think it's what I'm preposing, think Unix/Linux groups. But if this patch included it, that would really 'pave the way for CCK'. Or shall we ask the OG devs to try to get into core? where I think it belongs.

IF we limit it to CUD2 we dont conflict with any node level permissions. But most of all, we will make the patch VERY simple.
The only part in that patch I pointed to that is hard to do, is the 'R' part. This is because of Drupals architecture, that does not really "respect" view permissions.

IMO CUD2 (Create, Update own, Update all, Delete own, Delete all) is already a huge step forward. We can bend over the R part in a later, different patch. Or even a contrib module.

forngren: Could you explain how a "role" would differ from a "group"? From what I know of Unix (and Windows) permissions, I would call Drupal's roles "groups" in those contexts.

OG Is something different, it allows people to setup their own kind of "sub" community sites within a main site. See http://groups.drupal.org/ for an example.

On the subject of C(R)UD2, I'm fine with doing this if we can do so in such a way that modules implementing more fine-grained permissions (at the node-level, or taxonomy-level (OG, taxonomy_access, node_privacy_byrole, etc.)) can override the system defaults. I assume that's already being thought of though. :)

Can we leave the CRUD stuff out for now, and focus on a simple patch? I'd prefer to revisit the CRUD stuff later just to keep this managable. Let's get this into core ASAP, and build on it incrementally. Thanks!

Jaza: can you upload one or two screenshots for people to review or look at? Like that, people could give feedback on the UI.

we can't do the R in CRUD without slowing down drupal for node listings. The 'view' operation of hook_access() is not checked for a reason. That would be very slow and difficult for the pager. Really, I think thats a separate issue and shouldn't cloud this patch.

The discussion about crud can go on in this thread, I guess, http://drupal.org/node/53772. It's Bér's issue about the same subject

Latest patch includes all functionality for adding / editing / deleting / enabling / disabling / resetting node types.

Also, the admin overview page has been moved from 'admin/node/types' to 'admin/types', in an effort to improve usability. I am personally very against putting this stuff in the 'admin/settings/content-types' area, as it would be inconsistent with other interfaces for managing lists of entities (e.g. blocks, users, menus), which are all in the top level of the admin section; and as I don't like the menu structure of 'admin/settings/content-types', or the number of levels/clicks required to get there. Hopefully this spot in the menu tree will seem more logical to more reviewers.

Basic upgrade script is now in the patch for updates.inc. This only updates the node type for all nodes. It doesn't account for per-node-type settings (stored as variables), which I'm not even sure if it's possible to update automatically (since the variables don't seem to have a standard naming convention, e.g. prefixing all variable names with the node type).

In accordance with the wishes of Chx and Dries, additional permissions have not been added to this patch (I understand and mostly agree with your reasons, guys).

Note: there is currently a serious bug with editing the 'type' field of module-defined content types. When you change this value for such a content type, rather than updating the existing content type, a new duplicate one is created. I have now realised that it will be necessary to change the DB schema for the 'node_type' table, to add a field called 'orig_type', which will allow the original internal name (as defined by modules) to always be linked to the database entry for that content type.

Fixed a bug that prevented node types from being disabled.

This is how the admin overview screen looks with the patch applied.

This is how the screen looks for editing a node type with this patch.

I can't do an in-depth code review now, but the screenshots look nice. Thanks for posting these.

I'm not 100% about the '(disabled)' text in the 'name' column. That looks a little bit weird, and there might be better UI-ways to accomplish that.

In future, I can see more per-node settings moving to the node type edit page. Like, merge it with the stuff that is on ?q=admin/settings/content-types/story, for example. (Maybe that is already taken care of.)

Good job, and hopefully, others can focus on reviewing and testing the code during my absence! It's important to keep this moving, and to get it ready for inclusion in Drupal core.

Neil wondered if we really needed a new module (content.module), or whether this is best part of node.module.

To be honnest, I think you took things a little too far, but I'd love to discuss this further.

1. The various NODE_TYPE's makes the code hard to understand.
- NODE_TYPE_SYSTEM, is not used, and it is not clear what it would be used for.
- Why would some node types be invisible?
- Why would you want to disable a node? You can remove the permissions to post new ones. What are the side-effects?
- What would there be node types created by admins that are not editable?
- Why do we have both NODE_TYPE_MODIFIED_BY_ADMIN and NODE_TYPE_MODIFABLE_BY_ADMIN?
Correct me if I'm wrong but in its current state, it looks somewhat over-engineered to me. We'll want to simplify this a lot in order to get it into Drupal core (less defines, less status fields, less special cases).

2. The way node->status is used it not super-readable. Can't we create multiple fields rather than using all sorts of masks? It would be easier to run SQL queries against it, or to inspect the fields.

3. Please remove the database caching from _node_types() or show that it actually improves performance. Do you have information about the number of database queries we have with or without the caching?

There was a recent issue over on CCK about changing 'content- ' to content_ ' to enable theming.
It is found here: http://drupal.org/node/69472

I thought that discussion impinged on this patch.

To be honest, I think you took things a little too far, but I'd love to discuss this further.

I'm not surprised to hear this :P. To be equally honest, I have been a bit naughty, because the patch as it stands now is doing much more than paving the way for CCK. It is paving the way for a more solid and flexible node system in general. But I was hoping that we could get at least some of this in as part of the patch. I'd be happy to discuss what needs to be taken out of this patch - I can then move those parts to a separate patch.

1. The various NODE_TYPE's makes the code hard to understand.
- NODE_TYPE_SYSTEM, is not used, and it is not clear what it would be used for.
- Why would some node types be invisible?
- Why would you want to disable a node? You can remove the permissions to post new ones. What are the side-effects?
- What would there be node types created by admins that are not editable?

This is all intended to make the node system more flexible, so that modules can define node types that are not necessarily 'content' in the traditional sense. This patch introduces the concept of a 'node type' being different to a 'content type' - specifically, a 'content type' is a subset of a 'node type'. Hence, the NODE_TYPE_IS_CONTENT flag. When this flag is disabled, node types do not appear in the 'create content' listing - so a module could define its own custom form for adding a node of this type. Or perhaps the node type is not one that is ever intended to have user-created instances, so no create/edit form is needed for it. It may not be suitable for such types to be editable, or even to be visible to the site admin.

The NODE_TYPE_SYSTEM definition is a proof-of-concept type that embodies all of this. Since it doesn't actually apply to any of the node types in Drupal core at present, it isn't used. But contrib modules could use it, if they needed to create a node type similar to the hypothetical one that I have described above.

Ultimately, this paves the way for the possibility (NOT the actuality!) of nodes being used for entities in Drupal that are not 'content' as such (e.g. comments, users). I'm not saying that we should be converting any or all 'system' entities in Drupal to become nodes. But I am saying that we should be improving the node system to begin to support such entities, and that contrib modules should be able to define node types that are not content types.

- Why do we have both NODE_TYPE_MODIFIED_BY_ADMIN and NODE_TYPE_MODIFABLE_BY_ADMIN?

For the same reason that the menu system has MENU_MODIFIED_BY_ADMIN and MENU_MODIFIABLE_BY_ADMIN - the 'MODIFIED' flag lets the UI know whether or not the 'reset' option needs to be displayed. It is also needed when rebuilding the node type list, as it lets the rebuilding code decide whether or not the database fields for the node type override the module-defined fields.

Correct me if I'm wrong but in its current state, it looks somewhat over-engineered to me. We'll want to simplify this a lot in order to get it into Drupal core (less defines, less status fields, less special cases).

Apart from the stuff related to node type visibility and 'IS_CONTENT', I have tried to keep the number of new constants and such to a minimum. I have also tried to keep all the code related to user administration of content types out of node.module, and in content.module (so that users can disable content.module if they don't need that functionality).

2. The way node->status is used it not super-readable. Can't we create multiple fields rather than using all sorts of masks? It would be easier to run SQL queries against it, or to inspect the fields.

Personally I like the binary masks, and find them to be quite easy to work with. But changing the fields to individual booleans wouldn't be hard.

3. Please remove the database caching from _node_types() or show that it actually improves performance. Do you have information about the number of database queries we have with or without the caching?

Haven't benchmarked this, but I'm guessing that the performance gain from caching isn't huge. But this would depend a lot on the number of user-defined content types on a site.

@chx:

node type caching should be tied to the presence of fields module (CCK). My version contained that.

What is this mysterious 'fields' module, and where can I see it? It's not in the CVS version of CCK.

@Jaza - Could you give a concrete example or two of 'system' node types and thus should not show up in search results, node/add page, etc. I think that will help us understand the utility of this feature.

@moshe:

Could you give a concrete example or two of 'system' node types and thus should not show up in search results, node/add page, etc. I think that will help us understand the utility of this feature.

Sure - I'll use the two examples I've already given, of 'comments as nodes', and of 'users as nodes'. Neither of these would require the usual node add/edit form, and so neither of them would be displayed in the 'create content' listing. Instead, they would implement their own custom forms - the comment form is embeded/linked within node pages, and the user form is shown either as the registration form, or as the user admin form.

I can't think of an example in core of a 'system' node type that would not be indexed for searching. Perhaps comments, since they get indexed anyway, along with their containing node? Or maybe if comments were to become nodes, it would be more appropriate to index the comment nodes in their own right, and to them exclude them from being indexed as part of their containing node.

Jaza: I think that the node vs content type discussion is a good one. I've been thinking about this quite a bit, lately.

'user as nodes', 'comments as nodes', 'groups as nodes', 'categories as nodes' -- they are all good examples of screwed designs. They are abusing the node system and sacrify the user experience when doing so.

The solution is to introduce a new abstract type, but one that doesn't lend itself to poor design, weird UI design, etc. Users, taxonomies, comments and nodes could all extend thist new base type.

So, what you tried doing with 'node types' vs 'content types' is exactly what I had in mind myself. However, they way you did it, is 100% wrong IMO and certainly doesn't belong in this patch.

Please remove it from your patch, and get us back on track. Thanks! And let's not hijack this issue to talk about it.

New patch for latest HEAD. Changes in this version:
- All 'node types are different to content types' stuff is gone - no more NODE_TYPE_IS_CONTENT flag, and no more NODE_TYPE_VISIBLE_IN_ADMIN_LIST flag. Node types are now all always shown in the admin listing.
- Fixed up issues with the 'orig_type' field of node types. This field has now been added to the DB schema, and is never changed after first being defined. The 'type' field can now successfully be edited for the module-defined 'page' and 'story' node types.
- Changed some of the 'orig_type' logic in content.module, to instead track changes to the 'type' field using the 'old_type' variable. Some of this code is a bit messy, but from the testing I've done so far, it is pretty solid and reliable.
- Moved some of the settings from the 'admin/settings/content-types' page to the 'admin/types' page. Still needs a bit of cleanup, but we're almost ready to ditch 'admin/settings/content-types' altogether. Also added 'min_word_count' to the DB schema - this was previously stored as a variable by the settings page.
- Usability improvements - wrapped the stuff on the 'add/edit content type' page in fieldsets. Also changed some of the wording of help text, and the ordering, of form elements on this page.

Some extra notes:
- Node types can still be locked, but none of the core types are locked. I believe that although module developers should be strongly discouraged from locking their node types, developers should still have that option if they really need it.
- It is still possible, BTW, to lock the 'type' field of a node type (which is the primary key in the new node_type DB table). NODE_TYPE_NORMAL (the recommended flag for module-defined node types) locks the 'type' field by default.
- Regarding the messy code to handle the 'orig_type' and 'old_type' logic: the only solution that I can see for cleaning this up is to introduce generated numeric IDs for all node types. What are your thoughts on doing this, people?

numeric IDs is quite a familiar pattern (node ids, user ids, term ids). i have no problem introducing them. Their textual equivalent will always be one small join away.

Great work. This is getting better and better. :-)

1. Yes, numeric IDs are perfectly valid, and even recommended. As Moshe points out, we almost use numeric IDs exclusively. I don't know how big of a change that would be, but it certainly would be the best option, IMO.
2. Couldn't test the UI because I got a parse error in content.module. The content.module appears to be incomplete.
3. Why would anyone want to disable a node type? Node types can be 'disabled' by altering the permissions, or by deleting the node type all together. From a user point of view, disabling a node type is utterly confusing. Users will wonder what effect this has, and if it affects their existing content. I'd prefer to remove the NODE_TYPE_ENABLED for now -- unless you can convince us that it is absolutely necessary. I think it does more harm than good but maybe I'm missing something important.
4. I think we should also get rid of NODE_TYPE_SUGGESTED.

See also previous review for more comments. Great to see this moving in the proper direction! Would love to test out the UI as well.

Sorry: in previous comments, I forgot to explain the reasons for allowing node types to be disabled...

Why would anyone want to disable a node type? Node types can be 'disabled' by altering the permissions, or by deleting the node type all together.

The main reason for having this feature is in the case of modules that define more than one node type. You might need to use a particular module, but of the (say) 3 node types that it offers, you only need one of them. Not a problem - you can just disable the other two! When you do this, there are no longer any visible signs of that node type (except for a greyed-out row in the node type admin page), such as links to create content of that type. Of course, for modules that only define 1 node type, the simplest approach for disabling that node type is still to just disable the whole module. :-)

The place where this is most useful is in content.module itself. Content.module provides two 'built-in' types: page and story. Many people have requested that we remove one or the other of these types from core. But if we do that, then the upgrade script will have to convert all nodes of one type to be nodes of the other type (which will seriously stuff up some sites), or else it will have to do some funky tricks to convert all nodes of one type to some other (user-specified?) type. Also, many people really do use both page and story. The solution in this patch is to offer both node types, and to allow either of them to be disabled (but not deleted).

Speaking of which: in this patch, module-defined node types can be disabled (or reset), but NOT deleted. This is an important usability feature, IMO, because module-defined node types should not be allowed to just 'disappear' from a site. If a user is allowed to completely delete module-defined nodes types, then they'll delete (for example) the 'story' node type, and will expect later on (when they've changed their mind) that "since it was there when I installed Drupal, there must be a way to get it back". Also, when a module defines a node type, it has a right to expect that the node type will always be there.

I don't think that using permissions is the right approach, if you want to effectively 'turn off' a node type on your site. Permissions are for restricting per-role access to functionality, not for removing or turning off functionality altogether. Using permissions for this purpose is a hack, IMO. Also, modules that define multiple node types may choose not to offer separate settings for each node type (out of laziness, or for valid design reasons), in which case the user will have no way of turning off individual node types, unless they can be disabled.

From a user point of view, disabling a node type is utterly confusing. Users will wonder what effect this has, and if it affects their existing content.

It is confusing at the moment, because (a) we haven't yet worked out ourselves what effect disabling a node type should have on existing content; and (b) there isn't yet any confirmation page to warn/inform the user of such effects.

Some initial ideas on the effect that disabling a node type should have on existing content:
- Such content should not be shown on user-facing node listings (i.e. front page, taxonomy pages, listings in blocks)
- Menu items for such content should be hidden (through hook_db_rewrite_sql, I guess)
- Such content SHOULD be shown on 'admin/node'
- The pages for such content SHOULD be accessible to users with the 'administer nodes' privilege, but they should not be editable, and a warning should be displayed saying: "Warning: this piece of content is of type %type, which has been disabled. Until the %type content type is enabled, this piece of content cannot be edited or deleted, and elements of this page may be displayed incorrectly. You can enable this content type at the %type-admin-page."

Once we have a confirmation page in place for disabling node types (with information similar to that in the sample message above), I think we'll be able to avoid confusion and heartache for most users.

I think we should also get rid of NODE_TYPE_SUGGESTED.

As long as node types can be disabled, I think that it's only logical to provide this flag for modules. Obviously, if enabling/disabling node types is removed, then NODE_TYPE_SUGGESTED will be removed as well.

Oops - the file length / offset was wrong for content.module in the last patch (so the last few lines of content.module were not getting applied, even though they were in the patch). New patch is the same, except that file length is now correct.

Numeric IDs are fine internally, but I'd like to see them avoided in URL paths and especially avoided in CSS ids and classes.

@drumm - definitely. For URLs, I guess they'd only be shown on the 'admin/types/x' pages, but not on the 'node/add/x' pages. No way that they'd end up in anything CSS-related.

my 0,01 cent:
i like the disable node type feature
because if I'm able to disable a node type I can be shure that it is really not accessible for other users...

• I applied that patch, played with it for 1.5 hours. I disabled one of the content types, yet I had no idea what that implied. In addition, this allowed me to 'reset' a content type. I did that, but I had no idea what that implied either. What would be the use case for having to reset a content type, and how would that affect existing content?

  I appreciate your lengthy explanation, but I still think it does more harm than good. Sure, you can guess the high-level impact of these features, but God knows what this functionality specically. Until we figured that out, we shouldn't offer it. Please remove it for now, and let us focus on it after the base patch landed. Thanks.

• I don't think we need 2 built-in types. Shouldn't 'page' and 'story' by custom types that would be pre-configured? Looking over the page and story modules, I don't see anything in particular that makes them worthy of being built-in/locked/fixed types. For now, it is OK to keep them as built-in types though. We don't have to change it for the base patch.
• The ?q=admin/types page really looks nice. Two minor remarks:
  • I think it should be a sub-page of ?q=admin/content. Its current position in the navigation tree looks out of place.
  • I still think we should remove the 'disable' links. ;-)
• The ?q=admin/types/add page looks nice. Couldn't have done a better job myself. A couple small remarks though:
  • The form descriptions are really good. The one that lacks some information is the 'Type' field. It explains the format but not the field's purpose. I'd try to explain what the type-field is used for. For example, I'd mention that it might be used in URLs. That certainly gives me some context and affects the way I'll choose the name.
  • Isn't 'Type' supposed to be unique? If so, I'd mention it in the field's explanation.
  • I don't understand why the type-field gets prefixed with 'content-'. It doesn't seem to be the case for built-in/fixed content types. Is there a technical reason for this? I'd very much prefer my URLs to be consistent: either all of them have 'content-' or none of them have 'content-'. Give the user full control over it (if it can be done with minimal code complexity)?
  • Isn't 'Type' a required field? If there is 'auto-extract type name' code, please get rid of it.
  • Should the 'Name' field be lower case? If the name is going to be used in sentences, it might be a requirement and worth pointing out?

As it stands, here are some of my requirements in order to get a base patch committed:

1. Remove the 'disable' functionality.
2. Remove the 'reset' functionality.
3. Remove the 'auto-extract type name' code (if still there).
4. Remove the 'fields caching' or proof that it is absolutely necessary.

The content module should be enabled by default if the settings -> content type menu is going to be useful.
So the database files need

INSERT INTO system (filename, name, type, description, status, throttle, bootstrap, schema_version) VALUES ('modules/content.module', 'content', 'module', '', 1, 0, 0, 0);

Why are the settings for a content type found in two places? You can find some under settings - content types and others under content types. Let's move them all to one place.

I think 'administer content types' should be a different permission than 'administer nodes' (in menu definition of admin/types).

When creating a content type, the Comments fieldset is erroneously named Workflow.

Changes in this patch:
- enable/disable functionality is gone
- reset functionality moved to within the node type editing form, and made consistent with the 'reset to defaults' system that can be found in 'admin/settings' pages
- added some help text to the node type admin pages
- reorganised menu structure - 'admin/node/types' is now a local task (i.e. a tab) of 'admin/node', and the pages below it are secondary local tasks (i.e. sub-tabs). I really preferred having 'admin/types' (less clicks to get to), but using tabs is IMO better than having 'admin/node/types' hidden away in a nested menu structure, as is done by CCK in contrib
- 'auto-extract type name' code is gone (removing this is a -1 for usability, IMO - this will often result in entering the same text twice, for 'name' and 'type', which is a PITA - sure you won't change your mind on this one, Dries?)
- node type caching is gone
- removed 'content-' prefix from user-defined node types (this also eliminates the need for an upgrade path - node type names don't change due to this patch anymore)
- since node types cannot be disabled anymore, 'page' and 'story' are now defined as user-created node types, and they can be permanently deleted, just like other user-created node types. I don't think that this is ideal, but it's the only option now, since users need to be able to turn these types off somehow, if they don't want to use them
- content.module gets turned on as part of the upgrade script
- removed validation from 'name' and 'type' fields in node type editing form. Added 'recommendations' to the help text for these fields. Validation has been removed because (a) I'm concerned that it will make translation / i18n difficult (having a validation rule that excludes non-English characters is not a good idea, IMO); and (b) administrators should have the freedom to set their own naming standards, e.g. using First Letter Caps for node type names
- all node types can now be modified, but module-defined node types still can't have their 'type' field changed
- finished moving all settings from 'admin/settings/content-types/x' to 'admin/node/types/x' (e.g. publishing settings, upload module settings). The 'admin/settings/content-types' area is gone.
- new code to handle saving these settings as persistent variables (just like they've always been stored). Ideally, we should get rid of persistent variables in the future, but that will require more effort for extending the DB schema, creating an upgrade path, etc.
- made some fieldsets collapsed/collapsible on the 'admin/node/types/x' page
- changed the other modules in core to be compatible with the new node type system (i.e. blog, book, forum, poll)

Further reviews welcome.

Great work Jaza! Looking forward against this.

I've had a bit of a play with this, from a fresh cvs install (ie update.php not run)
- story and page content type definitions are not in a fresh install and content.module is disabled as well.

- Clicking Cancel when deleting a content type, goes to admin/types
- The view link in watchdog logs for content type additions, goes to admin/types

- When adding a new content type, I get:
Warning: mysql_num_rows(): supplied argument is not a valid MySQL result resource in .\includes\database.mysql.inc on line 166,
fixed this by changing content.module line 407:

-  if (isset($form_values['old_type'])) {
+  if (isset($form_values['old_type']) && $form_values['old_type']) {

- If no content types are available for the admin/node/types list, I get:
Warning: Invalid argument supplied for foreach() in .\includes\theme.inc on line 731,
fixed this by changing content.module line 196:

-    $rows[] = array('data' => t('No content types available.'), 'colspan' => '5', 'class' => 'message');
+    $rows[] = array(array('data' => t('No content types available.'), 'colspan' => '5', 'class' => 'message'));

- If I disable a module that defines a content type (eg book or poll), the type stay in the content types list, and the create content menu.
What should happen to content types of previously enabled modules?

Other than this, it's looking great, good work.

Great job! I think we're almost there. :-)

I could reprocude some of asimmonds problems.

I still think we should introduce a numeric type ID. Might allow us to clean up some of the code; especially the way we're setting $old_type and how it is passed around.

It took me a while to figure out what 'base' was. Turns out it is the defining module? Maybe call it 'module' instead?

Would NODE_TYPE_LOCKED_FIELD be a better variable for NODE_TYPE_MODIFIABLE_TYPE_FIELD? (Of course, it would be the inverse.)

As you mentioned, getting ridding of some of the variables (variable_get/variable_set) would be a good thing -- but can be tackled in a future patch.

Why does _node_types_build query the database? Couldn't the content module implement the 'node_info' hook, query the database in the node_info hook and return the content types like any other module would? Read: can't the content.module eat the node system's own dog food?

Can we use dashes in type names? We usually use dashes in URLs, rather than underscores.

Patch looks good! This might be the most important patch for the next Drupal version. Rock on, Jaza.

this is looking very, very, VERY nice.

I just set it up on a fresh head, patching, then installing from scratch using the patched mysql file. A couple thoughts:

1) content.module is NOT enabled by default. Should it even be *possible* to disable? I spent ten minutes or so trying to figure out why no content could be created.

2) no content types are defined by default, and it generates an 'Invalid argument supplied for foreach()' when the content admin page loads.

3) When no content types are defined, visiting node/add as user 1 says that I don't have permission, rather than saying that no types exist. Not major, but misleading for new admins.

4) the mysql_num_rows() issue noted earlier.

5) Once this is in core, we might consider a feature to merge two existing types of nodes. We already do the mass-update when a node type is renamed. Letting users merge 'opinion nodes and 'interview' nodes into 'article' nodes would help when users go nuts creating types, then realize a simple taxonomy would've been better.

This is really a spectacular piece of work, though, and I love the integration of the workflow settings, etc in the single setup screen. I'm already sad I can't use it on a client's site.

«Can we use dashes in type names? We usually use dashes in URLs, rather
than underscores.»

Unfortunately not. This is a mayor bug in flexinode (using flexinode-0 flexinode-2 etc names). It breaks in formsapi, in theme functions (theme_flexinode-2() is not correct PHP), and in some similar other places (referring to $flexinode-2 etc).

Hence we should remain with node_names instead of node-names IMO.

Thanks for the feedback, people! Latest changes:

- the 'cancel' link on the 'delete content type' page, and the link in watchdog entries, now link to 'admin/node/types' instead of 'admin/types'
- fixed mysql_num_rows() error when adding a new content type
- fixed 'invalid foreach() argument' error when on 'admin/node/types' and there are no types defined
- node types are now only listed on the 'admin/node/types' page and the 'node/add' page if the module that defined it is currently enabled (so when a module is disabled, its node types are not deleted from the database, but they are not displayed anywhere either)
- 'minimum number of words' now has an upgrade script from the old persistent variable storage, to its field in the 'node_type' table.
- all instances of the word 'base' (in relation to node types) changed to the word 'module'. Please test this one thoroughly, people - I may have missed a few
- binary flags completely abandoned, the old binary flags now each have their own field in the 'node_type' table. Once again, plenty of testing needed for this one
- new logic to cater to dashes-vs-underscores for the machine-readable type names. As per Ber, machine-readable type names cannot contain dashes - but they can contain underscores. As per Dries, all underscores are converted to dashes when displayed as URLs. I hope that I've managed to have my cake and eat it too, without creating too much messy code (it's just a few simple str_replace() calls here and there) ;)

Nearly there... just a bit more testing, and this baby should be ready to roll into core.

@Dries:

I still think we should introduce a numeric type ID. Might allow us to clean up some of the code; especially the way we're setting $old_type and how it is passed around.

I've been thinking about this a bit more, and I don't really think it's worth introducing a numeric type ID, in terms of how little gain it will give us. Sure, it will make the code for updating a non-locked node type cleaner. But we will still have to store $orig_type as a field in the 'node_type' table, and _node_types_build() will still have to use $orig_type when comparing database rows to values returned from hook_node_info(). Which brings me to my next answer...

@Dries:

Why does _node_types_build query the database? Couldn't the content module implement the 'node_info' hook, query the database in the node_info hook and return the content types like any other module would? Read: can't the content.module eat the node system's own dog food?

Unfortunately not. The _node_types_build() function already calls module_invoke_all('node_info'). But after it does that, it still has to query the database. This is because the node types stored in the database override node types that are defined through hook_node_info(). And anyway, content.module no longer implements hook_node_info() at all, because its pre-built node types are now completely 'user-defined', and as such they are defined only in the database.

Content.module can't eat the node system's own dog food, because it needs to alter (and in some cases override) the dog food that other modules have provided. =)

(This is my first time reviewing a patch, maybe I should've started with something smaller, but I really like the idea of having cck in core soon...)

First time trying this patch, patching a clean head checkout. As was said before, 'content' should be enabled by default, it's kinda weird to have a site with no content-types defined.

After enabling 'content', I went to enable 'blog' and got a "Unknown column 'module' in 'field list' query: INSERT INTO node_type (type, name, module,...". After modifying the db schema (alter table node_type add module varchar(255) after name;), everything worked fine.

(...Or did I miss something?)

(This is my first time reviewing a patch, maybe I should've started with something smaller, but I really like the idea of having cck in core soon...)

First time trying this patch, patching a clean head checkout. As was said before, 'content' should be enabled by default, it's kinda weird to have a site with no content-types defined.

After enabling 'content', I went to enable 'blog' and got a "Unknown column 'module' in 'field list' query: INSERT INTO node_type (type, name, module,...". After modifying the db schema (alter table node_type add module varchar(255) after name;), everything worked fine.

(...Or did I miss something?)

@Eaton:

content.module is NOT enabled by default. Should it even be *possible* to disable? I spent ten minutes or so trying to figure out why no content could be created.

The update script should enable content.module when you run it. If it doesn't, I'll look into fixing that.

In answer to your question, YES, it SHOULD be possible to disable content.module. I have been quite careful to design content.module in such a way that the node system still "works" just fine without it. Content.module is ONLY an administrative front-end for managing node types on a site. The really important back-end functions have, instead, all been written into node.module. So users can disable content.module if they don't want custom-defined node types at all, and if they just want to use the node types that are provided by their other enabled modules (e.g. book, forum, blog). I have tested this out, and it is perfectly possible to create and edit nodes of a module-defined type when content.module is disabled.

This is an important design issue, as it reduces dependence on content.module by other modules. IMO, the less 'required' modules we have in core, the better. Making a module 'required' means that other modules feel it's safe to depend on that module, which increases coupling between modules. Node.module was not 'required' before 4.7, and IMO it still shouldn't be 'required' - the ability to completely disable one of Drupal's key features (i.e. the node system) is a great way to demonstrate its modularity and its flexible design, IMO.

Awesome, thanks for the explanation.

A note, though -- update.php enables it, but the *initial installation* doesn't. That's what I was attempting, and that's how I wound up with a site that had *no* available content types and no obvious way to change that. :)

Thanks - I could have sworn that I updated all the database scripts, but the previous patch hasn't got those updates. Anyway, new patch with all DB scripts updated to use 'module' instead of 'base'.

The install-system patch just landed -- http://drupal.org/cvs?commit=36226. Individual database.40.mysql files and so on have been removed in favor of discrete .install files.

The db setup code here should go in system.install, I imagine.

No massive-overall-change-to-the-Drupal-install-system patch is going to stop THIS patch from going in! ;-)

New patch updated against latest HEAD. All database install/update code has been moved into system.install. There's also an update to the new 'profiles/default.profile' file, to enable 'content.module' by default, and to take out 'story' and 'page'.

Apart from that, the patch seems to still work fine with latest HEAD. Reviewing and testing is welcome, as always. Feel free to set RTBC if you feel it's ready to go in.

+1! Would install again!

I've thrashed it through on a fresh HEAD install and it's looking great. All the previously mentioned issues are fixed.

I dare to RTBC. Anyone else? Objections? Thoughts?

Actually, the only thing I can think of that would be REALLY nice is a method for modules to 'suggest' node types rather than explicitly defining them, ala views.module's 'default views.' That's not really necessary, though, and is probably best left for the rounds of revisions that will eventually come when CCK proper gets more momentum.

It would be nice if either Neil or Steven could do a review of this. :-)

How can $type->has_title and $type->has_body be set to FALSE?

In general, the whole node type loading API seems a bit circuitous. I walked in three circles trying to find where node types are actually loaded before I gave up and started looking for SQL queries. Do we really need all those one line functions that don't do a whole lot?

The new menu items for node/add/* should use the standard access checking. I filed a separate issue (http://drupal.org/node/73599) to correct this elsewhere in core.

And the upload enable setting has gone missing since it is still form_altering the old form. I'd rather see the content type editing stay where it was in node.module for now; it has many fields which are not specific to content-module-created node types. If we want to continue playing the move the menu item shell game (it has previously been under admin/settings, admin/node, and now admin/settings, if I recall correctly), then we can do that in another issue and patch.

False alarm on upload not working, my clean install didn't have it enabled.

But the rest of the last follow-up still stands.

Thanks for the review, Neil - muchly appreciated.

@drumm:

How can $type->has_title and $type->has_body be set to FALSE?

Because... some content types don't have a title field, or don't have a body field? :P (For example, the 'poll' type doesn't have a body field). All node types that are user-defined through content.module have a title and a body. But modules can choose to define types that don't have either or both or these fields. Also, when CCK hits core 'properly' (with fields.module etc), it will also be possible to have user-defined node types without a title and/or without a body.

In general, the whole node type loading API seems a bit circuitous. I walked in three circles trying to find where node types are actually loaded before I gave up and started looking for SQL queries. Do we really need all those one line functions that don't do a whole lot?

Nope, we don't need them. The 5 one-liners in question (node_get_types, node_get_type, node_get_module, node_get_names, and node_get_name) have now been removed. The _node_types() function has been renamed to node_get_types(). All function calls to any of the old functions have now been replaced with calls to node_get_types(). This change has affected a lot of little bits of code all over the place (I'm not saying that this is bad, I'm just letting you know). But from basic testing, it doesn't seem to have broken anything.

The new menu items for node/add/* should use the standard access checking.

Done.

Latest patch includes changes mentioned above, and re-rolled for latest HEAD.

I'd rather see the content type editing stay where it was in node.module for now; it has many fields which are not specific to content-module-created node types. If we want to continue playing the move the menu item shell game (it has previously been under admin/settings, admin/node, and now admin/settings, if I recall correctly), then we can do that in another issue and patch.

Are you saying that you don't want this patch to include moving everything out of 'admin/settings/content-types', and into 'admin/node/types'?

If so, then I'm sorry, but I must protest. This change is a major usability improvement, it is directly tied in with the rest of this patch, and I would really like to see it go in as part of this patch! If you look through some of the earlier comments in this thread, you'll see that many reviewers have strongly supported this change along with me.

I understand that the 'workflow' settings are not specific to content-module-defined node types. But with this patch, all node types (not just content-module-defined ones) can be managed from the 'admin/node/types' page. What's more, this patch makes the content module an enabled-by-default module, so that users will have access to all of these settings unless they explicitly disable the module. And also, although the 'workflow' settings for node types are currently stored in variables, we are planning to migrate them into the 'node_type' table (or into related tables) in the future.

I urge you to look at the benefits of the 'admin/settings/content-types => admin/node/types' change once more, and to reconsider your position. Thanks. =)

What's more, this patch makes the content module an enabled-by-default module, so that users will have access to all of these settings unless they explicitly disable the module.

I'd like to pop in and vote +1 for the new location. It makes sense, it's fewer clicks, and it puts a lot of settings that were previously buried in relatively strange places into a single spot. When CCK comes around, this will be a very natural place for it to hang off of.

As I'll be away from the keyboard a couple days, I'll leave it up to Neil and Steven to carry this home. :)

Dries, I have spoken to Neil and Steven, and neither of them is prepared to commit this patch without the final go-ahead from you. So it looks like this patch isn't going to get in until you commit it yourself.

I will be AFK for the next 7 days. If any final improvements need to be made to this patch, I leave it up to someone else to work on them. I really hope that by the time I get back, this patch is in core!

I will be reviewing (and committing when appropriate) as time permits.

Please test and review.

Whoops. Looks like the patch no longer applies --

Hunk #14 succeeded at 1626 with fuzz 1.
Hunk #15 FAILED at 1819.
Hunk #16 succeeded at 1849 (offset 3 lines).
Hunk #18 succeeded at 1939 (offset 3 lines).
Hunk #20 succeeded at 2233 (offset 3 lines).

It appears that patch #73599 changed the line:
if (module_invoke(node_get_base($type), 'access', 'create', $type)) {
to
if (node_access('create', $type)) {

That *seems* to be the only difference.

I'm not going to review this patch until it has at least one good review from someone else.

Here's a tweaked patch that does nothing but change the fuzz that was causing problems, and the one line that was choking. No functional differences, but now it can be applied for testing again.

@eaton: thanks for the updated patch.

@dries: Neil has indicated that he will review this patch when he can. Apart from that, other people should feel free to review and test as well.

my quick and rough review:

1. it's possible to have a content in certain type, delete that content type and still have that content around as node (but in weird no-title, no-body edit screen). See attached screenshot: the content type 'story' is gone but the node still exists

2. content type deletion: there is no explanation what content type deletion would do, there is just 'the action is irreversible' note. It feels like content deletion should delete all the content in that particular type and clearly explain that in error confirmation. Or is it too harsh?

3. is content conversion to another type (on deletion?) is out of scope for this patch?

4. somewhat off-topic: The default types with their descriptions are bit unclear, especially story: "/.../ Stories may be used as a personal blog or for news articles". This might give out wrong signals that story node IS the one to use for blog. It should be taken care of better wording and in (future) blog install profile

5. request for background info: i was looking for initial proposal for this patch but I did not find it? Was the patch development carried over from older issue? My questions are:
What does this patch enable when commited for devs? can CCK or others can now freely add new content types (how different it is from adding node types?) can they extend existing content types? what does it mean for the user interface, where the devs can plug in new functionality?

1. it's possible to have a content in certain type, delete that content type and still have that content around as node (but in weird no-title, no-body edit screen). See attached screenshot: the content type 'story' is gone but the node still exists

This is possible today when disabling a module. I think changing this behavior to a destructive one would be a mistake. Deleting all nodes of a particualr type is sometimes mecessary, but isn't the domain of this patch IMO.

What does this patch enable when commited for devs? can CCK or others can now freely add new content types (how different it is from adding node types?) can they extend existing content types? what does it mean for the user interface, where the devs can plug in new functionality?

It means that the ability to create new kinds of content -- new node types -- is part of core rather than part of an add-on module like CCK. There's obviously a lot of work necessary in CCK to support custom fields etc, but a good portion of its code was dedicated to just managing the creation of new content types by users.

Making that part of the drupal UI takes a load off of CCK, AND means that users who simply want to have a number of simple content types (blog posts, news articles, interviews) without the overhead of CCK can do it in core. This is a nice feature, but it's also about the infrastructure necessary to change the way drupal interfaces with node types.

Also, witness the number of tutorials and how-to's out there that explain how to hack story.module into a new node type using search-and-replace. Those kinds of tricks are handy, but lead to code bloat and frankenstein modules hanging around. Defining content types without coding a custom node is very very good. :-)

Eaton: I think that Kristjan/kika (a well-known usability expert, btw) is asking valid questions. It would be great if we could answer some (but obviously not all) of his questions by improving the UI. Other people will have to same questions ... and will be looking for the same answers. If you read up on the comments, you'll notice that these questions are recurring (I asked similar questions).

I agree, Dries -- I apologise if I sounded like i was dismissing all of his comments. I was trying to responsd specifically to #1 and #2. I just don't think that mass-deleting nodes when a content type is deleted via this screen is a good idea at all, any more than mass-deleting nodes when a module is disabled would be a good idea.

I do agree that making the warning on content-deletion more helpful would be good. Perhaps:

"Are you sure you want to remove the {foo} content type? There are {n} {foo} nodes currently on your site. They may not display correctly once you've removed this content type."

#3 -- conversion to another content type on deletion -- is definitely worth a look, and I agree that it would be a great help for those who've experimented and created *too many* node types on their sites.

sorry - just subscribing

re 1: ok with explanation. (Mass-deletion) should be fixed/improved as separate issue

re 2: I am buying that proposed error message

re 3: node type migration/conversion seems out of current scope, but should should be carried further. The biggest issue is the possible data loss / incompatibility between node types

re 4, 5: still open

Please don't have node types that have the minus sign in them (like CCK does... content-foo). I'm reading through the issue and I got to this comment where I see the code from CCK that replaces minus with underscore.... this is bad. Please only use underscores in node types and never use minuses. Keep in mind that whatever you call a node you will want to write theme functions (for its forms, for example) using that type name, and this requires that the node type consist of characters that can be used in PHP functions. See http://drupal.org/node/69472 for info on this issue in CCK. Sorry if there are no minuses in node types... I'll go back to reading the issue and patches =)

I finished reading the issues and was very hopeful that my comment was out of place and redundant because two others warned about the same issue before I did. Alas, code like this is still in the patch:

+      $type_name = !empty($type_name) ? str_replace('-', '_', $type_name) : NULL;

Why do we think we need minus characters in type names? This prevents the forms from participating in the theme system. Please find a way to avoid using minus signs.

Function names follow the same rules as other labels in PHP. A valid function name starts with a letter or underscore, followed by any number of letters, numbers, or underscores. As a regular expression, it would be expressed thus: [a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*.

Robert,

A cloesr look at that code reveals that it's purely cosmetic. Apparently there were complaints that underscores in URLs are uglier than dashes, and that code makes sure that they can be used interchangable *in menu paths*. By the time they get to the database, or theming/function calls, everything seems to be an underscore.

Can anyone else verify this..?

robert, eaton: Drupal URLs use dashes, not underscores. Mixing dashes and underscores looks ugly.

thanks for the clarification. Better safe than sorry =)

@robertDouglass: regarding the dashes-vs-underscores issue, I refer you to #94, where I posted a patch with this solution:

new logic to cater to dashes-vs-underscores for the machine-readable type names. As per Ber, machine-readable type names cannot contain dashes - but they can contain underscores. As per Dries, all underscores are converted to dashes when displayed as URLs. I hope that I've managed to have my cake and eat it too, without creating too much messy code (it's just a few simple str_replace() calls here and there) ;)

So at present, this is how it works:

1. User enters machine-readable type name. User is allowed to use underscores. User may not use dashes.
2. The name as entered by the user is stored in the database, and is used internally within drupal, for things like themable functions.
3. The name has its underscores converted to dashes, when and ONLY WHEN it is being displayed as part of a URL.

Hope this clarifies the issue. Like I said, this is the only way that I can see to satisfy both the "URLs-should-have-dashes-not-underscores" people and the "you-can-only-use-underscores-not-dashes-for-things-like-PHP-function-names" people.

Patch didn't apply anymore as something changed in blog.module. Some "code moving" in drupal.css got into the latest (precck_15) as well, now removed, as per chx suggestion. Here's a freshly rolled one...

At this time, it's 'recommended' that the human readable name "consists only of lowercase letters, numbers, and spaces", but if I choose to use, let's say, a '&' or a '<', it's converted into a '&' and '&lt' when editing the content-type. I understand the recommandation, but I don't think the name should be changed by the system, especially since "&lt" still contains a '&' and that's going to be mangled again (and again) on next edit... :(

Forget about my last comment... this is no different than other node titles, perfectly normal.

Hmm, the latest patch (#134) is not working for me:

HEAD with 50 existing devel nodes, then #134 succesfully applied..

• Everything seems to be fine. Existing nodes behave as they did before, which is good. Migration seems to be easy.
• Creating new content types works as expected. Creating content and editing existing content types as well. Forms are intuitive, can't see major usability issues there.
• I think the location of the content type listing/adding is good, however, for new users it might be a little difficult to find. However, this will change when Earl's major administration redesign actually gets commited.
• The "Minimum number of words" setting is not working at all for me. I edited the "page" content type to have at least 200 words, but I could still create a new page having only one word in its body.

So apart from this last issue it is RTBC for me, I think it's easier to file patches against it later on when it's commited. Especially concerning usability and documentation/help.

regards,
frando

Just ignore the first line in my previous comment. The patch IS working for me, apart from the "Minimum number of words"-thing i mentioned in my last comment. Sorry for the confusion. The rest of my review is correct...

1. If you change an existing content type, you can't reset it correctly by clicking on "Reset to defaults". I always got an error like "Body field label field is required." which has absolutely nothing to do with a reset.

2. Why is it recommended to not enter uppercase letters in the description? There are languages where all nouns begin with an uppercase letter. Also, I didn't experience any problem with it.

3. Some special characters should be forbidden or filtered out for the title of a content type (such as ><).

Otherwise, it worked fine.

- Fixed a lost change in blog module help text (removing gendered pronouns).
- Removed a couple global variables, $_node_types and $_node_names.
- Merged content module into node, with content type administration pages and forms conditionally included.
- Added some measures to avoid infinite looping since a few node functions are named like hooks, but are not designed to be.

Ideally, I'd actually like to see this broken up into even smaller chunks which are a bit more reviewable. Changing around the node metadata functions to node_get_types() and centralizing the node/add/... menu items seem like good steps. However, this is far enough down that path that we might be able to get by without it.

@drumm: big -1 to merging content.module into node.module, for reasons as quoted from #98 above:

I have been quite careful to design content.module in such a way that the node system still "works" just fine without it. Content.module is ONLY an administrative front-end for managing node types on a site. The really important back-end functions have, instead, all been written into node.module. So users can disable content.module if they don't want custom-defined node types at all, and if they just want to use the node types that are provided by their other enabled modules (e.g. book, forum, blog). I have tested this out, and it is perfectly possible to create and edit nodes of a module-defined type when content.module is disabled.

Some more reasons:

1. node.module is big enough as it is - if we can avoid making it bigger, then let's do so.
2. Putting the UI in as a separate 'content.module' is paving the way for CCK much better, IMO, because it is leaving us with more 'room for expansion' in this new and lightweight core module.

Jaza: I don't mind this functionality being part of node.module (and we can always break it out later on).

Drumm: thanks for the changes. Do you have anything else to add? Anything that needs to be done before this can be committed?

@Dries/drumm:
Why do you want to have the functunality of content.module in node.module?

I think Jaza's points are totally valid, and I can't see any benefit in merging the code over to node.module.

I'm divided in this question, IMO a deeper discussion (IRC?) would suite nice here.

I'm really on the fence regarding this patch.

It seems like this patch will cause a lot of reworking to need to be done in cck/content.module which is being tested and developed by a few people. It seems like over all it would be easier to focus on cck itself, additional field types, performance evaluations, etc etc. Once it is ready it can be moved into core along side existing node modules with an upgrade path. The old node modules could be deprecated and removed from core after a period of time. Once the old node modules are removed their API's can be deprecated and cck/content.module can be more tightly interwoven in the node system, and refactored over time.

My biggest apprehension is the impact changes to core, especially the node module interfaces, will have on cck, which seems to be slowly reaching maturity. How much of cck will need to be reworked as a result of these changes?

I see a lot of work has been done on the patch. I do see some value in having node types abstracted seperate from the fields. I spoke to chx, and he mentioned seperating fields from the node type... but It just seems like re-engineering node.module to be more like cck/content.module, and reengineering everything that has been done in cck as we go.

I have been quite careful to design content.module in such a way that the node system still "works" just fine without it. Content.module is ONLY an administrative front-end for managing node types on a site. The really important back-end functions have, instead, all been written into node.module. So users can disable content.module if they don't want custom-defined node types at all, and if they just want to use the node types that are provided by their other enabled modules (e.g. book, forum, blog). I have tested this out, and it is perfectly possible to create and edit nodes of a module-defined type when content.module is disabled.

1. node.module is big enough as it is - if we can avoid making it bigger, then let's do so.
2. Putting the UI in as a separate 'content.module' is paving the way for CCK much better, IMO, because it is leaving us with more 'room for expansion' in this new and lightweight core module.

I don't think people will want to disable user-defined content types in the future.

• They will define page and story, which are currently considered basic and essential content types for Drupal core.
• There is a good possibility that blog may be removed as a module-defined content type, as berkes has demonstrated.
• Book pages can already be non-book content types. It could be simplified by having book node type be user defined.
• Forum nodes are simple enough that is probably makes sense to make their node type user defined.

This shows that user-defined content types will be a key piece of Drupal and should be required, as node is now. Instead of having two required modules for our basic content system, why not have them in one?

On code complexity, having like functionality in the same module will offer greater possibilities for simplification in the future. Node.module could use work; for example node_page() is still done hook_page()-style, serving a few different pages. Building a clean module next to an older module isn't inherently cleaner than building clean code into an older module.

Content type administration is a lot of code, which is why I put it in a conditionally included file. Using .inc files is better than modules in cases such as this since it is conditionally included as needed and only when needed without the need for asking a user if it should be enabled.

In the previous patches before what I wrote the content type configuration was wholly in content.module. If someone decided they did not need dynamically content types, then they would lose the basic node type configuration (defualts for published, promoted, etc) we have now in node.module. I think future UI improvement will be made easier if the default workflow options are in the same place as the various names and descriptions.

I do plan on posting another revision later today. Node module API calls often collide names which collide with the hooks they call. I wasn't too happy with how I worked around this last night. This is one place where having a different name, content, is convenient. We don't want to change the API or hooks since the function and hook names are okay and changing would break a lot. I plan on centralizing dealing with this issues in node_invoke(), to either call node_content_hook() or node_node_hook() (instead of node_hook()) while maintaining that node is the module for the content types returned by node_get_types().

Now, when node-created content types are called, the hooks called are node_content_hook() to distinguish from API calls.

- Updated for HEAD.
- Fixed editing of module-defined content types (they were being re-loaded without retrieving from the database).
- Removed the breadcrumb cruft that is no longer needed.

patch.

Updated for HEAD.

Same patch without the extra CVS cruft-spew files.

Still getting: "warning: mysql_num_rows(): supplied argument is not a valid MySQL result resource in /home/bonsai/public_html/cck/includes/database.mysql.inc on line 174." when I go to edit the name of an existing content type.

Creating/deleting/creating nodes with content types is fine.

Eaton: what is in your 'node_type' table? Was this a clean install or upgrade?

The node_type table seems pretty straightforward -- it's the same data I would've expected. A quick and dirty dump of the tables is:

"page";"page";"node";"If you want to add a static page, like a contact page or an about page, use a page.";;"1";"Title";"1";"Body";"0";"1";"1";"0";"page"

"article";"article";"node";"Articles are content in their simplest form: they have a title, a teaser and a body, but can be extended by other modules. The teaser is part of the body too. Articles may be used as a personal blog or for news.";;"1";"Title";"1";"Body";"0";"1";"1";"0";"story"

"story2";"story";"node";"testing. another test.";;"1";"Title";"1";"Body";"0";"1";"1";"0";

This is a clean install with the latest CCK patch. I'll dig deeper when I get a chance.

I tested drumm's latest version of the patch, and produced an updated patch with the following changes:

- the mysql_num_rows() error (happens when you update an existing content type) has been fixed - node_type_update_nodes() needed to use db_affected_rows() instead of db_num_rows().
- minimum word count actually works again - node_validate() was still checking the persistent variables, instead of checking $type->min_word_count.
- fixed the 'num-x type-y nodes have been updated from content-type-old to content-type-new' message to use format_plural().
- added eaton's suggested improvements to the 'delete content type confirmation' message (this has kristjan's approval as well).

If nobody else can find more bugs, feel free to set RTBC.

Just did a quick run through; all those things seem to be working well. The only thing I noticed was that the 'delete content type' form button does nothing, just returns to the list of content types and says 'The content type was updated.'

The delete LINK in the list page works perfectly, though.

Simple error checking for the wrong string in the 'delete' case. Fixed.

Just wondering: did someone do some simple performance tests? :-)

quick review- patch still seems to apply, though with offsets/fuzz.

Why for story and page are the machine-readable names editable (and deletable), while for book it is not?

Why do the poll and story module files still exist if I can delete these content types or change them? Also, disabling the page or story module seems to have no real effect now except removing the link at /admin/help.

Also, with the story/page module enabled, its permissions appear twice- one under the node module section, and once under the story/page section.

The permissions for content types under node module need to be grouped better as well- maybe make the content name the first part of the permission: create page content -> page: create content.

This way, all the etries for page, story, etc will be naturally grouped together.

Just wondering: did someone do some simple performance tests? :-)

I just ran some simple tests with ab on my shared hosting account. I set up a simple site with caching turned off, a few nodes promoted to the front page (forcing it to node_load and node_view more than one node at a time). 1000 requests to /node generated the following results:

without patch:
Time per request: 250.712 [ms] (mean)
Time per request: 321.463 [ms] (mean)
Time per request: 268.363 [ms] (mean)

with patch:
Time per request: 259.767 [ms] (mean)
Time per request: 269.490 [ms] (mean)
Time per request: 265.129 [ms] (mean)

There's quite a vit of variability from one run of ab to the next, and I think that the spike in the 'before-the-patch' benchmark may have been an anomoly based on server load. In any case, there don't seem to be real measurable differences between the pre-patch and post-patch versions.

Why for story and page are the machine-readable names editable (and deletable), while for book it is not?

Because story and page are defined purely as 'simple' node types, editable and deletable by the user. book nodes are exposed by the book.module, and thus can't be deleted. In the future, it's possible that book.module might be refactored even more, so that any kind of node could be selected as its "top level" node type. For the time being, though, this isn't the case. It's a good candidate for a future patch, however.

Why do the poll and story module files still exist if I can delete these content types or change them? Also, disabling the page or story module seems to have no real effect now except removing the link at /admin/help.

Poll.module's node type isn't editable. Perhaps you're thinking of Pages? Both page and story are no obsolete, once this patch is in effect. It looks like the removal of those files didn't make it into the latest re-roll, but they SHOULD be removed as a part of this patch.

Also, with the story/page module enabled, its permissions appear twice- one under the node module section, and once under the story/page section.

See above.

The permissions for content types under node module need to be grouped better as well- maybe make the content name the first part of the permission: create page content -> page: create content. This way, all the etries for page, story, etc will be naturally grouped together.

This is a pretty minor cosmetic issue: I'd rather have them grouped by permission-type so that I can enable or disable 'create' and 'edit' and so on as a batch. At the moment, I think this is a minor enough point that it shouldn't hold up the patch for another round of changes.

There are three tasks remaining for this patch, IMO:

1. page.module needs to be removed
2. story.module needs to be removed
3. default.profile needs to have page and story removed from its module list

Tested and poked at repeatedly. Examined permissions, ran through a clean install, etc etc.

The benchmarks still apply, and the earlier comments about node types exposed by book, et al also still apply. Those modules currently do their own custom handling of display for those nodes, and thus would need more extensive rewriting to take advantage of 'user-defined' node types.

The only problem encountered was SQL errors on enabling poll.module, related to http://drupal.org/node/77179 -- not this patch's problem.

I am RTBC'ing this patch. I dare to dream!

Committed to HEAD.

Wohoo! Finally, great work you guys.

Ohhhh yeahhhh!!! It's finally happened! * does traditional Drupal dance of joy *

I'm currently working on #515454: Node type disappears if its "type" has already been used (same orig_type), which seems to have its origins in this patch.

In 62340: Pave the way for CCK (#64), _node_types_build() looked like this:

function _node_types_build() {
  global $_node_types, $_node_names;

  $_node_types = array();
  $_node_names = array();

  $info_array = module_invoke_all('node_info');
  foreach ($info_array as $type => $info) {
    $info['type'] = $type;
    $_node_types[$type] = _node_type_set_defaults($info);
    $_node_names[$type] = $info['name'];
  }

  $type_result = db_query(db_rewrite_sql('SELECT nt.type, nt.* FROM {node_type} nt ORDER BY nt.type ASC', 'nt', 'type'));
  while ($type_object = db_fetch_object($type_result)) {
    if (module_exist('field')) {
      $type_object->fields = field_load_type($type_object->type);
    }
    if (!isset($_node_types[$type_object->type]) || ($type_object->status & NODE_TYPE_MODIFIED_BY_ADMIN)) {
      $_node_types[$type_object->type] = $type_object;
      $_node_names[$type_object->type] = $type_object->name;
    }
  }
}

And in 62340: Pave the way for CCK (#78), it looked like this:

function _node_types_build() {
  global $_node_types, $_node_names;

  $_node_types = array();
  $_node_names = array();

  $info_array = module_invoke_all('node_info');
  foreach ($info_array as $type => $info) {
    $info['type'] = $type;
    $_node_types[$type] = _node_type_set_defaults($info);
    $_node_names[$type] = $info['name'];
  }

  $type_result = db_query(db_rewrite_sql('SELECT nt.type, nt.* FROM {node_type} nt ORDER BY nt.type ASC', 'nt', 'type'));
  while ($type_object = db_fetch_object($type_result)) {
    if (module_exist('field')) {
      $type_object->fields = field_load_type($type_object->type);
    }
    if (!isset($_node_types[$type_object->type]) || ($type_object->status & NODE_TYPE_MODIFIED_BY_ADMIN)) {
      $_node_types[$type_object->type] = $type_object;
      $_node_names[$type_object->type] = $type_object->name;

      if ($type_object->type != $type_object->orig_type) {
        unset($_node_types[$type_object->orig_type]);
        unset($_node_names[$type_object->orig_type]);
      }
    }
   }
 }

This is roughly the same function that is currently in D7 HEAD - barring DBTNG changes, etc.

The only change between the two functions is the lines

if ($type_object->type != $type_object->orig_type) {
        unset($_node_types[$type_object->orig_type]);
        unset($_node_names[$type_object->orig_type]);
      }

Which are precisely the lines that seem to be causing the problem in #515454: Node type disappears if its "type" has already been used (same orig_type). However, there's nothing in the comment thread here - and no comments in the code - that specify what these lines are supposed to do. Sure, they unset the $_node_types entries for any orig_type that isn't matched to a current type name, but I can't see where these orig_types would ever creep in.

Can anyone remember, or work out, what this code was supposed to do?