Problem/Motivation

Fields exports in features combine in a single export what are really distinct forms of data: the base field definition and the field instance information. Therefore, field definitions are duplicated in multiple places, one per field instance export. This duplication produces several difficulties including:

  • To make a change to the field definition, it is necessary to update multiple field exports rather than just one.

Proposed resolution

The current patch separates field exports into two parts: field_base and field_instance. New export will contain these new component types, but existing legacy features are still supported.

The admin UI will show "deprecated" components in red but without any checkboxes. This shows that the existing export contains a deprecated component, but doesn't allow you to select them for exporting into new features.

Existing features continue to work and do not get marked as overridden. In the UI the existing Field components get marked as deprecated, and the new field_base and field_instance components are auto-detected. Downloading or updating the Feature causes the new field_base and field_instance exports to be written and the old "field" components no longer are added to the *.info file. The new feature will be marked as Overridden until the previous features.field.inc file is removed from the export directory.

Under the new approach, fields base definitions typically will be exported to separate features than field instances. Features containing instances of fields provided by a different feature will be dependent on that feature. When features are regenerated, it may be necessary to refactor them to manage the new dependency relationships.

Remaining tasks

The patch is currently marked RTBC and no issues have been identified.

An outstanding task is update instructions for feature maintainers, instructing them that their existing features may need to be refactored when they are regenerated. Draft:

Version [release version containing patch] of Features 2.x includes an important change to field structure that may require reworking existing features when they are regenerated. Previously, field exports contained both base field definitions and field instance information. Now field exports are separated into two parts: field_base and field_instance. Under the new approach, fields base definitions typically will be exported to separate features than field instances. Features containing instances of fields provided by a different feature will be dependent on that feature. When features are regenerated, it may be necessary to refactor them to manage the new dependency relationships.

For example, if a set of features currently includes several features (myfeatures_blog, myfeatures_event, etc.) with content types that have a taxonomy term reference field field_tags, when regenerating the features it will be necessary to decide the best place to put the field_tags base definition. The definition won't typically be suitable to go into one of the existing content type features, since there is no dependency relationship between them. For example, often it will not be appropriate to make myfeatures_blog dependent on myfeatures_event. Instead, the field base might go into a general feature like myfeatures_core or perhaps a specialized feature like myfeatures_tags. Either strategy might introduce new dependencies between features - for example, myfeatures_blog might have a new dependency on myfeatures_core or myfeatures_tags. In this case, especially if the features are already in use on multiple sites, an update function might be needed, along the lines of the following:

/**
 * Enable the Myfeatures tags module.
 */
function myfeatures_blog_update_7000() {
  if (!module_exists('myfeatures_tags') && !module_enable(array('myfeatures_tags'))) {
    throw new Exception('This version of Myfeatures blog requires the Myfeatures tags module but it could not be enabled.');
  }
}

User interface changes

When exporting features, users will see separate components for "Field base" and "Field instance".

API changes

Adds a new API key called "supersedes" which can be used to mark an existing component as "superceded" by a new component. In this case, both field_base and field_instance "supersedes" the original "field" component. See the hook_features_api at the top of features.field.inc for this.

Original report by dasjo

as far as i can see, features always exports fields info + field instance info together.

from my point of view, this should be separated in order to allow for example a base feature to define some fields and sub-features apply the fields to content types via field instances.

this would also prevent duplicating field definitions across multiple field instances.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rickvug’s picture

Having the same issue here. As a stop gap I'm doing to try manually modifying the feature export to only define the field itself. At that point I'll try to add instances of that field to a content type and save that into a separate feature.

rickvug’s picture

Quick update, it looks like Features chokes if you manually remove the instance information from the field definition. Will need to keep experimenting with this.

wjaspers’s picture

subscribing.
I tried commenting out the field[] information in the .info file, commenting out the field information in features.field.inc, and combinations therein, here's what I wound up with:

commented .info commented features.field.inc result
No No Features reports a conflict. Unable to install either feature.
Yes No Features doesn't report a conflict. Enabling the feature doesn't install the fields.
No Yes Features reports a conflict. Unable to install either feature.
Yes Yes No conflicts to report; but, No fields will be installed.
yched’s picture

Features D7 currently treats fields as it did for CCK D6, where the $field definition arrays mixed properties specific to the field in a given node type, and properties common to all the field instances (name, cardinality, type, etc).
This last part gets replicated in each exported definition, which can lead to conflicts (e.g hand-edit one of those properties in only one copy and "revert" the feature, the change affects the field as a whole and the other copies become stale, i.e are seen as "overriden").

The Field API D7 cleanly separated $field and $instance structures, which is much cleaner conceptually and API-wise. But the definitions exported by Features D7 are composed of a 'field' part and an 'instance' part - meaning that the 'field' part is duplicated for each exported instance, and can get out of sync just like it could in D6.

Just pointing that out for now, I don't have a technical answer on how Features could / should handle that layered notion of one $field with several $instances in D7. Solving this will also be an interesting topic for D8's "Configuration Management", for that matter.

hefox’s picture

Assigned: Unassigned » hefox
hefox’s picture

Assigned: hefox » Unassigned
Status: Active » Needs review
FileSize
13.36 KB

Okay, the patch introduces two new components field_base* and field_instance. Field base is the base global configuration for the field, field_instance is the individual instance.

