While the moving of CCK-type custom nodes into core was a big advance, there is a serious problem with the current implementation: If an admin changes the name of a node tpye, the per-node-type settings for the theme are lost, and the taxonomy vocabulary will no longer be applied to that node type.

Jaza proposed adding a new hook to deal with this and realted issues:

http://drupal.org/node/80257
http://drupal.org/node/80253
http://drupal.org/node/80375

However, these have not been implemented.

While this feature may be important for some specialized applications, I really don't see the use case for the average admin and it seems likely to cause serious headaches. Accordingly, the attached patch prevents admins from changing the node type. However, the capacity to create non-locked node types is still present for those who want to use it via direct DB creation or a contrib module.

CommentFileSizeAuthor
#9 system_node_type_2.patch1.11 KBpwolanin
#7 system_node_type.patch1.12 KBRobRoy
#6 theme_settings_node_type_sync.patch.txt1.11 KBAmrMostafa
fix_custom_1.diff5.2 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

Actually, it appears that the permission names are based on the "human readable" name, so this problem is even more complicated.

eaton’s picture

eaton CreditAttribution: eaton commented

Permissions are no longer based on the human readable names -- that patch went in just a bit ago.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

ok, but then we still either need a patch like this to prevent the names from changing, or an appropriate set of hooks so modules can respond to the changes.

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented

I could reproduce the theme settings part, but taxonomy vocabularies works probably.

I think this is due to the updates here: http://drupal.org/node/80257
So the new hook was commited, and it seems along the patch was a modification to taxonomy.module to use it. So theme settings now needs to be patched to use the new hook to fix this issue.

Kjartan’s picture

Kjartan CreditAttribution: Kjartan commented
Version: x.y.z » 5.0-beta1
Status: Needs review » Needs work

Patch needs to be re-worked to use the new hook.

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented
Status: Needs work » Needs review
FileSize
theme_settings_node_type_sync.patch.txt1.11 KB

Attached a patch with the required hook_node_type implementation to update theme settings on node type changes.

I didn't try to drupal_set_message() (like in http://drupal.org/node/80257), because I find it unnecessary, if anybody think it should be there I will gladly add it, I added a comment there too.

RobRoy’s picture

RobRoy CreditAttribution: RobRoy commented
FileSize
system_node_type.patch1.12 KB

Code style fix.

I unchecked the 'page' node info on the Themes Global Settings page. Then renamed the 'page' type to 'Pagina'. Came back to global settings and it was still unchecked.

Works for me.

webernet’s picture

webernet CreditAttribution: webernet commented
Title: changing node type breaks taxonomy, theme settings » Changing node type breaks theme settings
Version: 5.0-beta1 » 5.x-dev
Status: Needs review » Needs work

Patch no longer applies.

-Taxonomy seems to be working fine after changing both name and type.
-Changing name doesn't affect the theme settings, changing the type (machine-readable name) does break those settings.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Priority: Normal » Critical
Status: Needs work » Reviewed & tested by the community
FileSize
system_node_type_2.patch1.11 KB

Patch still applies for me with offset. re-rolled patch attached which should apply without offset.

Patch seems to fix the problem with changing of node's machine-readable type.

Dries’s picture

Dries CreditAttribution: Dries commented

I don't like such a hook; it makes things get hairy real quick. It's not a good path forward because it is not obvious for module developers. It puts unnecessary burden on them.

I'd rather see us use numeric IDs than internal names.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work

If numeric IDs are not possible at this point in time (too much engineering), I suggest we lock down the ability to change the name of the content type.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Dries- a while back I made the same suggestion about locking down content-type identifiers, but the patch with this new hook went in a while ago and this and related issues are basically cleanup.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Dries- I think this hook is needed regardless of whether node types can be changed by the user.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

see related issue: http://drupal.org/node/88633

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Reviewed & tested by the community

