Provide support for exporting of altering of components

Actually, see the last patch here #1277854: Faciliate altering features, e.g., export field display settings separate from field data structure. I went with a different naming format more consistent with the existing hook. The existing hook was "default_hook_alter" and I added "override_default_hook_alter".

But the main comment is that you also need to call this hook in the features_get_normal routine as well. And it also currently requires the "features_include_overrides()" call to ensure the *.inc file from the override has been included. Otherwise certain functions such as Revert don't work properly.

Closing this as a dup of #1277854: Faciliate altering features, e.g., export field display settings separate from field data structure. The other issue will propose a new universal hook. Unlike the patch above, the new hook needs to be called from both features_get_defaults as well as features_get_normal. It also takes advantage of the array passing in the first argument to drupal_alter for more flexibility.

A possible, if somewhat hacky, alternative:

Features knows about the components, so it should probably know about the alter hooks too. So it should be able to dynamically create features_views_default_views_alter(), which simply calls hook_features_default_alter.

I'm not following why this is good. It seems like over-simplification for simplification's sake.

Updated issue summary.

Thanks for having me re-read that, this time I think it clicked better.

I don't see all Features components as relating to each other in the same way all Forms do. This request reads to me like having a way to combine hook_entity_load(), hook_form_alter() and so forth into a hook_alter(). Documentation, I think, is something that could do with some curation, or maybe automation, to answer the question of what's truly available.

I realize a lot of the components in Features have alter hooks, and many of them look fundamentally the same, but there are subtle differences. There's also much greater risk of accidentally applying the wrong alters to a given component--this is like rewinding hook_node_load() back to hook_nodeapi().

A more common option to alter hooks might make sense for all entity stuff (and Entity API will handle it) and another common hook might make sense for Ctools stuff (and ctools should handle it).

Is there a practical use case for such a hook that a developer would sit down and use that wouldn't just end up with code equivalent to a switch() to route modifications?

Even though the first version of my Field Settings Override sandbox put a call to an alter hook into the exported feature code, I no longer believe that is necessary. I have been able to pull that call back into Features itself as noted over in #1277854: Faciliate altering features, e.g., export field display settings separate from field data structure. I know we keep going back and forth on this, but when looking at some of the code examples above (like #10) I think we are getting confused by multiple issues.

When I talk about a "universal hook", I am referring to something similar to what Features already calls in features_get_defaults. The discussion is whether a similar hook needs to be called in features_get_normal. No where do I see the need for exportable code to actually call any alter hooks themselves.

So I guess I'm still confused as to the reason for this separate issue.

Nevermind. I had a fundamental misunderstanding of how Features worked with real exportables, such as Views.

Yes, the drupal_alter call *must* be placed at the end of the actual exportable code. That is the only place where it can reliably "override" the feature. For faux-exportables, this alter hook can be pulled into features_get_default and features_get_normal, but that doesn't work for real exportables. In the case of Views, it is Views itself (or ctools) that actually calls the default_hook, not Features. So with Views, the alter hook in features_get_default doesn't even get called.

So something like #10 is looking more like what we will need.

Updated issue summary.

Based upon #10, here is the Features patch for the new Alter hooks. Note that in D7, drupal_alter context values need to be variables and not literal string values. This is the patch that works with the new Features Overrides 2 sandbox.

It is very important to note that exportables such as Views cannot be overridden without the alter hook actually added to it's exportable. There is no way to place a single drupal_alter call in Features itself that will work.

