Document field data hooks: hook_field_views_data[_alter](), hook_field_views_data_views_data_alter()

Oh sure this definitive makes sense.

Let's tag it so that people can help on this issue.

This hooks is executed per field, not per field instance.

Do we need to or can we do something with $data['field_revision_' . $field_name]?

Well depending what you are doing, but in general the structure "field_revision_" contains nearly the same kind of information as field_data_...

Oh wow, thank you! committed to 7.x-3.x
We should also add that to Drupal 8.

Straight patch port from D7

+++ b/core/modules/views/views.api.php
@@ -327,6 +327,81 @@ function hook_views_data_alter(array &$data) {
+ *  A field definition array, as returned by field_info_fields().
...
+ *  A field definition array, as returned by field_info_fields().

These methods are certainly called different in D8. Additional we should add typehints. Result is an array of ResultRow, for field we probably have an interface, etc.

If there are Views hooks that exist in D8 and have no hook documentation, this is a critical bug because all hooks are supposed to be documented when introduced, according to the documentation gate.

And setting status according to previous review of patch.

"critical bug". Yeahhhhh.

Yeaaaaaah totally.

15: air-quotes-critical-bug-1428784-15.patch queued for re-testing.

Why is this in views.api.php and not field.api.php?

Why is this in views.api.php and not field.api.php?

IIRC this patch is documenting the hooks that are fired from field module implementation of views hooks.
Not sure about usual way of where to put the docs, two arguments:

1. field is a core module, enabled by default, so document it on views.api.php
2. field is exposing hooks(it does not matter where they come from), so document it on field.api.php

I would go with 2(they are not hooks exposed by views module), but waiting feedback from someone else before changing to NW.

This confusion about where to put the hooks would be a lot less, I think, if the first line of the hook documentation said something about Views. hook_field_views_data() currently says:

+ * Override the default data for a Field API field.
+ *

It should definitely say something different! Keep in mind these types of descriptions appear all over the place on api.drupal.org and kind of need to stand alone, out of the context of the file they are in.

That said, I agree these hooks probably belong in the views.api.php file. They are Field hooks used only for Views.

Other issues with this documentation:
b) @see lines should be at the end. See https://drupal.org/node/1354#order

c) descriptions in param/return sections should be indented 2 spaces, not one as they are in the patch.

d) ALL hooks MUST have sample function bodies.

e)

+function hook_field_views_data_alter(&$result, \Drupal\field\FieldInterface $field) {

To me $result is not a sensible name for the first parameter, at all. Can you change it to something that actually makes sense please?

f) The one-line description for hook_field_views_data_views_data_alter(), to me, was really unclear. Can you write something that gets across that it alters all Views data after the field's data tables have been defined? And really... I don't get why it has to be done this way? Why would the other tables' data not be defined at some point and then be defined later -- wouldn't hook_views_data_alter() be invoked after all of the hook_views_data() This really makes zero sense to me as it is written.

g) Also this hook has no param docs.

Assigning to me, per core mentoring today

1. changed the first line of the hook documentation.
2. moved @see lines at the end.
3. added 2 spaces into the descriptions in param/return sections as they should be.
4. added sample function bodies at the hook_field_views_data_alter.
5. renamed file name from FieldInterface to correct FieldConfigInterface.

Interdiff for #24 comment.

Sorry, here is correct interdiff for #24.

This patch removes a lot of blank lines that were separating things in the file, such as:

  * @file
  * Describes hooks and plugins provided by the Views module.
  */
