fix node_type_form submission process

subscribing

I think these 2 issues should be combined, since they are clearly related.

As I stated at http://drupal.org/node/94162, the real problem is that this code should be changed to assume that any modules doing a hook_form_alter will register their own submit handler rather than relying upon this code to save random form values into the variable table.

This worked in node_form so why not in node_type_form?

@profix898 - I think you're still right on this, since the object saved as chx suggests would be the original type object, not the altered one.

I think you're on the right track here, and I'll try to test the code. Anything that has an element added to the node form should probably use a _submit function. Modules without elements in the form should use hook_node_type with the 'insert' or 'update' $op. However, I think comment module and upload module should also implement hook_node_type to handle the case of the 'delete' $op so that the relevant entries are removed from the variable table.

@profix898 - I'm working with your patch now- I think you are confused about 'orig_type' vs. 'old_type' (not an intuitive distinction).

Ok, here's an improved patch- it seems to be working correctly on my localhost. Comment, node, and upload modules now all implement _submit functions to handle the 'node_type_form'. Comment, node, and upload modules also now delete the relevant variable if the node type is deleted via hook_node_type.

Note, I rearranged some more of the code in node_type_form_submit, so as to simplify the if/else logic.

I'll try to contact Jaza to get his review.

Great work, pwolanin! I tried out the patch, and it works really nice. I tested it for creating, updating, deleting, and resetting a content type (with comment and upload modules enabled), and it handles all of those cases perfectly.

You and profix88 are right - this is a much better and more flexible solution, than simply assuming that all modules will want to save their form values into persistent variables. It also allows us to get rid of all that 'unset all form values except x, y, and z' cruft. I ported that cruft over from the settings-saving code in system.module, but I should have cleaned it up in the first place.

New patch that fixes a few minor style issues, and that also makes the comment and upload settings get populated properly on the forms for module-defined node types (rather than just for user-defined node types). I think this is RTBC.

Once this is committed, http://drupal.org/node/94162 can be marked as duplicate, as one of the things this patch does is fix that issue.

It might be better as it was originally, since !empty() will return FALSE if the argument is set to an empty string, while isset() will return TRUE in the same case.

Yep, isset() seems to do the job fine in this case, and isset() is indeed preferred if it works. Latest patch (by profix898 in #18 above) is RTBC.

The upload and comment modules will serve as example code to be put into new modules since copy/paste works. They should be commented so they are understood.

New patch that provides doxygen comments for the comment_workflow_form_submit() and comment_node_type() functions (and equivalent comments for the same functions in upload.module). These comments are especially targeted towards module authors looking through core for candidate copy-paste code snippets, so they should be adequate :P.

Apart from that, same patch.

This would require every nodeapi module to change because of the API change.

@drumm - I don't understand your comment.

The only modules this affects are those that

1) use hook_form_alter to add something to the node type form
2) expect that the form values will get saved as variables automatically.

In core, this is only comment and upload.

This is also a much more robust solution then trying to unset unused form values....

I think this needs more discussion by the powers that be before it's contributed -- but that's about the philosophy of late changes rather than the quality of the code. The patch appears good, and works.

The handling of the 5.0 content type settings form has changed a LOT, and many modules use it. Only a few have yet been ported, so it behooves us to get this thing working smoothly and predictably before we find ourselves stuck. It's a tremendously frustrating issue to work around when setting up modules that add nodetype options/settings.

Thoughts? Dries? Drumm? Bueller?

Any module which implements hook_form_alter for the node type editing can expect that the form will be saved as a settings form with keys being saved as variables. This API change is very late.

What about:

- Add a #submit which is guaranteed to be at the end of the array. You should be able to do this with #after_build.
- I think #submit's $form_values are passed around by reference, so in this final #submit, you will be able to save a set of variables reduced by other #submit functions (like upload and comment).

I think this will maintain backwards compatibility and ensure backwards compatibility. You can even throw in some big fat TODOs to get rid of it later.

@drumm- I have to argue strongly against your proposal. I think that not fixing this problem with a clean use of the Forms API (as proposed) is going to create more problems down the road.

