Most field related modules are putting their entire Field API related code (hook implementations) into $module.field.inc already.

However, even though the separation makes totally sense and Drupal core even provides a facility to auto-load $module.field.inc, those modules still have to manually load their $module.field.inc include file an all pages to have them included.

This patch likely depends on

- #752226: module_invoke() doesn't work with hooks placed in include files via hook_hook_info()

and also, but rather optional, not strictly required:

- #968264: hook_hook_info() can lead to an infinite loop

Files: 
CommentFileSizeAuthor
#64 drupal-977052-63.patch18.51 KBthehong
PASSED: [[SimpleTest]]: [MySQL] 34,329 pass(es). View
#62 drupal-977052-62.patch18.32 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 33,442 pass(es). View
#43 drupal.field-hook-info.33.patch20.04 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.field-hook-info.33_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View
#35 drupal.field-hook-info.35.patch20.4 KBsun
PASSED: [[SimpleTest]]: [MySQL] 29,146 pass(es). View
#33 drupal.field-hook-info.33.patch20.04 KBsun
PASSED: [[SimpleTest]]: [MySQL] 28,820 pass(es). View
#31 drupal.field-hook-info.31.patch20.96 KBsun
PASSED: [[SimpleTest]]: [MySQL] 28,807 pass(es). View
#30 drupal.field-hook-info.30.patch24.66 KBsun
PASSED: [[SimpleTest]]: [MySQL] 28,776 pass(es). View
#28 drupal.field-hook-info.28.patch17.34 KBsun
FAILED: [[SimpleTest]]: [MySQL] 28,791 pass(es), 1 fail(s), and 1 exception(es). View
#26 drupal.field-hook-info.26.patch17.91 KBsun
FAILED: [[SimpleTest]]: [MySQL] 28,784 pass(es), 1 fail(s), and 1 exception(es). View
#21 drupal.field-hook-info.21.patch18.83 KBsun
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es). View
#19 drupal.field-hook-info.19.patch20.35 KBsun
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es). View
#13 drupal.field-info-hook.13.patch14.97 KBsun
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es). View
#11 drupal.field-hook-info.11.patch14.19 KBsun
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#8 drupal.field-hook-info.8.patch13.64 KBsun
FAILED: [[SimpleTest]]: [MySQL] 27,510 pass(es), 98 fail(s), and 554 exception(es). View
#6 drupal.field-hook-info.6.patch8.37 KBsun
FAILED: [[SimpleTest]]: [MySQL] 27,369 pass(es), 180 fail(s), and 265 exception(es). View
#2 drupal.field-hook-info.2.patch5.81 KBsun
FAILED: [[SimpleTest]]: [MySQL] 27,035 pass(es), 204 fail(s), and 783 exception(es). View
drupal.field-hook-info.0.patch4.29 KBsun
FAILED: [[SimpleTest]]: [MySQL] 27,050 pass(es), 183 fail(s), and 265 exception(es). View

Comments

Status: Needs review » Needs work

The last submitted patch, drupal.field-hook-info.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
5.81 KB
FAILED: [[SimpleTest]]: [MySQL] 27,035 pass(es), 204 fail(s), and 783 exception(es). View

So let's try whether that module_invoke() patch gets us down to the same test failures as in the other issue.

Status: Needs review » Needs work

The last submitted patch, drupal.field-hook-info.2.patch, failed testing.

yched’s picture

This definitely makes sense.

