SlideDeck

I looked through the module

- In the info file you can drop the line - ; $Id$
That was necessary for CVS, but is no longer needed for Git.

I ran the module through Coder and that comes out clean.

I installed the module and found the following:

- The message after installing shows html code and some arbitrary link text
<a href="/drupal7/admin/structure/slidedeck">Structure &gt; Simple Tweet</a>

- After saving a new slidedeck I get the error message

Deprecated function: Function eregi() is deprecated in slidedeck_validate() (line 243 of modules\slidedeck\slidedeck.module)
(I am running PHP5.3.5 - eregi() is deprecated as of PHP 5.3.0)

- When viewing the slidedeck, it is there and seems to function fine but the following error messages show up

Strict warning: Only variables should be passed by reference in slidedeck_block_content() (line 104 of modules\slidedeck\slidedeck.module).
Strict warning: Creating default object from empty value in slidedeck_block_content() (line 115 of modules\slidedeck\slidedeck.module).
Strict warning: Creating default object from empty value in slidedeck_block_content() (line 115 of modules\slidedeck\slidedeck.module).

Set this back to "needs review" once you have made the changes and I will test again.

You get a PHP Strict standards warning on line 117 of slidedeck.module - I have attached a patch file to solve that issue.

Other than that looks good to me, coder review still comes up clean.

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:

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• admin.inc in slidedeck.info: It's only necessary to declare files[] if they declare a class or interface.
• slidedeck.install in slidedeck.info: It's only necessary to declare files[] if they declare a class or interface.

