OG does not clean up after itself on uninstall

Related core issue: #366152: field_modules_uninstalled still exists, despite being an empty foreach loop.

It seems that, at some point, the idea was to auto-delete fields on uninstall of the module that owns it, but this was never implemented. The discussion in the issue referenced above suggests that this may never happen.

In any case, even if this gets implemented at some point, the question is what og should do in the mean time. As deleting the fields in the uninstall hook does not create any problems even if the auto-deletion gets implemented in the future, I think the best thing to do is for og modules to take care of the field deletion by themselves (for now).

Any thoughts?

Looking into this some more, I find that og_create_field actually does check if the field exists before attempting to create it, which should make the existence of an old field a non-issue.

   $field = field_info_field($field_name);
    if (empty($field)) {
      $field = field_create_field($group_field['field']);
    }

The problem is that field_info_field does only return an array if the field in question is 'active'. However, when uninstalling og, the fields that it owns are automatically marked 'inactive' in the database. This means that the above code will always do the wrong thing if $field_name is the name of an inactive field.

(The API doc for field_info_field does not specify that only active fields will be returned. I will file an issue for this in a second. field_info_field uses _field_info_collate_fields, which in turn calls field_read_fields without 'include_inactive' => TRUE in the second argument.)

Two possible solutions: (a) change the test in og_create_field so as to test the right thing, or (b) as suggested above, have the uninstall hook remove the fields owned by the module. The latter seems to be the right thing to me, but maybe I am overlooking something.

Okay, I'll probably have a patch ready by tomorrow.

Here is a patch. That does not work. But it should.

It will work as soon as #943772: field_delete_field() and others fail for inactive fields is fixed. Until this happens, there is hardly any way in which a module can clean up after itself in terms of fields it creates. (I still have to review the detail of the final patch there, but in conjunction with the attached patch, it works very well.)

The helper function og_delete_fields I define should be in core, or at least in a utility module, but for now, I guess it it okay where it is.

Note that even before the core bug gets fixed, og does not behave worse WITH the attached patch than without: Without the patch, re-enabling after uninstall will result in an error because an existing field is attempted to be created. With the patch, this does not happen, but until the core bug is fixed, re-enabling og.module will result in an error because re-creating the field attempts to create a database table that exists because it has not been properly cleaned up.

Which is to say: The patch does not make matters worse (it makes things slightly better: things break a little later ;). At the same time, as soon as the core bug is fixed, everything will work fine with the patch. So I advocate for committing it, even though it does not solve the problem right now.

I had thought of using og_fields_info(), but the problem is that at modules_uninstalled time, the module will be disabled (and partially uninstalled)---and module_implements(), module_invoke() etc. only work on installed, enabled modules. The only way this could be made to work if we change og_fields_info() into an install hook (i.e. one that lives in the .install files) and than use module_load_install() to get at that hook. Which would still be fairly costly (as we would have to do this on every module uninstall, just to figure out whether the module implements og_fields_info() ... or am I missing something?

I am confused ... have I been pulling from the wrong repository? I have been using git://github.com/amitaibu/og.git and your commits came through there ... but https://github.com/amitaibu/og/blob/master/og_language/og_language.install does not have an implementation of og_uninstall(). So I must be doing something wrong. Can you give me a pointer?

Of course you are right, though, that I forgot the --no-prefix. Here is a version of the patch that is identical with the prior one, except that it is generated with --no-prefix ...

And NOW I realize how stupid I am: I misnamed the hook implementations. Sorry about that.

This one should do the right thing.

> Maybe better "Test re-installation of module." ?

Done.

> OG-Sven ;)

Ups. Fixed.

> Missing space.

Fixed.

> > You can use assertFalse here.

> Sorry, this is not correct, you did it right.

I did, at that point. But the assertion after that really should be assertFalse():

+ $this->assertTrue(!empty($modules['og']), 'Enabling og module after uninstall is sucessful.');

Fixed.

> og-ui doesn't declare any field, so this isn't needed.

What about

function og_ui_field_settings_submit($form, &$form_state) {
  $bundle = preg_replace('/^.*__/i', '', $form_state['values']['bundles']);
  $entity_type = str_replace('__' . $bundle, '', $form_state['values']['bundles']);
  $field_name = $form_state['values']['fields'];

  og_create_field($field_name, $entity_type, $bundle);

?

(There were like, three more mistakes wrt to function signatures in the test case as well. Also fixed.)

Uh-oh. This is even more tricky. I was supposing (without much thinking) that the module that creates the field owns it. Which is not true, the module that defines the FIELD TYPE owns it. Which actually means that fields like "og_views" will be neither owned by og_ui (even if created through it), nor doe og_views (which defines the field in the og_field_info hook), but by the "list" module.

Which means that the current og_delete_fields() function will only really work on the audience field. Which is just as well, as even the fields created in, e.g. og_theme's install hook will simply remain active and undeleted if og_theme is deactivated or de-installed. Which means that the test in og_create_field() will
do the right thing, and upon re-installation, there will be no attempt on re-creating the field.

Which means the only issue really is the audience field---so, maybe, we should just remove this one with a simple field_delete_field() call in og_uninstall and leave it at that.

It would be more clean, of course, to also remove the other fields, but in order to be able to do that, we would have to either have og_create_field() create a record somewhere or find a way to make hook_og_field_info() visible at uninstall time.

Since what is really breaking things right now is the audience field, I'd suggest we go with the conservative approach and just delete that field on uninstall, allowing the possibility of ghost fields for the other modules (they will not really be ghost fields ... just og-specific fields that don't do their magic anymore).

This patch employs the 'conservative' approach mentioned above. It fixes the issue in that uninstall removes all fields of type "group", which are what really made things break.

Fields created by og modules that are of other types will remain undeleted and active, but there does not seem to be a sane way to fix this right now. Also, because they stay active, the tests for existence in og_create_group do the right thing on re-install, so nothing breaks.

The remarks with respect to #943772: field_delete_field() and others fail for inactive fields still apply: The current patch will only work once that issue is fixed, but it does not make matters worse. Sadly, the referenced issue is not moving forward, even though there is a working patch ...

Yes, but the test fails because of #943772: field_delete_field() and others fail for inactive fields --- and it does fails two lines later than before the patch ;).

But I am stupid---we can just mark the field data and instances to be deleted by hand.

A working patch is attached, with a rather lengthy inline comment explaining why we do the work of field_delete_field().