Features does not remove the several known deprecated files

I'm not sure what has been done in the past with this kind of change. I think Features just marked the file as "legacy" and never automatically deleted anything. I'm wary of automatic deletions. But I'd like to hear what others have to say.

maybe just add a "option" for drush FU to drop legacy parts that false by default

The legacy file caused my features with fields to show as overridden, until I removed the file from each one.

I agree with the automatic removal. Introducing a new and better method without cleaning up after yourself doesn't make sense. It's not what users expect. If you do not want to automatically delete, we should at least have a way to tell the user "go and delete example.features.field.inc now".

In general, I think the module could provide a bit more guidance and help regarding the split into field bases and field instances. When you recreate a feature originally created with 1.x, the deprecated Field components really stand out with their bold red letters (which usually means "error"). My first thought (not knowing about the change) was "WTF, did Features deprecate exporting Fields?!" I propose to use a different color to mark deprecated components and to set a message to inform the user how this is handled when you recreate the feature.

I think you should automatically delete the field file. Maybe you can show a DrupaL message (drupal_set_message) to notify users what was done and why. I suppose some people may not like having a file that is under source control automatically deleted by a module. However, if the module doesn't need it, and you delete it, I will notice when I do a git status command. I may wonder where it went, so the Drupal message would help so I don't waste time trying to figure out what happened.

Another concern is that clearly there is code that is still reading the old field file because it thought I had an override. Regardless of whether or not the file is deleted, we might want to make sure there is no code reading this file.

I'm ashamed to admit how much time I burned trying to figure out why I had an override before I figured out it was the deprecated field file. I thought it was being recreated because it had the same date and time as all the other files. But I realized that was because the old file was getting compressed into the tar with all the other files causing it's date and time to change. I'm not certain if it's just OSX (Mtn Lion) that changes the date-time of files when they are compressed.

+1 for deleting the file on a drush fu. It is what I would expect as a Features user.

I don't see any reason why features should be afraid of deleting a *.features.*.inc file. At the very least, it should stop considering that file when evaluating whether the feature is overidden (or is that Diff module's fault?). As a big git AND Features user, removing the legacy file would match my expectations.

@DamienMcKenna #12 patch did not work for me through drush. The *.features.field.inc was not removed.

I just applied this locally against beta2 and updated one of my features and it does delete the features.field.inc file!

@Elijah, what about through drush?

Clarification: I used drush fu feature-name to update the feature, not the GUI.

Me and Mike were talking about this today. Both of us are uncomfortable with features deleting files.

Mike was talking about perhaps it changing it to never including those files; right now it includes based on file matching pattern, not whether the componetes are used in the .info file. So only include component_x if features[component_x][] = item in the .info

It occurs to another way to do this is rename this file, e.g. featurename.features.component_x.inc to old.featurename.features.component_x.inc;

If someone is not using version control and has manipulated the file someway, they could loose information if features deleted it. I've used feature hooks to do dynamic items (like automatically add a field to content types, foreach(content type) $fields[field-cotnent type] = ...).

I'm not sure why you say never loading leads to broken features. That approach would likely mean a change in features_include_defaults (or whatever that function was) that would not include_once a file if there's not a corresponding entry for it in the .info file. So yea, the file would remain there in some cases, but never be loaded so should not cause problems like overriden status. Considering my history of using the hooks dynamically, I'm not a fan of that -- it's also bit too mysterious? No indication in the file itself that it's not being loaded like the rest. Course could update the file to add a comment to the effect or such.

The file name change idea means the file would not be loaded and is visually marked as "hey, something is up with this file". If someone could always rename it again to get it back working if they need to also.

Re #20: IMHO "someone not using version control" should not be a reason to hold back improvement.

Features is not a module for first-day Drupal users who don't know what they're doing. If you're not using some kind of version control by the time you need Features (if not a VCS, then at least backing up your code in numbered or datestamped directories), you're doing it wrong. Features is about generating code, so if you start altering that code, it's only logical that you have to take care when you recreate a feature. Just like Drupal core does not have take care of people who don't do PHP security upgrades, Features doesn't have to protect people who don't use version control.

If there is a way to solve this problem AND take care of people not using version control, then let's do it. If can't do that, let's just solve the problem. Just my €0.02 cents.

So, several problems here:

1) Fields are not removed. We have tried to maintain compatibility with existing Field features from previous versions. I don't want to force people to re-export all their features. So just marking the features.field.inc file as deprecated is not going to work. It is only deprecated *AFTER* you have exported to the new base/instance files. So patch #12 is not the correct solution.

2) The "Delete the Files" debate is the same as the "why doesn't Features delete a Field that is no longer in the export". We've had this debate many times in many other issues. I don't like anything that potentially causes data loss.

I have no problem not including the features.field.inc in the Download tar file. But I won't support deleting the file when using "drush fu" or with the new Generate button.

3) I'd prefer a solution that prevented features.field.inc from being loaded if the new features.field_base.inc or features.field_instance.inc files were detected and loaded. As far as I know, this is done in one place in features_include() in features.module, which is what I'll be testing for a patch.

I am somewhat neutral on which path should be taken but I do think that if the *.field.inc should be what prevents features to revert completely. So, maybe an option would be leave the file but when drush fd featurename that the *.field.inc file does not show up in the diff...

When our organization upgraded to this version of features none of our features would revert completely. After digging around I discovered everything did revert but the *field.inc file was confusing features, thinking that more reverts were needed.

Maybe this type of solution will satisfy both needs?

Fair enough. I would say to get the rename functionality in before the first stable then.

Building on the original idea I propose renaming the old file to the below so people can easily get context about this when they see it:

deprecated-do-node-1959076.featurename.features.component_x.inc

Working on this a bit more today.

One other comment is that the problem with delete or even rename is that it requires proper file permissions for Features to modify those files. It's one thing to add a "Generate" button that may or may not work, but people here in the issue queue seem to be used to using Drush where you have local file access and this doesn't represent some other Features users.

In other words, renaming the file can just cause errors for those users who don't have file permission access (and who normally Download the feature rather than using Drush or Generate). This is why I'm looking more at what hefox suggested in #18 which is to only include files listed in the *.info manifest.

OK, here is a patch for testing. It does 2 things:

1) It prevents including any *.inc file that is not in the *.info list and also requires the feature module to be enabled. Previously it was calling module_load_include for every combination of component and module, which resulted in a lot of unneeded file_exists tests and also caused potential problems from disabled features and deprecated components.

2) When Downloading the tarball, deprecated component files are excluded. This means if you happen to name a file "features.field.inc" it won't get added to your tarball. So don't do that.

This seems to work really well in all my testing. The only potential side effects are if somebody is somehow depending upon Features to include it's own *.inc file that happens to be named the same as a component export. Since people shouldn't be doing this, I think this is pretty safe and probably cleans up some other misc bugs with files getting included when they shouldn't.

Here is a better version of #26.

This patch also adds the check for superceeded components to the old deprecated file checker so it will display a message reminding you to delete the old features.field.inc file.

Also added a conditional so that it's only checking the file system for deprecated files when actually exporting the code. I don't think Features is calling features_export_render() in any other cases, but if it is, checking the file system would be bad for performance.

OK, more testing and this is looking good. Passed automated tests, so committing this to 218270d so I can roll a new RC release and get people testing it.