The field that we are trying to delete might not be there causing a method invocation on a non-object.

CommentFileSizeAuthor
#15 2022769-15.patch5.94 KBxjm
#15 interdiff-add-s.txt1.98 KBxjm
#13 2022769-13.patch5.93 KBfubhy
#10 2022769-test-and-fix.patch5.71 KBfubhy
#10 2022769-test-only.patch4.69 KBfubhy
#6 2022769-6.patch3.35 KBfubhy
#1 2022769-1.patch486 bytesfubhy

Comments

fubhy’s picture

fubhy commented
Status: Active » Needs review
StatusFileSize
new 2022769-1.patch486 bytes
andypost’s picture

andypost
he/him
Primary language Russian
commented

Nice idea, that field could be removed by user

fubhy’s picture

fubhy commented

Not only that, if you don't have any n ode types on your site it won't even be there in the first place. Try uninstalling with minimal or testing profile ;)

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs tests

So it needs tests, this case is not common

fubhy’s picture

fubhy commented
Title: Comment module uninstall currently broken. » Fix conversion of field_delete_field() in hook_uninstall()
Status: Needs review » Needs work

Found other occurrences too. Fixing all here in one issue.

fubhy’s picture

fubhy commented
Status: Needs work » Needs review
StatusFileSize
new 2022769-6.patch3.35 KB

Really? Do we need tests for this if statement? For forum module. Okay. But not for comment module... That falls under the responsibility of field_entity_bundle_delete() once the module disabled state is gone. Would just be a waste of time to write that test now just to remove it again in one or two weeks.

amateescu’s picture

amateescu commented
Component: comment.module » field system
Status: Needs review » Reviewed & tested by the community

Looks good to me, and the reasoning as well :) RTBC if green.

effulgentsia’s picture

effulgentsia commented

But not for comment module... That falls under the responsibility of field_entity_bundle_delete()

I'm not clear on why. comment_uninstall() calls $field->delete(). Why wouldn't comment.module then need test coverage for being uninstallable whether that field exists or not?

xjm’s picture

xjm
she/her
Primary language English
commented

Agreed, I think we need a similar test for comment, unless there's something I'm misunderstanding. Also please be sure to upload a test-only patch with any bugfix to expose the bug and coverage. :)

fubhy’s picture

fubhy commented
StatusFileSize
new 2022769-test-only.patch4.69 KB
new 2022769-test-and-fix.patch5.71 KB

There's no point in writing a comment test now, because once #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair is solved field module will be able to fully take care of field deletion (through field instance deletion) whenever an entity type providing module disappears (probably through hook_module_preuninstall()). That's where comment and forum are different. Forum attaches a field to a foreign entity type. Comment body, however, gets attached to the comment entity (which belongs to comment module). So when we delete all comment entity bundles (currently those are node type-bound upon module uninstallation) we can remove the fields too. So, to summarize: Comment module does not have to clean up for itself.

Anyways, not worth arguing about it... Copy&paste, 10 seconds, new patch done with tests for both (identical).

fubhy’s picture

fubhy commented

Actually, instead of deleting the coment_body field there we should probably delete the entire bundle already. It makes no sense to just delete the comment body field. What about the other fields on the comment entity?

Let's invoke entity_invoke_bundle_hook('delete', 'comment', 'comment_node_{node_type}'); for every node type instead.

EDIT: Yeah, umh... So as it turns out we already invoke entity_invoke_bundle_hook() for every node type. So.. That makes $field->delete() even more redundant. Let's just drop it. We can keep the tests if you fancy them ;).

amateescu’s picture

amateescu commented

Re #8 and #9: Sorry, I forgot to mention that the background for my support in #7 was from IRC :/

fubhy’s picture

fubhy commented
StatusFileSize
new 2022769-13.patch5.93 KB
xjm’s picture

xjm
she/her
Primary language English
commented

Thanks @fubhy! The test coverage is needed one way or the other (the fact that we had this bug in the first place demonstrates that the coverage is missing).

#13 looks good to me conceptually if things pass and fail as expected. :)

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs tests
StatusFileSize
new interdiff-add-s.txt1.98 KB
new 2022769-15.patch5.94 KB

Adding s... https://drupal.org/node/1354#functions

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented 
+++ b/core/modules/forum/forum.installundefined
@@ -122,7 +122,10 @@ function forum_uninstall() {
-  field_info_field('taxonomy_forums')->delete();
+  if ($field = field_info_field('taxonomy_forums')) {
+    $field->delete();

vocabulary is not deleted, also field could be re-used

fubhy’s picture

fubhy commented

Nearly no entity type in core cleans up all it's entity bundles before uninstallation. As mentioned earlier we can fix this generically once #2003966: Helper issue for #1199946 : Disabled modules are broken beyond repair lands. We can then just invoke entity_invoke_bundle_hook('delete', 'bar', $foo); on all entity types and their bundles in entity_module_preuninstall(). Let's not bother with fixing all these cleanup issues now. It's just a matter of time until we can fix them all in one go with 5 lines of code.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.