node_type_delete(): hook_node_type_delete will fail for a disabled node_type. Trying to get property of non-object in hook_node_type_delete()

#1: 943588_101023.patch queued for re-testing.

Marked #995756: Trying to get property of non-object as a dup of this issue.

Same patch, minus the name change.

We should add tests for this I think.

Typo in #6; correcting...

I understand changing the parameter name away from $type so as to avoid visual confusion with the array $types but calling it "$bundle_name" is also confusing, as it conflicts both with the documentation and with the naming convention used in other, similar functions in core.

How about compromising by calling it $type_name instead of $bundle_name?

#13: node_type_delete-943588-13.patch queued for re-testing.

Subscribing.

+++ modules/node/node.moduleundefined
@@ -614,16 +614,20 @@ function node_field_extra_fields() {
-function node_type_delete($type) {
-  $info = node_type_get_type($type);
+
+function node_type_delete($type_name) {

There is an extra line between the docblock and the function. Also, I disagree with changing the argument name -- $type is fine and is consistent with other functions (like "node_type_update_nodes($old_type, $type)", for example). But whatever.

+++ modules/node/node.testundefined
@@ -1141,14 +1141,25 @@ class NodeTypeTestCase extends DrupalWebTestCase {
+    db_query("UPDATE {node_type} SET disabled = TRUE WHERE type = :type", array(':type' => $type->type));

Rewrite this using db_update().

+++ modules/node/node.testundefined
@@ -1141,14 +1141,25 @@ class NodeTypeTestCase extends DrupalWebTestCase {
+    $this->assertFalse($type_exists, 'A disabled content type has been deleted from the database.');
+    ¶
     // Login a test user.
+    $type = $this->drupalCreateContentType();
     $web_user = $this->drupalCreateUser(array('create ' . $type->name . ' content'));
     $this->drupalLogin($web_user);
 

Extra white space on this line.

Powered by Dreditor.

Another bigger problem is that the added tests do not trigger the bug.

Testing-only version of above patch.

(Tests of a bugfix, without the bugfix, should fail)

Once again, tests pass without the fix, so the tests are not effective.

@jpsoto: What @pillarsdotnet is saying is that the test routines are not sufficient to trigger the bug. That is, without any patch to Drupal's behavior, all of the test routines pass. Essentially the test routines have failed to trigger the bug, and thus are insufficient.

It looks like this behavior was changed by this commit which came from this patch.

Types which are not provided by hook_node_info() are no longer disabled by node_types_rebuild().

Marking this issue as a duplicate of #986296: _node_types_build() accidentally marks node types as disabled because even though the symptoms are different, the cause (and fix) appear to be the same.

Sorry if this is the wrong place to post. I've chased through all sorts of issues about node content being left behind and this seems the closest to what I'm experiencing.
I'm defining a custom node in hook_install() with 'base' => 'bir_collection'. When going through the process of uninstalling, I get errors:

Notice: Trying to get property of non-object in comment_node_type_delete()

This only occurs when base is set to a custom type. If 'node_content' is used, no errors occur when uninstalling. This is due to the node type NOT being set as 'disabled' when base is 'node_content'.
Tracing this back, it can be seen the disabled node type is not picked up in _node_types_build().

// in hook_uninstall() for module
node_type_delete('bir_collection');

// Line 618: node.module
function node_type_delete($type) {
  $info = node_type_get_type($type);
  db_delete('node_type')
    ->condition('type', $type)
    ->execute();
  field_attach_delete_bundle('node', $type);
  module_invoke_all('node_type_delete', $info);

  // Clear the node type cache.
  node_type_cache_reset();
}

// Line 401: node.module
function node_type_get_type($node) {
  $type = _node_extract_type($node);
  $types = _node_types_build()->types;
  return isset($types[$type]) ? $types[$type] : FALSE;
}

Performing tests by calling _node_types_build()->types in a script shows the custom node type is not returned when disabled. So after disabling and trying to uninstall, the $info variable in module_invoke() is empty. Thus, causing the error posted above.

Am I missing something here in my uninstall hook? Should node_type_delete() not be used for node types created in modules?

Edit:
Found #1181030: Node types disabled by _node_types_build() when using node_type_save() with base != 'node_content' speaking about the same issue. Funny, because all these issues link back to one another but the problem still remains.

My solution is that just temporarily enable the node type in hook_uninstall, then proceed to delete the node type.

  // Temporarily enable the node type
  db_update('node_type')
    ->fields(array(
      'disabled' => 0,
    ))
    ->condition('type', 'bir_collection', '=')
    ->condition('disabled', 1, '=')
    ->execute();

  // Clear the node type cache
  node_type_cache_reset();

  // Delete node type
  node_type_delete('bir_collection');

Not a solution I like, I think node_type_delete should be able pick up the disabled node types. But this will get rid of error messages from other modules that implements hook_node_type_delete.

@realguess
Thanks, I will give your solution a try.

@realguess
Good shot! It solves my problem

@pillarsdotnet in #29 - thanks for finding that commit. However, the issue we have here is not actually solved by that fix. Our problem is that implementations of hook_node_type_delete in other contrib modules give errors because the $info parameter passed is empty or null.

I have found an alternative solution to the one from realguess in #31. At the start of hook_uninstall, instead of re-enabling the content type, you can call

  node_type_cache_reset();
  node_types_rebuild();

The call to node_types_rebuild() will include the disabled content types and the result will be stored in the cache for the remainder of the page requset, thus any further calls to node_type_get_type() will be able to return the info required. I have used this in Weblinks #2500119: Uninstalling produces error: Trying to get property of non-object in comment_node_type_delete() as we had exactly the same problem.

Hope that helps anyone else who comes across this error.

Jonathan