[D7] Commerce Bulk Product Create Entity Reference

As the commerce_bpc maintainer, I thought should do a (partial) review of this.

This is a useful module---entity reference fields can be a clever way to do configurable "product bundles". Another use case would be to "sell" any kind of entity (think nodes with "premium content", etc.).

Here are a couple of things I noticed:
- I recommend to name the product after the module "bpc_entityreference" instead of "commerce_bulk_product_create_entity_reference".
- You are using the "master" branch. From looking at other reviews in the queue, it seems that it is recommended to use a 7.x-1.x branch already in the sandbox---you'll need to do this for dev releases anyways, so you might switch right now, as well.

- The module does pretty well wrt to coding standards, but there are some minor issues that the coder review module can help you detect:
-- Three functions don't have a space between the closing ) of the argument list and the opening {
-- A couple of lines have extra white space.
-- One of the doc headers misses the parens after "Implements hook_foo"
(Make sure to set coder to "minor" to see all of those reported.)

- As per the recommendation in the bpc API doc, you keep track of the "combination" fields managed by your module in $form_state, and you do check whether there is a value in $form_state['commerce_bpc']['bpc_entityreference']['combination_fields']---but then you iterate over all fields in $form_state['values']['combination'] anyways. Since there is a check with entityreference_field_is_empty($columns, $field_name), this UNLIKELY to create problems, unless (i) some module has the same columns as entityreference and (ii) this module has its own combination-creating module. Granted, this is unlikely at present, but it is something that you should guard against. Basically, I recommend that instead of doing:

$fields = array_keys($form_state['values']['combinations']);
    foreach ($fields as $field_name) {

simply do something like

  foreach($form_state['commerce_bpc']['bpc_entityreference']['combination_fields'] as $field_name)

That way, you'll only loop over the fields that are managed by your module (saves a couple of iterations, too!).

- Your module has a nice feature that the current bpc "core" implementations do not have: Offer a "none" option, that, if selected, creates a variant that does not have a value for the field (along with those that do). That is a very nice idea, and one that commerce_bpc will adopt---thanks for the inspiration! Yet, there are two issues I have with the current implementation:
(i) The "none" option is appended to the end of #options---I think it would be better if it showed at the beginning---perhaps to an array_unshift, instead of just appending the value?
(ii) I think the "None" option should be there only of the field is not required---for if it is, no products without a value for the field should be created. Currently, the "None"-option is there regardless of the "required" setting.

(A note to fellow reviewers: The module includes a comment // force static is not actually implemented yet. This is appropriate, as any such implementation would be futile, given the core bug #765860: drupal_alter() fails to order modules correctly in some cases---basically, bpc_entityreference would be trying to add a setting to a fieldset that is only created later. This could be fixed by implementing hook_module_implements_alter(), if it were not for the core bug referenced.

A few notes:

(i) The correct name for the -dev branch would be "7.x-1.x" (see http://drupal.org/node/1015226 )
You can rename branches using git branch -m oldname newname, but I don't know how well this works with remote branches.
(ii) You are checking for !$instance['settings']['required']), but the "required" setting is not actually stored in the 'settings' sub-array,
so this should really be !$instance['required'], or better yet !empty($instance['required'] to guard against a missing setting.
(iii) I am not sure what you intended to do with the options and "None", but the additional option still shows up at the end. Also, you added the line

$options = array_merge(array('none' => t('None')), $options);

but $options is defined nowhere---presumably, you wanted to operate on $element[$lang]['#options'], instead.
(iv) With respect to white space, I recommend figuring out a way to configure your editor to strip out trailing whitespace on saving. Do it once and you'll never have to worry about this again ;)
Almost all editors can do this, there are some hints about configuring your editor for drupal development in the subpages (at the bottom) of this page:
http://drupal.org/node/147789

Also, you renamed the project to "bpc_entity_reference", while your module is called "bpc_entityreference".

Technically "bpc_entity_reference" is adhering to the standards better, but at the same time, given that the module you are integrating is (non-standardly) called "entityreference", a case could be made that you should use the same name. Your call. If you decide to change the name of the module to bpc_entity_reference, you'd have to rename the hook implementations, of course.

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/bpc_entityreference.module:
   +107: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 1 files, 1 minor warnings, 0 warnings were flagged to be ignored
  

• README.txt is missing, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• ./bpc_entityreference.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

        // When creating sample values the 'value' key is present, but when creating
  

• ./bpc_entityreference.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  

  function _bpc_entity_reference_fix_none(&$options, $instance) {
  

• ./bpc_entityreference.module: The description on the line after the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
  

  69- *  List of form options
  72- *  Field config instance
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./bpc_entityreference.info ./bpc_entityreference.module
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:

• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.
• I will limit my review to the formal part as Sven is obviously more familiar with the project's subject.

Okay, all the standards issues are fixed.

Functionality / code wise, I think this is ALMOST good to go, except:

You are using that attribute_field setting introduced/managed by the commerce_cart module (basically as a workaround for the non-working 'treat as static' setting) ... so you should depend on commerce_cart.

I am aware that this is a "do as I say, not as I do" moment (as the bpc_taxonomy module included with commerce_bpc does NOT currently depend on commerce_cart, though it should---and will, in the next (rc)-release), but still.

Once you add a dependencies[] = commerce_cart to the .info file, I think this is good to go.

This is green, then.

Git vetted user role granted. Welcome, and thanks for your current and future contributions!

Thanks to all for the careful reviews! Thanks @sven.lauer, @doitDave

Don't forget to review other applications, as this queue can get burdensome without lots of people helping out.