[D7] Fields in Regions

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxSilbermann2052197git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hello

my manual review:

.module 51:

/**
 * Get fields_in_regions config vars.
 */
function fields_in_regions_get_settings($theme, $node_type) {
  $query = db_select('variable', 'v');
  $query->condition('v.name', 'fields_in_regions__' . $theme . '__' . $node_type . '__%', 'LIKE');
  $query->fields('v', array('name'));
  $results = $query->execute();

  return $results;
}

To interact with variable table you have the drupal api functions variable_set and variable_get. You can save in a variable 'fields_in_regions__' . $theme . '__' . $node_type an array or an object with all the content.

It's recomendable use variable_get (https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/variab...) and variable_set (https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/variab...), variable_get cached the result...

.admin line 100s

To delete a varible you can use variable_del

It's possible that you have to review all about variable, but I think that is better if you can use only the api functions and not access directly to database.

Hello,

[general observation]
The git repository url should be http://git.drupal.org/sandbox/Silbermann/2052197.git (you currently have a colon in place of a slash).

[manual review]
1) You should create an .install file to implement hook_uninstall in order to remove the variables that your module saves.
Notably the 'fields_in_regions__' vars set in fields_in_regions.admin.inc:129

2) After installing the module and navigating to the configuration page I found that only one content type was appearing in the "Field settings per node type" section. Even though I had 4 content types set up on my site.

I then found the allowed types list in fields_in_regions.admin.inc:21

$allowed_types = array(
    'text_long',
    'image',
    'file',
  );

It seems to me that the module only looks out for content types that have fields that are either 'text_long', 'image', or 'file' types.

If this is the case then you should specify this in the module's description and README file. Otherwise users who naturally assume all their node fields are placeable will get unexpected results.

Link to git repository updated

assigning to myself for review

Hello,

I have installed the module, and made the following observations:

Basic Functionality

To start with, I love what this module does! After getting a few fields configured to display in my sidebar, I can safely say that this functionality is definitely useful enough to exist as a standalone module. That said, there is definitely some overlap with the functionality of Field as Block, a module which allows fields to be set up to display in their own blocks. The key advantage I see in this module as opposed to that one is that if one wants to add ten fields to the sidebar, the added burden of ten field blocks could be cumbersome.

Errors

I received the following error at /admin/structure/fields_in_regions after attempting to modify allowed field types:

Notice: Undefined index: field_settings in fields_in_regions_settings_form() (line 125 of C:\xampp\htdocs\demo\sites\default\modules\fields_in_regions\fields_in_regions.admin.inc).

It looks like you may need to work on the way that settings are initialized, or perhaps provide some default logic in fields_in_regions_settings_form(). Subsequent visits to the configuration page yield the same error.

UX

The most striking thing I noticed about this module's user experience is that all configuration has to be done from the module's settings page. If possible, it would be nice to be able to configure the region display from a content type's field display settings page (/admin/structure/types/manage/article/display).

Overall, I definitely love this module, and will consider using it for my next project.

I installed your module on a fresh install of Drupal 7.23 and created a content of type 'Article'. I then added a title, a body, and uploaded an image. When I clicked Save, I received the following warning:
Warning: Invalid argument supplied for foreach() in fields_in_regions_preprocess_page() (line 65 of /home/dschaeffer/www/sites/all/modules/fields_in_regions/fields_in_regions.module).
I then went to Structure and received these errors:

Warning: Invalid argument supplied for foreach() in fields_in_regions_preprocess_node() (line 133 of /home/dschaeffer/www/sites/all/modules/fields_in_regions/fields_in_regions.module).
Warning: Invalid argument supplied for foreach() in fields_in_regions_preprocess_field() (line 153 of /home/dschaeffer/www/sites/all/modules/fields_in_regions/fields_in_regions.module).
Warning: Invalid argument supplied for foreach() in fields_in_regions_preprocess_field() (line 153 of /home/dschaeffer/www/sites/all/modules/fields_in_regions/fields_in_regions.module).
Warning: Invalid argument supplied for foreach() in fields_in_regions_preprocess_field() (line 153 of /home/dschaeffer/www/sites/all/modules/fields_in_regions/fields_in_regions.module).

These warnings disappeared when I set the field settings for the image in type 'Article' to something valid.
They appeared again when I set the field settings for the image in type 'Article'
Before using foreach() you might check that the variable you are using foreach() is actually an array.

Improvements:
1. Warnings appear during normal use (see above).
2. add a version in your .info file.
3. line 199 in fields_in_regions.admin.inc has two semicolons. This isn't bad, but a minor flaw I found.
4. I noticed a couple of places where HTML is added to forms without using check_markup. I don't think your module requires the user to enter any html, but this may be helpful to keep in mind in the future.

See below for more details.

This review implements the checklist found here. https://drupal.org/node/1587704

1. Basic application checks
1.1 Application contains a link to the project page. However, the application does not contain a working git clone command. In the future you may want to include a working git clone command. git clone http://git.drupal.org/sandbox/Silbermann/2052197.git fields_in_regions.
1.2 I could not find any possible duplicates of this module.
1.3 User Silbermann does not have any other applications. I queried the project applications with Silbermann's name and did not find any applications that belong to the user. I did notice he had reviewed another.

2. Basic repository checks
2.1 The repository does contain code.
2.2 The repository contains a version specific branch (7.x-1.x).
2.3 The repository contains a minimum of handwritten code (at least 5 functions & 120 lines).

3. Security Review
3.1 I did not find any security flaws in the code. The only offer the suggestion of using check_markup before you add html to a form. This won't affect the result of this review, but it's something to keep in mind.

4. Licensing checks
4.1 Repository does not contain a 'LICENSE.txt' file.
4.2 Repository does not contain any 3rd party code.

5. Documentation checks
5.1 Project page contains detailed information regarding this module. I find that users know what to expect when they install this module.
5.2 Repository contains a README.txt file with helpful information regarding this module. The README.txt and the project page both contain similar information.
5.3 Code appears to be properly commented. In-line comments are placed where required, and there is not an overwelming amount of them.

6. Coding style and standards
6.1 I performed an automatic review at http://ventral.org/pareview. The review returned errors, but I believe these errors do not effect the results of this review. One error involves a space being where it shouldn't (line 68 of fields_in_regions.module). There are also some unused variables. http://pareview.sh/pareview/httpgitdrupalorgsandboxsilbermann2052197git

7. API and best practices review
7.1 Module makes use of Drupal's APIs.

https://drupal.org/project/fieldblock
https://drupal.org/project/panels
There might be other modules out there with the same functionality.

Duplication

This sounds like a feature that should live in the existing fieldblock project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the fieldblock issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

What I would really like to see from Fieldblock is a way to include more than one field in a block. Perhaps the community would be best served by a partial re-write of Fieldblock's functionality, in which users have the option to add a field to an existing fieldblock (in other words, de-couple the blocks and fields in the backend, and have the system auto-create blocks to store new fields when they are assigned to new regions as-needed).

Do you think that the maintainers of Fieldblock would accept such a radical change in functionality? If so, I will get on that.

Sounds good, I'm closing this application in the meantime. Feel free to re-open if the collaboration with fieldblock fails.

Feel free to get back to us whenever you have a new project that you want to promote. And thanks for contributing to the Drupal community!

Corrected module name of Field as Block.