The new code required is for modules is pretty minimal- as demonstrated for comment and upload. This is also consistent with the (late) API change adding hook_node_type to allow modules to respond to changes in the node type name.

Also, I think it's worth mentioning that this form really has changed substantially already in the 4.7.x -> 5.x addition of node types to core. In 4.7, this content-type settings page used the system_settings_form. In 5.x it does NOT. Also, the form_id has changed from $type .'_node_settings' to 'node_type_form'.

IF the problem is that $type is not passed around THEN my solution IMO is still solid. Just I have not explained it properly and it was a bit wrong, but just a bit.

So, we stuff $type into an arbitrary property which form API happily ignores. We did the same with $node -- it's passed around in #node. So, let's pass this around as #node_type. Once again: I understand the issue not and this is only a cure to the problem the title indicates.

OK, now reading more the issue, I understand it and uplod my patch.

_uphold_ . typo day?

@chx - no this is not a solution to either the issue in the title, or the related issues that the proposed patch in #23 fixes. The problem is that to even solve the bug that is at issue in the title of this post, one needs the $type object after form submission - like hook_nodeapi($op='update') or $op='insert'.

There are really 3 bugs that are solved by the patch in #23- drumm's proposal might solve 1 of the 3. Passing the unmodified $type object solves none, as best as I can tell.

@pwolanin - Could you please (re-)explain to me what the 3 bugs at hand are so I can understand a bit better? Thanks.

OK, this is what I think the 3 issues are:

1) (this post): if one has a form value that one doesn't wish to simply save as a persistent variable, the context is lost for any additional _submit calls, since all the form values pertaining to the type are unset, plus the value will automatically get saved as a persistent variable.

2) like the problem with the $user->data field, random form values (such as from_token) will get saved as persistent variables unless carefully unset. see: http://drupal.org/node/94162.

3) There is an issue with the comment and upload module form_alter code as initially outlined here: http://drupal.org/node/97231

All 3 of these are fixed by the patch. #3 could be separated out, but #1 and #2 are interlinked.

After a lengthy chat with pwolanin , we arrived at the following:

1. One issue will be fixing form_alters by passing around $type in $form['#node_type']
2. Completely independent of this, we will utilize the existing hook_node_type op insert and update so that modules can save their stuff. The blanket variable mechanism will go. We will cast $form_values to $type the same way it happens with $node . There is a small API change -- if your module added something to the node_type edit form, now you need to save it independently. However, currently you need go so very far to be able to save at all that this little change IMO well worths the thaw.

item #1 outlined by chx, patch at: http://drupal.org/node/97231
please review that first.

Ok, new patch attached addressing #2. To get away totally from adding _submit handles, we have to add another $op to hook_node_type to handle the case of a reset. So, for a reset the hook gets called twice with 'update' and then 'reset'. I struggled with this for a while, so I'm happy to take suggestions if there is a cleaner way to handle it.

also, at the moment there is a problem since a lot of things break if you put '0' as the node type. I tried to avoid fix this by changing calls to !empty() to strlen(), however this hasn't fully fixed it.

I think the hook_node_type functions need to check isset() on their form values, in case node_type_save is being called directly.

ok, this is getting there. Also fixes what I think is a bug in node_type_reset();

-  $info_array = module_invoke($type->module, 'node_info');
+  $info_array = module_invoke_all('node_info');

I think this was wrong, since a single module may define several types with different values for $type->module within a single implementation of hook_node_info.

putting the type object in the form allows a cleaner solution to the other bug you reported- see: http://drupal.org/node/97231

For this patch, I think the idea is that node_type_save() can be thought of a CRUD function like node_save(), so it's better to respond to a hook called form there. Obviously with this form (or the node form) you could add _submit handlers, but that's not the "standard"- what's implemented in this patch means that modules only have to implement hook_node_type, instead of both that plus a _submit function.

improved(?) title

tested patch in #43 with both PHP 4.4/MySQL 4.1 and PHP 5.1/MySQL 5.0- works as expected.