I think Dries' questions are answered -- there is no new hook, only an implementation of a hook and it's a bugfix actually. In the next release we can go for numeric IDs. If you really do not like this solution, surely we can come up with a patch which removes the ability to change names but that would be a serious regression as the node type form has no preview or confirm and after submit you would not be able to fix as much as a simple typo.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Needs work

What about all of the theme_{theme name}_settings variables? Should theme_get_settings() be used instead of a direct variable_get()?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@drumm - looking at http://api.drupal.org/api/HEAD/function/theme_get_settings, I don't see why it would be better to use this function. We're (essentially) just changing the value of one array key.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Reviewed & tested by the community

I still think this is RTBC.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Reviewed & tested by the community » Fixed

Committed to HEAD.

I had to dig around to find that these settings do not exist for individual themes, so this is okay.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Fixed » Active

I think Dries' questions are answered -- there is no new hook, only an implementation of a hook and it's a bugfix actually.

Actually, they aren't answered. What makes you think they are answered?

"... a patch which removes the ability to change names but that would be a serious regression ..."

A serious regression compared to what? Drupal 4.7 doesn't support any of this so there can't be a regression.

@Dries- I think this hook is needed regardless of whether node types can be changed by the user.

I don't see why, but I'd like to understand. Can you shed some more light on this?

I think we need to revisit this issue. See also http://drupal.org/node/88633.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

@Dries- regarding my comment about about why this hook is needed, I think it is important for modules to be able to respond to the creation or deletion of types, even if types can't be renamed. My general thinking is hook_nodeapi -> hook_nodetypeapi() == hook_node_type()

yched’s picture

yched CreditAttribution: yched commented

it is important for modules to be able to respond to the creation or deletion of types

Yup, CCK currently relies on this, for instance.

Paul Natsuo Kishimoto’s picture

Paul Natsuo Kishimoto CreditAttribution: Paul Natsuo Kishimoto commented

I wound up here via comments #50-53 on http://drupal.org/node/88633.

As a non-dev but a person with some SQL experience, I've got to wonder why SQL like FOREIGN KEY [...] ON DELETE RESTRICT ON UPDATE CASCADE isn't used in Drupal for both textual and numeric IDs referenced from other tables. Was there a debate that wound up with a decision against?

If node_type.type is text and a PRIMARY KEY, and is referenced by FOREIGN KEYs from other (including core) tables, then

  • the database will refuse to delete a node type while there are existing nodes of that type, and
  • changes to the name of a custom type (e.g. in a module update) will cascade from node_type to any instances of that type in other tables.

FOREIGN KEYS are in both Postgres and MySQL, although with slightly different syntax; why not use them? They're more robust and elegant than a programmatic solution, and force module developers to clean up after themselves.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

From mysql.com, from the 5.1 docs:

"For storage engines other than InnoDB, MySQL Server parses the FOREIGN KEY syntax in CREATE TABLE statements, but does not use or store it. In the future, the implementation will be extended to store this information in the table specification file so that it may be retrieved by mysqldump and ODBC. At a later stage, foreign key constraints will be implemented for MyISAM tables as well."

Currently, all Drupal tables are MyISAM.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented

I thought the applied patch was relatively straightforward. When the content type name used by the theme system changes, change the stored names. Sure, the fact that the database is using a string name changeable by administrators as a key is kinda bad. I've often wondered if the toggle post information line was in the right place in the UI (which would affect the database structure in some cases). But both of those would be larger changes and this fixed the problem relatively cleanly. It can be cleaned up later when we can break APIs.

AmrMostafa’s picture

AmrMostafa CreditAttribution: AmrMostafa commented

My patch relied on an already existent hook, which was introduced in an earlier issue: http://drupal.org/node/80257

Cheers,
- Amr

Paul Natsuo Kishimoto’s picture

Paul Natsuo Kishimoto CreditAttribution: Paul Natsuo Kishimoto commented

@pwolanin - ah, thanks! I guess it shows that I'm primarily a Postgres user.

drumm’s picture

drumm
he/him
Location NY, US
CreditAttribution: drumm commented
Status: Active » Closed (fixed)

Please reopen if further discussion is needed.