Basic cart

Hi,

You should remove the LICENSE.txt file. It will be added by the packaging system. I see a few coding standard issues. You might want to check out the coder module to help you with that.

Just by quickly reading through your code:

basic_cart.admin.inc:

  $checked_types = variable_get('basic_cart_content_types');
  $form['content_type']['types'] = array(
    '#title' => t('Content types'),
    '#type' => 'checkboxes',
    '#options' => $options,
    '#default_value' => !empty($checked_types) ? $checked_types : array(),
  );

could simply be

  $form['content_type']['types'] = array(
    '#title' => t('Content types'),
    '#type' => 'checkboxes',
    '#options' => $options,
    '#default_value' => variable_get('basic_cart_content_types', array()),
  );

Actually, the entire form could use system_settings_form() (not required though - but much easier).

I also get a PHP Notice:

Notice: Undefined index: und in include() (line 11 of sites\all\modules\basic_cart\basic_cart_cart_render.tpl.php).

In the code, line 11:

<span class="basic-cart-cart-node-summary"><? php print drupal_substr($node->body['und'][0]['value'], 0, 50); ? > ... </span>

UI / UX suggestion: line 230 in basic_cart.cart.inc:

drupal_set_message(t('There was a problem in submitting your order. Please try again later.'));

is an error. Use the error class:

drupal_set_message(t('There was a problem in submitting your order. Please try again later.'), 'error');

It would be interesting to have a quantity field, don't you think ? It's a bit weird to have to add a product 10 times to your cart if you want 10 items.

OK, Coder agrees with you ;-). Good work.

Carefull, in your basic_cart_cart_render.tpl.php (line 26), your path for the "delete" icon does not take into account the base path. If Drupal is not installed in the site "root" but in a sub-folder, the icon will not show up. Fix:

With a call to base_path():

print l('<img src="' . base_path() . drupal_get_path('module', 'basic_cart') . '/images/delete.gif" border="0" />', 'cart/remove/' . $nid, array('html' => TRUE));

Or, even "nicer", allowing themes to more easily extend your templates:

In .module, hook_theme():

   'basic_cart_cart_render' => array(
      'template' => 'basic_cart_cart_render',
      'variables' => array('cart' => NULL, 'is_checkout' => NULL, 'form' => NULL, 'base_path' => base_path()),
    ),

In .tpl.php:

print l('<img src="' . $base_path . drupal_get_path('module', 'basic_cart') . '/images/delete.gif" border="0" />', 'cart/remove/' . $nid, array('html' => TRUE));

I also see that you use variable_get() a lot, but you don't provide a default value. Usually that's not an issue, but I wonder about the emails you send in basic_cart.cart.inc: If the subject variable is not set, for example, it will be NULL. Depending on the mail configuration of the server (Microsoft IIS anyone ??), could this trigger an error ? Just to be safe, I would set a default value (just as with the 'basic_cart_content_types' variable I showed you before - documentation on variable_get()). Sorry, I see that you set default values in your .install file. My bad.

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

• Are you really, really, really sure that you want to maintain yet another shopping cart solution for Drupal?
• Are you sure that you do not want to re-use the Drupal Commerce Framework and build something simple from it?
• Project page is too short. Please add in detail how your project differs from Ubercart and Commerce.
• "You just placed an order on the website ' . variable_get('site_name') . '. Here are the order details:": do not concatenate variables in translatable strings like this, use placeholders instead.
• basic_cart_node_view(): doc block is wrong.
• "'#options' => $options,": you need to sanitize the node type names before using them in the "checkboxes" form element to prevent XSS attacks.

README.txt

• 7.x-1.0 dev release - not sure what that means. Dev releases are automatically created from the 7.x-1.x branch. 7.x-1.0 as a tag would mean a stable release.

images/delete.gif

• Is that GPL? (Probably it is, just asking to be sure.)

basic_cart.module

