Hi,
Ryan noticed in the multigroup issue (comment #457, please also see me comment in #465) that the multigroup had a rendundant snippet that it would haven't been necessary if content_field_overview_form() in includes/content.admin.inc was handling the result of fieldgroup_types() to swap the 'group_type' element of the form to a single-select element when more than one group types exist.
I'm using a separte issue so that it can help us deal with the rest of the monster patches.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | fieldgroup_types-409398-2.patch | 1.54 KB | markus_petrux |
| #1 | fieldgroup_types-409398-1.patch | 1.14 KB | markus_petrux |
Comments
Comment #1
markus_petrux commentedComment #2
markus_petrux commentedComment #3
yched commentedWell, the less we hardcode in the original field overview form, the better we stand for a UI that can be at some point moved to core for D7, without fieldgroup or multigroup itself moving to core. So it seems the form_alter model currently in use makes sense.
The cleanup in the display overview form seems appropriate.
Comment #4
markus_petrux commentedSorry, not sure if you mean if the patch is ok or not. I need to know this as this patch allows to remove some code in the multigroup module, which really belongs to CCK and its builtin fieldgroup features. Note that fieldgroup_types() is already invoked by CCK and fieldgroup module invoked hook_fieldgroup_types(), so it's a bug IMO if a module implementing this hook doesn't affect the 'Manage Fields' form and it is forced to use hook_form_alter() to so.
Comment #5
rconstantine commented@yched, Markus is right. This is a bug fix for current CCK functionality, not an add-on to CCK. The hook now exists, but multigroup wasn't using it correctly and was using form_alter instead. It would appear that my patch includes essentially the same stuff as in this one. One improvement in Markus' patch allows for the removal (I think) of the _fieldgroup_groups_label function - right Markus?
Anyway, bottom line is that this fixes existing behavior regardless of the three big patches.
Comment #6
markus_petrux commentedJust wanted to mention that I made the latest version of the multigroup dependent on this patch. That way I could remove code from the multigroup that really belongs to the fieldgroups integration code that CCK already contains in for the 'Manage Fields' screen.
If this patch is not going to get in, then I would have to add that code snippet back to the multigroup module.
Reference: #119102: Combo field - group different fields into one
Comment #7
rconstantine commentedRight. The original code should be fixed; not the patch.
Comment #8
yched commentedOK, committed. Thanks Markus.