In flag_features_export(), Flag kindly tries to add related modules as dependencies when a given flag type is exported.

That's all well and good for 2.x, when every module defining a custom entity had to manually implement hook_flag_type_info() in order to register it.

However, now that Flag supports entities on a generic level, this hook is only really ever used if you want to be able to flag custom schemas that aren't entities.

The problem is that if we export a flag on a custom entity, it will incorrectly assume that the entity is defined in hook_flag_type_info() and returned by flag_features_providing_module(), which it's not. It will throw a notice and won't add the dependency. (I ran into this issue exporting a Profile2 flag)

Initially I thought we might be able to assume that the entity name corresponds to the module name and add that directly to the dependencies but despite being the case 90% of the time it's a silly and risky assumption.

Therefore I think we might have to introspect the entity type and find out which module defines it in order to properly add the dependency.

CommentFileSizeAuthor
#18 flag-export-dependencies-1905004-18.patch1.37 KBalexweber
#16 flag-export-dependencies-1905004-16.patch1.38 KBalexweber
#8 flag-export-dependencies-1905004-8.patch1.36 KBalexweber
#4 flag-export-dependencies-1905004-4.patch1.09 KBalexweber
#1 flag-export-dependencies-1905004-1.patch1.02 KBalexweber
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexweber’s picture

alexweber CreditAttribution: alexweber commented
Issue summary: View changes

Updated issue summary.

alexweber’s picture

alexweber CreditAttribution: alexweber commented
Status: Active » Needs review
FileSize
flag-export-dependencies-1905004-1.patch1.02 KB

Patch attached that fixes this issue:

  • If the module providing the entity type isn't available in flag_features_providing_module (), it tries a new approach:
  • Calls entity_get_info() to find out out what module should be required for the flag on the custom entity
  • Adds the dependency as usual
joachim’s picture

joachim CreditAttribution: joachim commented

Good catch!

However...

> Calls entity_get_info() to find out out what module should be required for the flag on the custom entity

+++ b/includes/flag.features.inc
@@ -19,8 +19,22 @@ function flag_features_export($data, &$export, $module_name = '') {
+          $module = $entity_info['module'];

The 'module' property in entity info is an Entity API extension -- it's not part of core: http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...

I'm not really sure what we can do about this, other than try this, and then do nothing if we can't find the module that is responsible for an entity type. Hmm maybe the database schema would have it -- a bit convoluted, we'd have to get the base table for the entity, then find out who owns it.

alexweber’s picture

alexweber CreditAttribution: alexweber commented
Status: Needs review » Needs work

Hmmmm, good call, I didn't realize the 'module' key was an Entity API extension.

That said, how many custom entities you think exist out there that haven't been built either using Entity API or ECK? And ECK depends on Entity...
Therefore, I think it's pretty safe that 99% of modules that implement custom entities do it through one of the above channels so maybe we could just do an isset() on $entity_info['module'] and if its not well than that's just too bad! :)

Odds also are that if the flag is being exported as a feature that the entity's bundles also are so the dependency will already be there in the first place! ;)

alexweber’s picture

alexweber CreditAttribution: alexweber commented
Status: Needs work » Needs review
FileSize
flag-export-dependencies-1905004-4.patch1.09 KB

Patch attached just add that extra isset() call to make sure the 'module' key exists in entity_get_info()

If not, we just don't add the explicit dependency.

joachim’s picture

joachim CreditAttribution: joachim commented 
+++ b/includes/flag.features.inc
@@ -19,8 +19,24 @@ function flag_features_export($data, &$export, $module_name = '') {
+      if (array_key_exists($flag->entity_type, $modules)) {
+        $module = $modules[$flag->entity_type];
+      }

I take it this is a first pass where we see if there is a module with the same name as the entity? (eg, 'node').

Could this have a comment to say that please?

+++ b/includes/flag.features.inc
@@ -19,8 +19,24 @@ function flag_features_export($data, &$export, $module_name = '') {
+        // This is a flag on a custom entity. Let's figure out which module
+        // defines it.

Could this comment mention that we're looking for the Entity API 'module' property?

+++ b/includes/flag.features.inc
@@ -19,8 +19,24 @@ function flag_features_export($data, &$export, $module_name = '') {
+      // Just in case.

Not sure what this means!

> That said, how many custom entities you think exist out there that haven't been built either using Entity API or ECK? And ECK depends on Entity...

Agreed.

Though I can't help but notice that this will all fail with plain old taxonomy terms!!! ;)

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work
alexweber’s picture

alexweber CreditAttribution: alexweber commented
Assigned: Unassigned » alexweber

@joachim, I'll clean up the comments.

Now, what if we implement hook_flag_type_info() on behalf of taxonomy terms?

We already do that for nodes, users and comments, I see no reason Taxonomy wasn't included in the first place... after all, its a core module! :)

New issue: #1905750: Implement hook_flag_type_info() on behalf of non-required core modules

alexweber’s picture

alexweber CreditAttribution: alexweber commented
Status: Needs work » Needs review
FileSize
flag-export-dependencies-1905004-8.patch1.36 KB
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

I suspect this needs work now #1905750: Implement hook_flag_type_info() on behalf of non-required core modules is in.

alexweber’s picture

alexweber CreditAttribution: alexweber commented
Status: Needs work » Needs review

It actually looks good... that patch wouldn't have picked up taxonomy dependency without #1905750: Implement hook_flag_type_info() on behalf of non-required core modules but it works as expected now.

joachim’s picture

joachim CreditAttribution: joachim commented

Surely we should be looking at the 'module' property we're now putting in core flag type infos?

alexweber’s picture

alexweber CreditAttribution: alexweber commented

flag_features_providing_module() already does that for us :)

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] = isset($value['module']) ? $value['module'] : $module;
    }
  }
  return $modules;
}
joachim’s picture

joachim CreditAttribution: joachim commented

Urgh, sorry. Pre-breakfast patch reviewing :/

alexweber’s picture

alexweber CreditAttribution: alexweber commented

hahaha no problem! It's lunch time here ;)

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

Breakfast again, but slightly more awake! :)

+++ b/includes/flag.features.inc
@@ -17,10 +17,26 @@ function flag_features_export($data, &$export, $module_name = '') {
+        // and therefore has an extra 'module' property in it's information.

'it's' should be 'its'.

+++ b/includes/flag.features.inc
@@ -17,10 +17,26 @@ function flag_features_export($data, &$export, $module_name = '') {
+      if (isset($module)) {

That's going to fail if $data contains more than one flag. You need to reset $module to NULL or '' or something at the top of the loop.

alexweber’s picture

alexweber CreditAttribution: alexweber commented
Status: Needs work » Needs review
FileSize
flag-export-dependencies-1905004-16.patch1.38 KB

:)

joachim’s picture

joachim CreditAttribution: joachim commented 
+++ b/includes/flag.features.inc
@@ -17,10 +17,27 @@ function flag_features_export($data, &$export, $module_name = '') {
+    unset($module);

Is that going to work first time round? Reads a little weirdly.

Sorry... could this be change to just $module = ''; and then check it with empty() rather than isset()?

alexweber’s picture

alexweber CreditAttribution: alexweber commented
FileSize
flag-export-dependencies-1905004-18.patch1.37 KB

You the boss!

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Fixed

Fixed.

Thanks for persevering with this! :)

git commit -m "Issue #1905004 by alexweber: Fixed flag export to Features not finding module that provides the entity as a dependency." --author="alexweber "

alexweber’s picture

alexweber CreditAttribution: alexweber commented

phew! :)

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.