• l. 133 - no trailing dot
• basic_cart_add_to_cart_button() - consider using render arrays or at least not having linebreaks between ' ... and ... '
• l. 158 - consider using drupal_get_path()
• Probably the Thank you link shouldn't be displaying in the navigation.
  

basic_cart.cart.inc

• Some missing trailing dots in doxygen.

basic_cart_cart_render.tpl.php

• l. 27 - consider using truncate_utf8(), that has also support for the ellipsis you want.

basic_cart.info

• Add configure = admin/config/basic_cart/settings.

All this is super minor and should probably not block this project from being promoted. Finally a security flaw:

• XSS! - basic_cart_node_description is not properly sanitized or filtered. Try adding <script>alert('xss!');</script> to a node body.

Fixing status.

Get a review bonus and we will come back to your application sooner.

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

• "$options[$node_type] = check_plain($type->name);": you can use "array_map('check_plain', $node_types);" instead of looping over the array yourself.
• basic_cart_admin_content_type(): you can use system_settings_form(), then you don't need your own submit callback.
• basic_cart_checkout_thank_you(): you need to sanitize the variables before ouputting them to avoid XSS vulnerabilities. See http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-... . Please check all your code where you print variables and apply to appropriate filters.
• basic_cart_checkout_form_submit(): you should use drupal_mail() so that other modules can alter the mails with hook_mail_alter().
• "l('<img src="' . $base_path ...": you should use theme('image', ...) here.

I'm removing the review bonus tag, feel free to do at least 3 more reviews of other projects for adding it again.

You forgot to fix the CSS file. Also, please do not remove security tag, we may use it for statistics or to show examples of security issues.

I think ventral.org is not up to date, please see my attachment in comment #14. CSS coding standards: http://drupal.org/node/302199

manual review:

• basic_cart_checkout_form_submit() and basic_cart_mail(): now you are double escaping the content which is bad. You should only do it in one place.

Otherwise I think this is RTBC. Removing review bonus tag.

There are few niggly bits, but I have included a question which I need answered before proceeding.

The menu items not having any permissions is a rather important one too.

[bug] .info; extra full stop

On the configure line, there's an extra full stop which means that the "settings" part is ignored.
configure = admin/config/basic_cart/settings.

[needs attention] menu items have no permissions at all

The menu items should have granular permissions for admin to be able to configure who has access to do things. At present, anyone, including users who are blocked, can access cart, card/add, checkout, etc.

[suggestion] basic_cart_block_view

module_load_include should be used here, especially since you're missing the DRUPAL_ROOT.
require_once drupal_get_path('module', 'basic_cart') . '/basic_cart.cart.inc';

[question] basic_cart_cart_form

What is the driving force behind using such a wrapper instead of just building up a full form and serving that from the menu? At present it will cause major headaches for anyone wishing to make alterations. It also includes logic in the theme function instead of just display. Unable to change any of the form attributes without also delving into the theme function. This would really be much better off in a form instead of mixed like this. There is also a lot logic and calculation that should not be done in a theme function. Is there reasoning behind this method that specifically negates the benefits of following the usual forms api mothod? I can see that you are doubling up the use of the basic_cart_cart_render for both cart view and checkout, but this can be done with form api and then be able to follow the normal render and alter paths.

[suggestion] setting variables during install

Variable default values should not be set during install - instead the default value should be included as part of the variable_get(name of variable, default value if missing) function call.

[suggestion] non-parameterised cart/add card/remove

If you make these card/add/%node and add 'page arguments' => array(3), the node will be auto-loaded for you and passed as a parameter. I can see how it might be an advantage to not load the entire node again for something that is already in the $_SESSION.

The layout in the SS can be done with the Forms API. It's identical to a render array so things like tables, or nested divs can be done. I would highly recommend it as it shows proper use of the Drupal API.

As part of a normal form (completely untested)

$form['cartcontents'] = array(
  // Make the returned array come back in tree form so we can name sub-parts with the same key
  '#tree' => TRUE,
);

