ECK Extras:eck_extras

Hi Kolier,

Welcome to the review process.

I have had a quick skim over of the module's code and will do an install and full review at a later time.

In the meantime i have submitted an automated review of the module: available here.

In terms of manual review.

I would encourage you to use Drupal 7's Database abstraction layer near your call to db_query_range() on line 82 of eck_extras.module

I would also suggest the call to die() on line 37 of published.inc be replaced with drupal_exit() or removed entirely.

More review to come. Cheers, Charlie.

Hi,

From a manual review:

• The info file contains a version 7.x-1.0 but this should not be there as the drupal.org packager will add it automatically
• There is a master branch in your git repository which should be removed
• The information in the README.txt file should be expanded to give a better description of the module and its features. See the Module documentation guidelines for more info
• On line 59 in eck_extras.module when setting the value of $description_additional you include a string which is not wrapped in the t() function

There are also still numerous issues reported through automated review: http://ventral.org/pareview/httpgitdrupalorgsandboxkolier1700728git

Hi,

Functionnalities of your module look interesting and, IMHO, for some part need to be directly in the eck module itself.

So, you maybe should create a features request on issue queue of eck itself and send a patch.
If maintener of ECK thinks that features should not be in his module, then your module have a lot of sense.

EDIT: Should have read all of the links in the above comments before posting. Forget about my post!

(http://drupal.org/node/1705084#comment-6413758) !

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Fix error http://ventral.org/pareview/httpgitdrupalorgsandboxkolier1700728git

Sorry, wrong issue queue #1898950: Undefined index: config

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

To help future reviewers, I edited the original post and added a link to the repository page and added the git command to clone the project.

From the pareview
31 | ERROR | Empty installation hooks are not necessary
You don't need the empty hook_uninstall.

In eck_extras.install, you have two hooks that have the same code, you should extract this to its own function and call that function from each hook. This way you don't have to maintain the same code in 2 spots. Small example of this being that the db_update in eck_extras_update_7200 is formatted differently than the other. Since functions in the .module file are available to the .install file, the new function should be moved into the .module file so it can be accessed by hook_modules_enabled(see below).

This new function should also check to see if Display Suite's ds_forms module is installed/enabled. See module_exists

eck_extras should be more robust in the enabling, disabling of eck_extras and ds_forms in different orders. example: eck_extras installed, disabled, ds_forms installed then eck_extras enabled. I think using hook_enable instead of hook_install would catch this, but I'm just skimming the hooks and leaving the deep thinking to you. :)

Also eck_extras should watch the modules being enabled, to see if ds_forms is enabled after eck_extras is installed. Look into hook_modules_enabled
there's also hook_modules_installed.

Great module! I was looking for something like this a few months back.

Line(s) 80-84 of eck_extras.module: I'm not sure this will work with ECK and the implementation that you're using but there is a "machine_name" field type that will format the field and do the validation(as seen when creating a new view).

$form['name'] = array(
    '#type' => 'machine_name',
    '#maxlength' => 128,
    '#machine_name' => array(
      'exists' => 'foobar_machine_validation_callback',
      'source' => array('human_name'), // Human name field name
    ),
    '#description' => t('A unique machine-readable name for this Project. It must only contain lowercase letters, numbers, and underscores.'),
    '#weight' => -99,
  );

Line(s) 226-235 of eck_extras.module: You're using a db_select to check if a given alias is already in use. You should be using drupal_lookup_path() or drupal_valid_path() to check for this.

Also, be forewarned that this module COULD in theory be rolled into the ECK module. I'd recommend submitting an issue/feature request asking whether or not they will add this to ECK.

Switching to needs work

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

Added links to help future reviewers