function features_get_components($feature_source = FALSE, $reset = FALSE) {
   ...
    $components_all = $components_source = array();
    foreach (module_implements('features_api') as $module) {
      $info = module_invoke($module, 'features_api');

/**
 * Checks whether a component implements the given hook.
 *
 * @return
 *   The function implementing the hook, or FALSE.
 */
function features_hook($component, $hook, $reset = FALSE) {
  $info = &drupal_static(__FUNCTION__);

  if (!isset($info) || $reset) {
    $info = module_invoke_all('features_api');
  }

/**
 * Gets the available default hooks keyed by components.
 */
function features_get_default_hooks($component = NULL, $reset = FALSE) {
  static $hooks;
  if (!isset($hooks) || $reset) {
    $hooks = array();
    features_include();
    foreach (module_implements('features_api') as $module) {
      $info = module_invoke($module, 'features_api');
      foreach ($info as $k => $v) {
        if (isset($v['default_hook'])) {
          $hooks[$k] = $v['default_hook'];
        }

At least 3 different areas are invoking hook_features_api. It'd be better to add one centerlized cached function, right?

CommentFileSizeAuthor
#23 features-centralize-hook_features_api-1390848-23-D6.patch6.38 KBscottrigby
#19 1390848_features_api_19_7.x.patch662 byteshefox
#19 1390848_features_api_19_6.x.patch5.9 KBhefox
#13 1390848_features_api_12.patch6.94 KBhefox
#9 1390848_features_api_9.patch6.89 KBhefox
#6 1390848_features_api_6.patch6.71 KBhefox
#4 1390848_features_api_1.patch6.45 KBhefox

Comments

Grayside’s picture

Grayside commented
Title: Centerlized the call to hook_features_api » Centralize the call to hook_features_api()

Agreed. Seems like a fairly standard invoke_all wrapper.

function features_provider_info($component = NULL, $reset = FALSE) {
  $info = &drupal_static(__FUNCTION__);

  if (!isset($info) || $reset) {
    $info = module_invoke_all('features_api');
    // drupal_alter('features_api', $info);
  }

  if (!empty($component)) {
    return isset($info[$component]) ? $info[$component] : NULL;
  }

  return $info;
}
hefox’s picture

hefox commented
Title: Centralize the call to hook_features_api() » Centerlized the call to hook_features_api

Things to consider with this patch

  • Removes the (undocumented?) ability for a module to provide a different implemention of a component for the components that features provides -- only time I saw that used was with taxonomy, and it was broken anyhow. (this was the module_hook check in features_include)
  • Changes the definition of features_get_components() -- takes in a key or component to return instead of 'whether to return 'feature_source' stuff (which was only used in drush, so feels like a low impact change).

Why I think this patch is cool

  • Centerlizes the caching and retrieving information from hook_features_api
  • do to ^, allows to add and use new keys to that hook more efficiently.
  • Generally seems to clean some things up a bit, imho -- like getting rid of resetting module_implements!
hefox’s picture

hefox commented
Title: Centerlized the call to hook_features_api » Centralize the call to hook_features_api()

features_get_components is what I went with cause it is already in heavy use and was already caching all the information; not sure why it wasn't already being used in the other places.

hefox’s picture

hefox commented
Status: Active » Needs review
StatusFileSize
new 1390848_features_api_1.patch6.45 KB

sigh, the patch, oops. How many comments does it take to post a patch.. lala

Grayside’s picture

Grayside commented
Status: Needs review » Needs work

Variable names in features_get_components() are a bit confusing. Lots of in_ :P

Combined with lack of parameter documentation, the potential for confusion is high enough that I call it needs work.

hefox’s picture

hefox commented
Status: Needs work » Needs review
StatusFileSize
new 1390848_features_api_6.patch6.71 KB

The only $in_'s are the function arguments, aka two variables. Added the @params, though not the best explanation.

Better to do $in_'s then say, accidentally override the arguments in the function. *cough*

hefox’s picture

hefox commented
Status: Needs review » Needs work

Actually, ya, the $in_'s should change, but that's easy enough.

Patch still reviewable, just need a tiny bit of work to change some variables.

xen’s picture

xen commented

I've only eyeballed it, but it needs to be rerolled against head, doesn't apply any more.

Agree that the $in_'s should be changed.

In features_get_components:

if (!isset($all) || $reset) {
$all is not defined. Should probably be:

if (!isset($components) || $reset) {

Then:

/**
 * Returns component information for features defined by features.module.
 */

Shouldn't that be:

/**
 * Returns component information for feature source components.
 */

? Apart from that, it looks good to me.

hefox’s picture

hefox commented
Status: Needs work » Needs review
StatusFileSize
new 1390848_features_api_9.patch6.89 KB

Patch address the above and...

adds cache-ing of module_invoke_all features_api and drupal_alter. Not sure if it's worth caching this information, but generally think it's a good idea.

Also switches around args for features_get_components cause $component, $key makes more sense to me/seems more usual.

hefox’s picture

hefox commented

Ran all the current features test, and other than #1399440: Feature tests failure on image style against drupal 7.10 all passed.

xen’s picture

xen commented

I don't see any setting of cache?

hefox’s picture

hefox commented
Status: Needs review » Needs work

That's the 2nd time in the last two weeks I've forgotten that part. Duh. Updating patch.

hefox’s picture

hefox commented
Status: Needs work » Needs review
StatusFileSize
new 1390848_features_api_12.patch6.94 KB

When porting this patch to d6, nearly everything is the same but drupal_static and cache_set (saying to remind myself.

hefox’s picture

hefox commented

Some other notes on this patch:

  • it does change the function signature for features_get_components, but the chance of someone else using it to get only feature_source things seem rare (outside of features itself) and the places that were using it in .drush.inc have been updated.
  • Nothing clears features_api cache, but... what would change that information that doesn't involve a full cache clear? enabling a new module leads to cache clear, and that seems like the only thing that would be lead to a features api chance.
xen’s picture

xen commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

mpotter’s picture

mpotter commented
Status: Reviewed & tested by the community » Fixed

Excellent! Commited to 7.x-1.x.

hefox’s picture

hefox commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Patch (to be ported)

Don't see any reason not to backport this (keeping the two version as close as possible makes them easier to maintain). Need to update the patch for drupal_static and cache_clear differences.

Grayside’s picture

Grayside commented

Maybe features should just have a conditional backport of drupal_static, a la Real Name.

hefox’s picture

hefox commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new 1390848_features_api_19_6.x.patch5.9 KB
new 1390848_features_api_19_7.x.patch662 bytes

Found a bug when porting.

features_get_feature_components needs to return only components that have true value 'feature_source', instead returning components that have that key set. array_filter fixes that up. Updated description of function to try and clarify what it means.

Fix for bug attached and back port to 6.x (version number in patch name).

Grayside’s picture

Grayside commented
Status: Needs review » Reviewed & tested by the community
hefox’s picture

hefox commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Reviewed & tested by the community » Needs review

Talked to mpotter on IRC, he said I should commit it, so thus it is.

Anyhow, 6.x, up to bat!

hefox’s picture

hefox commented
Assigned: hefox » Unassigned
scottrigby’s picture

scottrigby
he/him
Primary language English
Location Brooklyn, NY
commented
StatusFileSize
new features-centralize-hook_features_api-1390848-23-D6.patch6.38 KB

Rerolled for 6.x-1.x.

So far seems ok but could use a bit more testing. Posting this patch for now & will look again asap

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Bump for testbot.

  • mpotter committed bdbf8a5 on 8.x-3.x 
    Issue #1390848 by hefox: Centralize the call to hook_features_api()
  • hefox committed e1b0187 on 8.x-3.x
    Issue #1390848: Fixing a slight bug from the previous patch that lead to...