foreach ($cart_item as $item) {
  $form['cartcontents'][] = array(
    '#prefix' => '<div class="cart-row">',
    '#suffic' => '</div>',

    'nid' => array(
      '#type' => 'value',
      '#value' => $item->nid,
    ),

    'qty' => array(
      '#prefix' => '<div class="cart-qty">',
      '#suffix' => '</div>',
      '#type' => 'textfield',
      '#title' => t('Qty'),
      '#size' => 4,
      '#default_value' => $item->basic_cart_qty,
    ),

    // and so on
  );
}

You could keep the cart part of the array in its own function that gets pulled into the other two forms (cart view and checkout). Let me know if you have any questions about that.

re: "access content"; a local permission would allow site admins to set a sub-user who can do this. The check for permission should also go on the code that adds those links to the nodes.

re: module load include; embarrassingly enough, apparently using this function is not recommended when loading files from the same module. I had to go ask around after reading up on it as it seemed to be a bit undefined. Sorry bout that. But, it should include the DRUPAL_ROOT, or something that negates the need for it. Because it's conditionally included, "include_once" should be used. "require_once" is for unconditionally including files. Require drags the file in at compile time, include drags it in when physically encountered in the code. So, either
include_once dirname(__FILE__) . '/mymodule.foobar.inc';
or
include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'mymodule') . '/mymodule.inc';
The former is probably faster. If you were loading another modules files, it would be done with module_load_include. Sorry about the confusion.

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

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

manual review:

• basic_cart_cart_form(): all the HTML should probably go into a theme function.

Otherwise still RTBC for me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

It looks like the HTML in the cart form might be my fault so I should probably go through it and propose something. The rather long and complex prefix/suffix should be offloaded to a theme function as they should only really be used for wrapping things in a single tag. The table like layout of the cart should probably be a '#type' = 'table'. Anyway, I need to play wiht this a bit to come up with a better suggestion as it seems I've just confused things.

Added 3 project reviews.

Couldn't you use #theme or #theme_wrappers on the form element? http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht...

So let's get a little more picky ;)

1. Please take a moment to make your README.txt follow the guidelines for in-project documentation.
2. Maybe you could use the existing images (at misc/ui/images) instead of your delete.git?

3.  * @file
    * Basic cart module file

   all comments should end with . / ? / !

4. It would be awesome if you'd use the core token system instead of implementing your own %placeholders% - that would be a great flexibility enhancement!
5. There's a logical problem: if I disable the Send an email to the customer after an order is placed checkbox only one mail will be send, but you're expecting always two mails to be send:
   

   if ($mails_sent == 2) {
       basic_cart_empty_cart();
       drupal_goto('checkout/thank-you');
     }
     else {
       drupal_set_message(t('There was a problem in submitting your order. Please try again later.'), 'error');
     }

6. It's not the usual practice to setup variables at installation. Usually there should be defined() a constant holding the default value, with this short constant name it's not that bad to provide a default value at variable_get('name', BASIC_CART_DEFAULT TEXT);

7. function basic_cart_block_view($delta = '') {
     // Check if the current user has access.
     if (user_access('use basic cart')) {
       switch ($delta) {
         case 'shopping_cart':

   You should check whether you're handling with the right block before you check the permission (otherwise the permission will be check for each block that is viewed -> not good for performance)

8. include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'basic_cart') . '/basic_cart.cart.inc';
   

   could be replaced with (I think)module_load_include('inc', 'basic_cart', 'basic_cart.cart.inc')

9. The default caching option of your block would be per-role I think doing this per-user (DRUPAL_CACHE_PER_USER) would make more sense
10. 'cart/add/' . $nid . '' what's that empty string good for?

11.   $node_types = node_type_get_types();
      if (empty($node_types)) {
        return NULL;
      }

    when I got no content types I get a blank page - you should provide some help here

hmm, I think these are minor, so..

welcome to the community of project contributors on drupal.org and many thanks for your contribution!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

I want to access the module variable basic cart (total price and quantity) and display them in another place how I can get an idea? Thank you.