ALCAR

Please read
Workflow
How to apply
What to expect
Checklist for applicants

Hi,

I tried to clone the code with:

git clone http://git.drupal.org/sandbox/henk/1549782.git alcar

but it failed with:

Cloning into 'alcar'...
fatal: http://drupalcode.org/sandbox/henk/1549782.git/info/refs not valid: is this a git repository?

Please ensure that there is a working git repository for us to be able to review this.

Thanks
Matt

Ah, I realised that I was actually doing:

git clone http://drupalcode.org/sandbox/henk/1549782.git alcar

Note that the above is using drupalcode.org and NOT drupal.org - this was because in the information in this issue you give a Git location of drupalcode.org - please correct this and provide the full git clone command, e.g:

git clone http://git.drupal.org/sandbox/henk/1549782.git alcar

Thanks,
Matt

An automated review of your module throws up a large number of issues, both with the layout of your files and with the style of your code. Take a look at the output here: http://ventral.org/pareview/httpgitdrupalorgsandboxhenk1549782git

I would suggest fixing as many of those issues as possible.

From a manual review:

The README.txt seems unfinished as it has statments like "more info soon" and some blank sections which should either have content added or be removed.

You seem to be working from a master branch - you should move your code to a version branch. See: http://drupal.org/node/1127732

Your info file contains a version line - this should be removed as it will be added by the Drupal packager when required.

alcar.module:

In hook_menu() you should NOT be putting strings within t() - these should be untranslated strings. See: http://api.drupal.org/api/drupal/developer!hooks!core.php/function/hook_...

In several places there appears to be commented out lines of code, in some cases whole functions - some of which looks to be debugging information. These should be removed. E.g. line 485: //print_r($result->data);

In alcar_get_list_args() you are outputting a lot of HTML - perhaps this would be better in a theme function or template file so that themers can adjust the output?

alcar.install:

You have some functions with no code in them - they should be removed if there is no need for them.

In your hook_uninstall() you should remove any variables which the module sets, e.g. those which are set in the admin pages.

filter_wheels.js:

All the code in this file appears to be commented out so I don't see the point in having it there?

alcar.admin.inc:

You are using a textfield for the admin to select HTTP or HTTPS - wouldn't this be better as either a drop down selector or radio buttons?

You are using a textfield for the admin to enter a password - wouldn't this be better to use a password field so that the text is hidden?

Generally, I think this module still needs a fair bit of work to make it ready for review - therefore setting to 'needs work'.

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 :-)

Still haven't addressed a number of issues raised above, specifically in #1 and #6.

Your repository contains the following branches: 6.x-1.x-dev, 6.x-1-0-beta1, 6.x-2.x-dev, master
NONE of these are valid branch names, with the exception of master, and as noted in #6 you shouldn't be using master anyway. Please read the checklist linked in #1 for a guide to naming branches. Likewise, I don't see any reason to have multiple branches at all because the code in 1.x is the same as in 1-0-beta1 and 2.x.

You have context mentioned twice in the dependencies in your .info file, and you have Views as a dependency which doesn't seem to be needed as you haven't implemented any Views hooks. You also shouldn't have shadowbox hardwired as a dependency - what if I want to use lightbox or colorbox instead? Likewise for fivestar - this should NOT be required. Site developers should have the option to include or not to include fivestar, this is not something you should force on them.

The visual appearance should be easily customizable, not hardwired. As mentioned in #6, you're coding a lot of the HTML generation explicitly when you should be using theme functions. Some of your HTML is just wrong too - for example:

$img_link = '<div class="img-list">' . l( $img, 'node/' . $nid, array('attributes' => array('target'=>'_blank'), 'fragment' => 'refresh', 'html' => TRUE)) . '</a></div>';

and

  $et = '<a><div class="taxonomy">' . $wheel['Offset'] . '</div></a>';

You also shouldn't be reusing the "taxonomy" class for your own purpose.

"unique" is spelled wrong all throughout, and while spelling is not a requirement for approval, this may cause problems for developers trying to use your functions.

I'm not sure what the purpose of alcar.features.inc is - it's not doing anything.

Permission names by convention should be all lower case. I'm not sure if this is officially spelled out anywhere.

You still haven't implemented hook_uninstall() as requested in #6.

It looks like your blocks will be empty of content if the user doesn't have the correct permissions. A better way to do this is to set up your blocks to appear only if the user has the correct permissions, or perhaps you could display an informative message like "log in to search for wheels for your vehicle" in the block instead of just leaving it blank.

This function is wrong:

/**
 * Function to get nid from sku.
 */
function alcar_nid_from_sku($sku) {

  $nid = db_result(db_query("SELECT nid FROM {uc_products} WHERE (model = '%s')", $sku));

  return $nid;
}

The reason is that there is not a 1-1 correspondence between nid and sku, so this query can and will in general return multiple results so db_result() should not be used here. You're also not guaranteed that there will be only one nid with that sku, so if this is important to you you're going to have to add code here to enforce that condition and return an error if it's not true.

'alcar_sheme' ? Do you mean 'alcar_scheme' ?

You're relying on https, which isn't guaranteed on a Drupal site. You should implement hook_requirements() to check to see if https is available as a condition for installing your module.
You do still have a few coding standards issues. http://ventral.org/pareview/httpgitdrupalorgsandboxhenk1549782git

I didn't even get around to the functional tests yet - there's still too much that needs to be fixed at this point.

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

If you reopen this please keep in mind that 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 :-)