Trello

There are lots of coding standard error found. Please check my review here http://ventral.org/pareview/httpgitdrupalorgsandboxdaven1795720git-7x-1x

Also set the correct default branch and remove the master branch.

Hi there,

I see that you set a few variables, but you should also delete them when the user uninstalls your module.
Create a trello.install file with hook_uninstall() and delete your variables with variable_del().

I didn't go much deeper into your code, since I'm no expert. But deleting these variables is something that can block module approval. Trust me. ;)

And there are still some minor coding standard errors:

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/./test_candidate/trello.inc:
+25: [normal] The $string argument to t() should not begin or end with a space.
+30: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
+31: [minor] in most cases, replace the string function with the drupal_ equivalent string functions

Status Messages:
Coder found 2 projects, 2 files, 1 normal warnings, 2 minor warnings, 0 warnings were flagged to be ignored

FILE: ...l-7-pareview/sites/all/modules/pareview_temp/test_candidate/trello.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
5 | ERROR | It's only necessary to declare files[] if they declare a class or
| | interface.
--------------------------------------------------------------------------------

You can manually check your code for coding standard errors at http://ventral.org/pareview or use the Coder module.

cheers!

Here is in the code you define forced the handler for submut buttons on all forms, for example:

$form['#submit'][] = 'trello_api_explorer_form_submit';

however the same handler is executed by default for forms if you define function with name name_form_submit()

Best regards,
Max.

Some small stuff:

• The automated code review at http://ventral.org/ is finding a few errors.
• trello.module line 18 - You should change the title to something a little more descriptive than 'Administer my module'. Maybe 'Administer Trello module'.
• trello.module line 63 - The comments for trello_get_conf() do not document the parameter or return value.
• trello.module line 124 - The comments for trello_auth_page() do not document the return value.
• trello.inc line 82 - The call to watchdog seems more like a debugging statement that's been left in the code rather than any kind of useful information for the user.
• trello.inc line 107 - The variable result should have a comment block documenting it.

Also, when I bring up the Trello Example Configuration page, I get the following error messages:

I went to trello.com, opened an account, created a board, and added some lists and a bunch of cards. However, I wasn't really able to find my information using the Trello API Explorer page you created. I was probably doing something wrong in entering the parameters, but I spent all the time I could studying the Trello documentation. It might have been easier if your example application worked.

Bob Humphrey

Just did a automated review using Code Sniffer and found some minor issues:

FILE: /mnt/www/html/pareviewsh/pareview_temp/trello.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 64 | ERROR | Empty CASE statements are not allowed
--------------------------------------------------------------------------------

FILE: /mnt/www/html/pareviewsh/pareview_temp/trello_api_explorer.module
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  68 | ERROR | An operator statement must be followed by a single space
  68 | ERROR | There must be a single space before an operator statement
 220 | ERROR | There must be a single space before an operator statement
--------------------------------------------------------------------------------

Did a manual review as well and code looks good just some minor spelling mistakes:

http://drupalcode.org/sandbox/daven/1795720.git/blob/refs/heads/7.x-1.x:...
http://drupalcode.org/sandbox/daven/1795720.git/blob/refs/heads/7.x-1.x:...

I did have a question about having two .info files in the same directory though perhaps move trello_api_explorer (module + info) into a subfolder? I could totally be wrong about this though.

Same Directory:

a) trello.info
b) trello_api_explorer.info

As an aside I thought it was pretty cool that you are supporting both functional and OO version of the trello api. Also bonus points for providing an example module on how to leverage the functionality. Will definitely be taking a further look at this!

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

1. Set error_reporting in your php configuration to E_ALL | E_STRICT and test module again
Problems apper on at least the following page:
- admin/config/trello/example

Notice: Trying to get property of non-object in trello_example_config_form() (line 87 of /var/www/_REVIEWS/trello/modules/trello_example/trello_example.module).

2. You do not need own submission handler for trello_config_form() because you use core system_settings_form($form) function that saves variables for you. Also you should not clear $form argument in trello_config_form():
-14: $form = array();

3. You do not need global variables
Instead, use the common practise with a staticly cached variables:

function trello_get_api_explorer_toc($table_of_contents = NULL, $reset = FALSE) {
  static $api_explorer_toc = &drupal_static(__FUNCTION__);
  if ($reset) {
    $api_explorer_toc = NULL;
  }
  if (isset($table_of_contents)) {
    $api_explorer_toc = $table_of_contents;
  }
  return $api_explorer_toc;
}

4. Use constants instead of hard-coded strings such as $base_url = 'https://trello.com/docs/api/';

