Extrafield Views Integration

Hi Ricardo,

Thx for posting your sandbox module - looks great - but ...
I just checked your module for coding-standard violations. ventral.org found a few things, that should be easy to fix.
May you have a look at: http://ventral.org/pareview/httpgitdrupalorgsandboxdasricardo1858896git

For follow up code-check use: http://ventral.org/ and enter your git-url there..

Greetings,
SteffenR

Regarding ventral.org the obvious coding-style issues seem to be solved.

I just wanted to install the module on a page with some existing extra fields and got a WSOD.
Maximum function nesting level of '500' reached, aborting!

I think it is caused by a 4-times nested loop, you are using to get all extra-fields ( extrafield_views_integration.module line 35)- maybe you should break these loops into sub-functions to solve the issue.

SteffenR

Project decription is not clear in README.txt:

The Extrafield Views Integration Module enables all Drupal core extra fields in
the system from type "display" as fields in views.

Not obvious what this means. Maybe give examples of what 'extra fields' are and why/how you'd want to include them in views.

@file blocks just contain author info created by your IDE and don't describe the contents of the files.

Non-standard function naming on _extrafield_views_integration_getExtraFields(). Mixes underscore and camel case. Should be all camel case.

Explaining extra fields is a bit difficult and maybe something for the documentation team. It seems there is no d.o page we could link to.

What about adding a line like:

"Extra Fields are a Drupal Core concept to add output to an entity display that does not need a full field implementation"

I mean explaining this is very difficult and you will only need or look for the module if you understood the concept already. And we shouldn't explain the concept of fields or displays in this README either.

So I basically vote for "Lets start a documentation issue so we get a page we can link to later".

@gigabates

As you are a native speaker maybe you could give us a suggestion about the README.txt?

Sure, I'd like to help. Just so I can understand the use case myself, I get that modules use hook_field_extra_fields() to define 'pseudo fields' that appear in a rendered entity, allowing users to control the display order etc. Wouldn't the module normally integrate these fields with Views using hook_views_data()? Are there modules that don't currently integrate with views, where you would use this module as a workaround?

IMO the real problem behind this is the lack of documentation on extra_fields.

Besides hook_field_extra_fields() I'v only found the following references in the API documentation but no real documentation on the concept itself:

1. field_extract_bundle()
2. field_extra_fields_get_display()
3. _field_extra_fields_pre_render()
4. field_info_extra_fields()

The problem is engraved by the fact that "extra fields" seems to be a misnomer as they are sometimes called "non-field elements" or "pseudo-field elements".

I would suggest moving this documentation issue to the correct place over in the Field Module issue queue:
#1860914: "extra fields" concept should be better documented

Nice module. Good comments and very concise.

I think this is good to go.

Manual Review
One small thing but not totally bad and shouldn't cause any issues. A couple of times you do a check if the return of a function is empty:
extrafield_views_integration.module - Line 33
extrafield_views_integration.views.inc - Line 22
Those are both not needed since those will always return an array() so running foreach on them will not cause any errors. Like I said, nothing major, just doing extra work is all.

Hi das_ricardo,

Looks fine for me now too - the nested loop error seems to be fixed by splitting off the loops .

SteffenR

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/extrafield_views_integration.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   14 | WARNING | Hook implementations should not duplicate @return documentation
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

• "extrafield_views_integration" is quite a long name. why not simply "extrafield_views"?

But otherwise looks good to me, so ...

Thanks for your contribution, das_ricardo!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.