(does #752226: module_invoke() doesn't work with hooks placed in include files via hook_hook_info() mean that hook_hook_info() is plain and simple useless right now ?)

sun’s picture

Yes, that other module_invoke() patch is crucial. However, in #2, I merged that patch in and we still have lots of failures. Somehow, that either means that those existing $module.field.inc files contain API functions being directly called by other modules, or otherwise, Field API not using module_*() to invoke Field API hooks.

sun’s picture

Status: Needs work » Needs review
FileSize
8.37 KB
FAILED: [[SimpleTest]]: [MySQL] 27,369 pass(es), 180 fail(s), and 265 exception(es). View

Alright, some conclusions:

- Merged revised module_invoke() patch.

- hook_field_settings_form() is not documented in field.api.php currently, and thus, missing in the list of hooks I put into hook_field_info().

- OT: File field settings are ugly.

Status: Needs review » Needs work

The last submitted patch, drupal.field-hook-info.6.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
13.64 KB
FAILED: [[SimpleTest]]: [MySQL] 27,510 pass(es), 98 fail(s), and 554 exception(es). View

- Added a stopgap fix for file_field_widget_process() and image_field_widget_process(). See #561226: Forms (elements) need to able to specify include files to be loaded for building for details.

- Ordered hook names in field_hook_info() alphabetically.

- Changed Image module Field API hooks to use module_invoke(), in order to lazy-load File module Field API implementations.

Status: Needs review » Needs work

The last submitted patch, drupal.field-hook-info.8.patch, failed testing.

sun’s picture

OH NOES. module_invoke() uses func_get_args(), which always passes copies of arguments.

sun’s picture

Status: Needs work » Needs review
FileSize
14.19 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

I guess we need a module_load_hook() to resolve this.

But nevertheless, I'd like to see what happens with attached patch.

Status: Needs review » Needs work

The last submitted patch, drupal.field-hook-info.11.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
14.97 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es). View

So here's the module_load_hook() approach, including further fixes for field.form.inc.

One could argue that the module_load_hook() code belongs into the existing module_hook() helper function tough.

Additionally, I realized that some Field API hooks are defined by Field UI (in field_ui.api.php), which explains why they were not included in field_hook_info() before. I think that it's wrong to define them in field_ui.api.php -- they belong to Field API, not the UI.

andypost’s picture

subscribe

yched’s picture

All field-type "hooks / callbacks" - hook_field_[load|presave|insert|...]() - are generally invoked via _field_invoke(), which avoids module_invoke() too. Thus module_load_hook() needs to be called in there too ?

The current patch passing the tests is possibly lucky - like, file.field.inc file happens to be always loaded already _field_invoke() tries to call one of those hooks.

Crell’s picture

IMO the hooks should be split into CUD and Read groups. It's a reasonably safe bet that we'll need the read hooks on most page requests, but the CUD hooks on a tiny fraction of page requests. Thus we should be able to load the one without the other.

sun’s picture

@all: Don't get mistaken by testbot's status. It has "0 passes". This patch doesn't work at all, the entire lazy-loading of hooks is broken in HEAD currently.

@yched: Thanks for the hint! Will investigate.

@crell: Premature performance optimizations are always wrong. I don't see how we can safely cache hook_hook_info() depending on a certain kind of page request (and, where in Drupal do we decide and enforce that "some" page requests are CUD and others not? Sorry, doesn't make sense to me right now.) Lastly, additional performance optimization is the last thing I'm worried about right now.

Currently, this entire construct looks so darn broken that I already doubt we can fix it - i.e., already considering a rollback. For that sake, the other issue has been marked critical.

Crell’s picture

It's hardly premature. We already know conclusively and with benchmarks that reducing the amount of unused code we load on a page has a considerable positive impact on response time on a non-APC site. Reducing wasted code weight was one of the key reasons for adding hook_hook_info(). (Code organization was another, but that can be hacked around more easily than performance.)

We don't need to decide if a given page is "read" or "write". That's the whole point of making the hooks lazy-load. They'll get loaded if they're used and won't if not. I don't think it's a stretch to say that a typical node/$nid page is not going to be calling the various field_attach_update() hooks. It's the same logic as the page callback files, which were probably the single biggest performance savings in Drupal 6.

sun’s picture

FileSize
20.35 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es). View

@crell: Now I understand what you mean. Unfortunately, that's no longer possible to do for D7, because File and Image modules already use $module.field.inc to implement those hooks + many contributed modules followed that pattern already.

Attached patch adjusts _field_invoke* and some more dynamic $function invocations I found.

yched’s picture

