From the code:

// @todo: When renaming a flag: Call field_attach_rename_bundle().

// @todo: When creating a flag: Call field_attach_create_bundle().

// @todo: When deleting a flag: Call field_attach_delete_bundle().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shabana.navas’s picture

Just came across this while I was looking at this issue.

// @todo: When renaming a flag: Call field_attach_rename_bundle().

For field_attach_rename_bundle(), wouldn't we be doing this in:
flag_form_submit()? We already have code like this:

  // If the flag machine name as changed, clean up all the obsolete permissions.
  if ($flag->name != $form['#flag_name']) {
    $old_name = $form['#flag_name'];
    $permissions = array("flag $old_name", "unflag $old_name");
    foreach (array_keys(user_roles()) as $rid) {
      user_role_revoke_permissions($rid, $permissions);
    }
    field_attach_rename_bundle('flagging', $old_name, $flag->name);
  }
 

Couldn't we just add that bit there like this:

  // If the flag machine name as changed, clean up all the obsolete permissions.
  if ($flag->name != $form['#flag_name']) {
    $old_name = $form['#flag_name'];
    
    //Notify field.module that the flag was renamed.
    field_attach_rename_bundle('flagging', $old_name, $flag->name);

    $permissions = array("flag $old_name", "unflag $old_name");
    foreach (array_keys(user_roles()) as $rid) {
      user_role_revoke_permissions($rid, $permissions);
    }
    field_attach_rename_bundle('flagging', $old_name, $flag->name);
    //Need to clear cache after this
  }

This effectively works. The bundles are renamed in the tables field_config_instance and field_data_field_* and the field is still attached to the renamed flag (which wasn't the case when I didn't add that bit). However, we need to clear the cache more rigorously after this.

joachim’s picture

Category: Task » Bug report
Priority: Normal » Major
Issue summary: View changes

I've just noticed on my test site that this leaves field instances kicking around after flags have been deleted. So I'm going to call this a bug.

joachim’s picture

Status: Active » Needs review
FileSize
1.68 KB
joachim’s picture

FileSize
2.23 KB

Oops forgot to remove the todos in the code.

joachim’s picture

Issue tags: +7.x-3.7 blocker
Anonymous’s picture

This patch still applies well, however I couldn't figure out how to test the functionality from the issue details.

I created a flag, renamed, deleted, created a new one, etc. - all garbage collection seemed to work on the flag tables however from the issue it sounds like it's around field attachment somewhere. Tested both without and with patch applied.

Only issue I saw with the code looking at it from someone who doesn't know the module code well is the field_attach_delete_bundle 'feels' as if it should be in the $flag->delete(), but that may be just my lack of understanding of the workflow of the module.

Happy to test again if needed and steps to reproduce can be posted.

joachim’s picture

FileSize
2.4 KB
1.74 KB

> Only issue I saw with the code looking at it from someone who doesn't know the module code well is the field_attach_delete_bundle 'feels' as if it should be in the $flag->delete(), but that may be just my lack of understanding of the workflow of the module.

That makes sense.
TBH I am not sure why we have to call ->disable() for a flag we've deleted, but that's another matter.

> I created a flag, renamed, deleted, created a new one, etc. - all garbage collection seemed to work on the flag tables however from the issue it sounds like it's around field attachment somewhere. Tested both without and with patch applied.

Yup, it's only a problem if you add a field to one of your flags, then rename the flag. IIRC these are the problems you then get:

- the field instance is not updated, so it looks like your flag lost its field
- the 'bundle' column in the field table won't get updated

If you delete a flag with a field on it, then the field instance & field tables won't get deleted. That's only an issue if you then seek to re-create a field with the same name.

Here's an updated patch with the change you suggested.

Anonymous’s picture

Ah, yes, I see!

Tested with & without patch, does indeed now update/delete the bundle field/record in the field_config_instance table, +1 from me.

  • joachim committed 71a08ad on 7.x-3.x
    Issue #1718926 by joachim: Fixed missing calls to Field Attach API.
    
joachim’s picture

Status: Needs review » Fixed

Thanks for testing!

Committed.

Status: Fixed » Closed (fixed)

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