+++ b/includes/features.ctools.incundefined
@@ -185,6 +185,11 @@ function ctools_component_features_export_render($component, $module, $data) {
+      $code .= "  drupal_alter( 'override_features', \$export, \$alter_context);\n";

extra space after drupal_alter(

+++ b/includes/features.field.incundefined
@@ -123,6 +123,12 @@ function field_features_export_render($module, $data, $export = NULL) {
+    $code[] = "  drupal_alter( 'override_features', \$fields, \$alter_context);";

See above

+++ b/includes/features.field.incundefined
@@ -123,6 +123,12 @@ function field_features_export_render($module, $data, $export = NULL) {
+  $code[] = '';

Should this be outside the conditional? If so, combine with the next line.

+++ b/includes/features.filter.incundefined
@@ -71,6 +71,11 @@ function filter_features_export_render($module, $data, $export = NULL) {
+    $code[] = "  drupal_alter( 'override_features', \$formats, \$alter_context);";

Why are these all like this?

+++ b/includes/features.image.incundefined
@@ -62,6 +62,11 @@ function image_features_export_render($module_name, $data, $export = NULL) {
+    $code[] = "  drupal_alter( 'override_features', \$styles, \$alter_context);";

hmmm.

+++ b/includes/features.menu.incundefined
@@ -103,6 +103,11 @@ function menu_custom_features_export_render($module, $data) {
+    $code[] = "  drupal_alter( 'override_features', \$menus, \$alter_context);";

o_O

+++ b/includes/features.node.incundefined
@@ -86,6 +86,11 @@ function node_features_export_render($module, $data, $export = NULL) {
+    $code[] = "  drupal_alter( 'override_features', \$items, \$alter_context);";

I think this is the last one

+++ b/includes/features.taxonomy.incundefined
@@ -72,7 +72,13 @@ function taxonomy_features_export_render($module, $data) {
+    $code[] = "  drupal_alter( 'override_features', \$taxonomy, \$alter_context);";

Nope, here's another

+++ b/includes/features.user.incundefined
@@ -64,7 +64,7 @@ function user_permission_features_export_options() {
+function user_permission_features_export_render($module, $data, $export = NULL) {  ¶

trailing whitespace

+++ b/includes/features.user.incundefined
@@ -93,6 +93,11 @@ function user_permission_features_export_render($module, $data) {
+    $code[] = "  drupal_alter( 'override_features', \$permissions, \$alter_context);";

Last one!

Sorry, it's a case where my personal coding style of 20+ years conflicts with the Drupal standards. Hard to retrain those fingers! I'll get it fixed in the next update.

Still actually trying to decide if I should be calling drupal_alter (and the $exports conditional) from within the render hooks or whether I should add a new "features_override_alter" function and then call that function from each render hook so it might be easier to support any additional conditionals needed in the future.

I must not be paying effective enough attention, because I'm *still* not tracking the value of this as more than a convenience for the Features Override module.

Could someone boil it down into simple terms for those not following the Multiple Issues + Architecture of Feature Overrides module home game?

Well, you are partly correct: This patch is only needed to make the Features Override module work. I wouldn't call it just a "convenience" because without this patch, there is no way to implement Features Override properly.

That is why this issue is marked as a "feature request".

Here is a new patch. In addition to fixing the whitespace issues in #19, it fixes more important issues with the taxonomy override and node overrides that were causing errors when creating new features. Also adds the override for user roles.

Hmm, one more bug left in taxonomy override. Here is new patch.

This would make the alter hook_override_features_alter, when it should be more like hook_features_override_alter. And that sounds like a terribly confusing name. Also, it needs to be added to features.api.php.

Naming issues aside, I'm still not convinced this is an appropriate thing to add to features. Assigning to febbraro for his thoughts.

I didn't name it hook_features_override_alter because of possible name conflicts with the Features Override module. But I agree that a better name would be nice.

febbraro is right down the hall, so I'm pretty sure I know some of his thoughts on this ;)

I didn't realize Features Overrides does not work with Features as-is, that's probably a decent reason to add a hook but I still don't grasp the architectural need.

Clearly Tim is on board, so I'm sure it's fine. I just don't think new hooks should be added unless their use case is obvious, and at the moment I guess I need to /really/ read code to understand it. Maybe when features.api.php is updated the docs will add clarity.

I wouldn't say I'm 100% on board yet, I'm reserving judgement until I see the api.php docs :)
Also, duh. That'll teach me to click on users profiles more and see who they work for/with.

I'd just like to add an uncompromising +1 to the idea of there being a consistent alter invokation inserted at the foot of all exported hooks before they return their data.

Rather than seeming ungraceful, I would contend it is quite a neat way of providing consistent altering of default hooks.

For those of us who use custom features as a way of keeping db config in (version controlled) source code in large projects we quite regularly need to hand-alter a feature to put a variable_get() inside it (or whatever). But such custom changes get wiped out all too easily by some other developer blindly drush features-update-ing the feature, not noticing that they've wiped out someone's custom modification and committing the change to version control.

With a consistent alter hook (the work of which is considered part of the exported code) finally we can put such custom overrides in a separate place which will not get wiped out automatically! Very very very Yay!

Having said that - I haven't thoroughly tested the above patch, so don't include this post as +1 for the implementation just yet.

From glancing over it, my first thoughts are:
1. It seems a shame that each export render callback needs to care about adding the alter individually. Given that features is generating the code itself, why can't it append the alter automatically? The only extra knowledge it needs to do this is what the variable happens to be called.
2. It seems like a much better idea to call some features function instead of drupal_alter directly. That way when there is a features module update changing something about the way the alter is performed I don't have to re-export all my features! Might want to wrap that in a function_exists though to give people the opportunity of disabling the features module without WSODing.

@jamsilver all the Features exportables should have alter hooks already. They may be under-documented, but they are there. I contend it is a better practice to keep those overrides in separate hook implementations, rather than creating a single function with the switch/case-as-sub-function D7 anti-pattern.

At this point, I suspect I'm the devil's advocate. Will lay off unless I have something new to say.

@jamsilver all the Features exportables should have alter hooks already

That's the point: they don't. These specific alter hooks need to be in the EXPORTABLE code, not in the module itself.

Given that features is generating the code itself, why can't it append the alter automatically? The only extra knowledge it needs to do this is what the variable happens to be called.

That's exactly the problem. Because there are no standards in the export code generation, different exportables return different arrays. In CTools, it does "return $exports", while in Fields it does "return $fields", etc. In the original Taxonomy exportable, it's a mess because it doesn't even put the return value into an array that can be altered before returning it.

So yes, we could have Features itself add this alter hook code, but only if we also go through and standardize the exportable code format.

It seems like a much better idea to call some features function instead of drupal_alter directly.

I did that originally when I first started working on this and some complained that I should just call drupal_alter directly. But now I'm starting to come back to this and I agree with you. Especially because of the messy code needing to pass a variable to drupal_alter instead of a literal string value. Also, if my issue below with a difference between CTools and other features is true, then the code to handle that could be put back into Features allowing the actual Alter call at the end of each exportable to be the same.

Finally, back to the original topic, it appears as if there IS a difference between the exported Drupal_Alter call between real exportables and faux-exportables. The test to only append the drupal_alter code when exporting to a file (the check of $exports being not-null) is only needed for real exportables (CTools). Adding this check makes the Field overrides not work properly.

So I need to work on a new version of this patch after some more testing. I need to determine if CTools is the exception, or if Fields is the exception.

Exported code altering itself before returning it is a great idea. +1 to that.

However, drupal_alter is not the right method since there is less standardization in the exportable components. A features function makes more sense because depending on the component, features can provide component-specific data handling and alter hooks. That way atleast all the quirks of different exportables are in one place and standardization can be easier.

Sounds like we need two different routines:

1) A "helper" function that can be called by the exportable to add the appropriate hook code to the output stream. This routine would take the name of the array as an argument, along with the component name and would return the code line to be appended to the export code.

2) The actual "drupal_alter" function to be called. This function is the one called from the end of the exportable code. It's responsibility is to call drupal_alter as needed.

I'll be working on this today to try and get it all cleaned up and will submit a new patch.

Here is the patch. It creates two new routines in Features.module:

1) features_add_override_alter($component, $var_name, $export), which you call in your exportable hook_features_export_render routine with the name of your component, the name of the variable your code returns, and the $export array that is passed to your hook from features. It generates the code line that you need to append to your exportable code.

2) features_override_alter($component, &$data). This is the actual alter hook called by the code added to the end of the exportable. It's basically a wrapper around drupal_alter. After further review I didn't find any need for a difference between CTools and Fields (difference was caused by a bug in Features Override 2). So at this time there isn't any logic in this routine. But having this routine makes it easier to add logic later without needing to rebuild all features exportables.

Oops, sorry about that. Uploaded wrong version of patch. Here is the correct one.

Adding this to the D7 release blocker and removing #1277854: Faciliate altering features, e.g., export field display settings separate from field data structure

That's the point: they don't. These specific alter hooks need to be in the EXPORTABLE code, not in the module itself.

If they don't have alter hooks, why not add them in the module? Why do they HAVE to be in the exportable?

So yes, we could have Features itself add this alter hook code, but only if we also go through and standardize the exportable code format.

So why not do this instead?

Separate from those questions, this patch is still unreviewable until it has a documentation hunk in features.api.php.

I'm adding my voice to the others uncertain about this.

It seems the incorrect approach that will lower performance and be bad developer experience (duplication).

Why is this a release blocker?

It seems the incorrect approach that will lower performance and be bad developer experience (duplication).

Perhaps you could elaborate on more details about this comment. I don't see where it will lower performance. If you are not running a module like Features Override then nothing is happening during the alter and drupal_alter is pretty efficient. It's the module that *uses* these alter hooks that will be responsible for performance and I welcome comments over in my sandbox module to help improve any specific performance issues there.

Why is this a release blocker?

Because we feel that properly supporting a way to override features is a crucial need for the Features module. The problems with the existing Features Override module have shown that it is not possible to properly implement overrides without some change to the core Features module. And so far we are lacking any other working method for overriding features to compete with the approach given in this patch and the associated sandbox.

Also, issue #1277854: Faciliate altering features, e.g., export field display settings separate from field data structure was marked as a blocker but was closed as a dup of this issue since the patch in this issue supersedes 1277854, so this issue now becomes the blocker.

Why do they HAVE to be in the exportable?

See #17. For a feature such as a View there is no other place to put an override alter hook. Views calls the default_hook for the feature directly and not via any function in Features that the alter hook could be called from.

So why not do this instead?

Because look how hard it is to get a *simple* patch accepted. Imagine the trouble if I start completely changing all of the exportable code. Also, it doesn't help for modules that add their own features and might use their own naming conventions. The patch in #34 addresses the issue of different variable names being used in different exportables and is a good compromise in my opinion.

this patch is still unreviewable until it has a documentation hunk in features.api.php

Well, I agree that the patch should not be committed until it has more documentation. But the lack of a hunk in the api docs does *not* prevent people from installing this patch and the sandbox in #17 on a test Drupal site and playing with overriding features and testing this code.

For those who are expressing uncertainty about this issue, I would like to suggest that you actually install the sandbox module listed in #17 along with this patch to Features and play with overriding features. The need to be able to override features, especially when working with features coming from a base distribution, is a serious need expressed by many in the community. The existing Features Override module does not solve this need. I have been working hard at this problem for many months now and have not found any way to properly override all types of feature without the alter hooks proposed in this issue.

I'm happy to hear from anybody who wants to demonstrate a better way to override features that actually works for all types of overrides (especially Fields and Views). I'm happy to modify this patch to incorporate any constructive suggestions (as shown in #34). What I am not happy with are non-constructive comments from people who either don't understand or believe in the need for overriding features or who make comments without actually using and testing the code being proposed.

As an example, Views provides hook_views_default_views_alter().

The less custom code and new hooks, the better. Utilize the existing hooks provided by the modules.

What I am not happy with are non-constructive comments from people who either don't understand or believe in the need for overriding features or who make comments without actually using and testing the code being proposed.

I am sorry you are not happy. I have tried the module, I understand it's purpose, it just appears to me that the current approach is heavy-handed.

As an example, Views provides...

True, but what about other features. Does every other feature not handled by Features itself have such an existing hook? And how would you propose writing a Features Override module that supports all of the special hooks of each special exportable. The title of this issue was "Provide a *universal* hook..." and that is what I'm trying to do. Less scattered hooks and better centralized hooks is better.

it just appears to me that the current approach is heavy-handed.

Then please propose a working alternative so we can make positive movement forward on this issue.

The title of this issue was "Provide a *universal* hook..." and that is what I'm trying to do. Less scattered hooks and better centralized hooks is better.

Less scattered hooks would be better, except you're not removing the other hooks, you're just duplicating them. If I have a default view that is being altered, I now have to check two different hooks to see where things are taking place.

Then please propose a working alternative so we can make positive movement forward on this issue.

This is a feature request. Two of the three maintainers have expressed doubts about the need for this patch, and the third hasn't commented at all. It hasn't been decided that this is even going to get in.
If you need extra hooks/alters for your module to work, it'd be good to know that you've used all other available hooks first, especially ones provided expressly for this purpose.

It's simple: the more junk you got in a module, the slower and more complicated to understand the module gets, so for a module as widely used as features, anything that gets put into it needs to be clear, useful for a variety of people, and not duplicate existing functionality.

I use alters perhaps too much -- using the existing alter hooks. There's only hook that doesn't have an alter I know of, and that's core hook_node_info due that lack of alter, and while I'd like features to add a hook_node_info_alter, that's just a one off due to how core is frozen in the active version most people are using (mm, should create a core issue feature request for that).

I believe there is an issue somewhere about altering that actual rendered export -- how about that? Features overrides and other modules could add whatever they want then, and it'll only run on exporting, and it'd have other possible uses, and doesn't duplicate existing functionality.

it'd be good to know that you've used all other available hooks first, especially ones provided expressly for this purpose.

I have. The existing hooks do not support overriding Field features, which is where this issue originally started (Field Settings override). The existing Features Override module attempts to use the existing hooks, and it does not work properly, not even for views. The original maintainer of that module started this thread because existing hooks were not sufficient. If existing hooks were sufficient then the existing Feature Override module would be working and in wide use.

Features overrides and other modules could add whatever they want then, and it'll only run on exporting, and it'd have other possible uses, and doesn't duplicate existing functionality.

I've gone down that path, but this simply does not work. The alter hook needs to be in the exportable code itself, not in the code that creates the exportable. You can either take my word for it after months of work on this, or you can play with the code yourself and try to get it to work.

In IRC hefox proposed a compromise: a hook for the actual code output, which your module can use to add whatever alters you need.

This is untested and undocumented, but its more about the idea.

@tim: you are still missing the crucial point. I went down that path myself in #15 and #16. It only works for faux-exportables and not true exportables (Ctools, views, etc). The alter hook needs to run when the default_hook routine is *executed* and not when it is generated/exported.

No, I'm saying use that hook to add in your call to a features_override_magical_alter() into the export.

mpotter, do you think you could jump on IRC? This would be much easier to discuss in real time, and we can still summarize and post to this issue...

#1064472: separate fields from field instances is a good idea for fields in d7, (which #1321176: Enable export of generic or abstract compontents (e.g., fields) that can be applied to multiple entity bundles and #1277854: Faciliate altering features, e.g., export field display settings separate from field data structure are duplicates of?).

Where features provides the hooks for defining and altering of components that core/contrib doesn't, if that is broken/not ideal, that should be fixed, but in a way that matches how others do it, ie a hook_whatever_default_what and hook_whatever_default_what_alter.

I love that there is debate, a change like this does need to be scrutinized and that is why we are doing it here in the issue queue instead of just adding it directly to the code. I have held off on committing various versions of this patch for, literally, months b/c I felt that it did not address all of the various scenarios we are interested in supporting. All of that said, I have been working with Mike on what this patch is/does/works for the entirely of this time. Many incarnations did not seem kosher and thus were not committed. This is a complicated problem and the easy fixes have been tried and either don't work at all, or as well as we'd like. This current version of the patch is supporting all of the major use cases we had w/r/t doing overrides. The fact that there is no standard/universal way to integrate with the other alters modules provide (no naming conventions of info hooks or anything) this, while seemingly heavy handed, was the simplest from a Features perspective to supporting all of the overrides we were interested in.

Mike has been down many paths with this, and just b/c you are not keen on this implementation right now don't be blind to the fact that he has tried several other ways to accomplish this, most of which have met with failure. This is something I'm interested in committing b/c it solves some real world problems we are having, so I'd like to see something we can all agree on, but one way or another supporting overrides is something I am very interested in.

Maybe an IRC chat will help?

OK after doing some IRC and also realizing that the "drupal_render" in #44 should be "drupal_alter" and that this simply allows Features Override itself to add the necessary hook code to the exportables, I think we are back on the same page. Will do some testing and get back with a new patch.

@nedjo good points about hook naming, well-formed alternatives. But should Features or Features Overrides own the namespace in this situation?

Also, to distil your meatier point, in order for this solution to work for Features Overrides, the Features Overrides module will need to be installed at the time the Features are originally generated in order to incorporate this hook. While it doesn't necessarily need to ship with any Distribution wanting to support it, the Features will have to be created in a dev environment where the render alteration has taken place.

This particular approach is also a relevant enabler for feature requests like #747028: Views export into separate files.

It also implies that separate modules could combine a render alter with hook_form_alter() to define new options in the feature (re)creation page and handle the practical results on their own.

OK, here is hopefully the final take on this issue. Based upon the suggestion in #44.

This patch now only does 3 very simple things:

1) Adds a drupal_alter in the features_export_render_hooks to give external modules a chance to modified the generated exportable code.
2) Modifies the taxonomy.inc to clean up the exportable code to look more like other exportables.
3) Includes the requested documentation in the api.php file

The namespace was easy in this case: drupal_alter('features_export_render' ...

It's up to an external module (like Feature Overrides) to handle how this routine works. I'll post an update to my sandbox later today that uses this new hook properly.

Seems like this is now a no-brainer for features. It really leaves everything up to the external modules such as Features Override to do the heavy lifting.

Rerolled to clean up the documentation a bit. It's now in line with the docs standards, except for in the places that all of the other functions diverge (mostly the & in @param).

Assigning to hefox because although I'm now 100% on board with this, I'm still a stickler for protocol and we should have an RTBC first.

I like the current patch; it adds multi use new functionality and looks clean.

(WTF @ taxonomy being so non-standard, tsktsk! :O!)

@nedjo I've come to realize that being able to alter the actual generated output per component is very useful. That's all this hook provides, it doesn't at all change the output until implemented.

What the Features Override module chooses to add to the end of the export is their decision :)

Not great to see stubbed out documentation, a simple usage example is always a good idea.

Other than that, this looks good to me.

Sorry to rain on everybodys parade, but I fail to see how the latest patch is of much use.

While it does allow features_override to kick in it's own alter function, it's pretty useless, in my opinion, as it requires that features_override was enabled when the original feature was dumped. We'll just get 2 kinds of feature modules: overridable and not overridable, and likely most of the latter. Relying on every feature developer having features_override installed and activated is doomed to failure.

Secondly, can anyone think of an usecase for adding code, or otherwise modifying the generated code, apart from the overriding issue at hand? Seems to me that this is a very specific general solution.

As for the original problem, and maybe this got lost in the amount of posts, or on IRC, but I don't see why the alter hook cannot be explicitly stated in hook_features_api(). The default function there is already declared there. And until every feature providing module has that, an new features_api_alter() (which really should be there, if you ask me) would allow people to fix up existing modules without too much trouble. It doesn't solve the problem of components that doesn't have neither a native, nor features provided alter hook, but that I consider a bug in any such component (a bug that could be somewhat mitigated by the current patch) . This is having the advantage of not adding in another set of alter hooks, while still making features_override able to discover the proper alter hooks to implement.

Several unrelated use cases have already been mentioned.

But you are correct, this does require Features Overrides be installed at the time a Feature is created.

So here's the question: Is Features Overrides a core concept to the Features module, or is it a specialized workflow for some distributions? Should it be part of the Features module, or at least the Features project?

We don't really use Features Overrides, we use the alter hooks. I suppose a stable, working Features Overrides would be worth exploring, but there are issues of performance and architectural design in pursuing that.

I think that keeping Features Override separate for a while is a good plan. No need to make Features any more complex at this point and it's easier to work on a separate module than to go through 60+ comments on every little issue. Once I get a bit more testing and convert my sandbox module into the 7-2.x release then people can start really beating on it in the issue queue over there to iron out performance, etc.

Perhaps many months from now when it's all stable and clean then we might consider merging it into Features itself as core functionality. But for now let's take a step-by-step approach.

Btw: I have updated the README.txt file in the Features Override 2 sandbox with more documentation and usage scenarios. If you are unsure how/why Features Override is needed, check out the usage cases there. I also have a new blog article about using this that should be posted next week sometime. I'll post a link here when it's available. If you aren't building large/complex sites, you might not see the use for Overrides (or even Features). But I've been working on Overrides for many months because it's absolutely required to deal with the situation of a client making changes directly to the Drupal UI on a production server and needing to capture those changes into override features. Having a working version of this module has saved me on this specific site. I could not live without it now.

And yes, that means I'm running my Features Override 2 sandbox on a real production site for a big client that gets a ton of traffic. It works (for me ;).

So here's the question: Is Features Overrides a core concept to the Features module, or is it a specialized workflow for some distributions? Should it be part of the Features module, or at least the Features project?

It needs to be supported by the features module. You never know when you'll have the need to override, and you just know that if it needs to be explicitly supported by the feature developer, it wont be when you really need it. And if we get everybody to support it, then it really need to be a part of Features? So I'd say that whatever is needed to support that a features override module can be installed, activated and work without rebuilding the features you're trying to override.

But could someone tell me why explicit alter hooks in feature_api isn't an option?

I still don't see the need. When exporting an override, it knows abort the alter hook, it should've just render it to the override feature and be done with it. Whatever other hooks might exist doesn't matter.

Just thought I'd throw my vote in as someone who really wants this to become a core part of Features. My use case is that I need to take a distribution maintained by someone else (Acquia Commons), and place my own functionality over the top of theirs. As I understand it, with the patches as they are now, if they don't choose to use the Features Override module themselves, then this is all for nothing!

Would it be useful if features_api included a key to say what, if any, alter hook is supplied for this component, and have the ability to add in the alter hook if one is not already provided? I believe that's what I was aiming for in #984472: Add hook_node_info_alter old patch.

To clarify, the goal is

• Every exportable component has an alter hook, one way or another.
• It is possible for other modules to find out what the alter hook is for any given component.

Between those two, have we covered the core problem Features Overrides needs solved?

(I'm going off the idea that a universal alter hook is basically a quick way to meet the above criteria.)

If that's the case, then my planned revising for #984472: Add hook_node_info_alter should satisfy that.

So, does the patch in #984472: Add hook_node_info_alter satisfy the need?

@Xen: The issue is that the existing alter hooks in Features do not allow you to actually modify the exported code. That is what is needed for a proper Features Override module to work. The existing Features Override that attempts to utilize the existing Features hooks does not fully work. Not sure what you mean by "don't see a need" since it is clear that many people need the functionality of overriding features. We are simply trying to find a clean way to support that which actually works.

The main issue with the proposed approach is that it provides no way to distinguish between feature components that support altering and ones that don't.

True, but right now Features Override 2 doesn't really care. It is certainly possible for it to export some code as an "override" that doesn't actually cause the base feature to override properly (because of whatever issue in the 3rd party exportable). As an example, the core Taxonomy feature doesn't work without the code changes listed in this patch and would be an example of a feature that didn't (before) support overrides. I like the idea of just adding a flag to the API info for an exportable. But I don't see using your #984472: Add hook_node_info_alter method. We don't need to add any additional alter hook names for this. We just need to know if the exportable supports being modified at the code level. Your issue is an overkill for the issue being discussed here. And I'd like to see it get as much scrutiny as this issue has gotten. The current patch in #55 is simple and clean, so let's keep it simple.

I think this issue is getting confused between the *existing* alter hooks and the proposed *new* render alter hook (which *is* universal)

@mpotter:
What I can't see is the need for is an render_alter function. As I see it, it's just an odd workaround for inconsistencies in regular alter hooks, and while I'm told there's "several unrelated use cases" for it, I still fail to see the utility.

I know a lot of people would love overridable features, I'm one of them, I just don't understand why an render_alter should be the best solution.

Let's take an example. And let's assume that the issues of missing alter hooks is resolved so all feature components has a corresponding alter hook, either deductible from the component name, or explicitly specified in hook_features_api(). Then, in order to override a view, features_alter has to generate the code that does the altering and render it in an MODULE_views_default_views_alter() function when the override feature is generated, and be done with it.

Using the patch at hand, features_override first have to add in it's general alter hook (which requires the feature being overridden to be rebuild), and then the only thing that changes is that features_override renders the altering code into it's own hook instead. I can't see why this should be an improvement.

@Xen: I think you need to go off and play with this patch and the sandbox module. I've worked on this for many months, along with some other people and the render_alter hook is the ONLY solution we have found that allows overrides to work for all types of Features. There has been a ton of work and lots of discussion in various issue threads. I'm not going to review all of that discussion and work here. I leave it as a challenge to you to come up with a better solution.

However, after discussion with febbraro, we agreed that Features itself should be implementing this new render alter hook (rather than Features Override) to add the appropriate helper code to support overrides. This will avoid the issue of needing to have the Features Override module enabled when actually exporting your Features. With this solution, all Features exportables will get their output properly modified to support overrides. Whether you enable the Features Override module itself is still up to you. Or perhaps other modules will make use of these code alter hooks.

So what I have added to this version of the patch are 2 new routines in the features.module file:

1) features_features_export_render_alter(&$data, $context)
Implements the new alter hook called by the features_export_render_hooks function. The purpose of this function is to add the helper function in (2) to the exportable code output. It looks for the "return $varname" line and inserts the helper function call just before it.

2) features_alter_code($component, &$data)
This is the helper function added in (1) to the exported code. It calls drupal_alter with the hook features_code (so your override module would implement modulename_features_code_alter). This is just a wrapper around drupal_alter to make the code added to the exportable in (1) simpler. Easier to make changes to this helper function as needed in the future.

Here is the patch. Please review this patch at a technical level. While I appreciate the discussion and ideas that have come out of this thread, we need to get this tested and implemented. If you want to talk about philosophy/need of features overrides, please start a new issue.

Coding style only review.

+++ b/features.moduleundefined
@@ -836,6 +836,29 @@ function features_enable_feature($module) {
+/* ¶

Should be /** and no trailing space

+++ b/features.moduleundefined
@@ -836,6 +836,29 @@ function features_enable_feature($module) {
+ * Implements hook_features_export_render_alter

Missing trailing '().'

+++ b/features.moduleundefined
@@ -836,6 +836,29 @@ function features_enable_feature($module) {
+ * to add the drupal_alter helper to the exported code to enable override support

Leave a blank * line in between, make it a full sentence

+++ b/features.moduleundefined
@@ -836,6 +836,29 @@ function features_enable_feature($module) {
+      // search for the "return $varname;" at the end of the exportable

Should start with a capital and end with full stop

+++ b/features.moduleundefined
@@ -836,6 +836,29 @@ function features_enable_feature($module) {
+      $data[$default_hook] = preg_replace('/\s+(return\s+\$([a-zA-Z_]+);)/', ¶
+        "\n\n" . $alter_line . "\n\n  " . '$1', $code);

No need to split this over two lines, unless you split $pattern, $replacement, and $subject into different assignments for readability

+++ b/features.moduleundefined
@@ -836,6 +836,29 @@ function features_enable_feature($module) {
+ * Helper function called at the end of exportable code to add the alter hook for modifying the data

Over 80 chars, doesn't end in ., and probably doesn't need to start with "helper function"

Also needs work for: Anything added to export needs to make sure the function exists as features is not an explicit dependency to a module generated by features unless it uses one of the exportable features adds (user permissions, fields, etc.).

@mpotter: What kind of attitude is that? You're telling me that this is the only working solution, but refuse to explain why? Instead you do a handwave and expect me to dig through code and unspecified issues, in the hope of enlightenment magically hitting me. You've worked on this for months, you should be able to quickly pinpoint the issue.

I leave it as a challenge to you to come up with a better solution.

I've suggested an alternative, but you wont tell me why exactly you think it wont work. I'm not saying that it's something that'll just magically work out of the box, it probably still requires some patches here and there, but I do believe the end result will be overall better from an architectural viewpoint.

Please review this patch at a technical level.

I am, and it just feels too hacky for me.

Also needs work for: Anything added to export needs to make sure the function exists as features is not an explicit dependency to a module generated by features unless it uses one of the exportable features adds (user permissions, fields, etc.).

Would your preference be to (1) add a check for "function_exists" or (2) to convert the helper function call into a direct call to drupal_alter? The only issue with (2) is that since drupal_alter passes $context by reference, the one-line addition to export becomes two-lines something like this:

  ...
  $features_context = "views_view";
  drupal_alter('features_code', $export, $features_context);

  return $export;
}

Fixed coding style from #77.

@tim.plunkett: what tool are you using for that? Is it the coder module or something better? Whatever it is, I need to get it.

You've worked on this for months, you should be able to quickly pinpoint the issue.

Yes, the issue is that the alter hook needs to be called from the actual exportable code and not from the Features get_defaults routine. If you want more details, then you need to create a site, create some Features, test the sandbox and patch. Then compare it to using Features Override. Then create more complex features. There is no way for me to condense months of work on production sites down to a simple post here. This is a complex issue and not something that casual thinking will really help with. You need to dig into it with a real site.

I am, and it just feels too hacky for me.

If that was the criterion for patches or modules, then half of Drupal wouldn't exist. Seriously, you think Features Override itself isn't a complete hack? Even Features has some seriously complex issues in it. This new render_alter hook is easily documented and easy to use. I don't see anything "hacky" about it. And without real technical reasons, that's not going to stop it from being committed.

Sorry but I'm getting very frustrated with this. On one hand I'm flattered that this little issue has generated so much commentary. Wish half the other stuff in some other modules got the same scrutiny. But on the other hand, I'm about to say "enough" and commit it so people can start using this functionality. Honestly, once you see new Features Override 2 in action you won't want to go back. I need to do more docs and probably a screencast, but it's truly functionality that I wish I had a year ago. It solves a huge problem with Features for people (like me) working from distributions who need to deal with client or site-specific overrides cleanly.

It's http://drupal.org/project/dreditor. Still manual, just way way way better.
Install it and you'll see all the trailing whitespace that's still in there. :)

I'll be blunt, because I'm like that.

Commit it as is and you'll make people angry at you and phase2.

The reason this issue has received so much attention is that a single company (Phase2) is forcing a hack into an extremely important module. While Phase2 is the core maintainers of features, Drupal is a community.

So, please, don't just commit it. The issues with this patch have been well stated by many.

We understand your use case and desire, but the change is hack and not good for features, and there has been multiple potential solutions mentioned to address it.

Edit:

A feature is a set of exported code to meet a certain use case -- for example, a question and answer feature. It may or may not depend on features. The features dependency is only due to some items not being exportable via it’s providers (e.g. cck’s fields, core’s taxonomy) so features needed to step in and provide support for that. Ideally, this would not be case. Features is a tool to make exportable-driven modules for specific use cases. The same module, ideally, could be made without features via manually exporting/programming each component. Features is a helper module.

For features to add “features” specific functionality to each export is counter-intuitive and limiting. The exported code needs to be have been exported with features (or the module maintainer needs to add the code in explicitly). Meaning there is an essential limitation with this approach that fails to account for all potential use case. It makes features override a module only for overriding features, instead of the more useful module for making overrides exportable. IMO: Features overrides should not be dependent on features other than for a tool to make it’s overrides exportable.

This change is not a simple extra few lines of code running every so often hidden somewhere in the giant chunk of features code. It’ll extra lines of code to all features (to put that in perspective, there’s 42,233 feature installs vs 103 features override installs).

This change also does not help with developer experience. You’ve point out that others can use the hook -- however, in every case but hook_node_info, there is already suitable and use-able alter hooks for the components (that are usually documented, maintained, blogged about, etc.) that can be used however the component is exported/programmed. This hook is less useful and developers will be confused on which hook to use and what the difference is.

Another important issue is that hook is embedded in the default hook. Thus, features or someone else needing it cannot get the unaltered version of the code (without having to do something really ugly).

My ideal development situation with features, features overrides, and using alter hooks:

• Features/modules can retrieve the unaltered definition of components.
• Features/modules can retrieve the altered definition of components.
• Features update would update the feature, but without any alterifications, including ones from features overrides and any custom/contrib module -- this is a separate issue, but would be hampered by the current patch. (#838612: Remove Alterifications from export is the corresponding open issue for this.)
• Features override could be used for any exportable no matter how it was exported -- for example, could export the changes made taxonomy_term view that views comes shipped with. This patch would not support that as limits it to “features” made modules.
• Features would not show as overridden when instead they were just altered - this issue you bring up as reason to go forward with this patch, but this it is a separate bug. “Fixing it” via this patch isn’t fixing it; the bug will continue to exist for everyone else. [766264] and #1345174: Alter hook implementations lead to false overridden status for features components are the corresponding open issues for this.

This is a hack to support a specific use (features overrides) that is done to get around existing problems. Adding a hack to cover existing problems is the wrong path for manageable, well performing, good code.

Fix Features, not hack.

My proposed path:

1. Fix existing feature bugs
2. Continue with features override 1.x, with some special handling for certain exportables (like feature itself does)

Note that hefox used things like clean grammar, styling, bullet lists... highly unusual.

Needs Work on the current solution because of the Features Dependency, but a broader Needs Work is fairly implied in what many folks in this thread are saying.

Obviously it is the maintainers prerogative to override, but if this were a democracy of maintainers, I'm seeing a split vote...

OK, so obviously the latest patch is a step backwards for people here.

Can we compromise and go back to the patch in #55 that adds the needed alter hook in features_export_render and pushes the actual code changes back into the Features Override module?

Not doing anything to solve the Override problem isn't an option. The comment "there’s 42,233 feature installs vs 103 features override installs" illustrates that the existing Features Override solution is broken, which is why it's not being used (in my opinion). My work to fix the current 1.x release all resulted in dead ends. I have not found any way to make a workable and general-purpose Overrides module using existing hooks without the new render_alter hook. My Features Override 2 sandbox uses a lot of the existing code from Features Override but uses the new render_alter hook. It works great. It is much much cleaner and easier to use than the existing Features Override 1.x module.

So what I'm repeating is that "fixing" Features Override 1.x is not possible (for me). If someone else wants to take a stab at it, go ahead. But I've solved the problem in Features Override 2. All it takes is this new render_alter hook. At this point I don't really care if the alter hook is implemented by Features Override or by Features itself...I just want to get the call to drupal_alter added to features_export_render.

In #57 we had this in the RTBC state. Can we get back to that state and decide what is needed in the #55 patch to get this completed.

You've gotten me interested in features overrides now -- in getting the 1.x branch to work (and then using it) for the reasons stated earleir about how it'd be a more useful module.

I don't have time today, but could you give me the weekend to see if I can can get it working a bit better, before you pursue the 2.x branch further? I know you've tried, but I have "fresh eyes" on the problem.

I think the render hook is useful, but not necessarily for features overrides.

OK. Things to look for (that my Feature Override 2 handles, but I had trouble with Features Override on) are:

1) For Views, try changing name of view, adding a field to a view, and removing a field from a view in an override
2) For Fields, try changing display settings, such as the Image Style of an Image field in the teaser mode
3) Create a Feature that defines a content type and a couple of fields (article title, body, image). In a separate Feature, add new fields to this content type and also add Overrides of the display settings for the base feature fields (e.g. add a alt_title field, then add an override for the original image field)

Those were my two most extreme test cases (Views vs Field faux-exportables) and represents some of the complexities of the production site I was trying to get this all working on. Think of things from the point of view of a site trying to use a distribution (like Open Publish) where you don't want to change the provided Distro features directly and need to do everything site-specific in separate features.

While you are at it, I'd appreciate it if you also installed my Field Override 2 sandbox and then did the same things just to compare the two modules and approaches. Give my new module the chance it deserves. The fact that not many people are using the existing Features Override tells me that just because it was first doesn't necessarily mean it was best. It was just doing the best that it could with the hooks that were available to it. Docs for Features Override 2 are in the Readme file.

The Features Override 2 sandbox doesn't yet pass muster in terms of coding style, etc to make it a real module yet (or v2.x of Features Override), but it IS being used on several of my production sites already, so the code is complete and pretty well tested.

OK, here's a quick stab at doing it with alters: https://github.com/xendk/features_override (don't want to create a sandbox project until they're deletable).

There's a required patch to features included. As there is no way to get a reference to an argument via func_get_args(), we need to be able to specify the arguments (to support non-object feature components). It's a quick hack that needs to be refactored.

Secondly, it wont work with any default hooks where the alter function doesn't exist or isn't named like the default hook (hook_image_styles_alter is one example). This speaks for specifying the alter hook in features_api (which in turn suggests hook_features_api_alter). Which means fields doesn't work until features provides an alter.

The states of overridden and overriding features seems most sane with the patch from #766264: Alter hooks causing status to always be overridden, but still the overridden feature likes to be in the overridden state. Which probably only is solvable by something like #838612: Remove Alterifications from export.

Works for my views, someone else break it now.

Which means fields doesn't work until features provides an alter.

I spoke too soon. Seems to work out of the box.

Secondly, it wont work with any default hooks where the alter function doesn't exist or isn't named like the default hook (hook_image_styles_alter is one example). This speaks for specifying the alter hook in features_api (which in turn suggests hook_features_api_alter). Which means fields doesn't work until features provides an alter.

Exactly! Isn't that the motivation behind this entire thread? "Provide a universal hook for altering default components". I cannot imagine going to every author of every feature exportable and asking them to support an alter hook just so we can do overrides properly.

As you saw in your #91, Fields do work because Features itself is calling drupal_alter($default_hook...) in get_defaults. It does still have issues with things being marked as Overridden as you mentioned. It's not a clean solution when you really start trying to use it on complex sites. It's an approach I tried to get working a couple months ago and eventually gave up on.

Seems like adding the single drupal_alter call to features features_export_render_hooks is preferable. My proposed method would allow us to override any feature even if the feature author has not specifically added support for it. Which is what I want if I'm building a site using various distributions that provide their own features that I need to tweak for the specific site.

To tell if any solution is working or not: If *anything* is still marked as "Overridden" when using an Override feature, then it's failing. It's not just about whether the data is being modified, but it's also about how it works in the UI. My purpose with overrides is to have everything marked as "Default" unless the client has specifically changed something that is not captured in the original feature or the exported Override feature. If something is always marked as "Overridden" then I can't tell what the client has changed recently.

Changing the topic to reflect what we're really talking about -- how to provide support features override better. The universal hook is one discussed way, which has numerous issues as mentioned before.

I had not as much time as I thought this weekend, but I did look into a few things.

• I'm not at all surprised fields can have issues. They're implemented in a way that's asking for problems (a way that causes unneeded duplication). Patch to clean that up #1064472: separate fields from field instances. Please review/offer opinions on this.
• Both features 1.x and mpotter's branch have coding standard issues. Code standards are easy to clean up (run it through coder, fix the major issues), so let's fix them in both branches? #1061564: Fix coding standards issues is a patch for 1.x (Xen's prob has an automatic white space trimmer, as xen's commit has white space changes mixed with functional changes, which is bad for reviewing). Also, can features override 2.x get it named features_override instead of features_override2?
• Xen, noticed you manually called features_api, it'd be nice if reviewed #1390848: Centralize the call to hook_features_api() which centralized the invoking of that hook.
• Issues filed in features that have to do with false overridden status: #1345174: Alter hook implementations lead to false overridden status for features components, #1042088: Feature stuck in overridden state due to buggy hook detection, #1254630: Feature Overridden but Components Default, #1355766: Says overridden when not, #969860: node type feature is marked as overridden though nothing has been changed, #1273356: Text format feature remains in overridden state no matter what. (this is not all, just a sample, and some are specific to component, but get generally idea there). Overridden status has issues and should be fixed. It's a bug and should just be treated as any other bug -- fix it, not work around it.
• After looking at both branches, it does look like xen's take on features override 2.x looks like a good direction, but need to play it more (probably Tuesday; going to a drupal meetup tonight but without my laptop :( )..

In reply to mpotter's last comment:

How a module implements it's exports, and altering of it's exports, is prerogative of the maintainer and adding one for them is presumptuous and bad practices. Thus why #984472: Add hook_node_info_alter, so an exportable can say what it's alter hook is and if it exists. (with hook_node_info_alter a unfortunate part due to frozen feature request core. Due to the reasons stated above against the universal hook, I don't like hook_node_info_alter added that way, but see it as desirable because there really isn't any other nice way to do it. that's not the case for majority of exportable; have we found a single other exportable without an alter hook?)

I can see two paths here:

1. Fix features - slower, thorough, don't know for sure if this path will provide useful features overrides.
2. Fix features overiding cappability - faster, hackish(?), working concept tested in production sites.

Regardless of all discussion here, as a person who wants to build sites today (I don't want to wait for D8) on top of Drupal distributions, overriding features without loosing ability to upgrade distro - I vote for solution #2 that febbraro and Mike are proposing:

However, after discussion with febbraro, we agreed that Features itself should be implementing this new render alter hook (rather than Features Override) to add the appropriate helper code to support overrides. This will avoid the issue of needing to have the Features Override module enabled when actually exporting your Features. With this solution, all Features exportables will get their output properly modified to support overrides. Whether you enable the Features Override module itself is still up to you. Or perhaps other modules will make use of these code alter hooks.

No hard feelings, just let's get things done. Please.

Thx.

The solution we're proposing, once working, would work out of the box with existing distros. It sounds like, other than existing feature bugs that people are already encountering in other situations and need fixing anyway, Xen already has a working solution.

The solution that mpotter is proposing would need all distros to be updated to support it. And it's hackish and violates various best practices.

@hefox: Thx for reply and clarification.

I like your way more than anything, that everything work out of the box for existing distros..but then, I wonder why febbraro and Mike - with so much experience in building Drupal distributions - didn't came up with such idea - and supported it in this issue? :/

Is it just time and money factor? Not enough to fix all the issues? I don't know, probably..although they said that some time (and money) was invested to find out best solution to this problem..right?

I'm not expert, but when I see bunch of issues and a patch (w/ accompanied module) that work - I'm really tempted to select what works now. :|

I will put my vote on hold for now.. :)

Exactly! Isn't that the motivation behind this entire thread? "Provide a universal hook for altering default components".

It is a quick hack, a proof of concept. The solution is not a brand new universal hook, the fix is to let the feature component specify it's alter hook in hook_features_api (which also has the advantage over the current assumption method that we can be sure that we hit the right one).

I cannot imagine going to every author of every feature exportable and asking them to support an alter hook just so we can do overrides properly.

Well, I can. Quoth Grayside:

@jamsilver all the Features exportables should have alter hooks already. They may be under-documented, but they are there.

I'll add that a whole lot of modules is using CTools to implement their exporting, which does altering. Which leaves a few exportables to fix.

As you saw in your #91, Fields do work because Features itself is calling drupal_alter($default_hook...) in get_defaults. It does still have issues with things being marked as Overridden as you mentioned

Well, that explains it. And no, fields seems to be the one with the fewest issues with erroneous overriden state (as in: I haven't seen it, but not ruling anything out), with or without the #766264: Alter hooks causing status to always be overridden patch.

My proposed method would allow us to override any feature even if the feature author has not specifically added support for it. Which is what I want if I'm building a site using various distributions that provide their own features that I need to tweak for the specific site.

It might, but if we add in a specification of alter hook to feature_api, feature component developers will soon adopt it. And with a hook_features_api_alter, you'll be able to fix up any module that haven't been updated (and no alter function at all is a bug report).

To tell if any solution is working or not: If *anything* is still marked as "Overridden" when using an Override feature, then it's failing.

Or showing issues with the state checking code. The reason your method (let's call it the 'render alter method', as opposed to the 'component alter method') seems like it works better, is that it alters the defaults that features sees to include the overrides. The problem with that is that features (or anyone else) can't distinguish between default and alters.

Using the content alter method, we have a chance of keeping things separated. One idea I have, is using a bidirectional diff format in feature_override_defaults, which would allow us to undo the alters from an DB object, to determine whether it's the override, the original feature or both, that's been overridden by the database (#838612: Remove Alterifications from export would be a requirement for this).

My purpose with overrides is to have everything marked as "Default" unless the client has specifically changed something that is not captured in the original feature or the exported Override feature. If something is always marked as "Overridden" then I can't tell what the client has changed recently.

Well, it's mine too, we're in complete agreement there. However, I'm also trying to take it a step further, so that a carefully selected and ordered features-update sequence would be able to update both override and base feature, without simply dumping the fully altered object to the base feature.

Xen: Did you mean the change the title back? It sorta looks like you opened this page back on comment 92, then didn't refresh to check for new comments since then.

Ahem... Doh...

Eeck, comment #100! :(

I'm not going to have time this week to look into this. I've got a major site due. I would encourage *everybody* interested in this to install Xen's variation of my Override2 module and start testing this to see what else needs to be fixed. It it really works with existing alter hooks, then I'll be all for it. I remember going down that road and having lots of issues. So I can't sign off on it till I really have some solid time to look at it.

So please more testing before we come to conclusions.

I like your way more than anything, that everything work out of the box for existing distros..but then, I wonder why febbraro and Mike - with so much experience in building Drupal distributions - didn't came up with such idea - and supported it in this issue? :/

I would definitely prefer a method that worked out of the box for existing distros. I just couldn't come up with something that worked. But this also means that Overrides should work out of the box for existing features. As a site developer I shouldn't need to worry about trying to get all of the features creators to update their module. I need something that works for my site right now today. If all ctools features automatically have the proper alter hook, then I might be happy enough with this. I could only verify that Views worked.

Initial good news! I have played around with the code Xen posted in #90. Made a couple minor changes, but overall it seems to work so far with my testing. Right now I've only done overrides for Fields and Views, but so far so good.

Will probably look at some variation of #984472: Add hook_node_info_alter to allow non-standard alter hook names.

The key piece of information that I was missing in all of this discussion was that Views itself already provides the hook_views_default_views_alter call. I wasn't aware of that. Along with the existing call to drupal_alter($default_hook...) in features_get_defaults, it seems like the major features should now be covered.

What I like about this approach:

1) It maintained 90%+ of the code for Features Override 2 and was a simple matter of tweaking how the alter hooks were handled.
2) Unlike existing Features Override 1 which tries to implement all of the various different alter hooks itself (which is what led nedjo to post this issue initially), the alter hooks are called from the actual Override exportable code. This is nice and clean because the override exportable knows exactly which feature component it is trying to override. So it can implement the specific alter hooks as needed. Keeps Features clean, keeps Features Override 2 clean.
3) The exportable code for existing features is unchanged. I agree that this is a big win. Distros do not need to care about providing specific override support.

What's amusing is that the patch to Features itself comes down to allowing arguments to be passed for alter hooks, much like I was doing in #1277854: Faciliate altering features, e.g., export field display settings separate from field data structure #17. Although I like the method Xen used to handle arguments a bit better. Should be a trivial patch for Features now.

So, here is the new patch for review. I will also update the Features Override 2 sandbox to reflect these changes.

@mpotter That's great to hear.

I'm not too happy about the arg passing hack, but I couldn't come up with an elegant solution quickly. So if someone can come up with a better method, do step up.

Well, it was better than my previous hack which just checked the name of the function for "_alter" at the end to add &$data.

Just updated the Features Override 2 sandbox to comply with Drupal Coding Standards. So definitely have a look and let me know if you see anything else that needs to be addressed before I push it as the 2.x release of Features Override. Will try to do that next week pending reviews.

@mpotter Views API is large and poorly documented (IMO).

That's the kind of thing I was trying to get to, without understanding the problem. It's the missing piece that resulted in a lot of the cross-talk.

Updated issue summary.

Not necessarily, it just needs to check if the returned value is an array or not and act differently if so.

It's ugly, but:

if (is_array($return) {
 // do advanced rendering
}
else {
 // function like normal
}

(Course, it'd be nice if the return of that function was a render able array instead, but that's not going to happen anytime soon).

@nedjo
As I said, it was a quick hack to get somewhere.

@hefox
I thought of that, but I can't remember if I found issues with that. Returning an array has the advantage that we can add more data. A 'file' key for overriding the file the function is written to, would be nice. As it is, I had to make the feature_override render to *.features.inc because the alter hooks needs to be loaded at all time and features only lets you define the file for the entire component. A 'file' key would enable us to dump the feature_override hook to its own file, but still dump the alter hooks to the core features file.

Here's the above idea with is_array() checks

And here's a github repo https://github.com/hefox/features_overrides3, in no way complete, that produces an output that looks like:

/**
 * @file
 * fake3.features.inc
 */

/**
 * Implements hook_views_default_views_alter().
 */
function fake3_views_default_views_alter(&$data) {
  if (!empty($data["blog_list"])) {
    $data["blog_list"]->display["default"]->display_options["title"] = 'Blogs Edit';
  }
}

Thus, the generated feature won't need features override, making features override purely for feature creation/updating. :O!

Horribly dirty/messy (nothing is commented) repo; only posting it now cause need to sleep

@hefox: would love to see a patch against my existing Features Override2 sandbox in my issue queue rather than a completely new module. Remember that I'm going to be post this eventually to the Features Override module as the 7.x-2.x version, so anything you could do to make that work easier would be appreciated.

What I don't like about putting the overrides directly into the Alter is that then you don't have normal "default hook" code for your override feature. That makes it difficult to use Features to handle doing the diffs and override detection since get_defaults isn't going to see your override data. I'll need to look at your detailed changes, but it seems like you have really deviated from making overrides work like other features just for the sake of making the override code independent of Features Override.

But I'd like to have detailed discussion on Features Override get moved over to the Features Override issue queue, or into my Sandbox queue. Let's try to keep this thread on track with the changes actually needed for Features itself.

So back to the args stuff...

#109 is a bit confusing because the patch given there doesn't have much to do with the code being shown or the link to the github. But in terms of handling code arguments better, the patch itself is clean and simple and I like the idea of using an array to detect the difference between just code and code needing arguments. So I am marking this patch as Reviewed by Community since it works great for me here and provides the needed support for alter hooks within features.

It'll be a patch eventually; threw it on github just to show where its. Going -- ie just a temporary fork

edit: the relation between the patch and the github sandbox is I was testing the patch against features_overrid2 sandbox when I noticed how the alters were being done (which surprised me) and decided to try my take on it.

My current thoughts is that the actual alter would be best like above (minimize dependencies, easier to see what happens), and how the alters are manifested/visualized can be calculated and hooks dynamically added, or the alters visually in some other way. I'm going to poke around it on my free time ,though this week is a bit weird, there's a code sprint during san diego that might be ideal time to play.

@hefox: In your github repo, it looks like you were ignoring the new Features Override 2 UI (stuff in admin.inc). It calls functions that you have removed from the main module. To be able to test your alternate approach to alters I'd appreciate it if you can do it in a way that maintains all of the work done on the UI so it is easier to test. Right now your module completely fails on my test systems. I know we are coming from this from two different perspectives: you want overrides that don't require any module, and I want a Features Override module with a better UI for the majority of users who will keep the modules enabled. So let's try to arrive at a solution that meets both needs.

You might want to install my clean sandbox module and run through the demo in my blog: Features and Overrides-Part-deux just to see how the UI works so you can maintain that functionality.

Definitely interested in seeing if this works with just the alter hooks and no normal default_hook. It prevents any "overrides of overrides" in the future but I'm not sure that's a bad thing.

Committed patch in #109 to 7.x-1.x-dev. Closing this specific issue. Discussion can continue in the Features Override module. The other related pending issue is: #984472: Add hook_node_info_alter

reroll for drush_make application against features 7.x-1.0-beta6

Hey guys, is there any combination of commit hashes and/or patches and/or github repos and/or branches of features and features_override that would provide the most up-to-date testing setup? It's hard to know when certain readme instructions are out of date, and I'm either getting "illegal operation -- contact admin" warnings on override generation, or straight-up WSOD.

I'll keep at it and post back if I have luck, but if you have an easy answer, it would be greatly approciated :)

FYI, getting the "illegal choice" msg with these:

head of features_override-7.x-2.x:
http://drupalcode.org/project/features_override.git/commit/efb1bf27469ab...

head of features-7.x-1.x:
http://drupalcode.org/project/features.git/commit/83e0a6cb8a35e75645204c...

with this patch: http://drupal.org/node/984472#comment-5580524

Any thoughts?

Ok merged 82a8c17 to 6.x-1.x for & patched for review (took me a few mins to find the commit, since the message references a different issue).

Woooooo scott!

thanks