Implement hook_hook_info() for Field API hooks

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 ?)

subscribe

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.

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.

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.

+++ 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.

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.

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.

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.

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.

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.

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.

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.

according to #40 ?

Are the @todo's on purpose?

Love the usage of module_hook()!

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.

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.

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.

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

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.

Tagging issues not yet using summary template.

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

subscribing from a @todo in file_entity

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..

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.

reroll

Thanks!

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.

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.

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.

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;

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.

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.

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.

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.

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

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.

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()

Are we going to backport this to d7 ?

What is a blocker actually ?

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.