Features install/revert does not remove additional fields from content type already stored in the db

I'm confirming that I have the exact same problem (running feature_revert from update.php script). I'm using features 6.x-1.0-beta4.

And I might be doing something wrong, but it seems that revert functionality is not working as expected even in non-update.php scenarios. Here's how I tested:
- I created a "test" content type, exported it as a feature. It had only one field, f1
- I created an extra field f2
- I tried to revert the feature, but it said there's nothing to do
- I checked the {content_node_field_instance} table and the field is there indeed

I was able to trace the problem down to the features_get_normal() function in features.module (a new addition in the latest beta, it seems). This function gets the status of each component of the feature by rendering it (with hook_export_render) and computing md5 checksum. However, the rendered feature components are pulled from the feature, and not the database! In my case, it renders only the f1 field, because only f1 is stored in the feature. Hence, it never renders f2, and no difference is found.

Can anyone else confirm this problem?

I have confirmed this. The node info is updated correctly, but when I try to update another site's feature then it is still present. Cache clears do not help and the feature says it is at a default state.

This maybe a cck bug/feature. I.e. require manual interaction to destroy data.

I think the problem is that CCK fields are actually separate components. The current structure doesn't know that there are new fields as a part of the node component, b/c the node component is separate from the field component. There needs to be some way to more directly correlate these child components to the parent component. This may conflict with other issues such as #597320: Add loose dependency detection & addition.

Problem confirmed here as well.
Changing to critical b/c this makes it impossible to use features to sync content types between machines/developers

Subscribing

Subscribing

I'd prefer to have that as an option (to remove the fields) then an always.

Till then, using an update function (http://drupal.foxinbox.org/drupal/code-snippits/remove-fieldsgroup-code) based on the code snippit in http://api.lullabot.com/content_field_instance_delete :).

@hefox

what would for the option the UI look like? One thing I have noticed is that the Features UI doesn't seem to have any space for options.

Not sure, I imagine a checkbox.

Could also not include it in revert and another button "Remove non-specified fields"?

Gotta also take into account how features-revert would work.

subscribing

Being that a single feature does not necessirly define all the fields that a content type may have (you can make features that include only fields, and not the content type, like an optional add on), whatever this is it likely should not be on revert for *a* feature, but more globally; I'm in favour of a page + drush command.

subscribing

Well, here's my current idea, which needs work, but also like review of the idea on how to do it

Adds a new command features-synch that invokes components hook_features_synch to synch the install, in content (only current) case, removes fields that not defined for content types that have fields defined. can be tested via drush features-synch but there is currently no output atm (again, needs work, but want to see people's opinions on the approach)

This is an interesting approach that needs some more thought:

1) This assumes that the only thing that needs to be synched for a content type is fields, but there could be other components of the node that need synching and any patch needs to provide a way for that to work. Otherwise the drush command needs to be renamed to indicate it is only for synching fields, which seems too specific for a Features command.

2) The basic method seems reasonable -- get an array of the fields currently defined for this content type and compare them to the fields created by the feature and delete the ones that are not defined. But this means there is no way to have fields defined in more than one feature -- any feature that creates fields will wipe out any fields created by other features. So it needs to be more robust somehow, if that's possible, so you can define an array of protected fields that will not be deleted. The drupal_alter might work for that if we use it to identify other features that have added fields to this content type to protect them.

I agree this is a critical problem, and this is a step in the direction of a solution, but I'd say it still needs work. I need this functionality for a client project I'm working on now, so I'll be playing around to see if I can identify any clean way to handle this.

One thing I would add is to implement hook_feature_export_options() in both content.inc and fieldgroup.inc to return arrays of the existing fields and fieldgroups. Then we have an easy way to compare the state of the current content type to whatever is defined in the feature. I would do that instead of trying to retrieve the field values in the sync function because it is more generic (and re-usable) that way.

Also the current patch makes no attempt to sync fieldgroups, which is important.

As said, it does need work

My thoughts were that synch should take in what component to synch (content, node, fieldgroups, etc.) and component have a associated hook like others are done (expect not be feature specific). And current command would be synch-all.

