Rethink the way features are enabled / disabled

There's been talk about on creating/adding install files to do update functions as needed (don't recall the exact issue), and sounds like a good idea. I hate the features rebuild in cache clear, and would much prefer if update functions/hook_enable/hook_install were able to replace.

(None of this needed for features that do not depend on features/true exportables of course.)

It's going to be a bother to re-educate users, but it seems worth it.

You handle that by just setting the option for rebuild on cache clear to be TRUE unless changed. So existing users are unaffected but people who know what they are doing can turn off the auto-build option and rebuild manually. With a module like Features that is so widely used, consideration of existing users is always a bit issue for me.

The part I'm a bit concerned about is the writing of the hook to the install file. Remember that people use features in many different ways. There are cases where a hook_enable or hook_disable might already exist in the feature.module file. There are cases where a feature.install file already exists and you'd need to check to see if the hook_enable has already been written or not and preserve the rest of the install file. Just lots of cases where I can see problems arising, so this is definitely not something that is going to make it into v1.0 at this point in the dev cycle.

An approach such as #658772: Provide way to disable rebuild during features_flush_caches() has a better chance of making it into Features soon since it's much less intrusive and doesn't affect existing installations. So I'd like to see a phased approach where we get that patch improved for now and then work more on a better comprehensive solution for the future.

Yes, fixing this up is a key need, for both performance and dependency handling. As well as the .info file parsing and related issues, there's the problem that on cron multiple features are rebuilt on a single page load, which can easily lead to out of memory errors.

As Mike noted in #4, though, rewriting .install and hook_enable() will quickly get messy. Can we just continue to do our work in hook_modules_enabled()? True, hook_enable() is called for each module in turn while hook_modules_enabled() is only called once for all modules that were enabled. But the modules are sorted first in dependency order, so our dependencies should be fine.

So it looks like there are two changes we'd need to make to features_modules_enabled():

• Find a less costly way to determine what modules are features modules.
• Add a rebuild call to the existing enable call. Maybe consider pushing the enable + rebuild calls into a batch operation to avoid out of memory errors.

As hefox said in #2, we're not really talking about all features and all components here--just features that include faux exportables. The only shortcut that occurs to me is testing for the featurename.features.inc file rather than parsing the .info file--but is this file generated for all features with faux exportables?

Why isn't rebuild already being called here? Just to avoid delaying the page load when enabling a feature or features? Maybe a batch would help here too--at least it would provide some feedback.

Here's a first rough take on how to rebuild features on enable via a batch operation.

On manual testing (enable or disable a feature at admin/structure/features) this works for some features but in once case (enabling debut_redhen)I got an infinite loop seemingly related to entity api and ctools.

This will generate an access denied error when enabling with Apps because an Apps redirect will be missed.

Haven't tested at install time or via Drush.

@bojanz: Yes, a bit tricky. If we end up taking this direction, rather than introducing a new hook, I guess you could use hook_module_implements_alter() in the install profile to move the features hook_modules_enabled() implementation to the beginning and then implement hook_modules_enabled() rather than hook_enable() in each feature.

I've been experimenting with @bojanz's code from the issue summary and seem to be hitting a couple of issues that will probably also occur in my patch.

1. When features_rebuild() is called (in hook_flush_caches()), components are rebuilt/created in the order that their component types are returned by features_get_components(). This order is important and differs from the alphabetical order that components take in a feature's .info file.

Specifically, user_features_api() returns 'user_role' before 'user_permission', with the result that user roles are - correctly - created before the user permissions that require them. But in a module's .info file alphabetical ordering will put user_permission components before user_role ones, with the result that the role doesn't exist when the permission referencing it is created.

We might need to call features_get_components() and then reorder a feature's components by type.

2. It appears that some data supplied by newly-enabled modules are not yet available when feature components are rebuilt. Specifically, I got the error "SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'module' cannot be null", which may result from newly-enabled modules' permissions not showing up due to a stale cache in module_implements().

We might need at a minimum to call drupal_static_reset('module_implements').

Updated patch with a fix to the first problem noted in #9. The second problem in #9 turned out to be caused by rebuilding a feature that includes a profile2_type in hook_enable(), before the new profile2_type is added in entity_modules_enabled(). So it shouldn't show up in this patch, if only because features comes after entity in hook_modules_enabled(). That said, the workaround I suggested in #8 could then break things again, by moving features before entity....

Switching to the 2.x branch.

Previous patch was missing the added file.

Has this patch been reviewed ?

I have site on pantheon that is exhausting my memory. Here is what support suggested:

| APR 01, 2013 | 11:27AM PDT

In terms of pinpointing the root cause as this looks feature specific as for the solution there is nothing platform specific. The volume of calls is the problem here and how the features module is handling the jobs being performed.

Features attempts to process everything as a single transaction without concurrency which is not the most efficient way to do this. In this thread the suggestion is to increase the timeout, but that does not really solve the problem. Doing this will be fine until you hit the PHP timeout and could cause the CPU and memory usage to spike.

