Follow-up to #1928054: Field formatter settings with CTool content_type entity_field plugin (Panels integration)

Is there any reason why the hook_field_formatter_settings_form_alter in ctools_fields_get_field_formatter_settings_form() is only invoked if the settings form is not empty? I would like to be able to alter empty settings forms too.

Comments

osopolar’s picture

Status: Active » Needs review
StatusFileSize
new1.53 KB

There should be no problem to invoke hook_field_formatter_settings_form_alter for empty settings-forms.

azinck’s picture

The approach in #1 will not work for modules that don't implement hook_field_formatter_settings_form() at all because the code path never makes it past the if (function_exists($function)) { check.

Updated patch which allows the form alter to be called even if there's no implementation of hook_field_formatter_settings_form().

azinck’s picture

StatusFileSize
new1.96 KB

Sorry, forgot the interdiff.

osopolar’s picture

My consideration was, that there should not be a settings alter if there is no hook_field_formatter_settings_form implementation. In other words something that does not exist could not be altered, could it? I guess it's more a theoretical/philosophical consideration then practical.

I also had the problem that I wanted to alter a non existing settings-form, so I had to declare it in may custom module like:

if (!function_exists('xyz_field_formatter_settings_form') {
  function xyz_field_formatter_settings_form($form, $form_state) {
  }
}

In the end, if it is fine to call a alter-function for something that in first place does not exist, then I am fine with #2.

nwom’s picture

There was a small problem with a notice being shown and a white space error. I have updated the patch. Please review.

Status: Needs review » Needs work

The last submitted patch, 5: ctools-alter_empty_settings_forms-2537980-5.patch, failed testing.

nwom’s picture

I forgot the case where the variable isn't yet set. Here is the new patch. Please review.

osopolar’s picture

Status: Needs work » Needs review
StatusFileSize
new2.03 KB

interdiff ctools-alter_empty_settings_forms-2537980-2.patch ctools-alter_empty_settings_forms-2537980-7.patch says:

--- b/includes/fields.inc
+++ b/includes/fields.inc
@@ -80,7 +80,7 @@
   if (function_exists($function)) {
     $settings_form = $function($field, $instance, $view_mode, $form, $form_state);
   }
-  if(!$settings_form) {
+  if (!isset($settings_form) || !$settings_form) {
     $settings_form = array();
   }

instead of if (!isset($settings_form) || !$settings_form) { we may use if (empty($settings_form)) {.

nwom’s picture

Status: Needs review » Reviewed & tested by the community

#8 works and applies cleanly.

japerry’s picture

Status: Reviewed & tested by the community » Fixed
Related issues: +#2828925: Plan for CTools 7.x-1.13 release

Looks good to me. Committed.

  • japerry committed 841a9ce on 7.x-1.x authored by osopolar
    Issue #2537980 by osopolar, NWOM, azinck: Alter field formatter settings...

Status: Fixed » Closed (fixed)

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