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.

Files: 
CommentFileSizeAuthor
#18 flag-export-dependencies-1905004-18.patch1.37 KBalexweber
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
#16 flag-export-dependencies-1905004-16.patch1.38 KBalexweber
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
#8 flag-export-dependencies-1905004-8.patch1.36 KBalexweber
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
#4 flag-export-dependencies-1905004-4.patch1.09 KBalexweber
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
#1 flag-export-dependencies-1905004-1.patch1.02 KBalexweber
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

Comments

alexweber’s picture

Issue summary:View changes

Updated issue summary.

alexweber’s picture

Status:Active» Needs review
StatusFileSize
new1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

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

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

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

Status:Needs work» Needs review
StatusFileSize
new1.09 KB
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

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

+++ 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

Status:Needs review» Needs work
alexweber’s picture

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

Status:Needs work» Needs review
StatusFileSize
new1.36 KB
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
joachim’s picture

Status:Needs review» Needs work
alexweber’s picture

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

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

alexweber’s picture

flag_features_providing_module() already does that for us :)

<?php
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

Urgh, sorry. Pre-breakfast patch reviewing :/

alexweber’s picture

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

joachim’s picture

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

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

:)

joachim’s picture

+++ 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

StatusFileSize
new1.37 KB
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

You the boss!

joachim’s picture

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

phew! :)

Status:Fixed» Closed (fixed)

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.