+++ modules/field/field.crud.inc	22 Nov 2010 22:43:59 -0000
@@ -457,6 +457,7 @@ function field_update_field($field) {
+  // @todo module_invoke() doesn't pass by reference.
   module_invoke($storage_type['module'], 'field_storage_update_field', $field, $prior_field, $has_data);

Shouldn't be an issue, none of the params are supposed to be altered

+++ modules/field/field.crud.inc	22 Nov 2010 22:43:59 -0000
@@ -911,6 +912,8 @@ function field_read_instances($params = 
+      // @todo (and elsewhere) module_invoke_all() doesn't pass by reference, so
+      //   what is the purpose of this hook?
       module_invoke_all('field_read_instance', $instance);

I don't think the intent here was to allow alteration of the $instance - which would open a blurry area. This being said, I'm not sure I remember what the intent was :-(.

Powered by Dreditor.

sun’s picture

FileSize
18.83 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es). View

Conclusion: No need for a new module_load_hook(). The existing module_hook() is exactly the right location.

Removed those two @todos. Not sure why the patch in #19 was ignored for testing.

Stevel’s picture

Status: Needs review » Needs work

I think all the combinations of

 module_load('file', $hook); 
 file_$hook($args) 

should be replaced by module_invoke('file', $hook, $args); instead, which I think is the proper way of calling other module's hooks, and can be thought of as the responsible for making sure the hook is included when needed.

sun’s picture

Status: Needs work » Needs review

That's not possible, because module_invoke() passes copies of the arguments, but the arguments are expected to be altered. Possible resolutions: 1) Turn all of those lines in Image module into if (module_hook('file', '...')) conditions (even though we expect and require the expression to be always true), or 2) use a @file-level phpDoc comment that explains that module_hook() usage. Repeating the same comment all over again is not an option.

beejeebus’s picture

subscribe.

webchick’s picture

Um. What on earth?

If there's a part of the code that's triggering a fatal error because the right file isn't included at the time, then let's include it at the time. This patch goes way over the top, throwing a gargantuan amount of changes around when we are absolutely done making gargantuan changes.

The two issues mentioned in the OP seem like legitimate bug fixes. This one seems like "oops, we totally forgot to do this" which == D8.

sun’s picture

FileSize
17.91 KB
FAILED: [[SimpleTest]]: [MySQL] 28,784 pass(es), 1 fail(s), and 1 exception(es). View

Merged in latest patch from #752226: module_invoke() doesn't work with hooks placed in include files via hook_hook_info().

Also added a hard-coded loading of file.field.inc in image.field.inc, as webchick suggested.

Status: Needs review » Needs work

The last submitted patch, drupal.field-hook-info.26.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
17.34 KB
FAILED: [[SimpleTest]]: [MySQL] 28,791 pass(es), 1 fail(s), and 1 exception(es). View

Updated to latest code from #752226 once again. Code in #26 was much cleaner when module_hook() returned the $function name, but that was vetoed.

Status: Needs review » Needs work

The last submitted patch, drupal.field-hook-info.28.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
24.66 KB
PASSED: [[SimpleTest]]: [MySQL] 28,776 pass(es). View

$form and $form_state are not passed to hook_field[_instance|_widget]_settings_form(), but that is essential for any kind of form constructing callback.

sun’s picture

sun’s picture

Any feedback? Latest patch passed.

sun’s picture

FileSize
20.04 KB
PASSED: [[SimpleTest]]: [MySQL] 28,820 pass(es). View

Removed the unrelated file field settings UI changes. This patch looks RTBC to me.

moshe weitzman’s picture

I think you have unintentionally? fixed a bug from the registry rollback. _field_filter_items() currently has a useless function_exists(). The patch actually loads code instead which is possibly what the author intended?

Lets clear that up and then this is ready.

sun’s picture

FileSize
20.4 KB
PASSED: [[SimpleTest]]: [MySQL] 29,146 pass(es). View

Yeah, I wasn't quite sure what to do with that function. It almost looks like hook_field_is_empty() is a required hook implementation for field type modules right now, since it is invoked unconditionally.

Attached patch fixes _field_filter_items() to only invoke hook_field_is_empty() if it's defined. The existing code additionally contained a stale comment that more or less implies that no filtering happened when there is no hook_field_is_empty() implementation previously (pre-registry?).

yched’s picture

The function_exists() in the middle of _field_filter_items() is indeed a remnant of the registry (the patch reverting that just did a mass s/drupal_function_exists/function_exists, leaving that useless call here.
I'm ok to remove it, but not to change the behavior of the function.

It has always been considered that hook_field_is_empty() is a required hook, and that a module failing to implement it is broken.
I guess this can be discussed (again), but preferably not in the middle of an unrelated patch.

sun’s picture

ok, that would mean to go back to #33, as we still need to call module_hook() to ensure that $module.field.inc is loaded.

yched’s picture

Other than that, I'm OK with the patch (patch #33, then). I'm in favor of this, [module].field.inc is a good pattern for contrib. I can hardly make a hard, pressing case for Dries and webchick, though :-/

One nice side effect is that the patch restores code tracability (where is hook_foo_bar() invoked ? re: run a full-search on 'foo_bar'), which is impossible with the $function = $module . '_foo_bar' workarounds for module_invoke() not being able to pass by ref.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

agree with RTBC for #33. If we are going to have a hook registry, it is foolish not to use it. module_hook() was just repaired a couple days ago so I think it makes sense for followups like this one at this time.

a bunch of code is mechanically changed in this patch. it isn't as invasive as it might look.

webchick’s picture

Dries has traditionally been against this sort of splitting, so this needs sign-off from him.

I personally believe the time for patches like this was over a year ago, and not hours before a RC.

sun’s picture

It's Drupal core that started to split Field API hooks into $module.field.inc. Contributed modules are merely following that lead.

If we do not fix the loading of the Field API hook implementations (which is what this patch does), then there is no point in splitting into $module.field.inc in the first place.

Especially because we are right before a RC, it makes little sense to question the $module.field.inc split. Instead, the expected functionality needs to made work. (i.e., without relying on a hidden magic require_once being squeezed into a module file)

I think it would be wrong to ignore this patch. The $module.field.inc split makes sense from a technical and performance perspective, especially because field hook implementations are not needed on every single request. Therefore, it doesn't make sense to load all of the implementations on every request - especially not when we already have the facility of module hook groups.

Aside from that, this patch fixes a couple of oversights in form-related Field API hooks, which absolutely have to be fixed either way, but wouldn't have been discovered without this patch.

sun’s picture

#35: drupal.field-hook-info.35.patch queued for re-testing.

sun’s picture

FileSize
20.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal.field-hook-info.33_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots. View

meh, got confused. Re-uploading #33.

yched’s picture

Issue tags: +Needs committer feedback

according to #40 ?

sun’s picture

#43: drupal.field-hook-info.33.patch queued for re-testing.

scroogie’s picture

Are the @todo's on purpose?

sun’s picture

Issue tags: -Needs committer feedback

#43: drupal.field-hook-info.33.patch queued for re-testing.

sun’s picture

Issue tags: +Needs committer feedback

#43: drupal.field-hook-info.33.patch queued for re-testing.

sun’s picture

There is a full range of important bug fixes in this patch.

casey’s picture

Love the usage of module_hook()!

catch’s picture

Just looked at this, I don't like the way this is going because as soon as Drupal 8 opens up I want to rip out all the hook_field_*_$op() 'hooks' and make them into proper callbacks defined in hook_field_*_info() instead.

I'm not changing status, but -1.

yched’s picture

re @catch : I opened #1032850: Get rid of Field API's pseudo-hooks.

I'm don't see why the direction we take in D8 would prevent the change in D7, though.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +needs backport to D7
sun’s picture

#43: drupal.field-hook-info.33.patch queued for re-testing.

webchick’s picture

Issue tags: -Needs committer feedback, -needs backport to D7

I'm not sure why this is tagged for D7 backport. D7.0 has shipped. Contrib modules have ported. Wide-scale re-organizing code files at this point is not on the table.

Dave Reid’s picture

New modules can use the new files split. Existing code would be unaffected.

catch’s picture

Priority: Major » Normal
Issue tags: +needs backport to D7

Downgrading from major, whether you can put specific hooks in includes or not does not qualify as 'major', I am still -1 overall but don't care enough either way to change status.

However I'm also adding the Needs backport tag because this may well end up only making sense for D7 at all and it's an addition rather than a change - no code needs to move in D7 at all. If it's rejected there (again), then it doesn't make a lot of sense to me to commit to D8 since we want to make more serious changes to field info hooks there.

xjm’s picture

Tagging issues not yet using summary template.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +needs backport to D7

The last submitted patch, drupal.field-hook-info.33.patch, failed testing.

aaron’s picture

subscribing from a @todo in file_entity

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.32 KB
PASSED: [[SimpleTest]]: [MySQL] 33,442 pass(es). View

Reroll with git prefixes.

The only part that was rejected was changing function_exists($function); to module_hook($field['module'], 'field_is_empty'); in _field_filter_items(), because of #1300482: Remove cruft function_exists() from _field_filter_items() leftover from the (removed) function registry..

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Tagging as novice for the task of rerolling the Drupal 8.x patch on account of #22336: Move all core Drupal files under a /core folder to improve usability and upgrades.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

thehong’s picture

FileSize
18.51 KB
PASSED: [[SimpleTest]]: [MySQL] 34,329 pass(es). View

reroll

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Novice

Thanks!

sun’s picture

#64: drupal-977052-63.patch queued for re-testing.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

There are a couple of suggested cleanups of Field API that would make this redundant. However, this is ready, those are not, and this makes a clear improvement over the status quo without making those other cleanups more difficult.

Let's do.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs profiling

Since this is supposed to be a performance improvement, someone please show the memory difference with APC enabled. That is a large array to be adding in hook_hook_info() that may well outweigh the performance benefit from loading less code.

catch’s picture

Assigned: sun » webchick
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs profiling

Actually it's OK because we only call hook_hook_info() on a module_implements() cache miss, so tag withdrawn :P

Since this is for backport and webchick wasn't sure about it first time around, moving to her for consideration. Patch looks fine to me for 8.x - all we're doing is allowing contrib modules to use the hook (and minor clean-up in image module), not a mass conversion of all core modules to the pattern or anything.

sun’s picture

Assigned: webchick » catch

I'd like to see a commit to D8 first, so this issue is off the table. It doesn't really matter whether this will be backported or not, or whether we might revamp the entire field system or not. I.e., if we'd release D8 in 2 months from now, then at least D8 would be fixed. That's the pragmatic thinking we should apply, IMHO.

catch’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs review

If it is not going to be backported, then I'd consider this won't fix for 8.x, because no-one has explained how we're going to maintain a manually created list of hooks so long that it scrolls on my work monitor.

+function field_hook_info() {
+  $field_hooks = array(
+    'field_access',
+    'field_attach_create_bundle',
+    'field_attach_delete',
+    'field_attach_delete_bundle',
+    'field_attach_delete_revision',
+    'field_attach_form',
+    'field_attach_insert',
+    'field_attach_load',
+    'field_attach_prepare_translation_alter',
+    'field_attach_preprocess_alter',
+    'field_attach_presave',
+    'field_attach_purge',
+    'field_attach_rename_bundle',
+    'field_attach_submit',
+    'field_attach_update',
+    'field_attach_validate',
+    'field_attach_view_alter',
+    'field_available_languages_alter',
+    'field_create_field',
+    'field_create_instance',
+    'field_delete',
+    'field_delete_field',
+    'field_delete_instance',
+    'field_delete_revision',
+    'field_display_alter',
+    // @todo http://drupal.org/node/968264
+    'field_display_ENTITY_TYPE_alter',
+    'field_extra_fields',
+    'field_extra_fields_alter',
+    'field_extra_fields_display_alter',
+    'field_formatter_info',
+    'field_formatter_info_alter',
+    'field_formatter_prepare_view',
+    'field_formatter_settings_form',
+    'field_formatter_settings_summary',
+    'field_formatter_view',
+    'field_info',
+    'field_info_alter',
+    'field_info_max_weight',
+    'field_insert',
+    'field_instance_settings_form',
+    'field_is_empty',
+    'field_language_alter',
+    'field_load',
+    'field_prepare_translation',
+    'field_prepare_view',
+    'field_presave',
+    'field_purge_field',
+    'field_purge_field_instance',
+    'field_read_field',
+    'field_read_instance',
+    'field_settings_form',
+    'field_storage_create_field',
+    'field_storage_delete',
+    'field_storage_delete_field',
+    'field_storage_delete_instance',
+    'field_storage_delete_revision',
+    'field_storage_details',
+    'field_storage_details_alter',
+    'field_storage_info',
+    'field_storage_info_alter',
+    'field_storage_load',
+    'field_storage_pre_insert',
+    'field_storage_pre_load',
+    'field_storage_pre_update',
+    'field_storage_purge',
+    'field_storage_purge_field',
+    'field_storage_purge_field_instance',
+    'field_storage_query',
+    'field_storage_update_field',
+    'field_storage_write',
+    'field_update',
+    'field_update_field',
+    'field_update_forbid',
+    'field_update_instance',
+    'field_validate',
+    'field_widget_error',
+    'field_widget_form',
+    'field_widget_info',
+    'field_widget_info_alter',
+    'field_widget_properties_alter',
+    // @todo http://drupal.org/node/968264
+    'field_widget_properties_ENTITY_TYPE_alter',
+    'field_widget_settings_form',
+  );
+  $hooks = array_fill_keys($field_hooks, array(
+    'group' => 'field',
+  ));
+  return $hooks;
Dave Reid’s picture

I didn't get any impression from #70 that webchick won't backport this. Only that it needs to be committed to D8 first. I think we have a misfire in communication here.

catch’s picture

Yes but since I'm not really keen on committing it to 8.x unless it'll get smoothly backported to 7.x, I assigned to webchick (who has the handy ability to be able to commit to both branches). Since sun assigned it back to me and asked for an 8.x-only review without regard to whether it'll get backported on that basis, I gave an honest one within those constraints.

Crell’s picture

From #69:

Patch looks fine to me for 8.x

But it doesn't look fine for 8.x if it's not getting backported to 7.x, which we don't know either way? Does not compute.

If this is not getting backported to 7.x, I'd actually propose breaking up that list further to better organize field hooks. (Do we ever use all of them in a single page request? I don't think so.) But that's a webchick question.

Either way, "who is going to maintain it", well, we maintain api.php files, don't we? Currently this is the only way we have to not have every possible hook implementation in all of Drupal loaded at all times. I'd love something better but this is what we've got, so let's use it.

That Field API is based on an insanely long list of not-hooks-that-we-lie-and-are-called-hooks is a separate problem, and not one that is going to be resolved in this issue.

catch’s picture

Yes it's fine for 8.x if it's backported to 7.x, but not if it's not.

I'm happy for this to be committed to 8.x so that 7.x field modules can organise their hooks how they want, because we may still have to have module_implements() groups in 8.x for release, and to keep things in sync.

However for example I have no interest in committing this without backport if it results in a bunch of 8.x patches to move core field module hooks around into includes. That is a waste of time compared other refactoring (which yched is already working on), so I'd rather wait and see how many hooks we have left before it goes in if this is the case, then it would be a shorter list to maintain anyway.

On $module.api.php, having a list of hooks in two places is no reason to add a third.

agentrickard’s picture

Have we considered splitting up the gigantic hook list into smaller, more targeted lists?

For consideration:

  • Field
  • Field attach
  • Field formatter
  • Field storage
  • Field widget
ralf.strobel’s picture

Looking at the last patch, I think you've missed a spot:

In field_ui.admin.inc, field_ui_display_overview_form()

The invocation of hook_field_formatter_settings_form(), line 1057, only uses a function_exists() to look for hook implementations.

I would also be very thankful for a quick backport to D7, by the way.

ralf.strobel’s picture

Also, I do strongly support agentrickard. A real benefit of using hook goups should be efficient lazy loading, so hooks should probably be separated by different kinds of use cases, or else the entire module.fields.inc will still be included on each page request.

My suggestion would be these major hook groups:

  • Cached info & Backend: hook_field_info(), hook_field_widget_info(), hook_field_formatter_info(), hook_field_settings_form(), hook_field_instance_settings_form(), field_widget_settings_form()...
  • Frontend forms: hook_field_widget_form()...
  • Storage: hook_field_insert(), hook_field_update(), hook_field_load(), hook_field_attach_insert(), hook_field_attach_update()...
  • Output: hook_field_formatter_view()
Sylvain Lecoy’s picture

Are we going to backport this to d7 ?

What is a blocker actually ?

Crell’s picture

Given that Field API is in the process of turning into plugins, not hooks, I think this issue can probably be marked won't fix.

sun’s picture

Status: Needs review » Closed (won't fix)

Righto.