There's some logical issues; you can export a field base without an instance; what does that lead to? There's a suitable use case, as may want to have 'field_whatever' exported in a base module, then have each instance exported in some child modules (I defiantly have that use case), however it might be confusing for new users. Not sure how to handle that.

Also has some possible order issues (already existing with the user ones -- ie permissions and roles) so not sure if required for this issue to fix it (I've seen several issues that touch on this, but haven't found the one that was specially about this). field_base is alpha before field_instance, and an enabled dependency module should go before a sub, so should probably run in mostly correct order.

*field_base being called field_base is sorta a BS; made it distinct so easily can be string replaced if someone has something better. field_config is the key that is mostly used in core.

L-four’s picture

@hefox
Creating a features seems to work fine but reverting and installing don't seem to do anything.
I am using Features - 1.0-beta6 and trying to export a file field.

hefox’s picture

Assigned: Unassigned » hefox
Status: Needs review » Needs work

Prob messed the rebuild hooks

hefox’s picture

Status: Needs work » Needs review
FileSize
18.17 KB

Yea messed the features instance one.

Updated, tested with both a fresh site install and features tests (new patch includes updating simple test)

hefox’s picture

Missed 2 files in patch

hefox’s picture

Assigned: hefox » Unassigned

(Once a patch is needs review, I believe assignment is suppose to be removed)

mrfelton’s picture

Status: Needs review » Needs work
FileSize
21.12 KB

I like this patch. This kind of separation makes perfect sense to me and is something I have been looking for in pretty much every features based site that I build. I have updated the patch so that it applies cleanly to 7.x-1.x.

Problem that I found is that if you define a field, and add the base component to one feature, and then add a content type to another feature that uses this field, both the base and the instance are automatically included in the new feature, where as what should happen is the instance is included along with a dependency on the module that defines the field base.

ezeedub’s picture

Should something like this be included?


/**
 * For backwards compatibility
 */
function features_field_load($identifier) {
  return features_field_instance_load($identifier);
}
kclarkson’s picture

I found this issue from http://drupal.stackexchange.com/questions/22174/how-do-i-resolve-this-co...

The image in the link above suggest that you create features for two different content types but put the fields and config in a separate feature that is required for the content types.

Are the patches listed about designed to satisfy this model?

Basically how should I use features if I have multiple content types using the same fields?

My main problem is that I am getting the "conflict" error for my content types that use the same field that is a taxonomy term reference. This is used to sort all of my content types by taxonomy.

Thanks

hefox’s picture

kclarkson’s picture

I applied the patch #17 from #1489456: Node type features with fields defined in other features show as overriden but it did not remove my conflicts of fields.

So for now I am forced to keep content types that share fields within the same feature. This will work for now but obviously it would be nice to have separate features that have fields that are shared throughout my features without seeing the conflict message.

gilgabar’s picture

Updated the patch in #12 to work with the current dev release.

This appears to work well. I have had no problems creating new features with this or importing those features. The only issue I have seen is with updating legacy features. It doesn't cause errors and works well for the most part. After you recreate the feature with field_base and field_instance components it will still leave behind the field components in a my_module.features.field.inc which will cause the feature to say that it is overridden. Deleting that file fixes the problem. So some effort should be made to do a better job of cleaning up old features on export. Otherwise this is great. It makes it much easier to organize features with shared fields in a sensible way.

kclarkson’s picture

Status: Needs work » Needs review
mpotter’s picture

Status: Needs review » Needs work

I think this still needs a bit more work. As mentioned above, it probably needs to clean out any previous features.field.inc file when no longer needed.

Also, I'm still not convinced that this is the best way to handle this overall. While I see the rare need to separate the field_base from the field_instance, for most users this just increases the complexity of Features and adds lots more components to your exportable.

I personally would like to retain the existing "field" exportable and then add the base/instance stuff as an "advanced" setting that most users could ignore. Maybe the patch already works this way (I'm sure you'll let me know), but let's ensure that both the old "full fields" and the new "base+instance" versions interoperate properly so people can choose which they want to use.

We did something like this in the Features Override module where you can export an entire override, or use the "advanced" item selection to only export a partial override.

ezra-g’s picture

While I see the rare need to separate the field_base from the field_instance, for most users this just increases the complexity of Features and adds lots more components to your exportable.

I personally would like to retain the existing "field" exportable and then add the base/instance stuff as an "advanced" setting that most users could ignore.

The importance of this feature varies by the use case. It might seem rare to you if you're exporting a single-site's configuration. However, this is an essential ability for Drupal distributions and other contrib projects that want to re-use fields across content types.

Retaining the existing field exportable along with the new approach proposed here would compound the complexity we'd be introducing: We'd have 2 different ways of exporting the same information (even more confusion), and the work to polish off this patch would increase significantly.

I suggest that the proposed approach be accepted, providing it's revised to include removal of the deprecated field files.

ezra-g’s picture

Also, while this is somewhat a feature request, the current behavior is arguably a bug, because as yched points out in #4:

...the definitions exported by Features D7 are composed of a 'field' part and an 'instance' part - meaning that the 'field' part is duplicated for each exported instance, and can get out of sync just like it could in D6.

ezra-g’s picture

Issue tags: +commonslove

#17 no longer applies to 7.x-1.x-dev.

ezra-g’s picture

Status: Needs work » Needs review
FileSize
20.27 KB

Here's a re-roll of #17 with "{$module_name}.features.field" added to the list of deprecated files that generate a warning.