But this means there is no way to have fields defined in more than one feature -- any feature that creates fields will wipe out any fields created by other features.

er, huh?

+  foreach(module_implements('content_default_fields') as $module) {

The fields synch functions gather all the current fields defined by any module/feature, then it cleans them up to be in type > fields type array, iterates over that so it will only effect content types that have fields defined for them, then iterates current fields for those content types.

Ie, doesn't care what module defines the fields, just care that content type has fields defined, so is a way to exclude still needed as users can manually delete fields and not use synch, and if so should it perhaps be a command line level exclusion?

OK, maybe I am wrong about whether the patch will detect fields added by other features. I am also wrong about implementing hook_feature_export_options(), since fields are added by the features.node.inc not by features.content.inc.

But still there needs to be some more generic way to detect what needs to be synched that includes things like fieldgroups as well as fields. Maybe use node_features_export to identify all the components that have been added to the node and compare them to those components as they exist in the database?

I'm playing with the idea of adding a hook_features_current_values() to return the current db values for comparison with the values defined by features.

Been chatting with yhahn/KarenS about this and the consensus seems to be against using a drush command for it. What everyone seems to agree to is that when reverting the node component of a feature, any deleted fields should be marked inactive. This way no data will be lost, but the field will no longer be available on that content type. Assigning this to me.

First go at this. I left fieldgroups out b/c I'm not even sure if it's a problem at all. Some of the code is there but commented out in case we determine that it is needed.

I tried the original patch out and ran into a problem. It adds new information to the node export which means no existing feature will work correctly without some sort of update. That seems more complicated than necessary so I played around with this to find a way to activate/inactivate fields without changing the export.

I've tested some fairly complicated things -- several features with shared fields where I come back and remove fields in some features but not others and then later add the fields back and it seemed to behave correctly.

I have concluded we don't need to do anything with fieldgroups, they behave rationally -- fields are added or removed from groups as needed. Groups are not deleted, but groups with no fields don't show up so that should be OK.

Oops, left in some debugging code.

I know there may be some important reason to change the node export, I'll let James chime in on that issue, but it seems to work fine this way.

Hmm, this patch does not apply for me to the DRUPAL-6--1 branch:

patching file includes/features.content.inc
Hunk #1 FAILED at 121.
Hunk #2 succeeded at 200 (offset 2 lines).
1 out of 2 hunks FAILED -- saving rejects to file includes/features.content.inc.rej

Hmmm, try this patch.

Well, I'm leaving this as needs work, because the big problem with the patch is that if you remove all fields from a node type that is defined in a feature, you have no way to revert the fields. I think this is why yhahn and I decided to go the route of adding the fields to the hook_node_info declaration, because we couldn't think of a way around this problem. Certainly open to other ideas.

I went ahead and cleaned up KarenS' patch anyways, in case we think of a workaround using this method. Changes to the patch:

- Renamed content_features_instances() to content_features_default_instances()
- Renamed content_db_instances() to content_features_db_instances()
- Cleaned up white space
- Ensure comments are 80 chars
- Change verbiage from inactivate to deactivate. Inactivate sounded funny to me. :)
- Removed unnecessary $fields = module_invoke($module, 'content_default_fields'); from line 133.

Can you provide an example of something that doesn't work? It works fine for me for numerous complex cases.

I really don't like changing the node export, that means every single feature that already exists will have to be re-created. I really don't think that will be necessary.

Here are steps to replicate the issue I was having with the patches in #24/25

1.) Create a feature that defines a node type with one field on Site A
2.) Deploy that feature to a different site (Site B).
3.) Remove the field from the node type on Site A. Node type should now have no fields.
4.) Update the feature.
5.) Deploy the updated feature to Site B
6.) Navigate to admin/build/features/FEATURE-NAME
7.) There is no button to revert the component, however if you have diff module enabled you can navigate to admin/build/features/FEATURE-NAME/diff and see that there are components to be reverted.

Ah, well saying that Features doesn't know reversion is needed is different than saying the revert won't work. I have all kinds of situations where Features tells me things are overridden when they're not or otherwise is a bit confused about the state of things.

