Update flag when machine name changes

Sounds like a fairly easy one to tackle, so tagging as Novice.

IIRC there's a hook for bundle name changes. FieldAPI makes use of this, for instance.

Steps to reproduce issue:
1) Create a content type
2) Add a flag that uses that content type (admin/structure/flags/add)
3) Edit the content type and change the machine name
4) Visit the flags administration page (admin/structure/flags) and notice that the flag still shows the old Entity Bundles name
5) Edit that flag and notice that no Bundles are selected
6) Select the bundle again and save
7) Notice that the Entity Bundles now shows the new machine name

I will roll a patch to fix this using hook_field_attach_rename_bundle.

Checking to see what happens when I put

  dsm(__FUNCTION__);
  dsm($entity_type);
  dsm($bundle_old);
  dsm($bundle_new);

inside of the function, I get the attached screenshot. The test I ran was to attach a flag to a node bundle. I hadn't considered that I would get a call from the comment entity. So, I think I need to test this with different entities to see what happens.

Edit: I don't think we can change the machine name in other entities. So, I don't think the hook will get called for them. Somebody should double-check me on this though.

Edit 2: The above is not true. We can change the machine name of a Taxonomy Vocabulary.

Here is the screenshot of what we get from the hook when we change the taxonomy vocabulary name.

I am not going to tackle testing every entity to which a flag can be attached. See attached screen shot for an example of a flag type that was added by a contributed module (OG).

Let me see if I can code the logic in a way that it should just work. I'll need another set of eyes to test things out though, to ensure that I'm not breaking anything.

> I am not going to tackle testing every entity to which a flag can be attached. See attached screen shot for an example of a flag type that was added by a contributed module (OG).

That's not a feasible way of tackling this: any contrib or custom module could be defining entity types that you have no way of knowing about. You simply have to code this in a way that's generic for all entities.

> I hadn't considered that I would get a call from the comment entity

IIRC, if you change the name of a node type, you implicitly change the type of the comments that get attached to that node type. So you're in fact changing TWO entity bundle names. That's fine -- our hook will get invoked twice, once for nodes and once for comments.

Yes, this is coded generically for all entities. What I'm saying is that I'm not going to test every possible entity.

Patch attached and title changed to reflect what the patch actually does.

Or paths crossed while I was going to attach the patch and you have used up comment #6, but I've left the patch with the same comment number.

The call from the comment entity does not matter, since I am JOINing to the flag file to look for a record that has the same entity type. When it's not found (as in the case of the comment entity) it won't do the update.

Edit: When I say that it doesn't matter, I mean it won't break anything. If we had a flag attached to the comment entity as well, then things will be updated properly for flag when that machine name changes. If we did not have a flag attached to the comment entity, that call won't result in any updates.

FYI: I have tested this with Taxonomy, Node and Comment flags.

Yay! I see that it has passed all test now. Now it's time for a person to test and RTBC.

Cool, thanks for working on this!

> When it's not found (as in the case of the comment entity) it won't do the update.

But the comment entity type WILL have changed its own type, won't it? See https://api.drupal.org/api/drupal/modules!node!node.module/function/node... -- this calls field_attach_rename_bundle() for nodes, then invokes hook_node_type_update(), and comment's implementation also calls field_attach_rename_bundle(), this time for comments.

So we also need to react for any flags on comments -- but that's fine, it looks like we'll do that.

