[D7] IdRelay Integration

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi there, nice module.

I have a few comments:

1) You should work on a release branch ( 7.x-1.x ) and not the master
2) You should add a hook_uninstall() in a MODULE.install file and clean
up any variables defined by your module.
3) Consider using Drupals builtin valid_email_address($mail) to avoid repeating errors others have already fixed. your custom function. Eg. your implementation don't allow plus-signs ( as in x+y@domain.tld )

Tom

Don't forget to add the "PAReview: review bonus" tag as indicated in http://drupal.org/node/1975228 once you did your reviews, otherwise you won't show up on my high priority list.

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/idrelay_integration_lists.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   111 | ERROR | Invalid @param data type, expected object but found stdClass
   169 | ERROR | Invalid @return data type, expected object but found stdClass
  --------------------------------------------------------------------------------
  

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. don't throw away your function comments just because they are longer than 80 characters. You should just make sure that the first line is a short description/summary what the function does. Further text can be added as paragraph in the comment.
2. "idrelay_integration_lists" why the long module name and not just "idrelay"?
3. idrelay_integration_lists_block_info(): doc block is wrong, see https://drupal.org/node/1354#hookimpl
4. idrelay_integration_lists_form(): doc block is wrong, this is not a hook. See https://drupal.org/node/1354#forms . Same for idrelay_integration_lists_form_validate(), please check all your function doc blocks. Almost all of them are wrong.
5. _idrelay_integration_lists_getHttpResponseCode(): function short description is useless because it just repeats the function signature. Please replace it with a meaningful sentence. Also elsewhere.
6. idrelay_integration_lists_form_submit(): why do you need this extra request in the submit handler? You are already subscribing people in the validation handler, right? Please add a comment.

Although you should definitely fix those issues they are not critical application blockers, so I guess this is RTBC. Review bonus tag is already removed, you can add it again if you have done another 3 reviews of other projects.

Assigning to kscheirer as he might have time to take a final look at this.

Project page

Please take a moment to make your project page follow tips for a great project page.

License

Please remove the licensing line from the module file. Drupal will add the appropriate version automatically during packaging so your repository should not include it.

• We don't use camelCase, please rename _idrelay_integration_lists_getHttpResponseCode. See https://drupal.org/coding-standards#naming
• Instead of _idrelay_integration_lists_xmlentities(), isn't the built-in PHP function htmlspecialchars_decode() the same?
• Regarding question #6 above from Klausi - the XML data being passed in the second request is slightly different, and attributes are set. While that is the case, there's a lot of duplication. For example, you read the username/pass combo in several places, just to pass them as arguments to _idrelay_integration_lists_getHttpResponseCode. Instead, just have _idrelay_integration_lists_getHttpResponseCode() read those variables, then it's only in 1 place. You'd never want to use a different user/pass combo, so why use function arguments?
• You could also use XML-handling functions like format_xml_string() to handle your data instead of doing it manually.

Setting to needs work for the project page, licensing, and camelCase.

----
Top Shelf Modules - Crafted, Curated, Contributed.

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/idrelay_integration_lists.module
  --------------------------------------------------------------------------------
  FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   119 | ERROR | Invalid @param data type, expected object but found stdClass
   170 | ERROR | Invalid @return data type, expected object but found stdClass
  --------------------------------------------------------------------------------
  

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:

• what about the module name as I mentioned earlier? Still quite long?

But otherwise the most important stuff mentioned by kscheirer seems to be fixed and this was RTBC already, so ...

Thanks for your contribution, kentoro!

I updated your account so you can 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 stay 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.