When I remove some, but not all, fields from a content type it works fine -- I have a revert button and it works. I didn't test removing *all* fields.

I'll see if I can figure out why that part doesn't work, but probably not until tomorrow.

Hmm, I wasn't saying the revert wouldn't work, I was saying that you had no way to revert. Sorry if I miscommunicated:

if you remove all fields from a node type that is defined in a feature, you have no way to revert the fields.

I am having the same issue as #23 (q0rban).
I am also on drupal 6.

I have tried the files: features-649298.patch, features-649298-25.patch.

@aronallen Are you sure you're using features 6.x-1.x-dev (http://drupal.org/node/401394)? Or checking out the drupal 6 branch from CVS (http://drupal.org/node/401392/cvs-instructions/DRUPAL-6--1)?

I've refactored q0rban's patch in #20. I'd like to clarify some of the requirements and constraints here.

The initial issue/request is to let Features manage this scenario:

• A feature, foo, has exported fields A and B for content type blog.
• The feature is added to a site where content type blog has fields A, B, and C.
• The feature should be marked as Overidden and the site admin should be given a chance to either
  • Update the feature, which would export field C to the feature.
  • Revert the feature, which would delete field C from her site.

There are concretely speaking, two problems with Features at the moment that prevents this from happening.

1. It has no way to detect that the feature is Overridden in this scenario in a meaningful way. Namely, while it can tell you now that there are fields in the DB not exported, it does not associate it with a component that can be reverted.
2. It currently does not ever delete or mark field instances as inactive.

q0rban's patch in #20 addressed both of these problems using the following methods:

1. Attach some metadata to the node type component in its exportable array describing the fields associated with the node type. This would allow Features to associate changes to fields on a node type directly with the node type component so that this component becomes the entry point for reverting extra CCK fields.
2. Mark fields as inactive on a node type revert if there are such extra CCK fields around.

As described by KarenS in #21 this led to the undesirable consequence of requiring updates to Features using old node type exports to actually make use of this functionality.

I've made the following adjustment to q0rban's patch to address this issue:

1. The metadata attached to the node type component is
   • A simple flag marking the node type as having extra fields. It does not actually track the fields on the node type.
   • Never actually exported to code. It is simply an "internal" marker in Features to allow node types to be reverted in this scenario.

The only remaining problem for me on this patch is that marking field instances as "inactive" is actually quite confusing from the user standpoint. As it stands currently it's very hard to "reactivate" a field, to the point where I'm not actually sure what workflow anyone had in mind. I'm curious whether there are ways to make this easier to understand in CCK or whether we should opt for field deletion in order to simplify the process.

I haven't had a chance to test out this patch, but just from reading it, it looks awesome. :)

Concern has been expressed about de-activating field instances, as that essentially blocks any further usage of that field's namespace. To resolve this, I propose that CCK should provide some way via the UI to delete or re-activate an inactive field instance. Please see #863226: UI element to delete or re-activate inactive field instances and voice your opinion.

Patch committed, with field disabling. http://drupal.org/cvs?commit=397250

I've posted an initial patch here for a deletion UI in CCK for disabled fields: http://drupal.org/node/863226#comment-3244418

KarenS said:

in the Features module we should be sure not to turn back on disabled instances if their field or widget modules aren't available.

I don't believe we're currently doing this check.

Yes, let's come to agreement in the CCK thread about how to best use widget_inactive (if we use it at all) before adjusting this in Features.

Hmm, removing fields from a feature is still a major PITA. IMO, this should be as easy as removing the item from the .info file and updating the feature, the same way all other components work.

subscribing

#37++

subscribing

I'd find 37 to be too unflexiable for my needs; for example, if needing to do stuff with the data before moving it, or, having another module take over the field that isn't enabled yet. I'd be concerned quite concerned with anything that might result in data loss, or make it hard to customize what exactly happens. I need control and flexibility!

I do want a way to just mass disable and/or remove fields.

It's random idea time, one idea I just had was a -content.

features[-content][] = "blog-field_something"

For the general case, when updating the feature if field doesn't exist anymore but did, the item be removed from the features[content] and re-added as -content, basically tracking that the field had existed but should be removed. Then a features-??? on it would remove the field if it exists.

