Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

> We do it for nodes, users and comments

We don't actually... we implement the hook ourselves and declare all the entity types we support there.

That won't help with #1905004: Error when finding dependencies in flag_features_export() because IIRC we don't say which module provides the entity types.

I'm not sure there's much point to this, as we don't need a dedicated flag handler class for taxonomy terms.

alexweber’s picture

> I'm not sure there's much point to this, as we don't need a dedicated flag handler class for taxonomy terms.

If we implement taxonomy_flag_type_info() and list it's handler as flag_entity, then the taxonomy module will get correctly picked up as a dependency with exported flags on taxonomy terms (and vocabularies).

The same exact situation also applies to the core File entity. It's an optional module and the dependency won't be discovered otherwise...

joachim’s picture

There's no declaration of the providing module:

function flag_flag_type_info() {
  // Entity types we specifically cater for.
  $definitions = array(
    'node' => array(
      'title' => t('Nodes'),
      'description' => t("Nodes are a Drupal site's primary content."),
      'handler' => 'flag_node',
    ),
alexweber’s picture

Joachim, I mean literally implementing

function taxonomy_flag_type_info() {
  // ...
}

The module used as a dependency is, in fact, the one implementing the hook. Correct me if I'm wrong but if we do it on behalf of the taxonomy module, not fr it, then the taxonomy module will correctly get picked up as the dependency!

PS - Despite being "rude" to implement this on behalf of the taxonomy module, using it's prefix and all, there's no chance this will break because core will never support this directly. Also, this exact practice is used in Views...

joachim’s picture

> then the taxonomy module will correctly get picked up as the dependency!

I don't think it would -- look at flag_fetch_definition().

alexweber’s picture

Joachim, I'm sorry to say this but you are wrong! :)

I've just tested this myself:

  • Using unpatched, up-to-date 7.x-3.x-dev
  • Create a flag for taxonomy terms
  • Export with features
  • Notice there's no dependency on taxonomy found

Add the following code to flag.flag.inc:

if (module_exists('taxonomy')) {
  function taxonomy_flag_type_info() {
    $definitions['taxonomy_term'] = array(
      'title' => t('Taxonomy Terms'),
      'description' => t('Taxonomy terms are used to categorize content.'),
      'handler' => 'flag_entity',
    );

    $definitions['taxonomy_vocabulary'] = array(
      'title' => t('Taxonomy Vocabularies'),
      'description' => t('Taxonomy vocabularies are used to group taxonomy terms.'),
      'handler' => 'flag_entity',
    );

    return $definitions;
  }
}

Repeat the above steps. It now correctly identifies the taxonomy module as a dependency!

Note: the module_exists() is kinda useless because even if Taxonomy doesn't exist this code should cause no side effects and it might actually be cheaper NOT to test for taxonomy module on every request ;)

joachim’s picture

Well I have **no** idea how that works! The flag definition has no idea which module provided it.

> Note: the module_exists() is kinda useless because even if Taxonomy doesn't exist this code should cause no side effects and it might actually be cheaper NOT to test for taxonomy module on every request ;)

Agreed.

> PS - Despite being "rude" to implement this on behalf of the taxonomy module

Not at all, it's a fairly standard pattern in contrib -- lots of it in Views for instance.

alexweber’s picture

> Well I have **no** idea how that works! The flag definition has no idea which module provided it.

It's pretty simple really: flag_features_export() uses information from flag_features_providing_module().

function flag_features_providing_module() {
  $modules = array();
  $hook = 'flag_type_info';
  foreach (module_implements($hook) as $module) {
    foreach (module_invoke($module, $hook) as $key => $value) {
      $modules[$key] = $module;
    }
  }
  return $modules;
}

module_invoke() keys the return array by the name of the module that implemented the hook, and we also key our own return array with the same variable. Since we implement it on behalf of taxonomy, module_invoke() it doesn't know any better and it just works! :)

I think what messed you up is that you're thinking about the flag definition but it's really flag_features_providing_module() that does the work here...

joachim’s picture

Title: Implement hook_flag_type_info() on behalf of the Taxonomy module » Implement hook_flag_type_info() on behalf of non-required core modules

Oh blimey, that's a bit wacky!

I guess for this issue we should move comment flags to a hook on behalf of comment module for the same reason. node and user flags don't need to move, since those modules are always enabled anyway.

alexweber’s picture

> Oh blimey, that's a bit wacky!

Hahahaa, yeah it is! :)

> I guess for this issue we should move comment flags to a hook on behalf of comment module for the same reason.

The reason I didn't suggest this in the first place is because you actually implement a flagging class for comment but, come to think of it, I don't think this makes a difference and the comment dependency is probably not being picked up either...

Patch on the way!

alexweber’s picture

alexweber’s picture

If #1905766: Don't allow flagging taxonomy vocabularies get committed, we should remove the implementation for taxonomy vocabularies... :)

alexweber’s picture

New patch that, according to #12, removes declaration for taxonomy vocabularies.

joachim’s picture

Status: Needs review » Fixed
FileSize
1.82 KB

Committed!

git commit -m "Issue #1905750 by alexweber: Added implementations of hook_flag_type_info() on behalf of non-required core modules." --author="alexweber "

Here's the patch I committed -- I tweaked comments a bit -- in case this gets backported.

joachim’s picture

Category: task » bug
Priority: Normal » Critical
Status: Fixed » Active

There's a problem with this patch -- the comment hook is not getting picked up, and I have no idea why.

This is now either a critical bug, or I have to revert the patch :(

joachim’s picture

      $comment_flag = module_invoke('flag_type_info', 'comment');

This returns nothing at all, even after lots of cache clears. The taxonomy hook is working fine, so what's different about comment module?

alexweber’s picture

@joachim, I've found out why this happens but can't provide a good explanation...

It has to do with module weights: all of comment, flag and taxonomy have weight = 0 so the hooks run in alphabetical order; first comment, then flag, then taxonomy.

Put this in hook_init().

  module_load_include('inc', 'flag', 'includes/flag.features');
  var_dump(flag_features_providing_module());die;

It will print an array with: "node", "user" and "taxonomy_term" as the keys.

Now change flag's module weight to 1, so it runs after taxonomy.

Now flush caches and run the hook_init() code again:

It will print an array with only "node" and "user" as they keys.

Now change flag's module weight to -1, so it runs before comment and taxonomy:

It will print an array with only "node", "user", "taxonomy_term" and "comment" as they keys.

Boom! :)

So, for whatever, reason, since we're implementing the hook on the behalf of the module, our module has to run before that specific module. Not sure why this works like this; I would have expected it to the opposite, requiring us to run after the module we're implementing the hook for, but there it is.

Not sure how to proceed and whether it's an option to reduce our weight and whether its worth it just to satisfy this requirement...

joachim’s picture

Wow. Nice work on the debugging!!!

I have figured out why this is. It's to do with the way http://api.drupal.org/api/drupal/includes!module.inc/function/module_imp... loads includes files based on hook_hook_info(). This goes:

- if the hook function exists, the module implements the hook
- otherwise, try to load the include file for the hook
- now check again if the function exists

So what happens for us is:

- comment. No hook. Try to load comment.flag.inc, which doesn't exist. Still no hook.
- flag. No hook. Try to load flag.flag.inc. Now we have a hook.
- taxonomy. Because flag.flag.inc is now loaded, the function exists.

The fixes I can think of are:

- move the comment hook implementation into the module file. Which is not brilliant.
- change our module weight, which is fiddly
- implement http://api.drupal.org/api/drupal/modules!system!system.api.php/function/... to tweak the order this hook runs in. also a bit fiddly.
- revert this patch and add a 'module' key to the flag type data, which would be optional, but which we'd set for the core flag types we define.

alexweber’s picture

> Wow. Nice work on the debugging!!!

Thanks! :) In my experience if something doesn't work that simply *should*, fiddle with module weights and see what happens...