I did a brief functional test re-exporting an existing pair of features and separating the field base from the field instance. The features show as default, and I'm prompted to remove a deprecated $module.features.field.inc file.

hefox’s picture

Status: Needs review » Needs work
+++ b/includes/features.field.inc
@@ -35,48 +57,55 @@ function field_features_export_options() {
+      $export['dependencies'][$field['storage']['module']] = $field['storage']['module'];

Missing part of checking not field_storage_default (Think that got added recently)

+++ b/includes/features.field.inc
@@ -35,48 +57,55 @@ function field_features_export_options() {
+ ¶

white space

+++ b/includes/features.field.inc
@@ -85,31 +114,58 @@ function field_features_export($data, &$export, $module_name = '') {
+    if ($field = features_field_base_load($identifier)) {

For better code readability, may want to rename this variable to $field_base (and $field_instance for loading instance) everywhere where relevant.

+++ b/tests/features_test.info
@@ -11,7 +11,8 @@ dependencies[] = views
-features[field][] = node-features_test-field_features_test
+features[field_base][] = "field_features_test"
+features[field_instance][] = "node-features_test-field_features_test"

Missing the addition of the new files? (git add .; git diff --cached)

I think something needs to be implemented to export current field exports to field_base/field_insert on features_update, but not sure which hook would be best. probably a new fields_features_export()

ezra-g’s picture

Thanks for the review!

I think something needs to be implemented to export current field exports to field_base/field_insert on features_update

In my testing, re-exporting a single feature did this automatically.

ezra-g’s picture

Here's a re-roll of 23 with the missing files included.

erikwebb’s picture

Status: Needs work » Needs review
anjjriit’s picture

will be this patch included in next dev version ?

girishmuraly’s picture

Status: Needs review » Needs work

Re: #26

+++ b/includes/features.field.incundefined
@@ -206,7 +286,7 @@ function field_features_rebuild($module) {
-function features_field_load($identifier) {

Oops looks like Field collections module needs the old function name. I got:

Fatal error: Call to undefined function features_field_load() in .../modules/contrib/field_collection/field_collection.module on line 1381

SocialNicheGuru’s picture

I can verify #29

Edit: update field_collection to use: features_field_instance_load instead

hefox’s picture

Could you upldat the patch for field_collection here for anyone else?

Miss Swan’s picture

Nice, this patch also works on the 7.x-2.x version!

impleri’s picture

I've tweaked this patch and made it work as an external module (which I'll be building off of eventually): https://github.com/impleri/featuresets

A few things I noticed: the field_instance revert code needed some work (e.g. it had calls to field_create_field instead of field_create_instance, there was references to $field_config which did not exist). I'm interested in developing a way to have shared feature *instance* definitions, so this was a great launching point for me.

kclarkson’s picture

impleri’s picture

Not at the moment, but it's something I'll look into since it's an aspect of the feature sets module concept.

nedjo’s picture

@impleri: could you point to the places in the patch in #26 that need fixing and/or upload a new version with the fixes you made? Thanks!

jrbeeman’s picture

Status: Needs work » Needs review
FileSize
23.59 KB

I've re-rolled the patch against the latest 7.x-1.x and with a small change to address the issue reported in #29, which I also ran in to. I did this by creating a wrapper method with the legacy name, but others may have an opinion on the best way to do this.

SocialNicheGuru’s picture

I am trying this separation of base and instance.

I am not sure if this is the right place for this but here I go.

I created a feature based on a field_collection field.
I want to be able to add this collection and all feature values to any content type that I want

I can enable the feature just fine
the non-field_collection fields that are in the field_collection do show up on the reports/fields page and are available to the system
But the field_collection does not show up on reports/fields
When I goto a content type and try to add the field_collection field that I created as part of my feature, it is not available as part of "Add existing field".
if I try to manually add it I get: "The machine-readable name is already in use. It must be unique."

If I enable another feature that has an instance of the field_collection feature, it will magically appears.

any help with how to enable the field_collection in a feature would be great.

jrbeeman’s picture

I can verify the issue reported by SocialNicheGuru in #38. This isn't just an issue with field collection fields. I recreated by:

1. Install Drupal 7 using Standard install profile and enable Features (with this patch).
2. Add a long-text "Deck" field to the Article content type (field_deck)
3. Create a new Feature (e.g. example_field_deck) consisting of just the Field base for field_deck
4. On a fresh install of Drupal with Features and this patch applied, enable your Feature (example_field_deck)
5. Attempt to add a new field to the Article content type. You won't see your exported field in the list of field types (makes sense) and when you try to add a field with the same machine name, you'll get an error. "The machine-readable name is already in use. It must be unique."

I'm not sure how we solve this, but I'm also not super familiar with the options we have. We aren't technically exporting field types, per se. We're exporting field base definitions. This currently results in a chicken-and-egg problem if I want to reuse a field base from another project.

nedjo’s picture

The issues reported in #38 and #39 are inherent to how Drupal's Field API works.

Yes, (abstract) field definitions are stored separately from (specific) instances. But the field UI presents as options only fields that have one or more instances. And the default properties assigned to a new instance are based on those of an existing instance.

So, while it's confusing for an end user, what's described in #39 is the expected outcome. An abstract field definition in a feature can be used only programmatically (e.g., to create an instance). Only once at least one instance is in place will it be possible to create further instances via the UI.

jrbeeman’s picture

Status: Needs review » Reviewed & tested by the community

Per @nedjo in #40, if this is expected behavior then I'd argue this is RTBC. I've verified against both Commons and a couple of my own install profiles containing this type of architecture. The UX / DX issue around getting to these field base definitions is a separate issue and one that would likely require core work to address.

tobby’s picture

I tested the patch in #37, and it worked for me. I did have to clear cache after patching Features for the new field_base and field_instance fieldsets to show up in the Features admin UI. Once I did that, I was able to recreate my features with no problems.

I should note that I also tested with a feature created with Features 1.0, upgraded to 2.0-beta1 (since that's now the recommended version) and then patched Features. Recreating that feature worked just fine.

mpotter’s picture

I also tested the patch in #37 this morning. The only downside is that any existing feature with fields is marked as overridden, which is to be expected. But the features still work without updating, and updating them properly marks the old features.field.inc file as deprecated and seems to work great.

I'll try to roll this into the 2.x-dev branch this afternoon and will look at making another Beta release next week when I can examine some other patches in the Issue queue.

Thanks for the work on this one...it's pretty complicated but will help a lot with distributions.

mpotter’s picture

NOTE: The patch in #37 works, so feel free to reference it in your *.make file as needed for your project or distribution. I am only marking this as "Needs work" to address the concern of existing Features being marked as Overridden.

Sorry for derailing this, but I'd like to still work on this patch before committing it. The fact that this patch causes existing Field features to be marked as overridden is a bad side-effect for many people, especially people who are using existing distributions. It basically requires the distribution owners to update all of their features and that is something that could prevent many people from adopting this newer 2.x branch. I've tried very hard to maintain high levels of compatibility with new releases of Features and I think with a little more work we can improve this patch to address this.

What I would like to do is retain the existing Field exportable for existing Features, but hide it from being used for new exports. To make this more general and help with future Features updates, I'd like to add the concept of a "deprecated" export that is placed in a different area of the new UI (within Advanced) so advanced users can find it if needed but otherwise new exports would use the new separate base/instance exports. This would allow existing Features to maintain their default state.

I've got some ideas on how to implement this fairly easily, so I'll try to work on this patch and post another version within a week or so. But anybody else with an idea on this is welcome to contribute also.

mpotter’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Reviewed & tested by the community » Needs work
ezra-g’s picture

What I would like to do is retain the existing Field exportable for existing Features, but hide it from being used for new exports. To make this more general and help with future Features updates, I'd like to add the concept of a "deprecated" export that is placed in a different area of the new UI (within Advanced) so advanced users can find it if needed but otherwise new exports would use the new separate base/instance exports. This would allow existing Features to maintain their default state.

Ok, but I encourage us to consider a much simpler alternative: Simply advise users upgrading from 1.x to 2.x to re-export their features. This can be easily accomplished with an [edit] drush fu.

ezra-g’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Needs work » Reviewed & tested by the community

I wanted to elaborate on the reasons why I feel this patch should be committed, with the need to re-export features documented and announced in advance of the next official Features 2.x release.

- Given the amount of code that the patch touches, not committing it to dev increases the risk that other patches will need to be re-rolled once this one is committed.

- It's a common convention in this scenario (maintainer agrees with the patch but wants to document or enhance upgrade process) in both core and contrib to commit large, progress-blocking issues to the dev branch and file more manageable smaller followup issues. [edit] Given how much of key Features code flow and the length of time that this issue has been open (just under 2 years) it seems like we've reached the point of the issue being "large."

- The problem that this issue solves (duplication field base/instance definitions, limited flexibility for distros/sites that have features that build on one another) is a larger problem than the followup issue would be, meaning it's a net improvement, even before the followup issue is filed.

- The followup issue would best be fixed with documentation telling existing distro/site owners to re-export their features. This is a proven approach, used for example when Views updated its API. The scenario with Features is actually less disruptive than the the introduction of hook_views_api() because the present change is happening before Features 2.x even has a full 2.0 release and the solution for distro/site maintainers is simple: re-export your features when upgrading from Features 1.x to 2.0. Given that we're at Features 7.x-2.x-Beta1, it seems like we have a decent amount of time to get the word out about this change.

The alternative proposal would add avoidable complexity to the Features module, in my opinion without a clear net benefit: How would this proposed added complexity be better than simply advising distro/site maintainers to re-export their features when upgrading from Features 1.x-2.0 in order to avoid having those features show up as overidden?

nedjo’s picture

@ezra-g: Could you quickly sketch out just how this patch is used in a set of features (e.g., for a distro)? How are you using it in Commons?

I'm wondering particularly about dependencies. Currently, with fields + instances exported together, it's possible to have different features provide identical fields without creating any dependency relationships.

Here are a couple of scenarios:

Scenario 1: you have three features (example_blog, example_event, and example_story) and each provides a single content type that needs to have a field, field_image (an image that accompanies a piece of content), attached.

Is it necessary to add the field definition to one of the three, while all three get field instances? Do the other two features become dependent on the feature that has the field definition? In this case, how do we determine which of the three should get the field definition? Or do we need a fourth feature to be a repository for the abstract field?

Scenario 2: you have a feature that provides generic mapping functionality without providing any content types or field instances (example_location) and several other features (example_event, example_organization) that provide entities that could be enhanced by locational information.

If example_location includes the field definition for field_location, and instances of field_location are added to example_event and example_organization, are the latter two dependent on example_location?

SocialNicheGuru’s picture

To answer #2:

I created a feature
sng_field_location
- it is a base field field_location
- this field also has all I need for geolocation of a location field

Now any feature that defines a content type that might need location will have sng_field_location as a dependency, but only need to define an instance of the field_location.

I am finding that creating features for fields that will be reused by many different, non-related features is a great way to go. Especially if you enhance the fields or need strongarm variables set for a feild like sng_field_location

nedjo’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
Status: Reviewed & tested by the community » Needs work

Restoring the status that looks to have been inadvertently reset in #46.

@SocialNicheGuru: that's what I imagined. So it sounds like this change would require not simple regeneration but, in many or most cases, significant restructuring of existing sets of features.

It's a very common feature set development pattern to have numerous features that share a given field without any dependency relationship between them. In my scenario 1 from #48, it looks like we would have to significantly restructure these feature sets, pulling the shared fields into one or more abstract features that other features depend on.

Beyond the work of restructuring, it looks like this patch will increase the challenges to interoperable features and apps. It's already challenging to build a set of features that doesn't have conflicting dependencies with features from another feature set. Add dependencies of field instances on abstract features that provide the field definitions and - because practically all features include one or more instance of a more generic field (like an image field) - it will become prohibitively difficult.

The interoperability issue might be partially addressed if we could add field instances without creating a dependency, with the result that the field is created if there's a feature present that provides the field definition but not if there isn't.

Neither the need to rework features nor new barriers to interoperability are necessarily reasons to nix this patch, but they do complicate the upgrade path and open up new challenges.

ezra-g’s picture

Beyond the work of restructuring, it looks like this patch will increase the challenges to interoperable features and apps. It's already challenging to build a set of features that doesn't have conflicting dependencies with features from another feature set. Add dependencies of field instances on abstract features that provide the field definitions and - because practically all features include one or more instance of a more generic field (like an image field) - it will become prohibitively difficult.

I don't understand the perspective in this comment. The patch here makes it significantly *easier* to create interoperable features because it no longer conflates field instances and field definitions. Dependency management is a challenge in general, but I don't see how that's specific to this case, nor how that broad challenge blocks this issue from moving forward.

because practically all features include one or more instance of a more generic field (like an image field) - it will become prohibitively difficult

This is inaccurate. With the present patch, Feature modules that define bases would *not* also introduce field instances.

dasjo’s picture

if i understand nedjo right, he is worried how to deal with / upgrade say two features A & B that share fields but don't depend on a common parent feature. the old approach duplicated the field definition across both features. after applying the patch, would you need to export the field definition separately and therefor create a dependency between A and B or have to introduce a parent feature C that holds the field definition?

SocialNicheGuru’s picture

I think this patch is great.

I just outlined one way that it might help people develop in the future. I happen to be using it that way now.

However the other scenerio where one feature defines a field and others use it is very viable.

Currently if two features use the same field like field_location, they both will define field_location base:
features[field_base][] = field_location

This will conflict so one of the features will not be able to be implemented.

With this patch, one of the features can define field_location base and the others will have just an instance of it:

Feature 1:
features[field_base][] = field_location
features[field_instance][] = node-blog-field_location

Feature 2:
features[field_instance][] = node-event-field_location

That way, they both can be enabled.

this means very little will have to be done for individuals.

nedjo’s picture

@dasjo: Yes, that's the way that this patch would necessitate refactoring of existing features. Not necessarily a bad thing, but a factor that will complicate the upgrade path and impact existing sites and distros, as is true any time you switch up dependency relationships or create new features.

@ezra-g: The interoperability issue is one I've described previously in a blog post on interoperable features (see the section on Dependencies) and a Linux Journal article on interoperable features.

Definitely the patch here has potential advantages within a feature set (e.g., one distro's features). But between feature sets (the features of different distros), it could increase barriers.

Currently, it's possible for mydistro_blog and yourdistro_event to both define a field_image without any dependency implications or, hence, any impact on compatibility. After this patch, so far as I can tell, mydistro_blog will be dependent on mydistro_base (or mydistro_image, or whatever), which supplies the field definition for field_image, while yourdistro_event will require yourdistro_base. [Edit] mydistro_base and yourdistro_base will be incompatible with each other, since they define the same component. [/Edit]. Therefore, the mydistro and yourdistro features will have new incompatibilities.

Are you seeing this differently? How are feature dependencies working within Commerce after this patch?

impleri’s picture

Another vote here for the patch. One of my reasons for supporting it can be seen in the following example:
In one of my sites, I wanted to switch my usage of field_date from the vanilla date to a UNIX timestamp. With the patch, that means updating one feature export/module rather than, say, three. The same goes with changing the cardinality for a field from 1 to unlimited.

While @nedjo is correct that there would be dependency issues if two modules are trying to define the same field, however that's avoiding the greater issue that two feature modules are defining the same field. If mydistro_event has a cardinality of 1 and yourdistro_event has a cardinality of -1, the two are incompatible, regardless of this patch. However, the existence of mydistro_override_event in 2.x (from what I gather in the BADcamp 2012 presentation) handles the issue regardless of this patch. With the featuresets module I made in #33 based on this patch, I'm able to manage field definitions and instances over numerous modules without needing to re-export every single one every time I make a small change.

If anything, this patch simply needs more work to interact with the override process in Features 7.x-2.x. In fact, I think that would make possible a second feature: defining a 'base instance' once and use the override process to adapt individual instances. For example, have a single field base and instance defined in the mydistro_page module (e.g. field_image and node-page-field_image) can be re-used in mydistro_event module (e.g. node-event-field_image) using the instance information from the first and overriding only what is necessary (e.g. the widget weight). In other words, I would prefer not to identify which widget or display setting I'm using for each instance if they're the same.

impleri’s picture

Status: Needs work » Needs review
FileSize
25.5 KB

This is my first try at rolling a patch. It's against 7.x-2.x. I've tested it manually and in simpletest.

nedjo’s picture

While this patch as is will increase conflicts between different features sets, without it the issue of conflicts arising from generic components in features already looms large. So I opened #1909770: Define best practices for handling generic (reusable) features components to try to address the issue of how to manage generic components (like field definitions) and facilitate interoperable features.

mpotter’s picture

Status: Needs review » Needs work

Sorry I seem to have derailed this issue further than I intended.

1) First, going back to "needs work" because #56 is just a re-roll and does not address the overridden issue with existing Field exports.

2) While I think #1909770: Define best practices for handling generic (reusable) features components is interesting, that still doesn't directly address what I think we need to move forward with this. All I'm asking for is a better way to deprecate an existing feature export without having existing features need to be re-exported. Basically setting up a different status tag for deprecated and not overridden. Overridden implies that there is a difference between the DB and code. For a deprecated feature, there is *not* a difference between the DB and code, there is just a difference in how the feature will be exported if updated in the future.

3) I do not disagree that this patch is important. I'm not looking for use cases or justification. I fully understand the usefulness and importance of this patch and am giving it a very high priority with my current busy project schedule. I just want it done a bit better.

4) While "other modules might do this", etc, those arguments hold no weight with me as *this* module maintainer. My philosophy is that normally feature export formats would not change between minor version updates. In other words, normally I would postpone a dramatic change like this patch into the 3.x version of Features, and not make this change between 2.x "beta1" and "beta2". It's because of the importance of this issue that I'm even considering making this change in the current beta release. And the only way this kind of big change will make it into a new beta is if we better handle the backwards compatibility issue.

5) I still intend to work on this myself (probably today) and will open another issue thread for a patch related to improving the "deprecated export" functionality of features. Once that is done, then it should be relatively easy to re-roll the patch in this issue against the new API.

impleri’s picture

@mpotter, I'm playing with making the old-style field still 'exportable', though not through the UI. The idea I'm working with is to show older feature modules as not overridden, and piping in the split-style on export. I'll try to get that included in this patch by the end of the week unless you push something new before I get the patch rolled out.

Also, I did not realise that you were treating the deprecation issue as one between betas of a major version. I think most of the people here wanting to see this treated it as a difference between major version (1.x and 2.x since there is not gold 2.0 yet).

mariomaric’s picture

Regarding #58.3 + #59, some usage statistics:

Release Jan 27, 2013 Jan 20, 2013
7.x-2.0-beta1 3,850 2,821
7.x-1.0 58,183 58,321
impleri’s picture

Rolled a patch which seems to work on my test environment. It'll consider [module].features.field.inc good enough for diff checking, export, and revert; but fields does not show up by default in the UI. On exporting, it automatically includes field_base and field_instance (if they're not already included elsewhere). Would be useful if exporting it would automatically uncheck a given field if the matching field_instance is checked.

impleri’s picture

Status: Needs work » Needs review
mpotter’s picture

OK, here is the patch. I was originally going to try and split the new method for deprecating a component from this field_base/instance patch, but realized that in order to test the new deprecated component you'd need the field_base/instance as a test case anyway. So here is everything as one large patch.

The changes to support backwards compatibility with "field" weren't too hard actually. Here is what I did in this patch:

1) The previous patch from #56 has almost been copied verbatim, but rather than *replacing* the existing "field" component, I've simply added all of the code to the beginning of the features.field.inc file.