1. +++ b/flag.module
   @@ -2572,6 +2572,32 @@ function flag_ctools_plugin_directory($module, $plugin) {
   +  $query = 'SELECT ft.fid
   +            FROM {flag_types} ft
   +            JOIN {flag} f
   +              ON f.fid = ft.fid AND f.entity_type = :entity_type
   +            WHERE type = :bundle_old';
   +  $query_args = array(
   +    ':entity_type' => $entity_type,
   +    ':bundle_old' => $bundle_old,
   +  );
   +  $fid = db_query($query, $query_args)->fetchField();
   

   Could you put the query and args in the function call, rather than create variables? It's easier to see how everything joins up (do keep the linebreaks, those are good). Also, any code analysis for security issues or upgrades will get confused by that.

2. +++ b/flag.module
   @@ -2572,6 +2572,32 @@ function flag_ctools_plugin_directory($module, $plugin) {
   +  if ($fid) {
   

   There might be more than one flag found!

   So you need to fetch all, rather than fetchField(), and then loop. Or just loop on the query result.

3. +++ b/flag.module
   @@ -2572,6 +2572,32 @@ function flag_ctools_plugin_directory($module, $plugin) {
   +    $query = 'UPDATE {flag_types}
   +              SET type = :type
   +              WHERE fid = :fid';
   +    $query_args = array(
   +      ':type' => $bundle_new,
   +      ':fid' => $fid,
   +    );
   +    db_query($query, $query_args);
   

   UPDATE queries must be dynamic in D7. Though here, you should be using the Flag API: load the flag, change its setting, resave.

Yes, when you change the node machine name, the comment machine name is also changed. What I was saying that if there are no flags set on comments, then the query won't get a hit on the join. As I mentioned in #10, I have tested with flags set on comments (specifically on the comment that is attached to the node type I changed) and verified that the flag for the comment was update as well.

Thanks for the feedback. Let me see about rolling the patch with your changes.

Let's see if this is what you had in mind.

Sorry this has completely dropped off my radar...

There's just a couple of tweaks to make and then this is good to go. I'm tagging it as blocking the next release so it doesn't get forgotten again!

Let's call this a bug too -- it does break people's flag configuration when they change things like node type names.

1. +++ b/flag.module
   @@ -2572,6 +2572,31 @@ function flag_ctools_plugin_directory($module, $plugin) {
   +  $fids = db_query('SELECT f.name
   

   $fids is a weird variable name here, as it's about to be an array of objects with a name property...

   I would use fetchCol() (see https://www.drupal.org/node/1251174) and then it's an array of $flag_names.

2. +++ b/flag.module
   @@ -2572,6 +2572,31 @@ function flag_ctools_plugin_directory($module, $plugin) {
   +    foreach ($flag->types as $key => $flag_type) {
   

   Don't say $flag_type here; that tends to refer to the entity type for the flag. Here we mean the bundle... but we already have two $bundle_foo variables in play... really, this is flag's fault for using $flag->types when these are actually bundles. So I'd just say $type here.

Let me see about getting this sorted out quickly.

How do you like this one?

It looks great, but I'm afraid I've only just spotted a bug!

+++ b/flag.module
@@ -2558,6 +2558,32 @@ function flag_ctools_plugin_directory($module, $plugin) {
+  foreach ($flag_names as $flag_name) {
+    $flag = flag_get_flag($flag_name);
+    foreach ($flag->types as $key => $type) {

Suppose both taxonomy terms and nodes have a bundle called 'foo'. We rename the node type 'foo' to 'bar'.

Taxonomy flags that apply to bundle 'foo' will be changed by the above code!

The best way to fix this -- and I'm really sorry I had you change the query earlier, I should have spotted this -- is to use $flags = flag_get_flags($entity_type). That way you get an array of only the flags on the entity type that the hook is being invoked for.

Sorry again for the runaround, and thanks for the super-quick reroll!

Let's see if I can reproduce the bug you have outlined.

Steps taken:

1) Create content type foo.
2) Create vocabulary foo.
3) Enable flag module.
4) Attach a flag to the foo node.
5) Attach a flag to the foo taxonomy.
6) Change machine name of foo node to bar.

When I go through these steps, I note that the foo node flag does point to the bar node bundle and the foo taxonomy flag still points to the foo taxonomy vocabulary. So, on my end I am not seeing any issue. Are you able to produce the "bug"? Will you document a set of steps for me to be able to reproduce it?

+++ b/flag.module
@@ -2558,6 +2558,32 @@ function flag_ctools_plugin_directory($module, $plugin) {
+                      ON f.fid = ft.fid AND f.entity_type = :entity_type

Oh duh. You're filtering on entity type here.

I'm really sorry -- it's late at night here and it's been a long day....

Still, if you're willing to reroll, using the API to get an array of flag objects would be simpler and more maintainable.

Yeah, also I've got the logic

      if ($type == $bundle_old) {
        $flag->types[$key] = $bundle_new;
      }

that is checking the bundle for a match.

You are saying that you want me to use flag_get_flags instead of flag_get_flag? I'm already getting the flag for a particular flag name. So, wouldn't I need to change the query as well? Is that what you are after?

Now I'm wondering if you just want me to skip the query entirely and just use the $entity_type variable that is passed in to the function.

Yup, instead of querying, you'd do just:

$flags = flag_get_flags($entity_type);
foreach ($flags as $flag) {
  // ...
}

The problem with this idea is that this would be really overkill. For instance, we would be renaming just one node bundle, but we would get all the flag names that are attached to nodes. So, we would get far more flag names than we needed. The query reflects both the entity (bundle) type as well as the bundle name.

We need to loop through the $flag->types because a flag can be attached to more than one bundle. But, we don't want to waste time looking at all the flags that are attached to the entity type that the bundle happens to be when they aren't even attached to that bundle. Let's just focus on the node entity type and say that we have lots of flags that are attached to nodes. For the sake of argument assume that we have five node bundles: article, page, flagme, static, notchanging; we have five flags, four of which are attached to various combinations of article, page, static and notchanging, and one of which is attached to flagme. When we change the machine name of flagme we don't need to inspect the four other flags, which is what will happen if we pull all the flags for the node entity type. With the query we will just pull the one flag that is attached to the flagme bundle.

Sorry, this totally fell off my radar -- thanks for the reminder.

Looking at this again, I still think this should use the API function, flag_get_flags(). Reasons are:

- it's the API. It's what we should be using to load flag handlers
- it does a load of work for us such as loading code-based flags, running flags through the alter hook, and so on. I think last time I looked at this issue I wasn't sure whether either of those were desirable, but on reflection, I think they definitely are.

I'm a bit tied down at the moment, but may be able to free up some time to have another go at this in the somewhat near future. Please ping me through my contact form if it's been a month or more and I haven't swung back around.

Well, how about that! I did get back within a month. Let me see what I can do with this right now. BRB!

Are we there, yet?

Great, thanks!

There's just one thing, but it's not that important, and you've rerolled this patch so many times I'm really loath to set this back to needs work. So if you're sick of this patch, leave it, and I'll commit it as it stands in a few days :)

Otherwise, here it is:

+++ b/flag.module
@@ -2558,6 +2558,21 @@ function flag_ctools_plugin_directory($module, $plugin) {
+      if ($type == $bundle_old) {
+        $flag->types[$key] = $bundle_new;
+      }
+    }
+    $flag->save();

We're only acting on the renaming of a single bundle. And one flag can only have that bundle once in its types array. So if we found the bundle, we can save the flag there and then, and then do a break; to leave the types loop and move on to the next flag:

$flag->types[$key] = $bundle_new;
$flag->save();
// Move on to the next flag.
break;

The other benefit of doing this is that we only save the flags we need to. If the flag doesn't have the bundle, we never get to save it. As the patch stands, all the flags for the entity will get saved. So that would be a performance improvement, but it's pretty much a non-issue (unless your site had LOTS of flags), because changing the name of a bundle is already an expensive operation, and not one that is expected to be fast -- Field module has to go and rewrite values in lots of different field tables, for instance.

Yeah sure, I probably should have put the flag->save() inside the if statement. As far as your suggestion to add a break, I brought up the issue in comments #25 and #26 about the performance hit and you still wanted me to use the API. So, I really didn't think about adding a break.

I'm willing to do another re-roll, but you'd have to be willing to go through the testing to see that the break works as desired. I really don't want to go through all the testing again on my end.

Are you game or do you want it to stand as is?

Let's go with this patch :)

It's very much an edge case that too many saved flags would crash the site.

Thanks again for all your perseverance with this one! Committed!