[D7] Field context

Tested on clean environment and got the same results I expected from provided Use case. Also code looks good.

Feature export/import works just fine.

I havn't tested predefined contexts, but otherwise module looks really good.

Checked working:

• Module project page contains information, repository contains code.
• README contains installation and usage instructions, easy to follow.
• Pareview.sh and coder reports no issues, just one minor warning: field_context/fieldcontext.rules.inc:114 should define the $wrapper on a line above the conditional for better readability.
  

  $wrapper = $element->applyDataSelector($element->settings['entity:select']);
  if (wrapper) {}
  

• Good inline-comments and doxycode documentation.

Needs work:

• The module should be either renamed to Fieldcontext in one word, or all files and functions should be renamed to field_context_. I'd prefer the 2nd approach.
• Does this module even make sense for users? As I understand the module you can assign a context to fields across nodes of different types and therefore always identify different fields within the same context. But are there any use cases where you have different user types with each custom fields? Adding the same context to more than one user field does not work, of course.
  

  The context Universum has already been applied to the following fields: field_moons.

Sorry, there was place for misunderstanding, when I wrote:

Does this module even make sense for users?

I'm not talking about site builders, administrators or developers, I'm talking about users in terms of entities. You know, for node types this module makes sense in my eyes, but for user entities I can't imagine a use case.

That's what I ment. Sorry for the confusion. I'm not questioning this module in general! I like the idea and approach.

I'm fine with (1.) if you rename your module to Fieldcontext. This would be consequent.

Automated Review

PAReview came up clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes. Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes. About 600 lines. Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

My notes:

• Initial brownie points for including automated tests, but points are deducted because running tests results in the following error:
  

  An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows.
  Path: /gruppe01/batch?id=459&op=do StatusText: OK ResponseText: 
  Fatal error: Call to undefined function fieldcontext_get_field_name() in /hom/inf5270/www_docs/diw/gruppe01/sites/all/modules/fieldcontext/fieldcontext.test on line 48
  

• (*) The docblocks describing the functions does not have the tags @param and @return according to the API documentation and comment standards. In particular, the API functions offered by the module need to have this documentation to be usable by others. This omission also makes manual review difficult, as the reviewer must in some cases guess what the intent of function is.
• There is no hook_help(). It is good coding practice to have this hook for every enabled module.

The starred item (*) is a fairly big issue and warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

Manual review

(Doesn't repeat items given "Yes"-status from last review.)

Coding style & Drupal API usage

Items from last review that are fixed (or moot):

• Automatic testing works as expected (cause of AJAX HTTP Error in last review was a configuration error on my part, sorry about that).
• Docblocks, including API docblocks, provide adequate documentation.
• A hook_help() is now available.

Items from last review that are not fixed:

• None

New items:

• There is a sub-module named Field context Test. It is placed in its own package ("Testing"), while the main module is placed in the default package (i.e. "Other"). IMHO, it is common practice to have modules and sub-modules belonging to the same project in the same package.
• There is no hook_help() for the sub-module Field context Test, nor is there any mention of it in README.txt. It is pretty obvious what it does, but it should at least be a brief mention of its existence in README.txt.
• The submodule Field context Test is placed inside a sub-sub-directory (tests/fieldcontext_test/. It is tiny, only 3 files with a total of 278 lines (including comments). I do not see the point of burying it that deep below the main project directory. Why don't it live in the project directory, or (if a more "tests" will be added later) in the sub-directory tests/)?
• (*) Enabling the Field context Test triggered the following error, which was not handled:
  

  FieldException: Attempt to create field name <em class="placeholder">field_painting_price</em> which already exists and is active. in field_create_field() (line 85 of .../modules/field/field.crud.inc).
  

  This in turn creates a situation where the sub-module is installed, but the database is not necessarily in the state it expects it to be.
  The reason I got this particular error is of course because I'd already worked through the example use case from README.txt (i.e. I'd already created field_painting_price "by hand"). However, the sub-module should handle the situation where the field it tries to a create a field that already exists more gracefully (e.g. check for existing fields before creating anything, and abort the installation if a field it wants to create already exists, giving the user a meaningful message about how to recover).

The starred item (*) is a fairly big issue and warrant going back to Needs Work. The rest of the comments in the code walkthrough are recommendations.

Manual review

(Doesn't repeat items given "Yes"-status from previous reviews.)

Coding style & Drupal API usage

Items from last review that are fixed:

• Main module (Field context) and sub-module (Field context Test) both in package "Field context".
• Sub-module Field context Test added to documentation. If I understand it correctly, the sole purpose of this is to provide test cases for the automated testing? If this assumption is correct, maybe the following should be added to README.txt: "You should never need to enable this yourself."
• Sub-module Field context Test moved up to sub-directory tests/.
• Field name collision handled more robustly.

Items from last review that are not fixed:

• None

New items:

There is no need to pass form_state by reference here:

function fieldcontext_form_field_ui_field_edit_form_validate($form, &$form_state) {

This is not a blocker, tho'. Moving to RTBC. Note that promotion will not happen until a git administrator has given this a second set of eyes.

Updated git clone to public URL.

Automated Review

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

• Nothing.

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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

Yes

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

Place code review here using this format:

Tests threw some notices about Array to string conversion on line 45. All passed on a clear D7.28 install.

Test coverage looks OK.

In fieldcontext_help(), build the URL with l() and pass that as a parameter to t(). Ignore this. I forgot about this usage.

Can you leverage cache_get() / cache_set() anywhere?

I wonder is fieldcontext is the best namespace, as Context is fairly ubiquitous. Think pretty hard about this, as
changing it will be impossible once you promote it. A variant of field alias seems logical, but there is already
a module with this name (that looks like it does something totally different).

Agree with @maijs, padding $form_state by reference is the normal thing in validate and submit handlers.

I am not seeing any blocking issues. This looks pretty well written w/ a good use of comments and includes tests. It also looks like @gisle has given this
a thorough look.

Thanks for your contribution, maijs!

I updated your account so you can 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 stay 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.

Not sure (and see my amended comments above, I had a brainfart when I did the review). I kinda like "metafields".