Posted by Dave Reid on October 18, 2010 at 10:58pm
46 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | field system |
| Category: | task |
| Priority: | normal |
| Assigned: | Dave Reid |
| Status: | needs review |
| Issue tags: | API addition, needs backport to D7 |
Issue Summary
We have several use cases of altering a field formatter settings and there is no easy way to do it. Patch adds two small alter hooks, hook_field_formatter_settings_form_alter() and hook_field_formatter_settings_summary_alter() since the latter is required in order to alter in settings to a formatter that does not natively provide any settings.
Comments
#1
Initial patch that requires the hook docs to be fleshed out, but works.
#2
To make module like Field Formatter Class even reasonable for mere mortals this patch is really needed.
I tested and the code works. Looking forward to the docs.
#3
Can you make the case that this is a regression from CCK in Drupal 6?
#4
This is sort of a regression and sort of a new feature that has been largely unused by many.
In CCK Formatters did not settings. So, they did a few things to overcome this. One example is the way imagecache worked with imagefield for the display settings. Since you couldn't specify settings all the available settings were listed for each imagecache preset. This list could be altered fairly easily. Another example is the swftools module that created a global config and housed it on custom config pages. These could be easily form altered and were straight forward forms.
But, the way D7 does it is totally new. Formatter settings are not bolted on through something else and this was a late addition to D7. So, we went from CCK with a bolt on formatter setting solution you could easily alter (even if just at the form level in fairly straight forward forms) to native formatter settings you have to be a FAPI guru with a lot of patience to begin to hack on (the formatter settings forms are not trivial to work on).
Because this is done in a whole new way from D6 CCK and we are just now finding in real usage there are some problems I like this patch. It does not change the API but does add two new alter calls (which are only called on the formatter settings page). I've already seen two separate use cases for this functionality. One I linked to.
Hope this helps.
#5
Even if you can alter the settings form and summary, how do you reflect the actual changes in the rendered array ?
We intentionally did not include an alter hook on 'formatted field output' render arrays, because such a hook would be called potentially 100s of times on listing pages. So right now the only way is hook_field_attach_view_alter(), which operates on the render array of the whole entity - you thus have to find your way in there to the sub array(s) you want to alter.
Note that widgets settings forms are currently not alterable either. If one is, the other should too for consistency.
#6
@yched - If you want to add or alter the structure I see no other way than to use hook_field_attach_view_alter() and I think that is acceptable for that. But, there are other cases where you may want to alter the config form in a way that has no impact on altering the render array. For example, what if someone wants to limit the image styles listed to a subset of the total image styles. This could be done through the settings form without altering the rendered output.
#7
+1 from #961412: Formatter settings button lost when summary is empty
EDIT
comming from #553292-13: Formatter settings lost when saving a field
and #830704-29: Entity forms cannot be properly extended
#8
The image formatter field system is so gross... theme_image_formatter, theme_image, theme_image_style, it's all so gross and ugly.
#9
subscribe (sorry)
#10
subscribe
#11
+1 for this. Now we're into D8, can we look into this API addition some more?
The Field formatter settings API module implements this in D7 contrib.
So far there are 4 contrib modules using these new hooks, which demonstrate their usefulness well.
#12
#1: 945524-field-formatter-settings-alters-D7.patch queued for re-testing.
#13
The last submitted patch, 945524-field-formatter-settings-alters-D7.patch, failed testing.
#14
Rerolled with git prefixes, and fixed the spelling of summary.
Still needs the api.php docs to be created.
#15
Ummm, I made #1308860: Allow modules to alter fields settings hook, and either completely forgot about this issue, or was thinking of something else at the time. I'm going to mark that one as a duplicate. Either way, here's what I wrote up there...
#16
Rerolling, also moving $form_state out of the $context into a proper variable, like hook_field_widget_form_alter has it.
#17
I fixed a typo ("formater") in the patch from #16 and added a comment to the second call to drupal_alter().
#18
patch is good but no go without tests :(
Do we have a same issue for field settings? EDIT reopened #1308860: Allow modules to alter fields settings hook
#19
CNW for tests.
#20
Written tests to see if the new alter hooks work. First patch: tests only, second patch: patch from #17 with added tests.
#21
Trailing whitespaces removed...
#22
Ok, now I know the test pass:
Punctuation... (the devil is in the details)
#23
#22: drupal-945524-22.patch queued for re-testing.
#24
The last submitted patch, drupal-945524-22.patch, failed testing.
#25
Reroll...
#26
Looks good and still applies, let's do this!
#27
+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -1060,6 +1060,17 @@ function field_ui_display_overview_form($form, &$form_state, $entity_type, $bund
+ // Allow other modules to alter the formatter settings form.
+ $context = array(
+ 'module' => $formatter['module'],
+ 'formatter' => $formatter,
+ 'field' => $field,
+ 'instance' => $instance,
+ 'view_mode' => $view_mode,
+ 'form' => $form,
+ );
+ drupal_alter('field_formatter_settings_form', $settings_form, $form_state, $context);
+++ b/core/modules/field_ui/field_ui.api.phpundefined
@@ -169,6 +169,18 @@ function hook_field_formatter_settings_form($field, $instance, $view_mode, $form
+function hook_field_formatter_settings_form_alter(&$element, &$form_state, $context) {
Context should be described in php doc for hook_field_formatter_settings_form_alter()
+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -1095,6 +1106,15 @@ function field_ui_display_overview_form($form, &$form_state, $entity_type, $bund
+ // Allow other modules to alter the summary.
+ $context = array(
+ 'field' => $field,
+ 'instance' => $instance,
+ 'view_mode' => $view_mode,
+ );
+ drupal_alter('field_formatter_settings_summary', $summary, $context);
+++ b/core/modules/field_ui/field_ui.api.phpundefined
@@ -169,6 +169,18 @@ function hook_field_formatter_settings_form($field, $instance, $view_mode, $form
+function hook_field_formatter_settings_summary_alter(&$summary, $context) {
Context should be described in php doc for hook_field_formatter_settings_summary_alter()
#28
Added phpdoc.
#29
This looks good to me.
#30
We're over thresholds atm so I unfortunately can't commit any feature patches. Help with smashing critical and major bugs would be greatly appreciated.
#31
We are over, since when?
#32
@yannickoo: You can see the number of major and critical bugs in the contributor links dashboard blog. The thresholds are 100 each major tasks and bugs; and 15 each critical tasks and bugs. See the threshold policy for details.
The test coverage looks good. However, I think we need to add example implementations for the hook documentation rather than empty stubs.
+++ b/core/modules/field_ui/field_ui.api.phpundefined@@ -169,6 +169,39 @@ function hook_field_formatter_settings_form($field, $instance, $view_mode, $form
/**
+ * Alter the formatter settings form.
+ *
+ * @param $element
+ * Form array as returned by hook_field_formatter_settings_form().
+ * @param $form_state
+ * The form state of the (entire) configuration form.
+ * @param $context
+ * An associative array with the following elements:
+ * - 'module': The module that contains the definition of this formatter.
+ * - 'formatter': The formatter type description array.
+ * - 'field': The field structure being configured.
+ * - 'instance': The instance structure being configured.
+ * - 'view_mode': The view mode being configured.
+ * - 'form': The (entire) configuration form array.
+ */
+function hook_field_formatter_settings_form_alter(&$element, &$form_state, $context) {
+}
+
+/**
+ * Alter the field formatter settings summary.
+ *
+ * @param $summary
+ * The summary as returned by hook_field_formatter_settings_summary().
+ * @param $context
+ * An associative array with the following elements:
+ * - 'field': The field structure being configured.
+ * - 'instance': The instance structure being configured.
+ * - 'view_mode': The view mode being configured.
+ */
+function hook_field_formatter_settings_summary_alter(&$summary, $context) {
+}
NW for that change.
Also, some minor code style issues (see below).
+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageDisplayTest.phpundefined@@ -75,6 +75,22 @@ class ManageDisplayTest extends FieldUiTestBase {
+ // Assert hook_field_formatter_settings_summary_alter is called.
...
+ // Click on the formatter settings button to open the formatter settings form.
...
+ // Assert the field added in field_test_field_formatter_settings_form_alter is present.
...
+ // Open the formatter settings form again to check if the settings are updated.
These comments are over 80 characters and need to be rewrapped. Also, hook and function names should always have
()in inline documentation.I'll fix what I can here.
#33
Attached:
t()from the custom assertion message (see http://drupal.org/simpletest-tutorial-drupal7#t).NR for the sample implementations. Please check over them; I didn't actually test them.
#34
#35
The last submitted patch, field_ui-945524-33.patch, failed testing.
#36
#37
The last submitted patch, 945524-36.patch, failed testing.
#38
#36: 945524-36.patch queued for re-testing.
#39
Patch is good, ready to be committed imo
#40
This looks like a nice simple addition and it comes with test coverage. Thanks!
Committed and pushed to 8.x. I believe this should be eligible for backport, so marking as such.
#41
Hi,
I created patch for D7.
Thanks,
A. Jaffar ali
#42
When the patch for drupal 7 wil reviewed, when could it be in core? Will there be any conflicts with field formatter settings module?
#43
@jaffaralia: Thanks, but can you also backport the tests?
+++ b/modules/field_ui/field_ui.api.phpundefined@@ -170,6 +170,41 @@ function hook_field_formatter_settings_form($field, $instance, $view_mode, $form
+ ¶
spaces
@yannickoo: you can review/test this yourself
#44
Here is the patch without the whitespace. I don't know how to customize the test because I don't know how to write them yet :/
#45
BTW there is a typo in the test for 8.x:
+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageDisplayTest.phpundefined@@ -75,6 +75,22 @@ class ManageDisplayTest extends FieldUiTestBase {
+ $this->assertField($fieldname, t('Field added in hook_ield_formatter_settings_form_alter is present.'));
It should be hook_field_formatter_settings_form_alter. The "f" from field is missing.
#46
Can you create a new issue with a patch to fix it?
#47
Oh, I think webchick fixed the typo in her commit.
#48
It's very difficult to backport the test because in drupal 7 there is no test module, just the field_ui.test and I don't know how to implement the hooks in the test file. Please advise how to do this.
#49
You have to create a test module for this, but I think easy way is to copy/paste it from Drupal 8, get rid of all the code that isn't needed and create a new SimpleTest that enables this module.
#50
Ok, moving back to 8.x first as this simply doesn't work - without a work around though. Found this out while working on #1234624: Limit the number of fields to display where I started implementing this hook in field ui as that makes sense.
The key problem is in field_ui.admin.inc line 1379
<?php// Only save settings actually used by the selected formatter.
$default_settings = field_info_formatter_settings($values['type']);
$settings = array_intersect_key($settings, $default_settings);
?>
Now, there is a work around :
<?php/**
* Implements hook_field_formatter_info_alter().
*/
function field_ui_field_formatter_info_alter(&$info) {
$info['image']['settings']['display_limit'] = 0;
$info['image']['settings']['display_offset'] = 0;
}
?>
I can live with that, but then we should make sure we update the documentation in field.api.php. Thoughts ?
#51
Ok, investigated how the modules in 7.x do this and they actually implement hook_field_formatter_info_alter() which makes sense, let's make sure we document this in the api.php so people know what todo.
#52
Moving back to 7.x - I also though the tests was bogus, but it isn't, let's try to get this ok for 7.x first.
#53
#44: drupal-field_formatter_settings-945524-44.patch queued for re-testing.
#54
#55
D7 backport, including the D8 tests.
#56
That is a straightforward backport. Looks good, applies and passes tests => RTBC
#57
And for bonus points, we're going to want this for D7 project_issue, which in turn impacts the D7 port of Drupal.org. ;)
#58
Means that this feature could be in the next Drupal 7 release and all the modules which depend on Field formatter settings module won't need that module as dependency.
#59
Uh-oh ...
... Do we have a naming conflict between this patch and Field formatter settings? Given that the hook is named the same between the two, will anyone with Field formatter settings applied end up double alter()-ing their forms if they upgrade to a core release using this?
#60
In the hook_update_n hook we have to disable and uninstall the other module right?
#61
#60: We cannot disable a contrib module from core, but we can open up an issue on the contrib module to deprecate the module when 7.xx ships. Given that Dave Reid is the maintainer, I guess that will be no problem.
#62
Oh sorry, absolutely not in the core but in the contrib modules which depend on that module. It's important to notify the maintainers from the modules which require it and Dave to close his (great) project.
#63
I guess that if Dave wants to take it easy on the ~3000 users of the module, he could release a 7.x-1.1 or 7.x-2.0 version that checks core version before it does its magic:
- If installed core version < 7.xx, then work the same as 7.x-1.0.
- If installed core version >= 7.xx, then only display a warning/message of some kind (site status page perhaps?) to disable and uninstall the module.
Does that make sense?
#64
Sounds good but I wouldn't put that message on the status page. I would check that in
hook_initand display a message viadrupal_set_messagewith a warning level.#65
There's an easier approach and that's simply won't fixing this issue for D7. The module exists and is stable. I guess it's Dave's call.
#66
#65: Please see #57. It would make the life of D.org D7 easier without adding yet another module dependency ...
#67
Not only for D.org D7 it is better because you don't have to install an extra module for this little functionality.
#68
Ok, missed #57, it's just that it's sometimes that people will miss these kind of announcements, I'm ok with both :)
#69
To avoid misunderstanding this means that we just have to wait for a D7 administrator who will commit #55?
#70
Seems like it would be a good idea to get some feedback from Dave Reid here, since he is the maintainer of the Field formatter settings module.
Looking at that module's code, I'm also noticing that it does some things with Views, so I'm not sure it would actually be as simple as turning off that module if/when these hooks get added to core.
To be honest, if that module didn't exist, we'd probably just add the hooks to core. But it does exist, has 3,000 happy users, lots of other modules that depend on it, etc. Maybe this is a case where we should just leave well enough alone? :)
#71
Okay, so in Drupal 7 there is no way to get this feature in. I can understand that you mention the Views thing. That feels like Imagecache in Drupal 6 :D
I will open a new issue with the "Views thing" for Drupal 8 of it's not in core yet.
#72
Seems like we should move this back to "needs review" while we figure all that out.
Interesting point - thanks!
#73
Created a followup issue with the patch #1875002: Allow other modules to alter field formatter settings in views fields.
#74
...friendly bump ;)
#75
So I would definitely but shutting down the field_formatter_settings module if this were accepted into core. I would have to wait until the next release of core in order to update the .info file of the module to specify that a version less than Drupal 7.2? is necessary to use the module and would error if you try to use the module with a higher version of core.
As for the hooks, I don't see any reason why having both the core and the contrib module invoking the same hooks.
For hook_field_formatter_settings_form_alter(), having it invoked twice means that the same form elements get written in. I haven't seen a use case where these form elements don't have hard-coded key values, so they would just get overridden on the second invocation.
For hook_field_formatter_settings_summary_alter(), having it invoked twice would mean that the summary possibly includes duplicated information. But it doesn't affect functionality and will not cause any bugs. This would be a good key to the user that they need to disable the module.
#76
As well as the two hooks, the D7 contrib API module provides a helper function to get the formatter settings:
field_formatter_settings_get_instance_display_settings()
Some of the field_formatter_settings modules ditched their own helper functions in favour of this.
We'd need to include this in D7 core along with the hooks, otherwise modules will have to go back to implementing their own helper function again.
It's not an issue for D8, because entity_get_render_display() is available.
#77
Since Drupal.org D7 upgrade needs this in order to optimally solve #1628044: Implement magic for the table of attachments on issues, which is a critical task, moving this to a task rather than a feature.
However, since just adding a dependency on http://drupal.org/project/field_formatter_settings in the meantime doesn't seem the end of the world, keeping it as just a "normal" task, rather than a major/critical one. If that's incorrect, feel free to bump accordingly.
#78
Correct, it'd make life better if we had this, but we don't technically need it. Therefore, not major/critical.
Thanks,
-Derek