> I have figured out why this is

Wow, nice! Thanks for explaining that, I had no idea :)

As far as the options go, I agree with you 100%, let's add an extra 'module' key and be done with it! It's transparent and nobody else will ever use it so it'll be our little secret :P

Edit: I'm not sure we really need to revert the patch, can't we roll a new one that moves the core module's stuff up into our hook implementation?

joachim’s picture

> roll a new one that moves the core module's stuff up into our hook implementation?

That would pretty much be the same thing, given that the last patch moved them out...

This way will be cleaner in the history too.

alexweber’s picture

Good call, waiting on the rollback to get on the new fix :)

joachim’s picture

Done.

alexweber’s picture

Here we go:

  • Add taxonomy_term entity to flag_flag_type_info()
  • Added 'module' key to both taxonomy_term and comment in flag_flag_type_info()
  • Use new 'module' key if available in flag_features_providing_module()
  • Updated flag.api.php to describe new 'module' key
joachim’s picture

Category: bug » task
Priority: Critical » Normal
Status: Needs review » Fixed

Perfect, thanks!

git commit -m "Issue #1905750 by alexweber: Added module property to hook_flag_type_info(), and explicitly defined taxonomy term flag type, so that non-required core modules correctly declare dependencies." --author="alexweber "

alexweber’s picture

Awesome!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.