That way it could be adjusted via existing features hooks and manually editing the .info file.

mmm...

features[disable_content][] = "blog-field_something"
..
disable[content][] = "blog-field_something"
..
remove[content][] = "blog-field_something2"

subscribing

subscribe. Having this problem with features 1.0 and a number of content types. 1.0 should have had the patch in it.

Is there a manual-edit hack to work around these issues?

i fixed my issue by making a run-once/throw away type of script that looked like

include_once './includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

content_notify('enable', $my_module);

suscribe, this also affects 7.x release

subscribe

In the case I was looking at, the issue was that the feature had been updated to remove a field on one machine and pushed to another (a fairly normal use case) and thus orphaned fields. We didn't care about those fields and just needed to get rid of them.

We have a number of content types that are a bit broken. Visiting the content type "manage fields" page showed things like this:

The messages were like this:

    * Extracurricular Interests (field_sp_extra_interests) is an inactive Content Taxonomy Fields field that uses a Select List widget.

and they were bogus: There was no longer a field_sp_extra_interests, and we were perfectly happy to get rid of it. No kittens were at risk of being killed. (Please check your kittens before continuing.)

The two ways to fix this:

1. In the database on the offending machine in the content_node_field_instance table, set widget_active=1 for the offending item, then visit the "manage fields" page and click "remove".

OR

2. Delete the appropriate row from content_node_field_instance and content_node_field

(Thanks to q0rban for the hints on how to do this)

AND

3. Weigh in on this patch to CCK which would add a UI in CCK to delete the disabled fields. http://drupal.org/node/863226#comment-3244418

;)

subscribing

subscribing

subscribe

rfay, that first solution didn't work for me, infact it caused various (odd!) errors to display regarding menu items. Anyway performing your second solution worked fine. Cheers.

subscribe

subscribe

subscribe

subscribe

sub

+1

Subs

subscribing, interested in a 7.x fix.

Attempted to port the fix to 7.x.

New to Drupal, so please forgive any errors in code or protocol. The patch is a git patch, but it looks like everyone else's are CVS. Should I be using CVS even though the project pages point to a Git rep?

A couple notes:

* When I find a field that needs to be disabled, I'm disabling the whole field (field_config.active) rather than the field instance. This works in my case because I don't have shared fields, but I assume would cause problems otherwise. I couldn't find a way to disable just a field instance in 7.x; field_instance.widget.active still exists, but doesn't seem to hide the field.