need a module defining multiple types for some more serious testing. Also note that for modules like book or forum, the default values for comment, upload, etc. for the node type form won't be set correctly without http://drupal.org/node/97231

How essential is this API change for Drupal 5? What can't be done without these hooks?

The hook is already present (used, for example in this recently committed patch: http://drupal.org/node/85062), so this is not an API change (as I understand the term). This patch does add a "reset" op to the hook, since it is needed (I think) for full functionality.

So, the reasons this patch is useful are 3-fold:

1. we don't need to patch the code in a vain hope that we can unset() every unneeded form value to avoid having it saved as a variable (see: http://drupal.org/node/94162).
2. Modules that do a form_alter to add new fields can (must) choose how to handle them- they could be saved in a table, manipulated, etc., rather than the form values being automatically saved as variables.
3. regardless of the above 2 considerations, hook_node_type should be implemented for comment and upload to handle the "delete" op.

Also, it reduces the lines of code by casting the form values into an object and I think it catches an additional bug in node_type_reset().

So, I guess the possible alternative is 4 separate patches that don't address #2 above.

This looks pretty bad, IMO.

IMO. http://drupal.org/node/85062 should not have been committed. Now, the proposed patch in this issue only seems to make it worse.

I think the best solution is to eliminate the "node_type" hook. Even if we get it right in core, it is a source of problems. It is going to take a long time before the contrib authors get this right ...

I propose that:

1. We lock the type field in Drupal 5. (This is not a regression unlike what some people claim!)
2. We remove the _node_type hook from Drupal 5.
3. Switch to numeric IDs in Drupal 6.

Anything else is going to turn this into a big mess that introduces nothing but upgrade issues in Drupal 6.

@Dries- IMHO, the ability to change the types names is dispensible for 5.x, but locking that down doesn't actually solve any of the bugs that are currently addressed by this patch.

Also, I'm having a very hard time seeing how numeric type IDs would work- it seems to me that would break phptemplate (e.g. node-forum.tpl.php), and break a huge amount of core and contrib code.

# We remove the _node_type hook from Drupal 5.
# Switch to numeric IDs in Drupal 6.

That seems like a big step back; handling the changing case can be difficult, but moving to numeric IDs means that code-sharing is also MUCH less predictable. 'news' nodes might have type id 5 on my site, and 9 on yours. Isn't that a problem?

I'm also not crazy about the idea of moving to numeric IDs. I'd also really hate to lose the ability to rename a type, as this is a huge usability thing. I have to maintain a site with a content type named "subhed" because CCK (at the time anyway) didn't have the ability to rename types, and it DRIVES ME NUTS. :)

subscibing

You could still have 'names' too (for use in URLs), but internally, it would use IDs.

I'm not fond of numeric ids either. The code-sharing issue Eaton mentioned also means complications when moving from staging server to production site.
If we're considering this, maybe the intermediate "lock machine-readable type name after creation" would be better for now ? OK, It does not allow any second thoughts, but that seems really annoying only for urls.

But then for urls we could use either a safe name derived from the 'human' name (with a numeric suffix in case of duplicates), or, as Dries suggests, an additional 'url name' type property.

Yes, names are essential. But in the database layer we need static numbers.

Its been awhile since there was a patch. Please post a new one using feedback, or summarize why a previous patch should go in.

Since "deriving from the 'human' name" was mentioned, don't overlook that the human name entered can be a non-english character string without a translation -- not ideal for a path.

Ok, new patch, slightly different approach:

1) Send everything in the form_values to node_type_save() - fixes the original bug report for this issue
2) do a thorough job of unsetting form_values before saving variables - fixes issue http://drupal.org/node/94162
3) minimizes the use of hook_node_type - only the 'delete' op needs to be implemented for upload and comment modules

also, other minor fixes

1) replaced instances of !empty() with strlen(). A (better?) alternative- insure in _validate that a type name is never '0'
2) use module_invoke_all('node_info'); as mentioned in #43: http://drupal.org/node/88633#comment-159286