-
 /**
  * @defgroup views_plugins Views plugins
  *
...
  // );
-
   // First, the entry $data['example_table']['table'] describes properties of
   // the actual table – not its content.
-

Why? Please reverse that.

Other than that, it looks pretty good. A few minor things to fix:

a)

+ * Override the default data for a Views API field data.

This should be: Override the default Views data for a Field API field.

b)

+ * Field module's implementation of hook_views_data() invokes this for each
+ * field in the module that defines the field type (as declared in the field
+ * array). It is not invoked in other modules.

I think this would be a lot clearer if there was a comma after "field" at the beginning of the 2nd line. Also start the first line with "The". This text appears twice in the patch (in two different hooks).

c)

+ * @param array $result
+ *  An array of views table data provided for a single field. This has the same
+ *  format as the return value of hook_views_data().
+ * @param \Drupal\field\FieldConfigInterface $field
+ *  A field config entity.

Needs one more space of indentation of the @param docs. Also, see previous review -- can we please change the parameter name from $result to something else that actually makes sense?

Also, grammatically this would be clearer if everywhere instead of "a" field, it referred to "the" field, because it is not just any field, it is specifically the field whose data is being altered.

I haven't reviewed the function bodies, but hook_field_views_data_views_data_alter() function body is still not complete.

Adding tag.

1. I returned the blanks lines.(Sorry, it was my Netbeans)
2. Changed first description line to "This should be: Override the default Views data for a Field API field."
3. Added comma after "field" and "The" in first lines.

This text appears twice in the patch (in two different hooks).

There are almost the same. Can you give some advise how to describe the difference ?

I haven't reviewed the function bodies, but hook_field_views_data_views_data_alter() function body is still not complete.

What do you mean?I don't know how improve it.

Thanks! Still some issues:

a)

* Override the default data for a Views API field data.

This is not quite right. What is being overridden is the Views information for a Field API field, not "Views API field". I still suggest:
Override the default Views data for a Field API field.

b) Also, please fix the other hook first-line descriptions similarly -- they were better, in my opinion, in the previous patch.

c)

+ * @param array $result
+ *   An array of views table data provided for a single field. This has the same
+ *   format as the return value of hook_views_data().
+ * @param \Drupal\field\FieldConfigInterface $field
+ *   A field config entity.
+ *
+ * @see field_views_data()
+ * @see hook_field_views_data()
+ * @see hook_field_views_data_views_data_alter()
+ */
+function hook_field_views_data_alter(array &$result, \Drupal\field\FieldConfigInterface $field) {

Again, can we ***please*** change $result to something better here? Such as $field_data or something else that makes sense? $result makes no sense in this context.

d)

 * Unlike hook_field_views_data_alter(), this operates on the whole of the views
+ * data. This allows the field module to add data that concerns its fields to
+ * other tables, which would not yet be defined at the point when

In the 2nd line at the end "to" should be "in".

e) What I meant by the function body:

+function hook_field_views_data_views_data_alter(array &$data, \Drupal\field\FieldInterface $field) {
+  $field_name = $field->getName();
+  $data_key = 'field_data_' . $field_name;
+  // Views data for this field is in $data[$data_key]
+}

This function body doesn't alter any data, so it is incomplete.

Thanks!

Don't hit retest please! We don't need that patch retested and it was an unrelated test failure.

i am updating patch #24 as per
#27 comment a,b,c
#32 Comment d,e

Thanks! This is looking a lot better. Now mostly small things to fix:

a) hook_field_views_data

+ * @param \Drupal\field\FieldInterface $field
+ *   A field definition.

A ==> The

b) hook_field_views_data

+ * @return array
+ *   An array of views data, in the same format as the return value of
+ *    hook_views_data().

Extra space of indentation at the beginning of the last line.

c) hook_field_views_data vs. hook_field_views_data_alter: The first line of one talks about the "Views data" and the other "views data". Choose one capitalization please. I don't care which but be consistent.

d) hook_field_views_data_alter should probably say that it is called on all modules, presumably?

d) hook_field_views_data_alter

+ *  @param array $result
+ *  An array of views table data provided for a single field. This has the same
+ *  format as the return value of hook_views_data().
+ *  @param \Drupal\field\FieldConfigInterface $field
+ *  A field config entity.

The descriptions need to be indented two spaces for both the @param items here. Also in both, "a" field should be "the" field (or field config entity).

e) In hook_field_views_data_alter(), the $result parameter really needs a better name. Please change it to something other than $result. Like maybe $data? Also the function body is apparently using $data, I think?

f) There are still some unrelated comment changes in this patch. Please remove them from the patch.

Updating patch as per #36. Please review.

Much better, thanks!

Still a few mostly minor things to fix:

a) Please get rid of this change that is still in the patch:

   // The handler descriptions are described with examples below.
-
   // Node ID table field.

That blank line is actually a good idea, and I do not know why this patch is removing it.

b) hook_field_views_data_alter

+ *  @param array $data
+ *    An array of views table data provided for a single field. This has the same
+ *    format as the return value of hook_views_data().