* When you add a new field to the feature and then load the features admin page (http://localhost/drupal/admin/structure/features), field_features_rebuild gets called immediately, which ends up updating the fields to match the new definition even though the user hasn't taken any action yet -- is this a bug in the current code, or am I misunderstanding.

Many thanks!

@dlerman, git is the right thing to do. We've left CVS way behind.

I'm sad to see this lose focus before it goes in *somewhere* though. I'd rather see it get fixed in D6 before it gets scattered all over.

However, without maintainer involvement it will not be fixed anywhere.

field_instance.widget.active still exists, but doesn't seem to hide the field.

This may be as simple as a caching thing. Did you try clearing caches after setting widget.active to 0?

field_features_rebuild gets called immediately, which ends up updating the fields to match the new definition even though the user hasn't taken any action yet -- is this a bug in the current code, or am I misunderstanding.

Yes, this is by design. Some things operate this way, and others don't. It's not a trivial problem to solve, though. Sorry I don't have good news there for you. :/

Thanks for the work on this though! I had assumed this had gotten updated for d7, but I suppose not. We definitely need a maintainer to take a stance and give some direction here.

@q0rban points out this was committed in #34.

I'm a bit baffled because all of my (bad) experiences were after that commit. So while I don't have confidence in where we are at, I'm going to put this to D7 in honor of #62.

Setting to CNR even though #35 expressed some reservations.

@q0rban Yes, changed field_instance.widget.active to 0, refreshed all caches via the UI, and the field still shows up in both the view screen and edit screen. Also tried loading a new feature that has field_instance.widget.active as 0 in features.field.inc and the field still shows up.

Subs

subscribing.

sub

Please help with the revision 6.x-1.0. Patches are not working o this revision.

Marking as a d7 stable release blocker

Can I through out the idea, if instead of inferring that the field should be gone, because it is not in the feature, instead have a way to export that a field should not be there? So when I do my update instead of removing the field from the feature, it would just be marked as NULL and then on revert as the field is NULL in code it would be made NULL in the db as well?

Do we think that would solve the issue of rolling out change where fields are removed?

I raised a similair idea above in #41, and others had opinions against; since then I recall someone else mentioning something like that.

It's my preference to have features only remove data/fields via something specific.

I think it would be interesting if Features had a concept of implementing update hooks. And other .install file madness.

It's ugly, but I can't think of another way to mark faux exportables for deletion than to literally tag it in the .info file. But let's be really clear about it-- no obscure tildas marking the difference between creation and data loss.

yes I would agree no ~

The idea of the info file is to say I want to control this components, and then we let the hook say what the value should be and NULL could be an option

So something like features[content][field_deprecated_field][status] = FALSE

Following the drush syntax for expanding the array structure for more detail.

Another approach might be features[<status>][content][] = field_deprecated_field where status is delete, disable, etc.

I agree whatever syntax evolves should be flexible to cover a number of workflow states. For example, you could allow whatever syntax to define disabled components that need manual activation.

Take a look at this Features Cleanup in my sandbox:

http://drupal.org/sandbox/PaulKimConsulting/1342954

Going to switch this over to a feature request, because really, it falls into that.

I wonder ...how hard it'd be to actually auto generate the hook_update_N and append it to end of .install?

When building features, track what items have been removed.
If faux exportables, add in update function.
Finding what update function to do would likely be a start at _update_7101 and find a non-used function.

Then generate the delete statements (hook for each faux exportables that takes in what needs to be deleted and renders a delete), and append to install (create blank install if doesn't exist).

I'm really curious how hard that'd be, and if it'd be a bad idea/maintenance hell.

My motivations in changing to want this is realization features has to be as easy to use out of box for an average user, but as customizable as possible for an advanced. Auto generated update functions means an advanced user can go and remove or adjust to needs, but for a regular user it'd just work (assuming they know to run update db).

Have features integration require defining a cleanup routine for export, or a callback function that an auto-generated cleanup can utilize in a standard way. E.g., component X passes ID Y to callback C.

Also, don't need to grep the file if checking schema_version from the system table is acceptable.

@Paul Kim Consulting: When looking at your sandbox, it appears that you are removing any content type that is not managed by a feature (along with panels, etc). That is very different than what is being discussed here, which is just removing *fields* within a content type that have been removed from the features.

I like the idea of generating an install update hook to handle this. It's also nice because it would document the change history to the feature "schema" in the myfeature.install file (via the generated update hooks). What might be tricky is dealing with any existing update hooks that might already be defined in the install file (generated manually). Several of my features have existing update hooks.

I'm removing the tag for "D7 stable release blocker". I'm not sure this issue truly blocks a stable release. However, if we decide to do something regarding an API change, this it certainly should be included in any API improvements that should be done (soon) for a release.

Guessing what the next update function, if a feature is following the the proper naming of update functions, shouldn't be hard -- find the current largest, 1 up that.

A general problem is currently features is actually "remaking" the feature both in the UI and with features update, which has the problems. If change that, and instead treat it as really updating the feature via both ui or feature update:

• Keeping any extra files the user has deliberately added (currently lost via features update via ui)
• Removing any files from components that have been removed from this features update (currently kept via drush features update) and update the .install

This means tracking what files/components started compared to ending with.

Really quick psuedo code:

hook_features_remove_render($data) {
  $output = '';
  foreach ($data as $component) {
     $output .= "  whatever_delete($component);\n";
  }
  return $output;
}

 // end of making features
  $output = '';
  foreach ($component_removed as $component => $data) {
    // If component had it's own file defining it, make sure it's removed.
   // Render the remove
     if (has remove render hook) {
      $output .= $remove_render($data);
    }
  }
  if ($output) {
    // Find update hook number,render update hook, grab or make .install, append to end. 
   }

Something along those lines.

I don't consider this a stable release blocker, and not sure this issue itself would cause big api changes (more likely api addition, that mostly relate to components features itself handles), though it is a big patch

@mpotter -- it does fields too, as well as menu items, mini panels, and panels. It uses the diff module to find all removed fields in the feature.

Subscribing

Very much looking forward to this functionality!

Subscribing :o)

PS. Thanks and appreciation to the maintainers of Features! You're awesome!

anandps: there is no longer a need to add "subscribe" comments on Drupal.org ... there is a shiney new Follow button at the top of every issue :)

