field_delete_field() and others fail for inactive fields

I have a hard time understanding why Drupal 7 is useless if field_delete_field() doesn't work on inactive fields.

I'm not saying this isn't a real bug, just IMHO not critical.

re #17 : a field can only be inactive if the module that provides its storage engine is disabled.
Without the storage engine, there's not much that can be done regarding the purge of inactive fields.

right, my bad :
field_config.active = 0 is 'field-type module' is disabled
field_config.storage_active = 0 is 'storage module is disabled'

I guess we could have field_delete_field() and field_purge_batch() use field_read_field('include_inactive') instead of field_info_field().
field_purge_batch() needs to tell apart fields that are inactive because of storage_active, and do nothing for those (nothing to purge if we don't have access to the storage).

Patch + tests welcome :-/

Thanks jpsoto.

A couple formatting issues :

+++ modules/field/field.crud.inc	19 Oct 2010 21:13:40 -0000
@@ -583,14 +583,10 @@ function field_read_fields($params = arr
+  foreach ($instances as $instance) {
+      field_delete_instance($instance);

wrong indent

+++ modules/field/field.crud.inc	19 Oct 2010 21:13:40 -0000
@@ -1026,11 +1022,11 @@ function field_delete_instance($instance
   // Retrieve all deleted field instances. We cannot use field_info_instances()
   // because that function does not return deleted instances.
-  $instances = field_read_instances(array('deleted' => 1), array('include_deleted' => 1));
+  $instances = field_read_instances(array('deleted' => 1), array('include_deleted' => TRUE, 'include_inactive' => TRUE));

The comment above needs to be updated.
More generally, it would be good to add a couple comments stating that we need to purge disabled fields too.

+++ modules/field/tests/field.test	19 Oct 2010 21:13:42 -0000
@@ -1988,10 +1990,21 @@ class FieldCrudTestCase extends FieldTes
+
+    // Simulate an inactive field

Missing trailing point

+++ modules/field/tests/field.test	19 Oct 2010 21:13:42 -0000
@@ -2021,6 +2034,16 @@ class FieldCrudTestCase extends FieldTes
+    // Make sure that the inactive field is marked as deleted when it is specifically
+    // loaded.

comments should wrap at 80 chars

+++ modules/field/tests/field.test	19 Oct 2010 21:13:42 -0000
@@ -2021,6 +2034,16 @@ class FieldCrudTestCase extends FieldTes
+    // Try to load the instance of the inactive field normally and make sure it does not show up.

comments should wrap at 80 chars

+++ modules/field/tests/field.test	19 Oct 2010 21:13:42 -0000
@@ -2832,6 +2855,12 @@ class FieldBulkDeleteTestCase extends Fi
+    // Simulate an inactive field

Missing trailing point

+ from my #8 :
"field_purge_batch() needs to tell apart fields that are inactive because of storage_active, and do nothing for those (we can't purge if we don't have access to the storage)."
We should probably log a message to watchdog() in this case - The field @field_name cannot be purged, because the storage module @module_name is disabled.

Powered by Dreditor.

Thks jpsoto, we're getting there :-)

+++ modules/field/field.crud.inc	21 Oct 2010 20:59:19 -0000
@@ -1024,13 +1020,15 @@ function field_delete_instance($instance
+    if (empty($field['storage']['active'])) continue;

(2 occurrences) According to the standards, should be

if (empty($field['storage']['active'])){
  continue;
}

Definitely needs a comment, too.

+++ modules/field/tests/field.test	21 Oct 2010 20:59:20 -0000
@@ -2021,6 +2034,16 @@ class FieldCrudTestCase extends FieldTes
+    // Try to load the instance of the inactive field normally and make sure it does not show up.

Still one comment line over 80 chars :-)

Powered by Dreditor.

Would be best to have bjaspan's signoff before this gets in.

Bug is that modules that both
a) define a field type and,
b) create actual fields of that type on install and want to delete them on uninstall,
leave stale, un-purgable data behind because the, the field type being disabled, the field is 'inactive' when purge runs.

Patch makes it so that fields whose field type module is disabled are purged too.
Fields whose storage module is disabled stay out of this, since we cannot purge if we can't access the storage.

The whole purpose of delayed field data purge was to allow field types's hook_field_delete() to run on each single deleted value, and perform the necessary cleanups (e.g file deletion...). If the field type module is disabled, the purge just drops the data without being able to perform the cleanup. Unfortunate if the module was just temporarily disabled.

OTOH, I see no other solution for the issue above. Field type modules being disabled has always been a pain case.

#20: 943772_101023.patch queued for re-testing.

#20: 943772_101023.patch queued for re-testing.

Thanks for re-rolling the patch. I think the failing tests mostly have to do with line 951 of field.crud.inc:

  if ($field_cleanup && count($field['bundles']) == 0) {

The second conjunct won't work any more, as field_read_fields does not get you the 'bundles' key---so you have to figure out manually if there are instances left (field_read_instances is your friend).

There are a couple of other issues:

• field_delete_field still uses field_info_instance---which only works for instances of active fields again. Here, too field_read_instances should be used. As it stands now, the patch does not delete instances because of this.
• field_purge_instance uses field_info_field_by_id, which has the same problem.
• Same thing in field_sql_storage_field_storage_delete (uses field_info_instances and field_info_field_by_id).
• And in field_sql_storage_field_storage_delete_revision (uses field_info_field_by_id)

I think fixing those should make everything go. Do you want to take another shot? I MIGHT have some time do to it myself.

Great. Besides the things I mentioned, looking at the failing tests, I also suspect that some delete function gets invoked on the same item twice. But that might go away once the other things are fixed.

Thank you for working on this! I think this is an important issue. For until this gets fixed, there is no proper way for a module to delete fields on uninstall.

Thanks for re-rolling. The patch now indeed solves the issue ... but, as the tests show, there is something seriously wrong in field_sql_storage_field_storage_delete. I will have a look tomorrow, trying to diagnose. If you, jpsoto, fix it before that, that would of course be awesome.

Oh, I see. Simple: The call to field_read_instances in field_sql_storage_field_storage_delete should have an element include_deleted' => TRUE in addition to include_inactive' => TRUE.

Yeah for automated testing.

I am too tired to re-roll the patch with the fix ... can you do it, jpsoto?

#34: 943772_101217.patch queued for re-testing.

Great!

I did a thorough review of the code, and it looks good. Tested it locally and it fixes the issue properly. I also went through the relevant portions of field.crud.inc and field_sql_storage.module to see if there is anything function call left that would introduce an analogue problem, and found none. I'd be happy to mark it rtbc, but I guess someone more seasoned than I should chime in first.

Very minor issue: The patch introduces white-space on an empty line. I would quickly roll a version that fixes this, but let us hear from others first.

@jpsoto: When I said "Yay for automated testing!" I did not mean to suggest that you should have run all the tests locally ... I just wanted to express how easy it was to pinpoint the problem given the tests.

Many thanks for pushing this, folks !

I do need to take a proper look at the patch, though - thus, setting back to CNR, sorry :-/
I've been on a tight schedule lately (client site going live yesterday), and couldn't find the time to look at the latest iterations. I'll try to do that asap.

#34: 943772_101217.patch queued for re-testing.

subscribing

Subscribing.

+++ modules/field/field.crud.inc	17 Dec 2010 20:04:31 -0000
@@ -1037,13 +1036,17 @@ function field_delete_instance($instance
-    $field = field_info_field_by_id($instance['field_id']);
+    $field = field_read_field($instance['field_name'], array('include_deleted' => TRUE, 'include_inactive' => TRUE));

field_name is not unique, so if a user creates and deletes multiple fields with the same name, field_read_field is likely to return the wrong one. It should probably be field_read_fields(array('id' => $instance['field_id'], ...) instead.

+++ modules/field/modules/field_sql_storage/field_sql_storage.module	17 Dec 2010 20:04:31 -0000
@@ -436,13 +436,15 @@ function field_sql_storage_field_storage
-function field_sql_storage_field_storage_delete($entity_type, $entity, $fields) {
+function field_sql_storage_field_storage_delete($entity_type, $entity, $field_ids) {

field.api.php documents this as $fields, not $field_ids. For consistency, please don't change the variable name.

Lastly, please upload a tests only version of the patch (since I assume you've crafted the test routine to trigger the bug).

Powered by Dreditor.

I don't think I agree with the approach of this patch. After discussing it with effulgentsia and ksenzee, my thoughts (I do not claim to speak for them) are as follows.

The whole point of the field delete system (aka "madness") in Field API is that we want to avoid orphaning data or preventing it from being cleaned up properly. This patch defeats that goal; it allows disabled fields to be "deleted" by just dropping the table, but that still constitutes orphaning data because the field type's delete hook is not called. From my point of view, with this patch field_delete_field() still fails for inactive fields; the bug is not fixed.

If we actually want to allow disabled fields to be deleted in a way that does not orphan the data, we have to allow the Field API purge code to load the disabled/uninstalled field type module code so as to run its hooks. Particularly since the purge operation may run an arbitrarily long time after the field type module is disabled, uninstalled, and possibly even removed from the code base, this is clearly nonsensical.

Therefore, we cannot allow disabled fields to be deleted. This leaves us two options: (1) A disabled field's data has to live forever in the database, orphaned, or (2) fields cannot ever be allowed to become disabled, and thus can always be deleted.

I favor the second approach: Fields cannot ever be allowed to become disabled. This means that a field type module cannot be disabled if any field, deleted or not, exists that uses it. This seems perfectly sensible; we already support module dependencies and a required module cannot be disabled. We can simply declare that field.module requires every module that provides a field type in use by any field, deleted-but-not-purged or not deleted.

Adding this dependency would solve the whole problem *except* for the specific case in this issue: A module that both defines a field type and creates a field using that type. With the proposed requirement in place, such a module could never be disabled or uninstalled because it would essentially require itself (indirectly through field.module). I see two solutions:

1. "Don't do that." Declare that a module that creates a field of a type that module provides cannot be disabled unless the field is manually deleted beforehand. If someone actually needs a locked field (do we support 'locked' fields? I don't remember) that is deleted on uninstall, they can create two separate modules, one that defines the field type and one that creates/deletes the field on install/uninstall.

2. Provide a one-step uninstall operation that calls three hooks: hook_about_to_be_uninstalled(), hook_disabled(), and hook_uninstalled(). hook_about_to_be_uninstalled() could delete the field because... no, actually, that won't work, because even if hook_about_to_be_uninstalled() deleted the field, the data might not be fully purged before the next two hooks were called, so we'd be disabling and uninstalling a module that provides an in-use field-type which we (or rather I :-)) have already declared forbidden.

So, the only solution I see is abolishing the concept of disabled fields and in the specific screw-case of a single module that defines and uses a type declaring that you have to do it in two modules.

Agreed on the shortcomings of this approach - as I wrote in #19, I'm sort of OK with the fact that the patch does a "best possible effort" of just deleting the raw data without the "additional cleanup" that hook_field_delete() allows.

"a field type module cannot be disabled if any field, deleted or not, exists that uses it"

Why not, but :
- what about major upgrade paths ? 'disable all contrib modules' is still the officially recommended 1st step ?
- technically, I'm not sure we can have that behaviour with core's current module handling ? AFAIK, module dependencies only rely on static info within *.info files - we won't be dynamically re-writing field-type modules' info files, right ?

re: "disable all contrib modules." We discussed in Copenhagen why the upgrade process is essentially impossible. In this case, a field type module is unlikely to have its own database schema/tables that are going to be upgraded, so it is likely that if you have a D(x+1) version of the module, just switching to it along with the core upgrade to D(x+1) will work fine. If you do not have a D(x+1) version of field type module, you can't upgrade and preserve your data anyway.

re: "module dependencies are static." Right, so if we agree that fixing this bug means that field type modules are required by existing fields of those types, then module dependencies need to specifiable dynamically, and that will become the point of this issue.

Seeing as I pushed this patch, I feel the need to say: I agree that this patch seems to work around a broken concept. The idea to allow fields to be "inactive" seems to be very problematic once you look at all the cases. Since changing this would break the API, I suppose we are stuck with the current behavior till D8?

What a cliff hanger? As someone trying to help out with regards to one such 'field-creating contributed module', noting this issue has led to a critical issue there (http://drupal.org/node/970696), I was snacking while reading this thread, hoping for a happy ending.

Any contributed module adding something to the database must be able to delete that something when uninstalled. Otherwise, you do not have a module.

A contributed module's entries in the variable table should be deleted upon that module's hook_uninstall(). Sound reasoning concludes that all database entries resulting from a contributed module using Field API should be deleted upon hook_uninstall() in similar fashion.

Any hook related to that database data deletion should be fired when the contributed module responsible for that data is being uninstalled.

If the module that created a field is disabled, that field should be disabled.

If the module that created a field is uninstalled, that field should be uninstalled.

If Field API cannot handle that, we have a serious core issue, one that is at least preventing the release of the highly-anticipated Media module.

Since I'm fairly new to Field API, and do not completely understand why it is built in a way that cannot handle the basic principle of Drupal modularity (though know the Drupal community is made up of very intelligent people who have always provided me good reasons for things like this), I really look forward to trying to help out and further learn this important API while doing so.

So let us get to the happy ending already! :)

following. This issue is causing me problems.

So, what can we do to solve this issue in D7?
With more and more contrib modules providing their own fields, this is a critical problem.
I'd be willing to give it time (write code), but I don't see any actionable item in the D7 context.

I just discussed this with bojanz. We agree on Barry's strategy:

• Let's prevent modules providing a field type from being disabled while there are fields using that type (this is a simple hook_system_info_alter() in the "field" module
• and let's advice people not to define a field in the module defining a field type (this could go to the documentation of hook_field_info()).

The rest is just the non-scalable design of the modules page and the uninstall page that is bitting us.

Something like this, perhaps?

Using module_hook() now.

What kind of feedback do we present users on the 'administer modules' page, regarding those newly required field-type modules ? "Required by: Field" ?

Setting 'required' => TRUE in the alter hook automatically adds Required by: Drupal.

Here's an alternative, doing what yched suggested.
Feels a bit hacky, though.
Also, missing a check_plain(), but I can reroll once I get more feedback.

Hm, and it's failing dependency tests. Maybe I should exclude core modules from the check?

Or perhaps the tests are testing for bad behavior --- should we be able to disable taxonomy while taxonomy fields still exist?

Fields are currently marked 'inactive' when either :
- their field type module is disabled
- their storage module is disabled

If we go that way (which I don't oppose if 'base system' folks + webchick agree),
1) should we also mark field storage modules required when some fields use them ?
2) if so, the whole system to work around 'inactive' fields (code + API : params for field_read_fields(), 'field_config.active' db column + indexes, docs...) becomes pointless; should we remove it ?

I'd go with the minimal amount of effort, fixing the actual problem.

I could go either way with #1, but it's pretty obvious that disabling your field storage is going to cause problems with your fields.
#2 smells like D8. No reason to do it in the D7 patch.
Still, should be done.
bjaspan said:

So, the only solution I see is abolishing the concept of disabled fields

so he obviously agrees.

Are we talking about introducing those dependencies?

[Field type] module "required by" Field
[Field storage] module "required by" [Field type] module

Fine by me. Note that we don't need the 'required' => TRUE.

Note : stopping field-type modules from being disabled when in use means you won't be able to update a site to D8 until there are D8 versions for all your field types (and provided the modules didn't change names).

For the record, this would have played pretty bad if applied to the D6 -> D7 upgrade. There will always be exotic field types that get abandoned, or deprecated in favor of a new module.

But what happens if Module X depends on field type Module Y (for example Image), because it creates a field of that type (image field), and then our code makes Module Y depend on Module X.
We get a circular dependency. Or is it a non issue?

@yched
They can't be disabled until their fields get deleted. Nobody is stopping people from uninstalling the module creating the fields (removing the fields themselves) and then disabling the field type module.
It's a bit sucky, but the upgrade path was never a walk in the park...

@Bojanz : this de-facto means "no upgrade to D8 until all your field types are available, which might be never, or be prepared to *ditch* some of your data before you can move forward".
That's not a minor shift compared to our current upgrade philosophy.

What happens when a module providing a field type disappears? Anything potentially bad?

"What happens when a module providing a field type disappears? Anything potentially bad?"

If the module is gets disabled, the corresponding fields are marked 'inactive' and stay out of the entity operations (load, save...), as if they didn't exist. The data just sits there, untouched.

If the module just "disappears" (code removed from the server), harder to tell. The field does not get to be marked inactive, data is loaded and saved as usual, but the invocation of the field-type specific callbacks (hook_field_load, hook_field_validate, hook_field_presave...) is a no-op. Might have strange effect on the existing data when updating a node. Not sure either what happens when Field UI tries to update the field / instance definition.

Maybe the coffee has not taken effect yet this morning, but it seems fields are a lot like modules (i.e. fields are modular).

Module dependencies can be specified in module_name.info file.

Field dependencies should exist in module_name.info file, since the module is responsible for introducing such fields.

Proper relational database design is what this issue is essentially about. How primary/foreign keys in database tables are set determine dependencies (i.e. table relationships).

There is a reason why deletion policies for some are simply not to delete deprecated data (preserving table relationships). But given that we are dealing with a content management framework, do we have the luxury of that 'cop-out'? No.

Fields are all about the database. I'm hoping whoever makes decisions regarding field fundamentals has a very firm grasp of relational database design, because we need that now (not kick the can down the road to D8 developers) for the right deletion policy applying to our framework.

As such, priority for this issue should be set to critical.

I would love to dig into the code on this (at least to learn what structural limits are effecting this issue), but I have too much on my plate in the near term, so I can only offer surface points, hoping people on the front line coding a solution will find inspiration.

I find no sound reason why all lead Drupal developers should not be focused at least to some degree on this issue. Their voices promptly need to be read here.

subscribe

This is a critical for Drupal Commerce. Reassigning to D8 since nothing is going into D8 until after it's gone into D7.

I also don't quite understand why this is being kicked down the road to D8.

@rfay: Do you mean Commerce relies on the current behavior?

Commerce needs this to be fixed.

I'm not saying anything outrageous here: Webchick won't commit anything to D7 that hasn't been finished in D8 already. So it doesn't do any good for this to sit in the D7 queue. Oh well. I don't like it either.

Is that kind of error message related to the present issue ?

inactive fields are not shown unless their providing modules are enabled. The following fields are not enabled:

    Type (field_business_type) field requires the Hierarchical Select widget provided by hs_taxonomy module
    Ambiance (field_ambiance) field requires the Hierarchical Select widget provided by hs_taxonomy module

I installed the dev verion of hierarchical select which isn't ready to go on a production site yet. I disabled it, then uninstalled.
Since then I've got this error message on my profile type page.
So I tried to reinstall the module. But though the module is enabled again the error was not removed and the fields are still disabled.
Is there at least workaround, even in the database to remove these fields and their data for the moment ?
Or is it a pure Hierarchical select issue ?

@Jerome, not sure, but the approach in #858722-61: Cannot reinstall Commerce modules after uninstall due to field deletion failure might work for you. You'll have to adapt it to your need of course.

Sorry for not posting in the right queue then.
@rfay thank you that helped me to find a solution.
For those it may concern : my issue's solution is : Firts I did enable the module again (hierarchical select in my case).
Then, I went in phpmyadmin to the field_config table and set the 2 disabled fields status to active with the edit tool.
That did the trick for me, now the fields are visible again and I can delete them the drupal way.

@rfay: Ah, now I understand your reasoning ... you had the version numbers switched in your original statement ("since nothing is going into D8 until after it's gone into D7"), which confused me. So it is agreed that this should be fixed even in D7, if possible.

If I may summarize:

• There seems to be considerable consensus that the concept of 'inactive fields' does not quite work in practice and should be abandoned.
• This entails forbidding disabling modules that provide field (storage) types as long as fields exist that use them.
• This change should come with a recommendation to not create fields of a (storage) type that is provided by the same module.
  (I think this really should be a recommendation, rather than a hard rule ... modules that allow their fields to be deleted in the UI, as the current OG does, might be an exception.)
• This creates a serious issue about major version upgrades, as it would essentially preclude upgrading until all field (storage) type modules that are in use have new versions. Relaxing the "disable all non-core modules before upgrade" recommendation would not solve this problem.

So what should we do? One possibility would be to have a override that allows to exceptionally disable modules that have fields in the upgrade process. But that would leave orphaned fields (that are not even marked 'inactive', if we get rid of this notion ... at the same time, keeping the 'inactive' status just for this case seems overkill) ... any other ideas?

A related issue: A proper 8.x solution, if we go this route, would remove the whole 'inactive field' code ... which of course cannot happen in 7.x, as this would break the API. Suggestion: We use this issue to develop a "minimal" fix, which may make inactive fields redundant, but does not remove the code dealing with them. Then, we can backport this patch and open a new issue to remove the (now obsolete) code dealing with inactive fields in 8.x.

Here is an idea that might just be crazy enough to work: How about we introduce a hook that allows arbitrary modules to "claim ownership" of a field? The module that provides a field (storage) type for a field would automatically own it. A module that owns a field could then only be disabled if there is another module that claims ownership of that field.

Not sure whether this is workable, but the idea would be that one could then implement a module for use in upgrading that can claim ownership of arbitrary fields (selected in the UI) of modules that do not have new versions, and provide a dummy-implementation (which always reacts as if there were no data for the field). This would allow disabling these modules, but leave the data sitting in storage until an updated version of the original module comes along ...

This would surely make the upgrade process more complicated, and it also feels like introducing a whole layer of complexity that is about as complex as the current 'inactive fields' concept, but at least the complexity could be sealed away in a "needed only for major version upgrades" module.

When upgrading for 7.x to 8.x, if some modules providing field types are not available, we are in the second case of #73. We need to make sure that nothing bad happens in that case: one way of doing that is to make sure that a field that relies on a module that does not exists is automatically marked as inactive.

make sure that a field that relies on a module that does not exists is automatically marked as inactive

That means regularly and automatically scanning all existing fields to check whether their providing module is still available. Not sure when. We don't want to do that on every page load.

Also, the envisioned solution of preventing the disabling of field type modules when there are actual fields using them raises UX issues.

What if I *want* to get rid of field_type_module_foo ? If we go with #62, all I see is that 'field_module_foo' is marked as "required by Field module", and its checkbox is greyed out. And Field module itself is required.
Nothing in the 'administer modules' UI or Field UI tells me :
- that this dependency is not 'static' like the others, but is caused by my current config state and might get away if I take some action,
- that I'd need to go and delete a series of fields (which ones ? the Field UI doesn't tell which modules implement the type of a given field) before the dependency is removed, the checkbox ungreyed out, and I can actually disable the module.
Plus : if we want that approach to fix the issue at hand, it's not even that simple. The field type module cannot be disabled until I have manually deleted all fields of some type(s) *and* cron has run enough times that the actual field data has been purged while the module is still around. Yikes.

If the appraoch gets a buy-in as is from webchick or Dries or UX folks, I'm happy to be ruled off, but my personal feeling is that our current UX around module dependencies is not ready to be bent that way without some more work.

Rather than forbidding the field type module to be disabled, is there a way we could insert a nag screen / confirm form when the user submits the 'admin modules' form, warning them that they might want to delete fields a, b, c... first ?
Might be doable through a form_alter() in field.module - although we'd need a similar safeguard for drush pm-disable, which seems only doable with dedicated code within the drush command...

I agree that there are big UI issues and that just using the existing UI with a "required by: Field" does not cut it. And the issue of having to wait until the field data has been purged on a cron run before one can disable a module is something I've been wondering about, too.

At the same time, just having a nag screen does not solve the problem that has surfaced in commerce (if I understand right) and that brought me here: If a module is uninstalled (and cannot clean up its fields on uninstall, as it is now), things break if that module is activated again (as it will try to create a field type that already exists). Though perhaps that could be fixed by adapting the code dealing with hook_field_schema ... and warning module authors that they should always check if the field they are going to create exists already. This seems hackish, though: The way things are supposed to work is that after an uninstall, it is as if the module had never been installed. So a module should not have to worry about working around possible remainders from a previous install ...

The greater problem is that the only logical place to remove field types (or rather, their fields and data) is at uninstall, but that cannot happen. So we either have to change what "uninstall" means (uhm ...) or figure out a way to allow deletion (or at least, marking for deletion) of field data in hook_uninstall. Or insist that fields are cleaned up an earlier stage, so that they are gone by the time hook_uninstall() is run.

An alternative to not let modules be disabled until all their field data is gone would be to not allow them to be UNINSTALLED before this is done. This would mean keeping the concept of inactive fields, but also make sure that data is neither orphaned nor sticks around on a re-install. UI-wise, modules that still have field data could be listed on the uninstall page (but not be selectable), with a help text explaining why these cannot be uninstalled (and a note whether the issue is just that data is not purged, or whether the field is not deleted). This would mean that a user might have to re-enable a module to delete the fields before he can uninstall it.
The only problematic scenario on this approach would be if someone disables a module and then removes it from disk before uninstalling it. But this is something that should not be done anyways, so ...
Note that there also would be no problem on major version updates, as the modules could be disabled.

On this latter approach, there probably still should be a message / confirmation screen warning about disabling a module that still owns fields, explaining that uninstalling the module will require deletion of the fields (and informing the user that inactive fields will not show in the UI).

What about displaying inactive fields with their delete button in the field API UI? Like the inactive bookmarks under the title "inactive fields:"
With some text infos such as:
This field was provided by the module foo which is disabled.
1) If you want to delete the module foo you need to delete this field first.
2) If you want to activate this field you need to enable module foo first.

@Jerome: That would be on the assumption that we forbid uninstalling before all fields/field data is purged ... this would be possible, except for the "delete button" part, as a field cannot be deleted if its module is disabled.

Also, a general problem with the approach suggested in #89 is that field data can also only be PURGED if the storage module is active. So we still would have prevent disabling the module that provides storage if there are deleted, unpurged fields around. Arg.

At least if you can see the inactive fields, you don't have to scratch your head and wonder where they are. When I ran into that problem, I had to go to the database, then set these fields active status back to 1 to be able to delete them (see #82). Once you've already uninstalled the module, even reinstalling the module doesn't get rid of those inactive fields, and that was the only solution I found.

Could this issue cause that kind of notices when disabling some modules, like link and phone number, or is it something else? http://drupal.org/node/1130502#comment-4361264

Marked #1117000: DatabaseSchemaObjectExistsException: Table field_data_field_my_field already exists as duplicate.

Oh dear, we really need to get this one solved.

We do. Frankly, I don't quite understand why this is not treated as a more urgent matter---true, it is not quite "critical" in the sense this status is used, but the issue thoroughly fucks up enable/disable/uninstall/install cycle, so this is something that really needs to get fixed ...

In the mean time, maybe we should at least put a note into the documentation of field_delete_field(), saying that the function will only work if field is "active" and hence not in the uninstall function? Possibly with a link to this issue?

I think this is critical. Essentially it means that normal, expected, Drupal behavior is completely broken. You can't uninstall a module and then install it again.

What do we want?

I want to enable the module, start using the fields, fill them up with data, data gets stored.

When I disable the module, it tells me that it is going to mark this module's field data as inactive, and that they will be marked as active if I re-enable the module.

When I uninstall the module, it tells me that all data for the module's field types will be deleted. It will give a report of the fields and data in question. Then I can click on 'yes, delete all that' and it will do so.

This means that field_delete_field() must be able to work on inactive fields, since the module should be responsible for deleting the data on uninstall. Or there must be a new API function, such as field_delete_inactive_field(), or better yet: field_uninstall_fields().

Yes, but the behavior as outlined cannot work, at least not for storage modules: If the module is disabled, we cannot delete/purge the data. Similarly, even with a non-storage module, the module may have to do clean-up after deletion, but if the module is disabled, its hooks will not be run. So you end up with orphaned data.

Regardless of whether we keep the concept of inactive fields around, the only sensible way seems to be to delete fields and their data while the (storage and field type) module is in the "enabled" state---this means a module that is disabled without first deleting all its data will not be able to be uninstalled. Hence you would have to re-enable the module, delete the data, and then disable again and uninstall. Complicated, but at least after an "uninstall", it would be as if the module never had been there---which is how it should be.

The remaining problem is that Field API does deletion indirectly, only marking for deletion at run time and purging later on cron runs. But that would mean we have to ensure that a module cannot be disabled (and uninstalled) if it has marked fields for deletion that have not yet been purged. This behavior may make people unhappy (as they will have to wait / run cron manually, perhaps multiple times before they can disable a module, but it is the only sane way, I think.

So, what we need is a way to tell whether a module is responsible for data that is not yet purged (we may have such a way in place already, I don't know the internals of Field API very well).

subscribing

I made a module to shed some light: http://drupal.org/sandbox/Mile23/1171894

I spent a train travel re-hashing my own ideas about this, and then re-reading this entire thread.
I came more or less to the approach sven.lauer proposes in #89 (4rth paragraph, "An alternative to...") :

- Let field type modules be disabled, but don't let them be *uninstalled* until all their field data is purged. Add a notification/warning when they are disabled. Thus, we allow modules to get temporarily disabled.
Problem, though : modules can be uninstalled by drush. If we need to patch drush to enforce that behavior, most existing setups won't know the difference...

- Let field_delete_field() act on disabled field types (more or less as per patch #34)

- State that hook_field_delete() implementations should live in the module's .install file (just like hook_field_schema() was moved to .install at some point before 7.0). That way, we can load the *.install file before trying to invoke hook_field_delete(), while the module stays disabled.

- The same module should not define a field type and create fields on install / expect to be able to delete them on uninstall. Implement the field type in a helper module, and depend on it.

- We punt on cases where the *storage* module is disabled. We just can't delete data if the data is inaccessible. You'll need to re-enable the storage module.

- Additionally, we could try to acknowledge that the whole issue about "delayed purge" is actually a non-issue for 95% field types, because 95% field types don't implement hook_field_delete(). File and Image need it to keep track of file references and possibly delete unreferenced files, Media field type needs it too, but that's probably about it. We might consider having field_delete_field() check whether the field type implements hook_field_delete(), and just delete the damn thing if not.
This does mean that the storage module will be notified of a single "delete this field's data, please", and does not get to delete each value individually. Works for sql_storage (drop table), not sure about other storages like Mongo.

#105 is a a bit complex, I reconsidered the "prevent field types in use from being *disabled*" appraoch (roughly, patch #62).

I had two major concerns about this :

- major upgrades (you just can't disable a contrib module as recommended in the official upgrade steps) : would be alleviated if we find a way to automatically mark fields as "disabled" when the module becomes "absent" (see #85 - #85). We might want that ability anyway - see #1001060-23: Undefined index _field_info_prepare_instance_widget, feedback welcome.

- lack of UX feedback (#87) : might be worked around with possibly reasonable UI work.

However, patch #62 does *not* prevent the "required" field type module from being disabled through drush, because drush_module_dependents() looks in a place of system_rebuild_module_data() that is out of reach of hook_system_info_alter().

So, this approach has the same issue than the one in #105 : "if we need to patch drush to enforce that behavior, most existing setups won't know the difference..."

subscribing

I think it's OK if we have to patch drush, if it's bypassing hook_system_info_alter() then that's a bug in drush, eventually people will update their drush installs - it takes some people a long time to get around to updating core too. Also if we want to, we can add a specific note to the release notes for the core release this goes out in, telling people to also grab the latest drush release (assuming we could time that).

Created #1175440: pm-disable doesn't take hook_system_info_alter() into accout in drush queue.

Oh my. I was blissfully ignorant about this issue until today. I'm happy to patch drush and coordinate releases as needed.

Just a note I'm 100% in favour of using hook_system_info_alter() here and setting required.

I tried to do this for filter modules where the filter is used in a text format here #562694-67: Text formats should throw an error and refuse to process if a plugin goes missing, that patch ran into some issues that still aren't resolved in core, so this issue may also have to deal with those.

Attached patch expands on #62, and provides the minimal amount of UI feedback to make this viable IMO (re: my objections in #87) :
- on the modules list, we provide an explanation as to why the module is required, and provide a link to the 'Field list' page. This involves hacking system_modules() a bit, to be able to provide additional feedback.
- For each field, the 'Field list' page displays the name of the module providing the field type.
This provides a way to determine which fields you'd need to delete if you really want to get rid of some field_type module.

Still needs addressing : find a way to automatically mark fields as "inactive" when the module becomes "absent" (or "not compatible with the current core version" - see #85 - #86).
In #1001060-23: Undefined index _field_info_prepare_instance_widget, I wrote :

"Such cases could be caught at run time by adding extra checks in _field_info_collate_fields() (see patch over there - we'd additionally need to write back the 'inactive' state into the {field_config} table)

I'd much rather have a way to easily re-check the state of enabled / disabled modules and udpate the list of inactive fields accordingly. Right now, field_modules_disabled() is the only chance we get to mark fields as inactive, and if this gets skipped for some reason, there's no easy refresh.

'Cache clear all' seems a good spot for this. However, the only hook fired in drupal_flush_all_caches() is hook_flush_caches(), which is supposed to return a list of cache tables, and is not really intended to execute actual code."

Opinions welcome.

IMO it is fair to interpret hook_flush_caches() as an opportunity for modules to do own housekeeping in addition to returning tables. So, your suggestion is a valid approach IMO.

hook_flush_caches() is currently abused for two things - flushing caches and garbage collection on cache bins.

Somewhere between http://drupal.org/node/1167144 and http://drupal.org/node/891600 I'd like to add hook_cache_bins(), and a gc function that system_cron() runs, that'd separate cache flushing from identifying what cache bins are. So eventually hook_flush_caches() shouldn't get run on cron, and will just be for doing a full cache flush. That needs to get backported to D7 one way or the other.

@catch : not sure I get you - is that a "yes, abusing hook_flush_caches() to refresh the {field_config}.active column against the current list of enabled modules should be OK" ? ;-)

Sorry, yeah I think that's a decent use of hook_flush_caches().

Alternatively a hook_cron() (but one that ensure's it's not called too often, which system_cron() completely fails to do).

Updated patch :

- Uses field_flush_caches() to update the status of {field_config}'s 'active' and 'storage_active' columns.
This catches the case of :
modules enabled / disabled (even though field_system_info_alter() theoretically prevents field types in use for being disabled - that's still not true through drush, that's also not true for direct module_disable() calls)
modules whose {system}.status column is manually set to 0 / 1 (for instance in update_fix_compatibility() for a major upgrade)
modules disappearing / reappearing from the codebase

- removes existing implementations of hook_modules_[enabled|disabled](). hook_flush_cache() runs on modules being enabled / disabled anyway.

- Attempts to fix failing tests (didn't actually run the tests)
testEnableForumTaxonomyFieldDependency got added in #821290: Unexpected error after disabling - unistalling - re-enabling forum module, ugly issue involving enabling forum, then disabling taxonomy and forum, then enabling forum --> Ecxeption, attempt to create the taxonomy_forum field, that already exists. Now, you just can't disable taxonomy while the field is still around. As such, the test doesn't make sense in ModuleDependencyTestCase any more, moved it to forum.test.
testUninstallDependents choses forum and taxonomy to test dependents enabling / disabling, we can use forum and comment instead (forum depends on comment too)

Not sure I'll be able to push this much further myself, I'll be away from my coding setup for a month at the end of the week :-/.

Still @todo :
- fix "drush dis", help welcome in #1175440: pm-disable doesn't take hook_system_info_alter() into accout
- write tests for the new behavior :
field type modules cannot be disabled while some fields use them, deleted or non-deleted;
manually setting {system}.status to 0 or 1 and running drupal_flush_all_caches() sets the fields to active / inactive
(already tested in FieldCrudTestCase::testActive() : ) directly calling module_enable() / module_disabled() and running drupal_flush_all_caches() sets the fields to active / inactive

Oops, previous patch missed one local commit.

Fixed tests :

- Moving testEnableForumTaxonomyFieldDependency from system.test to forum.test requires a bit of adaptation (helper method not present anymore, setup...). No biggie.

- FieldCrudTestCase::testActive() fails show a change in functionality, though.
In #118, I wrote "removed existing implementations of hook_modules_[enabled|disabled](). hook_flush_cache() runs on modules being enabled / disabled anyway". That's true for the "administer modules" UI and for "drush enable / disable", but that's not true for direct API calls to module_[enable|disable](), of course, which is what the test does. Adding explicit drupal_flush_all_caches() calls fixes it, but with the current patch, direct calls to module_[enable|disable]() do not update the field 'active' column like it did before. Happens on next cache flush.

We could reintroduce the implementations of hook_modules_[enabled|disabled]() (probably trying to reuse code), but that would cause the code to run twice on regular enable / disable through the UI or drush, once during module_[enable|disable](), once during the subsequent drupal_flush_all_caches().

Er ? Reuploading ?

#123: field_delete_field_inactive-943772-121.patch queued for re-testing.

Broken test client ? Anyway, you guys will need to take it from here, I'll be away for a month or so :-)

Thank you yched, for working on this.

I see that the testbot is happy now.
Applied the patch to 7.x (applies cleanly), clicked around, read the patch, everything seems fine.

There's a tab left here:

+  function testEnableForumField() {
+    $this->drupalLogin($this->admin_user);
+    

But that can be fixed before commit, lets get more comments first.

Tested with Drupal core 7.2 with countries module field

I can confirm that the module can not be disabled and bellow the following message is displayed:

Required by: Drupal (Field type(s) in use - see Field list)

which points to admin/reports/fields where you can easily access and manage these fields and delete them if uninstall is needed. I think it's great. Thank you Yched

+1 for RTBC

Thanks from me too, yched, for working on this important issue.

I just tested the patch, and it does what it is intended to do, but I see a problem: admin/reports/fields is provided by field_ui.module. The latter is not required by core, and if it is disabled, the link on the module page does not work.

I am not sure how to handle this, but at the very least, the link to admin/reports/fields should only be inserted if field_ui.module is active.

While we are at it, maybe it would be nice to expand the help text on the top of that page, explaining that modules can only be disabled if all their field instances are gone? Relatedly, would it be a good idea to include deleted-but-not-purged fields in that list? Both of these things could (should?) happen in their own patch to field_ui, of course.

And a quick question: This patch does not make the *storage* module of existing fields required---but wouldn't we want to behave those just as the field type modules? I.e. have them marked required as long as there are (unpurged) fields left that use that engine?

And another thing, more to check if I understand the idea: Basically, the only legit way for a field to get into the "inactive" state would be during major version core updates, right?

@sven.lauer :

- "admin/reports/fields is provided by field_ui.module" - doh, you're right of course. The message needs to be a little smarter, and only display the link if field_ui.module is enabled. If it's not, we could display an "enable Field UI to get more info" advice instead, but that feels overkill IMO. If you disabled Field UI, it's to be expected that you don't get feedback on your fields.
As I wrote earlier, I won't be able to roll patches for the next couple weeks - care to reroll this ? ;-)

- Also agreed that this admin/reports/fields page could be improved in several ways. Listing fields pending deletion might be a good idea, but is not strictly required IMO, since there is no significant action you can take on them, except waiting for cron to run enough times. We don't even have a proper mechanism in place to report "deletion progress", or predict how many more cron runs are needed...

- "Do we want to make storage modules required too when they are in use ?". Not fully sure about that, the case is slightly different IMO. I'd rather leave that for a followup if needed, as it's not strictly required to unblock the situation here. The current patch does recheck the field's 'active' status according to the storage module availability on "cache clear".

- "The only legit way for a field to get into the "inactive" state would be during major version core updates ?".
Yes, plus cases where the storage module goes away.
Plus cases where the module just disappears from code base (technically similar to "D7 version is present but in a D8 setup"m which would be the case hapening during major upgrades), or is manually disabled in the system table. Those are not "legit" cases, but according to the # of reports of #1001060: Undefined index _field_info_prepare_instance_widget, do not seem uncommon.

The message needs to be a little smarter, and only display the link if field_ui.module is enabled. ... care to reroll this?

Re-rolled.

I tested the patch (both on 8.x and 7.x) and (re)read it, looks good.

+1 from me to RTBC the version in #133, modulo the whitespace problem mentioned by bojanz in #128, which still is there in the new patch.

I tested the patch (both on 8.x and 7.x) and (re)read it, looks good.

+1 from me to RTBC the version in #133, modulo the whitespace problem mentioned by bojanz in #128, which still is there in the new patch.

Applied whitespace correction.

And some quick replies @yched:

Also agreed that this admin/reports/fields page could be improved in several ways. Listing fields pending deletion might be a good idea, but is not strictly required IMO, since there is no significant action you can take on them, except waiting for cron to run enough times. We don't even have a proper mechanism in place to report "deletion progress", or predict how many more cron runs are needed...

True, while it would be awesome to have such fine-grained information, right now, there is NO way to know that there are fields left to purge, or which ones they are---except for our newly introduced message on the module page. It would be nice to give people a place to check for this, and the field list seems the logical place. Anyhow, this does not belong here, I'll open a separate issue for this.

- "Do we want to make storage modules required too when they are in use ?". Not fully sure about that, the case is slightly different IMO. I'd rather leave that for a followup if needed, as it's not strictly required to unblock the situation here. The current patch does recheck the field's 'active' status according to the storage module availability on "cache clear".

I agree that this is enough to get the current issue resolved. I'll open a follow-up issue once the current patch is accepted.

That was quick! Marking this RTBC based on the previous reviews and my own.

Coming over from #1001060: Undefined index _field_info_prepare_instance_widget. Patch applied cleanly to D7.2. Have been reading up on this issue, but still a bit unclear regarding how much of the "Undefined index: module in _field_info_prepare_instance_display() " this patch is suppose to fix. At least with the patch applied do I still have a lot those messages. Please let me know how I can help out.

Edit: Removed the list of error messages, and reattached as a text file in #144, sorry about that.

The issues are not directly related, I think, but if the warnings result from improperly removed/deactivated field modules, the patch should 'fix' things by marking those fields inactive on the next cache_clear. Inactive fields are generally treated as if they were not there (which is what this issue is about, as this creates problems on re-install).

Did you try clearing the caches after applying the patch?

@yched mention that this issue should indirectly fix the issue http://drupal.org/node/1001060#comment-4589172. Anyway, yes I did a "drush cc all" after applying the patch, also restarted Apache to flush out any other caches.

Cheers

This issue affects text and UI, so tagging string freeze and UI freeze. You'll have to excuse my getting pendantic about what I'm sure most people consider to be niggling details. The issue is that this patch is slated for backport to D7, and D7 is string frozen. So we need to make sure those strings are perfect before we commit so we don't have to break them twice.

Here's what the patch does, for the UX folks:

First, when fields for a field type-exposing exist, you'll get a new entry under the dependencies information that says "Required by Drupal (Field types in use - see Field list)". The module checkbox is also force-enabled so you can't turn it off:

You click through the interface and delete your fields with whatever module (this is positively painful because that list fields page totally sucks, but that's out of scope for this issue). Then you go back. Now you get the following message on the module row: "Required by Drupal (Fields pending deletion)" and the module checkbox is still disabled, even though all of your fields are gone.

At this point, you are totally stuck. The action item is to run cron, but there's absolutely nothing here to tell you that.

I'd recommend one of two approaches here:

1. Add a "run cron" link to the message (janky as heck, ughhhhh :\)
2. Actually delete the effing fields when you say you're going to delete them so that the manual cron run is unnecessary. Some logic like "if (you just deleted a field) && (there are no other fields like it) then { delete() }" But I know yoroy/bjaspan has objected to that in the past.

Because if we don't do that now, we're going to get support requests about that cryptic message, and we won't be able to further refine that in later releases of D7 without breaking strings *again*.

On the patch itself:

This sentence leaves me with the hanging question of "Well what should I do then??" It'd be nice if we could move this text to just one place for this info, where people would be most liekly to go, and then add the answer to that question, too.

What's up with these watchdog() calls being removed? That looks unrelated to this change, and causes even further translation changes, so I'd prefer to leave it out unless there's a good reason.

The approach on the whole, though, looks sound.

So for deleting the fields, we could add a field_purge_batch() to the field UI field instance delete confirmation form or somewhere.

This could get extremely slow, possibly even time out, if you have a lot of deleted field pending.

I like this better than a link to run cron though.

What'd I'd really like, but it is off-topic here, is field_purge_batch() moves to a queue, and we add a silent queue runner to Drupal 8 (i.e. 1px gif that shows up after you submit a form triggering a queue insert), that would allow for kicking off queue runs without either the batch API screen in the middle, or adding the work inline on form submits.

Sorry about the long list of error messages in #139, attaching the vaguely related list error messages as an textfile for the sake of order.

Getting all field data to be effectively purged might require much more than one single cron (or field_purge_batch()) run. We currently delete 10 values per cron run... Could be more, but no value will ever make sure we can safely delete all in one single request...

Up in #105, I wrote :
"Additionally, we could try to acknowledge that the whole issue about "delayed purge" is actually a non-issue for 95% field types, because 95% field types don't implement hook_field_delete(). File and Image need it to keep track of file references and possibly delete unreferenced files, Media field type needs it too, but that's probably about it. We might consider having field_delete_field() check whether the field type implements hook_field_delete(), and just delete the damn thing if not.
This does mean that the storage module will be notified of a single "delete this field's data, please", and does not get to delete each value individually. Works for sql_storage (drop table), not sure about other storages like Mongo."

That would still leave "pending deletion" messages when you delete a field whose type actually implements hook_field_delete(), though (Image, File, Media...).

re @webchick : I removed the watchdog calls because they would be printed at each cron run (instead of currently at each module_enable(), which makes little sense anyway). Those watchdog calls were part of the initial Field API patch, wrong idea to begin with, there's no point in logging this kind of information IMO.

Getting all field data to be effectively purged might require much more than one single cron (or field_purge_batch()) run. We currently delete 10 values per cron run... Could be more, but no value will ever make sure we can safely delete all in one single request...

I completely agree with this, but I think webchick's main concern was that you can do this:

- install a module
- create a field
- delete the field because you hate the module
- uninstall the module

without waiting for cron to run.

I opened #1189464: Add an 'instant' queue runner which would let us do the same thing without the inline processing.

If fields are pending deletion because you have thousands of items using them, then tough luck IMO, but I think it's worth adding the single field_purge_batch() call (possibly with a hardcoded argument to 10 instead of using the variable) to smooth that particular case out. At least compared to telling people to run cron manually if the existing patch can't go in as is.

Here's a patch and interdiff with the field_purge_batch().

I did not try to improve the comment about field providing modules and modules that create fields so still CNW.

I looked at the comment and it looks out of date to me.

If you create fields and instances, your module will be marked required until they're deleted, since the Field UI doesn't differentiate between fields and instances created via the UI or hook_enable(), then it just means you need to delete the instances and fields before your module can be disabled - which is the point of the patch.

So I just removed the comments for now.

That leaves one DX gotcha in the following case:

Module creates a field type, field and instances.

hook_uninstall() would delete the field instances and field.

The site using this module does not use Field UI at all.

Now they're stuck with a module that they can't disable.

That probably needs to be documented somewhere, but I'm not sure where, so I'm marking this back to CNR.

Those are real test failures due to the new tests we added in #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it).

The tests use standard profile.

Standard profile creates a taxonomy term reference field.

This means you can't just go ahead and disable taxonomy module without first deleting the field instance.

Don't know about the vocabulary failures, that looks a bit different.

That test would really benefit from #913086: Allow modules to provide default configuration for running tests so that we don't have to go in and manually undo what the standard profile adds there.

I will probably add the ugliness a bit later with some kind of snarky comment and a @todo though to get us back to passing.

Turns out that #913086: Allow modules to provide default configuration for running tests, doesn't actually help this patch, would've been nice though.

The new tests from the other issue caught a bug in forum_uninstall() - was not deleting the field it creates on install - this was leading to taxonomy and options module being required, hence it wasn't possible to disable all of the modules.

I'm still getting strange exceptions in the vocabulary CRUD test, not had time to look into those yet.

Per #1191290: system_cron() should not run hook_flush_caches(), but use a cached version we should ad a helper function that is run both on cron, and via hook_flush_caches(), rather than rely on the current behaviour of hook_flush_caches() being run on cron (which it should not be given current usage patterns).

Marked #902794: Field deletion as a duplicate of this issue. The other issue is older, but closing this issue as duplicate instead would definitely be the wrong decision!

Lots and lots of functions related to deleting fields and bundles and instances were calling field_info_field().

Taxonomy vocabulary test passes now, I didn't check other tests to see if the fixes here broke something else.

Unless I missed something stupid, this is going to cascade all over the place - fix one test and another will break.

#133: field_delete_field_inactive-943772-133.patch queued for re-testing.

Those tests definitely fail.

I really like what I see in #142. However,

This means you can't just go ahead and disable taxonomy module without first deleting the field instance.

as well as the impact on upgrades sounds like a show-stopper to me -- just because we have configuration and data for something somewhere shouldn't prevent site admins from disabling functionality (temporarily for debugging or permanently).

What if we

1. keep the extremely nice pointers on the modules page as informational hints
2. prevent fields/instances of disabled field type modules from being shown/deleted in Field UI ("grayed out"), but also on the API level

?

If you prevent fields/instances from being deleted, you have to prevent (at least) the following from happening:

- deletion of bundles - since that can delete instances and fields.
- uninstallation of modules that provide field types.
- uninstallation of modules that provide bundles and/or entity types.
- uninstalling any of these things if there is related data in the system belonging to other modules that are disabled - since their hook_uninstall()/hook_$something_deleted() etc. hooks will not run.

Whatever happens, we have to either prevent people from nuking stuff that their sites require on, or just say that as soon as you disable a module you're completely on your own and stop pretending it actually works.

The upgrade path issue I am not sure is valid:

- We already have a requirement that no hooks are invoked on update.php (it's not actually enforced yet, but it's a requirement), so do modules really need to be disabled on there any more?

- the upgrade issue only affects people upgrading from Drupal 7 to Drupal 8, I'd be happy to have a critical task to sort out the upgrade path mess in D8 if we can fix this critical bug for Drupal 7 without blocking it.

So I'd strongly support what we have (assuming we can even get that to pass tests with the multiple levels of broken-ness with disabled modules at the moment).

Back home again, slowly getting back up to speed. Many thx @catch for keeping pushing at this.

I"m running interdiffs on the succession of patches posted since #136 (#148, #149, #153, #157) :
- I don't understand the test changes added between #148 and #149
- I'm not sure why #157 embarks in a series of field_info_fields() --> field_read_fields() replacements. That looks like the initial approach that prevailed until patch #34, which aimed at making field_delete_field() actually work with inactive fields. After #49 and some back and forth discussion, the thread shifted to the current "prevent disabling field types in use" approach - but I don't think we want both ?

@yched, so the issue is that forum module has to inform the field API that its bundle is going to be deleted when it is uninstalled. All the Field API functions that deal with the deletion of bundles use field_info_field() or field_info_instances(), since the bundle is 'inactive' when forum module is uninstalled, they can't locate the fields/instances, and therefore cannot delete them. That's why I switched to field_read_fields() but then things cascaded from there. When the test failures increased rather than decreasing I gave up on this approach but haven't been able to face coming back here since then. In other words it's good to have you back ;)

This was all shown up by the new tests added by #1029606: Regression: Not loading the .module file causes a fatal error when uninstalling some modules (as does loading it) that attempt to enable, disable, uninstall then re-install every core module.

It may be worth going back to #148/9 and #153 and starting again from there - although if it ends up with the same code as later patches we're definitely in trouble here.

This should be green (I only ran the tests that failed in the recent patches).

Basically, I skipped #157 (no massive field_info_fields() --> field_read_fields() replacements).
This is @catch's #153, plus :
- the bits from #157 that moves the 'refresh "active" column' code into a standalone field_sync_field_status() function.
- a simple fix for the TaxonomyVocabularyUnitTest notices
in short : TaxonomyVocabularyUnitTest::testUninstallReinstall() does module_disable('taxonomy').
We need to explicitly call drupal_flush_all_caches() just after that so that field_sync_field_status() runs and marks taxonomy_term_reference fields as disabled. field_info_*() then doesn't try to massage them, which creates the notices.

Not ideal. Previous iterations of the patch already added such manual drupal_flush_all_caches() calls after direct module_[enable|disable]() in tests. Back to what I outlined in #121 :
HEAD currently runs (the equivalent of) field_sync_field_status() in hook_modules_[enabled|disabled](). We want it to run on hook_flush_cache(), so we moved it there. It still runs when the "administer modules" form is submitted and in "drush enable / disable", but not with direct API calls to module_[enable|disable](), which a lot of tests do. Adding explicit drupal_flush_all_caches() calls fixes it, but with the current patch, direct calls to module_[enable|disable]() do not update the field 'active' column like it did before. Happens on next cache flush.

We could re-add a field_sync_field_status() call in hook_modules_[enabled|disabled](), but that would cause the code to run twice on regular enable / disable through the UI or drush, once during module_[enable|disable](), once during the subsequent drupal_flush_all_caches(). Not that bad, probably...

Thoughts ?

Forgot to add the interdiff with #153.

Hmm, maybe the cache flush was all that was needed, that's good if so.

I have a nagging feeling that something still isn't quite right here, but assuming tests pass on this one it's a massive improvement compared to where we are now, and I can't put my finger on what exactly should be broken at the moment, so this looks ready to me otherwise.

Updated issue summary.

This approach still feels wrong to me, as outlined in #163 and also by others in earlier comments. I've tried to capture some details of this issue and started to revise the summary. I wasn't able to figure out what the exact problem is (nor how to reproduce). I think we badly need a proper issue summary here to make the right decisions, before attempting to implement a solution.

Updated issue summary.

Updating summary

Expand problem and details. Debated solutions still TODO

Sigh... I don't know how we will come to an agreement on this one :-/.

Anyway, many thks @sun & @catch for starting an issue summary. I tried to expand / rephrase the "problem" part for now, I'll try to add the "possible fixes" part shortly.

subscribing

markup error

Updated issue summary.

updating summary.

Updated issue summary.

i don't fully comprehend the problem, but my hunch is one possible solution is to use a finite state machine
can someone point out a single function that shows the logic of deleting fields ?
if we had a finite state machine then such a function would exist, and solving the problem may become much easier

Updated issue summary.

Updated summary :
- Added the proposed fix for the "original' issue - implement field types in a separate module, and have the main module depend on it
- Added a "drawback" about lack of user control / feedback on cron purges

(thks @catch & @rfay...)

- Added the proposed fix for the main issue - implement field types in a separate module, and have the main module depend on it
- Added a "drawback" about lack of user control / feedback on cron purges

Clarified the "foo" and "foo_field" example in the summary.

Great, we have a summary - what now ? ;-)

For the record, drush got fixed and doesn't let you disable field-type modules that are dynamically marked required : #1223334: Don't disable modules if we cannot disable dependencies. No specific info is provided, though; the user feedback does not yet differentiate 'regular' required modules from 'dynamic' ones (required because of the current config of the site)

We could re-add a field_sync_field_status() call in hook_modules_[enabled|disabled](), but that would cause the code to run twice on regular enable / disable through the UI or drush, once during module_[enable|disable](), once during the subsequent drupal_flush_all_caches(). Not that bad, probably...

I think this is OK, but we should add a @todo in there and a followup issue to track it. There may be existing issues re: cache clearing in module_enable() open around rather than a new one.

Patch seems RTBC apart from this.

@catch : I'm actually not sure what your previous comment means :-)

Sorry, running the check twice on module_enable()/module_disable() doesn't seem too bad in the scheme of things.

maybe i still don't understand this problem but it seems similar to C++ destructors :
when an object reaches end of scope, then the destructor is called to cleanup its data .

so the question is what is "end of scope" for modules? isn't it simply hook_disable ?

so one solution could be is to require module developers to take care of cleaning up,
just like C++ classes have to implement their own destructors so should modules that create
fields in hook_install also delete them in hook_disable .
and modules that define bundles should call field_delete_instance, like i did here in entity_cleanup_example_disable()

but all this can be done automatically by a utility function, so field_create_field should register which
module called it and when that module is "out of scope" it takes care of deleting all the fields it created before it goes out of scope .. indeed field implements hook_modules_disabled, so why doesn't it go ahead and call field_delete_field on them ? looks like i still don't understand the problem

can you add to the summary a description of what consists an "end of scope" for modules as far as field api is concerned ?

and i don't like the proposed solution, i think it could create much frustrations for many users .
and on theory it is just a bad idea to make a core drupal module dependent on custom or contributed module

also can you add to the summary a concrete example of orphaned data ?
should it be something like : module A creates field and module B alters it data, then module A
is uninstalled but module B is never called to clean up it's altered data ?
i mean everything in database can be "marked" for deletion at some point so what orphaned data are you talking about ?

documentation before field_purge_batch talks about pseudo-entities, maybe there should also be pseudo-fields defined for deleted fields so that they could be purged latter on?
same as field_modules_disabled marks fields as inactive, then field modules should as well mark other related data as marked for deletion .
for the file field example isn't it enough only to mark files for deletion as well ?

how about sql "on delete cascade" ?
whenever a module wants to attach a resource to a field, then it should inform field api about this resource and field api will make sure that the table related to that resource has "foreign key .. on delete cascade" clause ..
the question is what resources are not sql records ? maybe a predefined resources type deallocators can be defined ..

edit: actually "on delete cascade" is not good since some tables may not define foreign keys since they may refer to several different tables.

when an object reaches end of scope, then the destructor is called to cleanup its data .

so the question is what is "end of scope" for modules? isn't it simply hook_disable ?

So that is the problem, if you want to go to concepts, it is more of a caching/weak references issue.

A module does not go out of scope until after hook_uninstall() has run. Disabled modules are in limbo where you can neither use them, nor are they properly out of scope. I want to remove this limbo for Drupal 8 since it is the root cause of many, many issues in the core queue, not just this one (uninstall doesn't work properly, updates don't work properly, lots of other situations where configuration is dependent on modules other than fields).

If a module is disabled, it can be brought back into scope at any time, or it might go out of scope. There is no way to differentiate.

With many modules this is not such a big deal, with modules that integrate with highly dynamic systems like Field API, the module still holds some references (probably circular ones) to objects that are very much in scope - i.e. the field data and the entities they are attached to.

how about sql "on delete cascade" ?

We support MyISAM so that is impossible. We also need to fix this in Drupal 7 without a major API change.

and on theory it is just a bad idea to make a core drupal module dependent on custom or contributed module

This whole issue is about the fact that we have already done this, years ago in practice. However we did it without realising all the consequences, which is why we have critical bugs like this is the queue.

My view is that we should recognise the issue for what it is, patch it up as best we can for D7, then seriously look at interdependencies and the surrounding issues as a priority in Drupal 8, so we can work our way back out of this corner.

The cache flush issue is minor and should be handled in a follow-up. Lets kill this critical now.

i understand you want to close this already, and fix things in drupal 8
but here is my last thought on the matter :
for each purge job there should be a pseudo field data structure that resembles views hook_views_data, so that each item in the structure describes how to join resource table to the field table so that an sql could be constructed to delete the resources that references the field instance .
this way the field pseudo structure can be constructed and saved on module install, then if the module goes away the purge can still go on cleaning things using the structure .

@yakoub
There is no guarantee that the fields are in SQL storage and not in MongoDB or something else.

Plus the problem is not about finding the storage, please (re)read the summary :-)
We don't need the module defining the field type to access the storage, we need it to know if additional cleanup tasks are needed for each deleted value (e.g. delete files for filefield), and perform them.

Sorry folks, still a couple things on the current patch :

- I don't like the explicit "field_purge_batch()" call added after the "field_delete_field()" in forum_uninstall().
That's a convoluted DX pattern for module developers - "on uninstall, after deleting the fields you created on install, don't forget to run an explicit purge pass, so that users with few field data (e.g people just evaluating a module) can disable the field type modules right after that"

That field_purge_batch() should be taken care of within field_delete_field(). Problem is, node type deletion (or, worse, uninstall of an entity type, = deletion of all bundles) can cascade to 10(0)s of field deletions, so we probably want to control that automatic purge by an optional param for field_delete_field().

Besides, the current field_purge_batch() call won't work as intended right now :
- If there are already fields/data pending deletion, there's no guarantee that this purge pass will clear the very data you want to clear. This would require an additional $field_name param for field_purge_batch() to clear the data from a specific field.
- If the field had data, completely clearing the data+instance+field requires at least *two* field_purge_batch() calls.
See the code logic over there : if the EFQ returns no result (all data has been purged), then wipe the instance and field.
Should be switched to : if the EFQ returns less results than the number we asked for, then wipe the instance and field.

All of those could possibly be done in a followup patch ? I don't think I remember how critical that field_purge_batch() in forum_uninstall() was. Could we just remove it for now ?

(Leaving RTBC for now)

There were originally test failures without the field_purge_batch() however it may be redundant after subsequent changes.

I agree with the rtbc here, at least in the sense of this needing feedback on the general approach. Last time this was rtbc webchick was skeptical about doing this at all. At the same time there is no other viable approach put forward by anyone in well over 100 comments.

we need it to know if additional cleanup tasks are needed for each deleted value (e.g. delete files for filefield), and perform them.

so how many options for cleanup tasks are there , or in other words what type of resources besides sql and files can there be ?
the field purge pseudo data can be like array('sql' => array(..), 'files' => array(..) )
and if there are other eccentric cleanup tasks, what percentage of cases would they make ? we would be solving the bug for 90% of the cases .

Removed the explicit field_purge_batch() calls from patch #167 (in forum_uninstall() and field_ui_field_delete_form_submit()). Let's just see what happens.

That's the same rough failure as #943772-151: field_delete_field() and others fail for inactive fields.

@yakoub - it's completely arbitrary, contrib modules can do whatever they want attaching data to field types in whatever storage they want, and field data can be stored in any kind of storage too.

Ugh. OK, re-uploading #167 for now...

Corrected trailing whitespace. No code changes.

how is this issue handled in drupal8, can you direct me where to read about it ?

@yakoub: this code, and most other code in core at the moment, is the same between Drupal 7 and Drupal 8.

for the future, maybe hook_field_delete_field should be cancelled ? and all modules should define
their resources as part of hook_field_info where field module takes care of allocating only the resources that it supports , and any module requiring special resources should be supported by an api similar to "Field Storage API" that should handle all resource allocations .
but for the current version i guess making a dependency requirement is the best solution .

I'm still getting Required by: Drupal (Fields pending deletion), so it seems that field_purge_batch(10); is not quite working properly. Not sure if that's because I did something wrong...

However, this issue has been hanging around for a pretty long time, so I'd rather get something in which we could always clean up after.

Committed and pushed #197 to 7.x and 8.x. I'll mark this fixed for now, and we can cover any other things in a follow-up issue. Thanks a lot for the hard work on this, folks.

Thks webchick.

I'm still getting Required by: Drupal (Fields pending deletion), so it seems that field_purge_batch(10); is not quite working properly

That's probably #190 above. I'll open a followup for this soon.

Meanwhile, as per #167 / #178 / #186, created #1264728: Refresh field 'active' state in module_enable() / _disable() to perform field_sync_field_status() in direct module_enable() calls as well.

Opened #1264756: Inline field purge within field_delete_field() about #190 - but no patch yet.

Additionally, the Issue Summary (far) above pointed UX shortcomings in the approach that eventually got in (forbid disabling field-type modules until fields are deleted and purged) :

"UX during the field purge : lack of control (only happens on cron), lack of feedback (no progress report on the purge running behind the scenes - when will foo_field.module become disable-able ?)"

This could probably be addressed with a "purge everything now through batch API" button somewhere in the UI (on admin/reports/fields, probably). Not sure I'll be able to work on a patch myself right away, though.

My website crashed because of this (and others are too). I was able to go in and manually delete fields from the database and then delete all tables I saw remaining from the module. Thanks for making this critical and I hope to see a fix ASAP. I would fix it myself but I am having problems learning the basics:(

I appreciate all the work thats done, just want to share my experience with this issue.

until this get fixed, i suggest someone writes in the summary what is the "cleanest" procedure to manually delete inactive fields .
maybe the responsible module should be enabled , then delete fields through field ui .

i once again draw your attention to defining new api for allocating additional field resources #200, call it "Field resource api" , this way even if module gets disabled then field can call on "field resource api" to de-allocate each field resource without being dependent on contrib module for that .

So: For the moment, the best practice would be to create separate modules for fields and/or field storage as dependencies, so that they can remain enabled while, e.g., content types which use them can be uninstalled?

Hmm. In fact, maybe enabling a module implementing some dangerous mixture of hook_field_info(), hook_field_storage_info(), hook_node_info(), and hook_entity_info() could generate a warning message.

Followup patches needing review :
#1264728: Refresh field 'active' state in module_enable() / _disable()
#1264756: Inline field purge within field_delete_field()

[edited out, duplicate post]

Since pushed patch from #197 one of my module cannot be disabled because there is a "Required by: Drupal (Field type(s) in use - see Field list)" even if there is no instance of the module implemented field.

(In table field_config_instance' my custom field appear with the 'deleted' field set to 1)

@drikc
As long as it's in that table, it exists (it's just marked for deletion). Run cron so that it gets deleted.

@bojanz
There is no way to 'mark it for deletion' the field belongs to the module in question, the one that cannot be disabled, because of a field dependency on its own field. As it stands you must manually delete the field from the db and then you can disable/uninstall the module.

Cron has no effect because you cannot disable the module, which is required to uninstall, which is when its fields get deleted right?

In a separate issue (because this one is already "fixed"), there needs to be a call for some kind of repair mechanism when already-installed modules break the "Don't do that" rule stated in the summary.

oh lol... im just seeing that "Don't do that" section, sorry somehow I missed that bit.

Then, I've created an issue for installed modules that stuck with the "Required by: Drupal (Field type(s) in use - see Field list)" message.

It summary the need of moving field(s) into a separate module and give a hook_update_N() example to install the newly created field module: #1284332: Module cannot be disabled because Drupal claim "Field type(s) in use"

Added an API change notice.

Also see #1199946-17: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed

Yep. :\ #1331922: Message "Required by Drupal (Fields Pending Deletion)" baffles users

Clarified the "foo" and "foo_field" example.