2) I added a new API key called "supersedes" which can be used to mark an existing component as "superceded" by a new component. In this case, both field_base and field_instance "supersedes" the original "field" component. See the hook_features_api at the top of features.field.inc for this.

3) Modified the features_export_render and features_export_prepare functions to prevent them from exporting deprecated components during the final export to code (when $reset == TRUE)

4) Modified the admin UI to show "deprecated" components in RED but without any checkboxes. This shows that the existing export contains a deprecated component, but doesn't allow you to select them for exporting into new features.

5) Modified the "node" and "taxonomy" exports to add field_instance to the "pipe" of related components. This allows the field_instance components to be auto-detected for export when you add a content type to a feature, just like the previous "field" components were handled. Not sure why this was missing from the original patch, but it's important for novice Features users who select a content type and want to export all of the fields in that type. With the new UI you can always de-select any field bases or instances that you don't want to export

I did a fair amount of testing with both existing features and new features. Existing features continue to work and do not get marked as overridden. In the UI the existing Field components get marked as deprecated, and the new field_base and field_instance components are auto-detected. Downloading or updating the Feature causes the new field_base and field_instance exports to be written and the old "field" components no longer are added to the *.info file. The new feature will be marked as Overridden until the previous features.field.inc file is removed from the export directory.

Everybody should please test this new patch. As soon as it gets community testing I'll commit this to the repo.