But in the database layer we need static numbers.

Well, we need something static. Until 4.7, it was static strings. It would be really better if we found a way to keep it that way.

'Numeric type ids' vs 'string type ids' is sort of flexinode vs cck. In that area, cck is nicer to work with IMO.
Machine names for types and fields are locked in cck - there has been some complaints about that (and about the fact that these machine names are 'derived', actually), but JonBob sticked to this IIRC, and I think this is precious.
You know at once what it is you're themeing, for instance, and you don't have to shift numbers when moving form one server to the oher.

sorry, crossed posts, that was not intended.

alternative- very similar, but adds to _validate instead of using strlen().

Is there a reason to have empty($type->type) in _validate. Why not set '#required' => TRUE on that element in the form?

oops- use this- bad patchfile in #63

@RobRoy - this field is required, but the built-in validation for required fields specialcases the value '0' even though it evaluates as empty.

@pwolanin - So you're special casing to make sure this value isn't '0' then? That should be a valid name according to the text and the preg_match. Or am I missing something?

Since we are (I think) in bug fix mode, while I agree that this whole node type system needs some re-engineering, we now need to just get it working so we can have an RC!

Saving the form keys seems to me to be a more robust solution than what was in place before. Also, this way when you do a form_alter to add elements, if you don't want that element saved as a variable, you can add it to this array of keys. Maybe the array needs a better name (and obviously more comments/documentation) like 'node_type_ignored_keys' or 'node_type_excluded_keys'.

Last night I was thinking it may be possible to use array_flip() and array_diff_key() to generate an array of the form_values that we want to save as variables, so we don't need to unset anything (so any _submit handlers would work as expected).

Also, if we don't want to have to implement hook_node_type('delete') each module that adds elements to the node_type form, I think we'd have to convert this to a multi-step from so that we could pass the full type/form_values in. Otherwise node_type_delete() knows nothing about the elements that other modules have added to the form, right?

Ok, slightly improved further- the form values aren't changed, so I think this means additional _submit functions get the full form values.

patchfile...

For question #2, the outcome is the same either way (they are unset).

For question #1, a module implementing hook_node_type(0 does now indeed get the full type object with all form values, and can act however it wishes. Why is that bad? It's like hook_nodeapi('update') where any module can see the entire saved node.

Re-visit comment #50, please. I still think we need to (i) lock the name or (ii) switch to numeric IDs. Our time is better spent working on (ii). Working on the patches in this issue is somewhat a loss of time and effort.

@Dries- have you looked at the latest patch in detail? The renaming works fine- this patch retains the original code where variables set by being added to the node type form get automatically renamed without the module having to implement hook_node_type. Locking the name won't change much of anything (and would not solve any of the bugs addressed by this patch). Also, I think using numeric types will require a fundamental re-engineering of the node system- lots of pain for little gain.

Setting back to "normal" priority

The bugs this patch now addresses:

1) variable cleanup by node, comment, and upload modules when node types are deleted.

2) In the process of saving persistent variables from the node type form values, elements are more thoroughly unset so that things like "op_story" aren't saved as variables.

3) form values are left untouched, so a contrib module can get the full data on the type either by implementing hook_node_type, or by adding a _submit handler to the form

4) a potential bug is fixed with the code for resetting a module-defined type

5) a misplaced "break;" command is removed

@Dries:

I still think we need to (i) lock the name or (ii) switch to numeric IDs.

Ummm... no we don't.

Either of those options would be a major step backward. No more easy-to-remember, easy-to-customise, easy-to-use-in-themes, configurable and meaningful node type names. We've already spent the effort on getting configurable low-level node type names into core. Why not commit this patch, which will fix some of the remaining bugs that this change introduced, and that will allow us to more fully reap its benefits?

I think the best solution is to eliminate the "node_type" hook. Even if we get it right in core, it is a source of problems. It is going to take a long time before the contrib authors get this right ...

Anything else is going to turn this into a big mess that introduces nothing but upgrade issues in Drupal 6.

