Add alter hooks for the "faux" exportables

Marking this as critical + task as it's something that should be in 1.0 stable.

The one issue with these OTOH is that most of these faux exportables call their hooks for a specific module using module_invoke() rather than module_invoke_all(). This means that the array of components being passed to the alter hook actually doesn't contain all the components. This at least means that these alter hooks won't behave like most other alters (e.g. hook_menu_alter(), hook_views_default_views_alter(), etc.) as you will only have access to a partial set of components.

I'm also curious as to what behavior you're seeing in terms of state detection - if the alters only run on rebuilds, do you end up with each component being marked as overridden?

This is essential functionality, imo.

My use case is that we want to add a 'related documents' field to the event content type from atrium_calendar. If we can't alter it, we would have to fork the atrium_calendar or deal with merges and so on. This is a clean way to allow other features to add functionality. Will keep this thread up to date with any valuable experieneces.

This patch wasn't applying cleanly for me on beta6 so I rerolled from the module directory.

Any tips on having this hook fire? I have turned on/off the module, reset to defaults, reverted, made changes to the cck field and updated the feature in an attempt for _features_rebuild() to fire off the hook but so far it has been a no-go.

It seems that $items is always empty in _features_rebuild.

So I've used this successfully. I hope I am not presumptuous setting to Ready & Tested. This patch is very very useful to responsible atrium development. Specifically that we can build and modify existing atrium features without blocking upgrade paths. I would love to see this in beta7.

There are rough spots of course. The checking of statuses for individual components seems to be wrong. Often says 'overridden' when it is not. The 'update path', i.e. getting a feature to rebuild requires a modification to the content type (as far as I can tell). It is going to be a hard/impossible thing to fit this into the UI.

However, for most part no one will be using this unless they know about it and are fairly strong developers. My suggestion would be to add this and iron out the details later since 99.999% of the time, no hooks will be registered.

PS if you saw my message about it not applying to latest CVS, ignore. It applies fine.

Marking my #775514: hook_content_default_fields_alter as duplicate, as number 2 last adds hook_content_default_fields_alter

I suspect the key to overridden is providing a way to remove changes by the module altering to what is compared

Manually added in some module_hook to check if module actually implements the hook. Was causing issues in the alter hooks as empty $defaults are passed in.

Fields added via alter make the feature altering show up as a dependency for the feature, and when the intent to is to be an addon, that is quite problematic.

Looking at the code, I think the most useful alter to achieve this, ATM, is
+ drupal_alter('features_populate', $pipe, $export, $module);

inside the helper function _features_populate instead of after the initial _features_populate call, as I want to remove references to the new field, not reference to the dependecy, as modules should be able to declare the add on as a depedency without that being removed. (_features_populate is a recursive function that goes through and turns fields to depedency if another module declares them.)

Updating to add fieldgroup, remove node_info (core hook, would also need change change _node_types_build to really be effective, ie hackcore)

Oops, need to call it after the !empty check

Updating patch

As I've just added Features integration to Custom Formatters, this issue is of particular interest to me as I would like Custom Formatters that are used by Views or Content Types being exported to also be exported.

I have tested this patch and the addition of hook_features_populate_alter() is perfect and does everything I need it to do.

Hope to see this make it's way into the next build.

Cheers,
Deciphered.

This will really help with the aim of making features work across different distributions.

Smallish changes:

* Instead of invoking each of these hooks separately, do so at load time via a helper function.
* Add some API documentation.
* Fixed minor error in 'features_populate' drupal_alter() call. Didn't add this to the API documentation though.

@hefox: could you explain why we need this new hook_features_populate_alter()? Why aren't the existing hook_features_pipe_COMPONENT_alter() and hook_features_export_alter() enough? If indeed it's needed, please also add the API documentation for hook_features_populate_alter(). Thanks!

Missed the features_populate hunk.