When you use "a" and "an" in English, it indicates something that is unspecified and indefinite. That is not appropriate in this case. The docs should say "The views data for the field." not "An array of ... for a single field.", because that is specifically what is being altered by the hook.

c) Same hook -- since this is called on all modules, I am a bit confused by the sample code. It doesn't seem to be checking the field type, but just making a relationship as if it is an entity reference field. That doesn't seem right. Where did this code come from? Is it missing something?

d) The @param docs are now missing from hook_field_views_data_views_data_alter

Most issues from #38 fix except (c) as I am also slightly confused by the sample code. ( and more importantly I think it is outdated )

I have another set of issues to raise PHPStorm highlights to follow issue.

In a strict sense there is a mismatch between the annotations and the function definitions in 11 places.

Whereever the function definition defines a parameter to be of type ViewExecutable
The annotations use the fully qualified namespace name \Drupal\views\ViewExecutable

so either

1) The documentation is correct and the functions definitions should use \Drupal\views\ViewExecutable OR alternatively the "use \Drupal\views\ViewExecutable;" is missing from the top of the file.

2) The original coders are using the "bubble upwards" feature of unqualified namespaces and obtaining the first ViewExecutable to match...thereby implying the fully qualified annotations are wrong.

I think number 2 is highly unlikely.. but in a critical issue I don't want to be flamed for extending the scope of issue.
If reviewers think that a side benefit of documentations patches it to catch these things early then I will gladly add the use statement

That's not quite right:
- The API module will make the correct link if there is a use statement.
- Our docs standards require that the fully-qualified class name be used after an @tag (like @param \Fully\Qualified\Class $foo or @see \Fully\Qualified\Class) but in other places you can use the class name without the namespace.

It is correct, it is defined that hook documentations should use the full namespace, not for api.module but for users which are copying those as examples, so that they don't have to look up the correct use statement too.

Documented in https://drupal.org/node/1353118. It's actually a bit weird, though, because that says to use "use" if you have multiple hooks with the same argument. I think that was moved around incorrectly, you can see the original whole paragraph in https://drupal.org/node/1353118/revisions/view/1767012/1819308

This page contradicts:
https://drupal.org/node/1354#classes

Hm... guess it doesn't. Actually apparently the current standard is:

If you use a namespace on a class anywhere in documentation, always make sure it is a fully-qualified namespace (beginning with a backslash).

My mistake...

I'm not sure what we are supposed to do for hook function signatures in api.php files though... I don't know that we have adopted a standard for that?

Sorry! This has been sitting for a while without a real review.

Let's not worry about the details of whether we should put a "use" statement in the api.php file or the full class in the hook signatures. Either way I think will get the point across to users of what class is expected, and I think the current patch is fine. Hook functions are *documentation* after all, and people should figure out that in their own module they'd need to shorten and use "use". So let's leave this as is. Or, we could just put the full namespaced class name in the @param (which already has it), and omit the namespace in the function declaration line? I don't care too much either way.

With that out of the way, I reviewed the latest patch.

This seems a bit odd:
hook_field_views_data(\Drupal\field\FieldInterface $field)
hook_field_views_data_views_data_alter(array &$data, \Drupal\field\FieldInterface $field)
vs.
hook_field_views_data_alter(array &$data, \Drupal\field\FieldConfigInterface $field)
[note the different class for the same input variable name $field]

Looking at field_views_data(), which is where both of these hooks are invoked, it looks like it is passing the same exact $field variable into both hooks. So this cannot be right.

The rest of the patch looks good!

I tried to find the FieldInterface but didn't succeded. So, I think $field is referencing
\Drupal\field\FieldConfigInterface. I have updated the api.php accordingly.
Please review now.

Thanks! I believe you are right. field_views_data() does:

 foreach (\Drupal::entityManager()->getStorage('field_config')->loadMultiple() as $field) {

So each $field is a loaded 'field_config' entity, which is entity type FieldConfig, whose interface is FieldConfigInterface. Also, this is the documented type of the first argument to field_views_field_default_views_data().

So this patch looks great! The only thing I would suggest is:

+ * @param \Drupal\field\FieldConfigInterface $field
+ *   The field definition.

in the first and last hooks. The 2nd line here should probably say "The field config entity." like it does in the other hook.

thanks for review. Updated patch as #46.
Please review now.

Thanks! Looking good now.

Committed/pushed to 8.x, thanks!