Once again, I disagree. hook_node_type() was needed in order to make configurable node type names work. Now that it's in, we are able to make them work just fine. Contrib modules will also be able to make them work just fine.

Come on! You intend to roll back an entire new hook, and one that was absolutely necessary? And at this late stage? This is unheard of!

And as for making it harder for contrib authors to upgrade their modules: errr, harder compared to what? Since when has Drupal ever cared about that? Anyone remember the 4.7 Forms API?

After some recent experiences with the 4.7 version of the CCK module, which only allows you to specify the machine-readable name of a node type ONCE, and which never allows you to change it, I am convinced that completely configurable node type names are the way to go. At whatever price. And really, I don't think that committing patches like this one, and leaving hook_node_type() in core, is that big a price. We let people define a node type name once, so we have to let people change that node type name later on.

Let's put the 'U' in CRUD, please. Because CRD is just no fun.

I think Dries's point is that unless module developers know to implement this hook, their modules may break when node types are changed, and that it may cause very weird bugs that are hard to track down (for example, who would've thought themes could be broken by changing content types?). Basically, it's rather fragile in its current state.

However, I do tend to lean toward what Jaza's saying -- that rolling this back, especially at this late in the code freeze, would be a major step backwards. Themes will break, modules will break, and it would be a huge step backwards in usability. As long as core gets it right, module developers will have examples to work from and won't be left in the dark.

Has anyone review the last patch? I'd like to see this finalized.

Note, re: webchick's comment, that due to concerns expressed about the initial approach, this version of the patch does not mess with the existing code that automatically renames variables for any setting added to the node type form. So, for node, comment and upload, implementing hook_node_type to cleanup unneeded variables is good, but if a contrib module doesn't do it, it won't cause any breakage.

Ok, now that I've seen the documentation for multi-step forms, a possible improvement/complication to the current patch would be to make the node type form a two-step form, where confirmation of deletion is a second step. This might let us capture all the "added" form values, so that they could be deleted automatically just like they are now saved automatically.

OK, here's a better (much simpler) idea. The array keys used to save persistent variables are themselves saved as a variable. Then, they can be used upon node type deletion to automatically delete all of that node type's persistent variables. This simplifies things, since modules do not have to implement hook_node_type to just handle variable cleanup upon node type deletion. Also added more comments to the code.

Nice work on your latest patch, pwolanin. I tested it for creating / updating / deleting node types, and it works fine.

Attached is an updated patch that:

1. Removes the if (empty($type->type)) check in node_type_form_validate(). This is not needed: the 'type' field is a required field, so FAPI validates it automatically, and will give an error if it's empty.
2. Wraps the new comments to 80 chars (comments shouldn't be on lines longer than this).
3. Removes a few stray trailing spaces.

I don't think that this is an ideal solution. The 'node_type_excluded_keys' thing seems a bit hackish to me. But I think that it's the best solution we're going to get, if we want to maintain backward compatibility with the way things work currently (in 4.7 and in 5.0 beta-x), and if we want to avoid forcing (core and contrib) modules to implement hook_node_type() for every node-type-related piece of data that they store in persistent variables.

The 'node_type_variables' variable (used to track which variables to delete later on) also seems like a bit of a hack, and probably won't prove to be 100% reliable at deleting all node-type-related variables. Also, this patch provides no mechanism for removing array elements from this variable when modules are uninstalled, which means that this variable can only grow - it will never shrink. However, I'm not sure that it's worth the trouble of coding such a mechanism, since it's unlikely that this variable will get enormously big anyway.

Moving forward, I'd like to see this whole 'automatic saving of variables' thing removed from the node type form, and I'd like to see new database tables (and/or columns) created for these settings instead. The data in these dedicated tables/columns would be maintained through hook_node_type(). We shouldn't be promoting the use of the persistent variable system for storing node type settings - it's an incorrect use of the variable system, and it can potentially create a flood of persistent variables that will degrade a site's performance.

But for 5.0, let's at least fix it so that (a) unneeded FAPI elements on the node type form don't get saved as persistent variables, and (b) module authors aren't forced to use the automatic variable saving system for node type settings if they don't want to. This patch does all that.

@Jaza - you are wrong on point #1 - create a type with name '0' and see what happens... lots of your other code has checks like if !empty($info->type).

Also, the 'node_type_variables' variable is re-created every time a node type is saved (and it only stores the key fragment like 'comment' relating to variables 'comment_page', 'comment_story', etc.). So, it's not going to grow nor ever be more than a few strings.

possibly a better alternative for validation of node type names, would be to require that names be at least two characters long.

In case it's not clear from above, the problem is that empty('0') == TRUE but in form.inc, the "required" field validation code special cases and allows to pass any value === '0'. Requiring two characters solves the problem since empty('00') == FALSE

Here is an alternative which sets a minimum name length of 2. This last bug is an aggravating one, since it's really a consequence of the weird PHP standard of evaluating '0' as empty. the alternative to disallowing '0' or requiring at least two chars is to change a lot of other code to use strlen() or some other check than empty().

Also added a note in the form text suggesting that the first 4 characters of the name be unique, since the index is only on the first four chars.

coming late into the discussion. but reading the latest patch looks good; i still have to test it. w/o it, many real-world contributed modules, such as location, will be difficult or nearly impossible to implement (with current functionality anyway; these modules could go the route of cck fields and add new tabs, i suppose).

also, i second that switching to numbers for type names is a bad idea. already maintaining sites with a dozen or more node-xyz.tpl.php files, and it was already a big chore to rename all those files (and custom code) after one of the 4.7 cck updates a few months ago. i guess if it were db-only, that would be acceptable if necessary, as long as there were some way to still access a configurable machine-readable type name elsewhere (both for template files & for easily-readable customized site-specific code, another real-world need).

I think either this is RTBC- the patch in #83 was marked as RTBC by Jaza and would get all but one of the bugs. We could open a separate issue to deal with the edge case of a type name of '0' if enforcing two-character names is not the way to go as implemented in the patch in #86.

I'm OK with the two-char minimum length for now. We should probably revisit this later, and clean up all the code so that it uses a check such as:

if (isset($type) && $type !== '') { /* ... */ }

Which is the best way to avoid the empty() trap of the value '0' being evaluated to FALSE. I see no reason why people shouldn't be able to have a type name of '0' (although I can't imagine why you'd want that as a type name). But that will be a separate issue and a separate patch.

I agree - this patch is RTBC.

since the RC is out, does this patch need to be altered to avoid changing any existing t() strings?

I've finally confirmed for real that the 'reset to default' code doesn't work for a module that declares more than one node type under 5.x. The patch in #86 does indeed fix theis bug.

In case you want to confirm, checkout my in-progress module: contributions/modules/synth_products

Does this patch need to get broken up into sub-patches/issues in order to get committed?

I still think this is the wrong solution. Sorry.

@Dries- can you please be more specific?

Should I split this out, since there are actually 3 or 4 different bugs addressed by this patch?

two small bugs and patches broken out here:

http://drupal.org/node/104346

http://drupal.org/node/104349

A 3rd sub-patch here: http://drupal.org/node/104352

All three of these are pretty easy.

Again, I propose that:

1. We lock the type field in Drupal 5. (This is not a regression unlike what some people claim!)
2. We remove the _node_type hook from Drupal 5.
3. Switch to numeric IDs in Drupal 6.

The other 3 related patches show how ugly of a problem this is, and how much cruft we need to add to fix this. Contributed module owners will be forced to do the same.

@Dries- sorry to disagree, but those three sub-patches seem (to me) typical little bugs, rather than an indication of a fundamental problem.

Submitted two more sub-parts of this as separate issues:

#4- collect "garbage" if a node type is deleted:
http://drupal.org/node/104572

#5- cleanup the saving of node type variables:
http://drupal.org/node/104575

Marking this as duplicate W.R.T. the 5 sub-parts above. Note, that some potential code simplification/cleanup is omitted in these 5 smaller patches (e.g. casting the forms values directly into an object), but nothing I would consider a real "bug" is omitted.