NOTE: SO ANNOYING. Got a validation error uploading the patch, so will try again.

mpotter’s picture

OK, here is the successful patch. Must have been a network glitch.

impleri’s picture

It's working for me. Woohoo. I'm not sure, though, if using red for deprecated items is best since that's the same colour as conflict. Perhaps a strikethrough or greyed out to ensure people don't think it's a conflict that they must do something about?

mpotter’s picture

Well, I didn't really want to invent another style (I don't like strikethrough myself because it's hard to read). What I actually used is "Auto-detected Conflict" since if the deprecated code was exported it actually *would* cause a conflict. But if people really think it needs yet another style, feel free to post a new patch. But I *want* people to notice the deprecated export so that they will update/export the new version asap.

dddbbb’s picture

'Deprecated' sounds a lot like 'defecated' so how about brown? :)

I'm sorry...

In all seriousness, I've been following this thread for a little while now and I'm all for this patch. Top work.

impleri’s picture

Yeah, I don't feel too strong about the deprecated style.

Can someone else review this patch so it can be marked as RTBC and added to 2.x-dev?

ezra-g’s picture

We've been using #64 in Drupal Commons since around the time it was posted and everything seems to be working as expected and as it was with 37.

We have one support request related to fields for a user who may be upgrading between Commons Beta2 (which used 37) and the latest dev (which uses 64) but the request is difficult to understand and I suspect, unrelated to this patch (#1919618: Fields of message types disappear).

From my perspective, this patch is RTBC.

Thanks, mpotter!

ezra-g’s picture

Status: Needs review » Reviewed & tested by the community

I haven't been abel to reproduce the issues referenced in #1919618 and marked the issue as such.

I think the present issue is RTBC.

nedjo’s picture

Issue summary: View changes

Provide summary.

nedjo’s picture

Agreed that this change feels like a solid step forward.

It would be good to compare with how field exports are structured in D8.

Since this is a significant change I've taken a stab at writing a summary for the issue, including a draft change note explaining what will be needed to refactor existing features. Please review and edit.

caschbre’s picture

I applied the patch from #64 however I'm not quite seeing where I get the option to export the feature_base vs feature_instance. Maybe I'm confused at exactly how this patch works. Should I able to go to admin/structure/features/create and under the components section and be able to just export the base vs instance of a field?

edit: Nevermind... hadn't quite gotten the caches cleared.

caschbre’s picture

Here's a use-case that I'm not sure works... or maybe it's not supposed to work this way.

I'm attempting to create a general feature that has the field_base definition for all of the fields. I'll then create separate features for content types (or groups of content types) that contain the field_instance.

So to do this, I created a temporary content type and added all of the fields I wanted. I then created the generic feature and exported only the field_base for each of the field. If I take this module to a new drupal install I can enable it and see that the db tables for each of the fields are created.

However... if I go to a content type now expecting to add the field instance, these fields do *not* show up in the list of existing fields.

Is that expected behavior? Is that even covered in the scope of this patch?

scottalan’s picture

I'm using 2.x-dev and I applied the patch in #64 http://drupal.org/node/1064472#comment-7042482. When I run drush fd my_feature features is trying to add the fields back to the .info file. I have the field_base and field_instance in the .info file and the old field file has been deleted. Has anyone seen this behavior?

I've tried fr and fu...no luck

impleri’s picture

#73 As far as I can tell, this is expected. Drupal doesn't allow field_bases to exist without field_instances, but it does define them separately.

#74 Does fl show the feature as overridden? Do fr and fu actually run on the feature or do they report there's nothing to change?

scottalan’s picture

Here is what I get:

drush fd my_feature

results in:

Legend: 
Code:       drush features-revert will remove the overrides.
Overrides:  drush features-update will update the exported feature with the displayed overrides


Component: info
  features[ctools][] = views:views_default:3.0
  features[features_api][] = api:1
> features[field][] = node-document-body
> features[field][] = node-document-field_document_file
> features[field][] = node-document-field_radioactivity
> features[field][] = node-document-field_topics
> features[field][] = node-document-og_group_ref
  features[field_base][] = field_document_file
  features[field_instance][] = node-document-body

drush fl - The feature is not overridden.
drush fr - Current state already matches defaults, aborting.
drush fu - Doesn't change anything. I still get the same output (as above) on a drush fd

impleri’s picture

Are those listed in the info file's features_exclude[]. That section is added automatically on export.

scottalan’s picture

They are not... Should I have a listing for each of the fields it's trying to add back?

i.e.,

features_exclude[field][node-document-body] = node-document-body
features_exclude[field][node-document-field_document_file] = node-document-field_document_file
features_exclude[field][node-document-field_radioactivity] = node-document-field_radioactivity
features_exclude[field][node-document-field_topics] = node-document-field_topics
features_exclude[field][node-document-og_group_ref] = node-document-og_group_ref

is that correct?

I grepped: features_exclude and saw examples such as: features_exclude[field_instance][og_group_ref] = og_group_ref

I take it this should automatically get added?

EDIT:
I notice that the features_exclude gets added when exporting a feature from the UI but does not get added when executing drush fu my_feature

mpotter’s picture

scottalan: Try exporting the feature from the UI instead of via "drush fu" and see if that works. In my testing it isn't adding the [field] items to the export, so I'm not sure what is wrong.

The features_exclude lines are purely for the new UI to determine if an auto-detected dependency should be checked or if you have specifically unchecked it. I suppose it's possible that is interfering with this patch, but feel free to manually remove those features_exclude lines from the *.info file if you wish.

And yes, I'm still alive and I still want to commit this soon. I've just been swamped with too many client projects lately and haven't had any time to work on Features. My goal is still to get an RC1 release soon so I can have the 2.x Stable release ready before DrupalCon.

scottalan’s picture

Completely understand!! It just so happens that I just exported a new feature (from the UI). I went ahead and left all 'auto detected dependencies' checked to see what would happen. I see that there are '30' feature_exclude lines it added. They were for every one of the 'auto detected dependencies'.

So basically this is what I got...

dependencies[] = node
dependencies[] = text
features[ctools][] = strongarm:strongarm:1
features[features_api][] = api:1
features[node][] = chapter
...30 more lines with every dependency it auto detected prefixed with features_exclude

What I don't get is that I think I should need every one of the [dependencies], [field_instance], and [variable] it is actually trying to exclude.

It is however excluding the [field] and that seems ok considering we are using [field_base] and [field_instance] now.

I'll see if I can debug to see what is happening when I get a chance.

Thanks,
Scott

EDIT:
I just realized that I was including fields from another feature and that feature has a hook_features_pipe_alter() but it is only affecting [field_instance] and [field_base]. Just thought I throw that in there as well.

EDIT:
it appears it's just on a single field so that has nothing to do with it. Jumping to conclusions.

mpotter’s picture

Status: Reviewed & tested by the community » Fixed

This has been committed to 9cfc9ae.

ezra-g’s picture

Huzzah!

Great work, mpotter et al!

mpotter’s picture

Status: Fixed » Active

Looks like I need a bit more work on this. It's mostly working, but I can confirm scottalan in #76 that "drush fd featurename" is reporting the old field components, even though the feature is not overridden. I think it's just a drush issue, but I'll look into it.

Seems to be a minor side-effect that most people probably won't even notice (since the feature isn't overridden, most people won't be doing a drush fd)

mpotter’s picture

OK, here is a patch that fixes the bogus "drush fd" output. The new deprecated code is only handled when preparing the final export, so a reset=TRUE was needed in the detect_overrides function.

Since this also has the side effect of resetting some static caches, it might be better to handle the deprecated stuff with a different argument instead of relying upon $reset. For let's try this for now and see how it works.

mpotter’s picture

Status: Active » Needs review
mpotter’s picture

Here is a better version of the patch that adds a new argument to features_export_prepare that determines if the deprecated components should be added. This should fix the problem without any cache side-effects.

mpotter’s picture

Status: Needs review » Fixed

Committed #86 to 54e2810.

DamienMcKenna’s picture

Should this have a follow-on issue to make the drush commands remove the old example.features.field.inc file?

DamienMcKenna’s picture

Clarification on my comment in #88: when you use the features-update Drush command it creates the two new files (example.features.field_base.inc, example.features.field_instance.inc) but leaves the unused example.features.field.inc file.

DamienMcKenna’s picture

It should also be noted that using "features-diff" will still show the old "features[field][]" items, even when a feature is updated to the new format and even though they're no longer actually created, though "features-list" does not show that the feature is overridden.

DamienMcKenna’s picture

DamienMcKenna’s picture

jbrown’s picture

Had some difficult git branch issues with this. Updating a feature meant fields were lost. We had to re-export all the fields individually into the features with drush fe. On master branch we updated all fields to use the separate files. This meant merging master into a dev branch was quite difficult.

We had to:

  • sync live database into site on dev branch
  • drush fra to put the branch's fields into db
  • merge master => dev - manually removing conflicts
  • drush fra
  • drush fe new fields from dev branch
DamienMcKenna’s picture

@jbrown: Yes, it's proving to be quite problematic, I spent most of a day updating one site with about 15 entity features due to Features being inconsistent on how it decided the fields should be exported, including having to re-export the entities multiple times before Features was completely happy.

Elijah Lynn’s picture

FYI to everyone. Here is a blog post by mpotter that summarizes this thread a bit, head on over to http://www.phase2technology.com/blog/new-field-bases-and-instances-in-fe...

Status: Fixed » Closed (fixed)

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

mexicoder’s picture

I have to say I am still in agreement with the earlier concerns of @nedjo about these changes.
I am a heavy features user and I use features as a key part of deployment on large scale sites. I understand the rationale of splitting fields up in this way but the current implementation is problematic.
In the past I could export segments of the site split into standalone features. Features could reuse fields (like field_image) without having a dependency to each other. As base and instance were included together you could be sure that the base and instance matched for that particular feature. Sure there could be situations where different features would define the same field differently. That to me was a true conflict and an issue to be resolved.
In the new system it seems like the options are to

1. Have conflicts highlighted in my features.
2. Not allow conflicts in which case field base ownership is assigned to the first feature to use the field
3. Create an abstract feature to hold field bases that other features depend on and manually set up my features

My thoughts on these are
1. This just looks bad in a set-up that looked to be working fine using the previous api
2. It seems crazy to allow an unrelated feature to define and be the master for a base field without any set of rules other than first in. It also means we set up dependencies between features for no other reason than sharing a field.
3. We introduce a whole new workflow to creating features including abstract features which will need manual maintenance

Can anyone suggest a sane workflow that they have working on this new implementation?

mexicoder’s picture

Issue summary: View changes

Fix copy and paste error.

ezra-g’s picture

Given the age, length, number of contributors and topics in this thread, I'm marking comments as read only.

@dumbass, please file a new issue for any new discussion.