Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When I uninstall a node module, I am getting.
Notice: Trying to get property of non-object in comment_node_type_delete() (line 343 of C:\wamp\www\drupal7\modules\comment\comment.module).
Hook_install() doesn't do anything about comments, so it will be set for whatever the defaults are.
Comments
Comment #1
spydmobile CreditAttribution: spydmobile commentedThis 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
Comment #2
pkosenko CreditAttribution: pkosenko commentedI 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
Comment #3
samir_mankar CreditAttribution: samir_mankar commentedThe following patch works.
Comment #4
Mile23Let the testbot have a go....
Comment #5
Mile23I think this is a good patch.
Anyone disagree?
Comment #6
pkosenko CreditAttribution: pkosenko commentedI 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.
Comment #7
David_Rothstein CreditAttribution: David_Rothstein commentedhook_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?
Comment #8
pkosenko CreditAttribution: pkosenko commentedAs 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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedSo 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.
Comment #10
jucedogi CreditAttribution: jucedogi commentedI'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.
Comment #11
ZalemCitizen CreditAttribution: ZalemCitizen commentedNot 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
Comment #12
PatrickMichael CreditAttribution: PatrickMichael commentedI was experiencing the same issue on uninstall of my custom module, #11 worked for me, thank you!
Comment #13
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedCould you please paste code from your custom modules (the install / uninstall part)?
That would help to write a test case for that.
Comment #14
apadernoComment #15
Debasish147 CreditAttribution: Debasish147 commentedComment #16
apadernoFrom the previous comments, it doesn't seem it was fixed.
Comment #17
apadernoLooking at what Drupal core does, this is the "calling chain."
node_type_delete_confirm()
) has a form submission handler (node_type_delete_confirm_submit()
)node_type_delete()
, which invokeshook_node_type_delete()
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()
withFALSE
as argument. This happens becausenode_type_delete()
doesn't check the value returned fromnode_type_get_type()
, andnode_type_get_type()
could returnFALSE
.Comment #18
apadernoThe correct code for
node_type_delete()
should be the following.Comment #19
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI 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 nonode_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).