function slidedeck_validate(&$form, &$form_state) {
--

function slidedeck_add_more_blocks_callback($form, &$form_state) {
--

function slidedeck_add_more_blocks($form, &$form_state) {
--

function slidedeck_add_more_remove_one($form, &$form_state) {

function slidedeck_list() {

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• slidedeck_widget: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
• "$result = db_query('SELECT id, title FROM {slidedeck} ORDER BY title');": use db_select() instead.

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

• ./slidedeck.module: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function slidedeck_validate(&$form, &$form_state) {
  

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

  function config_slidedeck_settings($form, &$form_state, $formtype, $slidedeck = NULL) {
  function config_slidedeck_settings_submit($form, $form_state) {
  function get_all_blocks() {
  function get_skins($skin_key = FALSE) {
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./slidedeck_widget/slidedeck.skin.css:                    ASCII C program text, with CRLF line terminators
  ./slidedeck_widget/skins/skin2.css:                       ASCII C program text, with CRLF line terminators
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• slidedeck_uninstall(): uninstalling the schema manually is not needed in Drupal 7, so remove that function.
• slidedeck_requirements(): you should not use t() in there, see the example http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...
• slidedeck_requirements(): do not embed dynamic data such as links directly in the translateable string, use place holders. See http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/t/7 . Same in slidedeck_install(). And in slidedeck_block_content().
• slidedeck_library(): indentation error for the nested array structure.
• "$library_path = 'sites/all/libraries/slidedeck.jquery.lite.js';": don't hard code that. Use it as a fallback if the function libraries_get_path() is not available.
• "'#default_value' => check_plain($data["block_" . $block_count]),": no need to check_plain() the default value, the form API already handles that for you. Also elsewhere.
• config_slidedeck_settings(): indentation error in the foreach().
• slidedeck_widget folder: still appears to be third party code, no?

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

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

  270-     from and form state values.
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./skins/skin2.css:                                        ASCII C program text, with CRLF line terminators
  ./skins/skin2.css~:                                       ASCII C program text, with CRLF line terminators
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• remove all backup files from your repository, e.g. "skin1.css~"
• slidedeck_block_content(): why do you use get_t() in there and not just t()?
• "$skins[$key] = check_plain($value['value']);": no need to do this here. The Form API will sanitize #default_value and #options (on select boxes) anyway. See http://drupal.org/node/28984
• "'access arguments' => array('administer site configuration'),": don't abuse this permission, create your own.

Mistakenly mentioned it as drupal 6 it is for drupal 7.

windows file permissions

The entire repository is executable files. See Changing file permission masks on Windows.

master branch

If you're no longer using the master branch, please remove all files from it and commit only a README file telling people how to change to one of the available branches. See Moving from a master branch to a version branch.

project page

There is code on the project page not wrapped in <code> tags which makes it a bit difficult. Have you considered including a .api.php file? See Module documentation guidelines

missing newline characters

All text files should end in a single newline (\n). This avoids the verbose "\ No newline at end of file" patch warning and makes patches easier to read since it's clearer what is being changed when lines are added to the end of a file. See Coding standards

.install

Spacing and formatting issues .. double spaces abound. hook_schema layout should be changed so it's spread over more lines like every other module. Also, useless filtering:

global $base_path;
..
filter_xss($base_path . 'admin/structure/slidedeck')

There's no chance of user input in this to have a need to filter_xss .. it should simply be calling the url() function.

.tpl.php

When multiple <?php ?> are happening, it's much easier to follow loop and conditions if you use alternate format;

<?php foreach($array as $element): ?>
  #do something 
<?php endforeach; ?>

.admin.inc

doc blocks are erroneously prefixed by spaces

hard coded paths

use the Libraries API instead of hard coding the path like this. Someone might not want to have this module in sites/all

$library_path = 'sites/all/libraries/slidedeck.jquery.lite.js';

considered breaking up code over multiple lines?

This block is near impossible to read

function slidedeck_block_delete_submit($form, &$form_state) {
  $delta = $form_state['values']['sd_name'] . "_" . $form_state['values']['id'];
  $slidedeck_delete = db_delete('slidedeck')->condition('id', $form_state['values']['id'])->execute();
  $and = db_and()->condition('module', 'slidedeck')->condition('delta', $delta);
  $slidedeck_block_delete = db_delete('block')->condition($and)->execute();
  drupal_set_message(t('The Slidedeck block %name has been removed.', array('%name' => $form_state['values']['sd_name'])));
  cache_clear_all();
  $form_state['redirect'] = 'admin/structure/slidedeck';
};

but can be broken up (and some of the lines compacted strung together) to give something like this, which break things up into logical units and makes it a tad easier to read. No point storing values you don't ever use either.

function slidedeck_block_delete_submit($form, &$form_state) {
  $delta = $form_state['values']['sd_name'] . '_' . $form_state['values']['id'];
  
  db_delete('slidedeck')
    ->condition('id', $form_state['values']['id'])
    ->execute();

  db_delete('block')
    ->condition(
      db_and()
        ->condition('module', 'slidedeck')
        ->condition('delta', $delta)
    )
    ->execute();

  drupal_set_message(t('The Slidedeck block %name has been removed.',
    array('%name' => $form_state['values']['sd_name']))
  );

  cache_clear_all();

  $form_state['redirect'] = 'admin/structure/slidedeck';
};

This is also much like the hook_schema stuff. It's not really needed, but it does make reading the code and maintaining the code in the long run a much simpler task. Do you remember how code you wrote a few years ago works when you look at it now?

slidedeck_load

does $id need checking before it's used in SQL? Why are you doing a foreach on an array and replacing the contents with the last item? If there's only one result, just use a fetchAll or fetchAssoc on the query.

slidedeck_block_content

drupal_add_js will not get called if block caching is turned on. Ditto drupal_add_css. Use #attach instead.

$block['content']['#attached']['css'][]

slidedeck_block_content and slidedeck_block_view functions look like they should be combined. There's no logic in block_view, just some statements and then it calls the other function.

You also theme the output of the block before returning it - simply return a render array with the #theme set so that other modules can manipulate the contents of the block before it is rendered.

hook_library

I am very puzzled by this. This is how you're meant to use libraries api to get the path.

$library_path = libraries_get_path('slidedeck.jquery.lite.js');

Just also spotted that you're forcing the "lite" version. You should also perhaps check for the non-lite version if the lite one is missing? Is that even practical?

always use brackets

The following code is incredibly hard to read, due to missing brackets and missing indentation. As per the Coding standards, always use brackets even when it's just a single line.

$id = '';
if (array_key_exists('id', $values))
$id = $values['id'];

condition always false

In the following code, $theme is never declared so it will always be false.

function slidedeck_get_all_blocks() {
  module_load_include('inc', 'block', 'block.admin');
  static $blocksarray;
  if (empty($blocksarray)) {
    global $theme_key;
    drupal_theme_initialize();
    if (!isset($theme)) {
      // If theme is not specifically set, rehash for the current theme.
      $theme = $theme_key;
    }

hard coded lib path .. again?

same path, different code! 3rd different way of getting the path.

function slidedeck_library_path() {
  $library_path = 'sites/all/libraries';
  return $library_path;
}

changed the branch command

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

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

• slidedeck_install(): $base_path is never used.
• slidedeck_schema(): use new lines for all the array items, like it is done for all other drupal schema definitions.
• slidedeck_list(): contains still the hard coded library path.

Looks great and pretty much ready for RTBC. There is one blocker though which is the file permissions of the files in the git repository. Not all the files are +x any more, but most of them still are.

The non-blockers are just coding suggestions. The descriptions on the schema is part of the Coding Standards though.

blockers

windows file permissions

The entire repository is executable files. See Changing file permission masks on Windows.

non-blockers

slidedeck_block_delete_submit

The use of a db_add() here is unecessary as AND is the default. You could get away with:

db_delete('block')
  ->condition('module', 'slidedeck')
  ->condition('delta', $delta)
  ->execute();

hook_schema

Each table, and each field should have a non-translated description of each field.

Again, looks good.

Looks good enough to eat off.

Your git commit messages and git identity need a bit of help in the future - Please refer to Commit messages - providing history and credit about giving yourself some credit and properly formatting commit messages.

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

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

• slidedeck.module: @file doc block should use the full length of 80 characters of one line.
• slidedeck_load(): "@returns" should be "@return", also in other places. "@param integer": For primitive and non-specific types, use the lower-case type name; int, string, bool, array, object, etc.
• "@params" should be "@param".

But that are just minor issues, so ...

Thanks for your contribution, gopagoninarsing! Welcome to the community of project contributors on drupal.org.

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.