5. Use t() function properly
trello_api_explorer.module

-105: $fieldset['#description'] = t('API reference: ') . l($url, $url);
+105: $fieldset['#description'] = t('API reference: <a href="@url">@url</a>.', array('@url' => $url));

A couple of questions:

1. Why not use drupal_http_request() function?

2. You really need $include_curl_handle param in trello_api() function?

#13 has blocker issues, so I'm setting this back to needs work.

5. Use t() function properly
trello_api_explorer.module

-105: $fieldset['#description'] = t('API reference: ') . l($url, $url);
+105: $fieldset['#description'] = t('API reference: <a href="@url">@url</a>.', array('@url' => $url));

This is not a correct use of t() function, it will returns the error: "The submitted string contains disallowed HTML" if you try to translate. It should be something like this:
$fieldset['#description'] = t('API reference: !url', array('!url' => l(t($url), $url)));

However, for links enclosed in translatable text you should use t() and embed the HTML anchor tag directly in the translated string. For example:

t('Visit the <a href="@url">settings</a> page', array('@url' => url('admin')));

This keeps the context of the link title ('settings' in the example) for translators.

http://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

So it seems that <a> tag is allowed. But in this way also have translation available.

<?php
t('Visit the !url page', array('!url' => l(t('settings'), 'admin')));
?>

The encouragement in http://api.drupal.org/api/drupal/includes%21common.inc/function/l/7 to include link tags in translation strings surprises a lot of people. But that is, indeed, what the documentation says you should do, so the advice in #13 should be followed. The formats in #15 and #18 are a common practice, but #13 follows best practice.

• In trello.admin.inc, I don't think it makes sense to translate the domain name in t('at trello.com').
• Why isn't trello_api_explorer in the modules directory?
• In trello.module, the const TRELLO_API_VERSION syntax is only supported as of PHP 5.3. This raises your PHP version requirement above Drupal's 5.2.5. You should either make that requirement explicit in .info file (see http://drupal.org/node/171205#php), or change to the define() syntax, removing the 5.3 requirement.

I was hoping to mark this as reviewed and tested. Unfortunately that last one is a blocker, as it can cause fatal errors on older PHP versions. If you can fix that, I'll try to finish this up quickly.

Some things that should probably be changed in the module:

• trello_api_explorer.module: line 106, don't use @url for the URL itself, rather use !url and pass '!url' => check_url(...)
• line 109, matches[1] is not set if the match failed: you should probably check its result
• line 111, you are adding unsanitized data from an external string to a #title, so they should be sanitized
• note that you can also probably make this more php-like by using the index as "foreach ($matches[1] as $k => $v)" and use $k instead of using an outer variable incremented numerically
• released code should not contain any dpm() or dsm() calls if it does not check for devel module.
• there are a bunch of coding standard violations: you should probably use coder_review and/or codesniffer.
• since this is a web service, the config path should probably be admin/config/services/trello rather than admin/config/trello

You're right about the devel dependency: I had only checked it on trello.info, not trello_api_explorer.info.

However, here are the coder_review results I get on on trello_api_explorer:

Line 156: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.
    '#description' => t('API reference: ') . l($url, $url),

Line 156: The $string argument to t() should not begin or end with a space. (Drupal Docs)
    '#description' => t('API reference: ') . l($url, $url),

Line 211: in most cases, replace the string function with the drupal_ equivalent string functions
    sprintf('$t->%s(\'%s\'%s);', strtolower($method), $path, $query ? ", '$query'" : ''),

Though it's worth fixing, nothing in #25 seems like a blocker to me. It looks like all previous feedback is addressed now, so I'm marking this reviewed and tested.

These appear when you enable the "minor" review level and the off-by-default checks in coder_review (not in coder_sniffer).

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

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

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

  
  FILE: /home/klausi/pareview_temp/trello.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 4 WARNING(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
   104 | WARNING | Code after RETURN statement cannot be executed
   105 | WARNING | Code after RETURN statement cannot be executed
   107 | WARNING | Code after RETURN statement cannot be executed
   108 | WARNING | Code after RETURN statement cannot be executed
  --------------------------------------------------------------------------------
  
  Time: 0 seconds, Memory: 12.00Mb
  

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. You have to get a review bonus to get a review from me.

manual review:

1. The Trello class file should be listed in the info file with the files[] directive.
2. trello_api_explorer_form_submit(): contains dpm() debug code.
3. trello_example_block_view(): you could use theme('item_list') for your list markup.

But that are not blockers, so ...

Thanks for your contribution, daven!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

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.

Thanks to the dedicated reviewer(s) as well.