The suggestion by #3 on that thread is the better way to handle the features management, using batch operations for large feature sets:

https://drupal.org/node/1053608

The thread that it points to is here. However, the status needs review rather than accepted or closed:

https://drupal.org/node/1572578

The worfklow outlined in the description the problem is possibly with how features is managing those operations.

From a quick refreshier, there's two issues here, when features rebuild happen, and many big features rebuilding use many resources/takes many time (thus the batching)? I'm not totally convinced that should be addressed just in the call from features_module_enabled.

Due to that, adding a simple patch that just adds in the features rebuild

Need to refresh what files have been included

Proper order for components

Oops, can't call features get components till after that cache is cleared in features_include(TRUE)

Just stumbled on this issue. I have been tackling the same problem in January for Features 7.x-1.x in #1265168: Rebuild the file list properly when a feature is enabled or disabled. This was the relevant part of my patch:

diff --git a/features.module b/features.module
index 6aeb2d7..42fd679 100644
--- a/features.module
+++ b/features.module
@@ -300,19 +300,29 @@ function features_modules_disabled($modules) {
  * Implements hook_modules_enabled().
  */
 function features_modules_enabled($modules) {
-  // Go through all modules and gather features that can be enabled.
-  $items = array();
+  // Check if any of the enabled modules create new components.
+  $new_components = array_intersect(module_implements('features_api'), $modules);
+
+  // Check if any of the natively supported modules have been enabled.
+  $new_modules = array_intersect(array('features', 'block', 'context', 'field', 'filter', 'image', 'locale', 'menu', 'node', 'taxonomy', 'user', 'views', 'ctools'), $modules);
+
+  // Check if any of the enabled modules contain features.
+  $new_features = array();
   foreach ($modules as $module) {
     if ($feature = features_load_feature($module)) {
-      $items[$module] = array_keys($feature->info['features']);
+      $new_features[$module] = array_keys($feature->info['features']);
     }
   }
 
-  if (!empty($items)) {
-    _features_restore('enable', $items);
-    // Rebuild the list of features includes.
+  // Clear components cache and load new include files if needed.
+  if (!empty($new_components) || !empty($new_modules) || !empty($new_features)) {
     features_include(TRUE);
   }
+
+  // Enable the new features.
+  if (!empty($new_features)) {
+    _features_restore('enable', $new_features);
+  }
 }

Ref. https://drupal.org/files/1265168-36-features-module_enable.patch

Another problem that we've encountered with Features is that you cannot create nodes during an install.

Features only creates the field bases / field instances on hook_enable(), which occurs after hook_install(). So basically if you have a feature that adds some fields to (for instance) Page, you cannot create a Page during hook_install() of, say, a profile.

I've done some debugging and this is what I came up with:

field_create_instance()           - Creating instance body for bundle: webform
PROFILE_install()                 - Running PROFILE install
field_instance_features_rebuild() - Rebuilding feature field instances for module: feature_inst
field_create_instance()           - Creating instance field_address for bundle: institution
field_create_instance()           - Creating instance field_feedback_introduction for bundle: institution
field_create_instance()           - Creating instance field_mail_feedback for bundle: institution
field_create_instance()           - Creating instance field_mail_invitation for bundle: institution
field_create_instance()           - Creating instance field_type for bundle: institution
field_instance_features_rebuild() - Rebuilding feature field instances for module: feature_sg
field_create_instance()           - Creating instance field_education_type for bundle: survey_group

Rerolled #18 to remove extra comment

This fixes #2080999: WD cron: EntityFieldQueryException: Unknown field: field_oa_date in EntityFieldQuery->addFieldCondition. see https://drupal.org/node/2080999#comment-7898053

#1597186: Do not cache included components in features_include_defaults() might be a duplicate of this issue.

#1597186: Do not cache included components in features_include_defaults() isn't a duplicate of this issue - see comment #9. There is an issue with installing Panopoly with Features 2.0-rc2 or 2.0-rc3 (version 2.0-rc1 actually works!) that only that patch fixes - not this one.

This fixed the install issue in OA2 that was causing all features to be built at the end (and exceeded max execution time). Doesn't seem to have any downside or any decrease install time, so I've committed this to 676484f.

Agreed that we sometimes need a batch operation. But the need for a batch is not specific to a given distribution. The patch applied from #21 helped avoid timeouts only in the limited cases that:

• features are enabled only one per page load (for instance, through an install-time batch operation), and
• no given feature on its own exceeds a timeout.

Any time many or complex features are enabled at one time - for example, through the Admin > Structure > Features screen - there may be a need to batch the operation to avoid a timeout.

So it may be better to directly address the problem in Features - as I began to do in #12 - rather than adding a workaround/override.

We could also consider changing the variable_get() call in drupal_set_time_limit() in _features_restore() to a default value of 240 rather than FALSE. (240 seems to be the default used in e.g. https://api.drupal.org/api/drupal/modules!node!node.module/function/node...). [Edit: see #1892296: Allocate sufficient time for migration processes for a similar approach in Migrate.]

Committed #27 to b43ecaa.