What became of this issue? It just burned me on a client site today.

I have faced same issue with Features 6.x version

Finally i have managed to remove the inactive field manually using this code manually

include_once './includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
db_query("UPDATE content_node_field_instance SET widget_active = 1 WHERE field_name = $field_name;")
module_load_include('inc', 'content', 'includes/content.crud');
content_field_instance_delete($field_name,$type_name, TRUE);

this has to be fixed in features.

Phew !!!

I am glad I found this issue. I thought I was going crazy trying to figure this out.

So what is the final decision on this feature request ?

Here is my workaround to find fields stored in database.
http://dropbucket.org/node/570

Any news on this ?

FInally going to mark this as "won't fix" until somebody proposes something better.

1) Features will never do anything that might cause content to be lost on your site. This includes deleting fields. Just because you remove a field from a feature doesn't mean you might not still have content stored that would be lost if features auto-deleted it. I've never been in favor of any sort of auto-deletion.

2) Now that fields are split into bases and instances, this gets even more complicated.

No offense to mpotter but I think this decision stinks and I think you are lessening the value of features by not implementing proper field deletion. The alternative to letting features delete is this 32 step process listed here:

http://www.hook42.com/blog/deleting-drupal-field-saved-features#comment-...

Which is ridiculous and could be avoided by just having a confirmation prompt warning before a feature is updated.

We delete information all the time. It's part of proper database and data-structure house cleaning and site building.

I can't speak to the complexity of bases and instances though - but I recently had to write a "cleanup after features" update function. It wasn't hard but it's (that features don't delete) just one more thing to remember, forget, and have to relearn again.

dshumaker,

Could you post your cleanup after feature update function?

Hi @frob,

Here ya go:

/** 
 * Delete unnecessary person and user fields
 */
function mymodule_people_customizations_update_1() {

  $fields = array(
    'field_person_cv', 
    'field_person_website', 
    'field_person_type', 
    'field_user_picture',
    'field_user_about',
    'field_person_position',
  );
  foreach( $fields as $field) {
    if (field_info_field($field)) { 
      field_delete_field($field); 
      $message = 'Deleting field = ' . $field;
      dsm($message); 
      drush_print($message);
    };
  }
}

I agree with @dshumaker.
Not having the an option to delete even if I really really really want to sucks.
I totally understand it is a safety measure and get the idea that Features should not delete content, BUT .. I just presumed (like others) that Features did this. So perhaps if this is a 'Wont Fix' then there could possibly be a note in the documentation somewhere ?

Regarding that blog post link in #94...

32 steps is being ridiculous. Particularly in the context of this discussion.
You don't need to do all those steps to delete a field.

Most of that list is irrelevant in this discussion.

This is really the only relevant part:

You can instead add a hook_update_N function to a custom module and delete the field with code (see field_delete_field).

Sure you have to handle cleanup of anywhere that field was used, like changing views etc. but you'd have to do that anyway, features isn't going to check your whole site for any config that may have used that field and remove it for you because that would be absurdly complicated.

The only downside to features not supporting this is that the alternative options are not pointing and clicking around a site, you have to either write a little bit of code or run a drush command or something.

I will agree though that it needs to be obvious that this isn't supported because people will definitely get tripped up by this.

Still getting this problem. Sad that this is not fixed in Features.

drush field-delete field_field_to_delete --bundle=page -y
drush @sites field-delete field_field_to_delete --bundle=page -y
page could also be any content type.

Given that Config can do this in D8, maybe it's time we accept this into features for D7.