Remove Alterifications from export

On my list.

So a quick reading of this patch makes me think that this approach is not sustainable.

In particular, I'm not happy about the idea of stripping alters out after the fact, and on a per-component basis at that.

To me, it would make more sense to take the following approach:

• For proper exportables, don't run alter hooks when comparing states and re-exporting default objects. This will prevent the overridden state from showing up when objects are in the default state and have been altered.
• For faux-exportables the situation is trickier. I think it may be possible to handle it with a more complex workflow, but I am still weighing the benefits for the added complexity:
  • Alters must run against the default state when rebuilding the component as the component will get written to the DB.
  • Run alters against the default state when comparing against the normal state. If the normal state has not been changed since rebuild, the object will not show an overridden state.
  • When exporting, compare states again (as above). If in the default state, export the object without alters. This preserves the object prior to alteration, allowing the alters to continue to occur and affect the object as expected.
  • If the state is overridden all bets are off. Object is exported as is (including alters and any user overrides).

I've been using the alter functions. They're great. Quick question related to this issue.

I think this is what yhahn was saying in #3, but I wanted to clarify (so I know whether to open a separate issue).

Once we have the ability to "un-do" an alter, does that mean that a feature that does an alter will have it's alters listed as components? And if the alters are overridden then the feature will give the option to revert? If this issue won't give that ability, then I'll open a new issue. I think what I'm suggesting is very powerful because it allows features to "stack" on one another and have their own revert ability.

Example: I can create a feature to modify the atrium calendar view (without creating a cloned view). Users can just enable my feature without doing any other switching and it has the maximum likelihood of working with every other cusotmization they have made (where as creating a cloned view could require a bunch of changes on their end).

That's the value in the "stacking" approach (as I see it). I think the ability to know if your _alters() have been overridden is a key part in that.

This seems to be a symptom of a bigger problem. Trying to generalize the changes in #5, I rolled a separate patch of mine, which introduces a hook that "builds" the exported arrays. In effect, it vaguely mirrors the Forms API workflow in that it builds the baseline data structures and then renders them separately. In between, a drupal_alter easily changes the array "build", as it were, right before it is rendered, almost as-is (apart from possibly reordering).

However, I've run into a little snag. To explain the problem, I should first present the import/export workflow as I envision it. There is a universal, base layer, in which data and objects are installation- and platform-neutral. Relationships between objects in the feature and beyond are linked by natural names, machine names, GUIDs, and so on. In other words, they persist everywhere.

Then, when the feature is deployed on a platform, the general use cases are instantiated in terms of the data on the site. Default functionalities in the feature are extended, data is modified, you name it. Modules on the platform that natively uses machine names can be properly namespaced according the KIT Feature Specification, and the IDs are used universally. Modules that do not must instantiate the objects, and for each relationship to these objects translate universal IDs to local IDs. This forms the instance layer, almost imaginable as the database of a drupal site in relation to its codebase.

Basically, the way I see it, here's how the current workflow would look after incorporating the changes mentioned in the first paragraph (N.B. build is not related to rebuild, but rather export build):

Export:  Instance => Build => Alter build => Render => Base code
                                          |
                                      [COMPARE]
                                          |
Import:  Instance              <= Import <= Alter defaults <= Base code

Where marked "[COMPARE]", Features compares these versions to determine component state. Regarding the "Alter defaults" step, I believe it is more logical to include any arbitrary change, as it is in the current Features source, in order to account for third-party modules that make modifications without concern for Features whatsoever just to piggyback their configuration. (Code cleanliness is not a problem here as it can be dealt with by having a separate drupal installation exclusively for feature development.)

Extra information, features-related or not, is introduced in the "Alter defaults" hooks. Some of it should go in the exported features for reasons mentioned in the previous paragraph. Anything that should not be included is generally not applicable anywhere else. In the most normalized sense, this means only local identifiers. The workflow above offers opportunities to universalize IDs just before rendering and localize IDs just before importing.

Here is where the problem begins. In the import workflow above, universal IDs can be translated to local IDs (disregarding race conditions for now) only in the "Alter defaults" step. However, this would completely defeat the purpose of code comparison since the base code has been prematurely instantiated.

In fact, everything to the left of "[COMPARE]" is the instance layer. Everything to the right (ignoring rendering processes) is the universal layer. A possible solution to premature instantiation is to fill in the apparent gap in the workflow above. The process effectively becomes (disregard the naming if it doesn't suit):

Export:     Instance => Build => Alter build => Render => Base code
                                             |
                                         [COMPARE]
                                             |
Import:  Instance <= Alter import <= Import <= Alter defaults <= Base code

Looking at the workflow this way, instantiation/universalization (and other low-level stuff) can be done to the left of "[COMPARE]". "Alter defaults" essentially becomes a chance for various modules to generally influence the parts of data structures that determine its significant state on both the left and right sides, as opposed to mere phases of the import/export workflow.

So, in response to #3, I think adding the build/import (you can come up with better names) addition is the fix needed for low-level alterations. High-level alterations should stay (per #693024: Add alter hooks for the "faux" exportables) since that's for a different purpose, i.e. introduce alter hooks for faux-exportables where there were none, like proper exportables.

My patch for the "build" part will follow.

Here's the aforementioned patch. Most changes are merely splitting hook_features_export_render into that and a preceding hook_features_export_build. The significant changes are in features.export.inc where:

1. The signature of hook_features_export_render is augmented with an optional $build = array() argument at the end. This is compatible with all the existing implementations.
2. Call to hook_features_export_build is added, followed by drupal_alter on the build data. Naming is not ideal, but that's a minor issue really.
3. A quick fix in features_get_normal to call features_export_render_hooks instead of invoking the render hook.

I've left ctools and context well enough alone, since I wasn't quite sure what to do with them (probably nothing because ctools takes care of most to be altered anyway).

I don't quite like the fact that it's all going in features_export_render_hooks. But, as it stands, the code actually works. I've tested it by exporting a feature containing bits of everything from the existing code, then checking/recreating the feature using the patched code.

I should mention that the patch in #7 doesn't include the import hook, since I'm not sure about the naming. Can any maintainer settle on a better name? I'd be glad to help reroll the patch.

This looks like about as graceful an approach as I can imagine short of introducing core hacks to the behavior of drupal_alter().

EDIT: I decided to take a quick peek at ctools to see where changes would need to be made to bring that on board. It makes sense to use the same pattern if possible. ctools_export_object() and _ctools_export_get_default() popped up as worth keeping in mind.

#766264: Alter hooks causing status to always be overridden has been marked as a duplicate of this, but after reading through this more closely, I'm not sure if the work here would address the issue of altered components causing a feature to be marked as overridden.

I suspect he's not looking over here at the moment :)

I didn't find this issue and took a different approach in #1352812: Default component alters are included when features are regenerated, where I sketched in a method that could be called to determine if a given feature was being regenerated, in which case a module would skip altering that feature.

I took a crack at updating the summary, including describing the problem, but I don't really follow what's being proposed in #6 and #7 so I left that part for others to fill in.

Didn't mean to change status.

Updating issue summary.

Clarify scope of problem.

Add disadvantage to Facilitate skipping of alters

Issue affected by old alterification method: #2113479: Fatal error: Call to a member function condition() on a non-object.

This seems old. Is this still an issue? Seems like with Features Overrides we have a clean alter system now. If this is still an issue maybe somebody can clarify it and update it.