[D6] APN (Apple Push Notifications)

Welcome!

Please remove your .gitignore from git and add itself to gitignore.

package = Apple Push Notifications Is it really necessary to put your module into it's own category?

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxlawik1507982git

We do really need more hands in the application queue and highly recommend to get a review bonus so we will(/can) come back to your application sooner.

regards

Hi,
Manual Review :
1. Format should be "* Implements hook_foo()." or "Implements hook_foo_BAR_ID_bar() for xyz_bar()."
2. You should PARREVIEW your script. You have many errors.
See http://ventral.org/pareview/httpgitdrupalorgsandboxlawik1507982git
3. apn.module => contains Indent errors and also Inline comments must end in full-stops, exclamation marks, or question marks.
4. Data type of return value is missing at line 170 in apn.module.
5. 6 | ERROR | Additional blank line found at the end of doc comment
9 | ERROR | Class name must use UpperCamel naming without underscores
11 | ERROR | Expected 6 space(s) before asterisk; 9 found
12 | ERROR | Expected 6 space(s) before asterisk; 9 found
13 | ERROR | Expected 6 space(s) before asterisk; 9 found
13 | ERROR | Missing parameter type at position 1
13 | ERROR | Parameter comment must be on the next line at position 1
14 | ERROR | Expected 6 space(s) before asterisk; 9 found
15 | ERROR | Line indented incorrectly; expected 2 spaces, found 8
15 | ERROR | Variable "sMessage" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _
16 | ERROR | Variable "sMessage" is camel caps format. do not use mixed case
| | (camelCase), use lower case and _

There are so many errors in your module.so please PARREVIEW before submiting to review.

I'm wondering about the following
9 | ERROR | Class name must use UpperCamel naming without underscores

i've seen many active projects using underscores in class names.
why isn't this module allowed underscores?

regards,
curious adnasa asking.

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

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

  
  FILE: ...pace/drupal-7/sites/all/modules/pareview_temp/test_candidate/apn.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   308 | ERROR | The second argument to watchdog() is missing
   716 | ERROR | The second argument to watchdog() is missing
  --------------------------------------------------------------------------------
  

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:

1. "watchdog('apn', "Attempting to add message:<br />UID: $uid<br />Text: $text<br />Badge: $badge<br />Sound: $sound<br />Expiry: $expiry");": do not insert variables directly into translatable messages, use placeholders instead. And please use the "@" placeholder to automatically sanitize the input. Please check all your watchdog() calls.
2. I would not recommend to put the PHP library in your module folder, because then users will have to download it again every time they update the module. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
3. Your module files is quite big, have you considered to put your page callbacks into dedicated *.admin.inc or *.pages.inc files?
4. "module_load_include('inc', 'apn', 'apn.logger');": no need to use module_load_inlcude as you are including files from your own module which you already know where they are. Use something like require_once dirname(__FILE__) . '/mymodule.inc';
5. "or die("Could not get APN keys for user.")": do not use die(), you should always terminate gracefully. Log the error or throw exceptions or return status codes, but please do not stop the execution flow like that.
6. apn_get_keys(): why do you need the while loop?
7. "'SELECT * FROM {apn_user} WHERE `uid`=%d;'": there should be a space before and after the "=".
8. 'INSERT INTO {apn_keys} (`uid`,`key`, `enabled`, `app`) VALUES (%d,"%s",%d, "%s");': there should be always a single space after a coma.
9. apn_disable_key(): why do you need to check $key first? If it is an empty string there will not be any matching entries anyway?
10. apn_cron(): doc block: there should be an empty line between the first line and the additional description, see http://drupal.org/node/1354#hookimpl
11. apn_install(): no need to set variables upon installation as you can use default values in variable_get() anyway.
12. As you are assuming that the ApnsPHP is always available, e.g. in apn_cron(), you should implement hook_requierements() to check if the library exists.
13. apn_settings_form(): why do you unset $form['#submit']?
14. apn_settings_form_submit(): why do you need that function? system_settings_form() should take care of that for you.
15. apn_send_form(): why do you call system_settings_form() if you don't use it anyway?
16. apn.logger.inc: do not concatenate the translatable log string like that, pass the dynamic arguments to watchdog() as third argument.

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

1. It's not clear what apn_library_check() returns. The function name suggests that it will return either TRUE or FALSE, but it actually return the library path if it exists. Consider renaming the function.
2. You selects everything from the table in apn_status(), you could just do a count query and db_result(), which you do in other functions.
3. You use watchdog() extensively in several functions, but you don't return the actual status to the implementer. Consider failing early by throwing exceptions.
4. The watchdog function is called with invalid arguments in apn_register_key(), apn_unregister_key(), apn_unregister_key_unknown(), apn_user_disable() and apn_user_enable(). The severity variable is the fourth argument.
5. You have a minor coding standard issue in apn_send(). try should have a space after try.
6. You declare $push as a an instance of a specific class, but you do a check if it's actually an object in some functions. This is unnecessary since PHP will throw errors about that. This concerns apn_push_add_message(), apn_push_send() and apn_push_exit().

Also, you have a typo in apn_send():

    watchdog('apn', "Attempting to send message:<br />UID: @uid<br />Text: @text<br />Badge: @badge<br />Sound: @sound<br />Expiry: @expiry",
      array(
        '@uid' => $uid,
        '@text' => $text,
        '@badge' => $badge,
        '@sound' => $sound,
        '@expriy' => $expiry,
      )
    );

@expriy shoudl be @expiry.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

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

Otherwise looks RTBC.

no objections for more than a week, so ...

Thanks for your contribution, lawik!

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.