Support from Acquia helps fund testing for Drupal Acquia logo

Comments

spydmobile’s picture

This problem still makes my modules look bad for trying to clean up after themselves, would this be a potential fix?
http://drupal.org/node/874000#comment-4771916

pkosenko’s picture

Issue summary: View changes

I agree with post #1 (spydmobile). As a new user, I discovered that cleaning up after my feeds extension module got me the unwanted error messages, since the $info object was apparently not being found, even when there was in fact NO work for the comment_node_type_delete() function to be doing. I would suggest that this fix be implemented in Drupal 7: http://drupal.org/node/874000#comment-4771916

samir_mankar’s picture

Assigned: Unassigned » samir_mankar
FileSize
816 bytes

The following patch works.

Mile23’s picture

Status: Active » Needs review

Let the testbot have a go....

Mile23’s picture

Status: Needs review » Reviewed & tested by the community

I think this is a good patch.

Anyone disagree?

pkosenko’s picture

I have tested the PATCH against the current Drupal 7.27 release and the patch is fine . . . it solves the problem of erroneous error messaging when there is no $info object. Since the added functionality simply passes the object through if it exists, I don't see how it should cause any problem when there are content node types to be handled. It is just the existing functionality.

So basically, I agree with Mile23.

David_Rothstein’s picture

Version: 7.14 » 7.x-dev
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

hook_node_type_delete() is documented as receiving a node type object (and that's certainly what node_type_delete() looks like it's sending it). So the existing code looks correct to me.

How is this bug triggered? Is there some code out there that's invoking the hook with the wrong parameters?

pkosenko’s picture

As the original patch proposal notes, sometimes NO object is being passed at all to the node_type_delete() function. There is no mechanism in the function to account for that. When there is no node object, the function tosses an error, even when there is no comment node type available to delete. So . . . the function is NOT in fact always getting a node type object, which is the issue.

https://drupal.org/node/874000#comment-4771916

In my case this happened when I created nodes in module code, then tried deleting the nodes. When the module was uninstalled, there was no comment node type to uninstall.

David_Rothstein’s picture

So which code is calling node_type_delete() with an invalid type passed in?

It doesn't look like it's anything in core, since this is apparently the only place in core that calls node_type_delete(), and it doesn't run on uninstall.

I guess I could imagine adding some protection to node_type_delete() itself to verify that the passed-in node type is valid before using it. However, we shouldn't patch the Comment module to deal with it, since any such bug would be an issue for other hook_node_type_delete() implementations as well.

jucedogi’s picture

I'm getting this when trying to cleanup my custom module when using hook_uninstall().

Why not simply check if $info is actually an object as expected in the code? Something like:

comment_node_type_delete($info) {
/**
* $info is FALSE when invoked at hook_uninstall()
* after calling node_type_delete('my_node_type')
**/
if ($info !== FALSE && is_objet($info)) {
// code
}
}

So this leaves me wondering if I should actually be calling node_type_delete($type) at hook_uninstall() to do a thorough cleanup of my module's footprint or not.

Also I only get those warnings when using the site's module admin area. When using drush the recent log messages stays clean for some reason. Possibly that drush runs on PHP 5.4 whereas apache has PHP 5.6... can't think of anything else at the moment.

ZalemCitizen’s picture

Not a good thing to alter core modules, as always :p

See patch content in this issue, reported for Web Links module : https://www.drupal.org/node/2500119

To add in the opening of your custom module hook_uninstall() function

// By this point the 'weblinks' content type will be marked as disabled in the
// {node_type} db table, and this causes errors in hook_node_type_delete(). To
// avoid this we reset the node_type cache and call node_types_rebuild(). This
// makes the database query not exclude disabled content types, and the result
// is stored in the cache for the remainder of the page request.
// @see https://www.drupal.org/node/2500119
node_type_cache_reset();
node_types_rebuild();

PatrickMichael’s picture

I was experiencing the same issue on uninstall of my custom module, #11 worked for me, thank you!

Fabianx’s picture

Could you please paste code from your custom modules (the install / uninstall part)?

That would help to write a test case for that.

apaderno’s picture

Assigned: samir_mankar » Unassigned
Debasish147’s picture

Status: Postponed (maintainer needs more info) » Fixed
apaderno’s picture

Status: Fixed » Postponed (maintainer needs more info)

From the previous comments, it doesn't seem it was fixed.

apaderno’s picture

Title: Trying to get property of non-object in comment_node_type_delete » node_type_delete() doesn't check the value returned from node_type_get_type()
Status: Postponed (maintainer needs more info) » Active

Looking at what Drupal core does, this is the "calling chain."

Considering the case of two users deleting the same content type from the UI, there is an issue. Supposing the slower user reaches the confirmation form before the other user confirms deleting the content type, the second user confirming the deletion will cause Drupal to invoke hook_node_type_delete() with FALSE as argument. This happens because node_type_delete() doesn't check the value returned from node_type_get_type(), and node_type_get_type() could return FALSE.

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();
}
function node_type_get_type($node) {
  $type = _node_extract_type($node);
  $types = _node_types_build()->types;
  return isset($types[$type]) ? $types[$type] : FALSE;
}
apaderno’s picture

Component: comment.module » node system
Status: Active » Needs review
FileSize
916 bytes

The correct code for node_type_delete() should be the following.

function node_type_delete($type) {
  if (($info = node_type_get_type($type)) !== FALSE) {
    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();
  }
}
poker10’s picture

I think the patch #18 looks good and it fixes the problem. It does not make sense to trigger all hooks when node type does not exists.

I was not able to simulate this behavior thru UI, because if I open two confirmation pages (content type delete confirmation page), the submit on the second page would return 404, because access checks are done first (checking the parameter %node_type), so no node_type_delete() is run in this case. I think it would need to be a precise doubleclick to trigger this problem in UI.

I was able to simulate the problem by simply calling the node_type_delete($type); two times on the same content type. I have created a simple test with this and reuploading the original and test only versions of the patch.

Just one idea - maybe better suited to the follow-up issue. Since this function does not return anything for user to know if a content type was / was not deleted, wouldn't it help to add some logging there, that there was an attempt to delete non-existing content type? When the IF condition will be introduces, this will silently pass and the user will assume, that the content type was deleted (but note that this is already happening in node_type_delete_confirm_submit() anyway, also without that IF condition).

The last submitted patch, 19: 1565892-19_test-only.patch, failed testing. View results