Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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().
Comment | File | Size | Author |
---|---|---|---|
#7 | interdiff-1718926-4-7.txt | 1.74 KB | joachim |
#7 | 1718926-7.flag_.field-attach-calls.patch | 2.4 KB | joachim |
Comments
Comment #1
shabana.navas CreditAttribution: shabana.navas commentedJust came across this while I was looking at this issue.
For field_attach_rename_bundle(), wouldn't we be doing this in:
flag_form_submit()? We already have code like this:
Couldn't we just add that bit there like 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.
Comment #2
joachim CreditAttribution: joachim commentedI'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.
Comment #3
joachim CreditAttribution: joachim commentedComment #4
joachim CreditAttribution: joachim commentedOops forgot to remove the todos in the code.
Comment #5
joachim CreditAttribution: joachim commentedComment #6
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commentedThis 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.
Comment #7
joachim CreditAttribution: joachim commented> 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.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commentedAh, 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.
Comment #10
joachim CreditAttribution: joachim commentedThanks for testing!
Committed.