(Hm, missed those hooks, damn! But if functionality like #838612: Remove Alterifications from export gets in, I don't need that populate_alter, but Deciphered mentioned that they found it useful. Does the other hooks fit your need, Deciphered?)

@hefox

I could do everything I needed with hook_features_populate_alter() so I didn't even look at any of the other hooks.
I'll have to look into it and let you know.

Cheers,
Deciphered.

Tested and confirmed that hook_features_pipe_component_alter() works perfectly in place of populate_alter().

Thanks guys.

I think this still needs more review for the alter hooks specifcially?

Just a note: If latest #649298: Features install/revert does not remove additional fields from content type already stored in the db gets in before a part of it that will have to be rewritten also for this patch, as it's doing module_invoke_all('content_default_fields' and then disabled fields it doesn't find for features with nodes, which when adding fields via alter would likely screw ew up :P.

The change committed here (http://drupal.org/cvs?commit=395724) captures the essence of the concepts in #12 and #13: you can now call features_get_default($component, $module_name = NULL) to get the defaults for any features-supported component (both faux-exportables and regular exportables like Views, CTools, etc.) for any given module (or all if you omit module_name).

I've attached patch that would add drupal_alter() for any component. Note that rather than using hook_features_{$component}_alter I think it would be best to follow the convention used in both Views and CTools to call drupal_alter($default_hook). This will keep the code simpler and the convention consistent between all the exportables. Concretely, this means alter hooks would be named like:

• hook_content_default_fields_alter()
• hook_fieldgroup_default_groups_alter()
• hook_views_default_views_alter()

and so on.

As for removing/omitting alters, please see my notes here: http://drupal.org/node/838612#comment-3228270

After discussing with hefox on irc, we decided that this patch should go in first and then we should explore options regarding removal of alters.

http://drupal.org/cvs?commit=396152

Nice work everyone!

Wasn't sure whether to reopen or make a new one, but as it seemed very much part of this one.

Views invocation of hook_default_views_alter is for all currently defined views.

Due to the recent changes, now features invocation can be per module, or not.

This is quite problematic, when say, a module that depends on another goes and try to change that's module's default views. Errors everywhere >.O!

While I know the code can be changed to check if xyz view exists (and am), hook_default_views_alter isn't a feature defined hook, so it should likely match what their defining module does for best compatibility

Actually, there's three types of drupal alters
1) Current, variable per module or per modules invocation (module_invoke)
2) (module_invoke_all)
3) None (like node_info).

Looking over the code, don't know the best place to put it, as that function features_get_default expects to be able to be called only once, so it's all messy. Perhaps just not using the helper functions for views is the answer? However, it'd likely be more useful to have those hook define how/where their alter is done, but due to how features_get_default is done, eee!

  $cache[$component][$m] = module_invoke($m, $default_hook);
  if ($alter) {

Mm, this is running alter even if empty return from module_invoke

function features_get_default($component, $module_name = NULL, $alter = TRUE, $reset = FALSE) {
   ...
    if (!isset($cache[$component][$m]) || $reset) {
        ....
        if ($default_hook && module_hook($m, $default_hook)) {
          $cache[$component][$m] = module_invoke($m, $default_hook);
          if ($alter) {
            drupal_alter($default_hook, $cache[$component][$m]);
          }

As far as I can tell, The cache doesn't check weather alter is true or not, so unexpected behavior can happen when called without reset.

   features_get_default('content'); => altered version
   features_get_default('content', NULL, FALSE); => altered version, suppose to be unaltered

however

   features_get_default('content', NULL, FALSE); => unaltered version
   features_get_default('content'); => unaltered version , suppose to be altered

Here's a quickie patch for #23. Should I open a new issue for the views issue?

Hm, here's a re-work of 24 with some additional items.

Adds call a drupal_alter(node_info when rendering node info export, as that's the only way I can figure out to get a drupal_alter for node_info, then adds some stuff to indicate the drupal_alter in features_get_default shouldn't be run. Sorta half backed, slowly trying to figure out a way to deal with #21.

Made #984472: Add hook_node_info_alter for 25 instead

Somehow I think that was a typo. I don't want this to fall off the radar :)

Had a small bug in 25 and while looking over it figured it would likely be better in a new issue, so opened one with 25's patch instead and closed this one (though should have done 'fixed' instead of 'closed fixed').

Though, forgot about the views issue. mm

Fixed is generally reserved for issues that are being committed. Are you saying you think this issue should be discontinued and the discussion continued in the other issue? I assumed that issue depended on this patch already being applied...

If I'm understanding, I'd call this a Won't Fix for lack of something better.

The original part of this issue was fixed, but noticed some issues with what was done so re-opened, but when looking over the patch figured it'd be better as it's own issue then piggy backing on the previously fixed issue, so opened the new issue and closed this.

Oh.