[D7] GetResponse Integration

welcome

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:

• README.txt is missing, see the guidelines for in-project documentation.
• The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
  

  ./getresponse/getresponse.module
  

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

  ./getresponse/getresponse.css:      ASCII C program text, with CRLF line terminators
  getresponse/getresponse.feed.inc
  getresponse/getresponse.css
  getresponse/getresponse.module
  

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See http://ventral.org/pareview/httpgitdrupalorgsandboxgetresponse1408798git.

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.

Your code got some coding standart issues (See http://ventral.org/pareview/httpgitdrupalorgsandboxgetresponse1408798git, you can use this site to re-test your self)

please don't paste the whole report in here, it's enough linking it or attaching it as txt file.

My mistake...

Hi,

Manual review :

• Your module uses curl without checking its available.
  Not everyone has curl installed. Please use hook_requirements to ensure it doesn't get installed without it. Alternatively, look into using drupal_http_request().
• You make a version number in your info file. See this page

Automatic review :

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

FILE: ...all/modules/pareview_temp/test_candidate/getresponse/getresponse.module
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  22 | ERROR | An operator statement must be followed by a single space
  22 | ERROR | There must be a single space before an operator statement
 121 | ERROR | Space found before square bracket; expected "][" but found "] ["
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Needs Work

btw: to use hook_requirements for curl, have a look into the install file of simpletest module

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended here: http://drupal.org/node/1011698 .

manual review:

1. the module and info files should be located in the root of the repository, remove the additional subfolder level.
2. getresponse.module: why do you need to check if the constants have been defined yet?
3. getresponse_help(): all user facing text must run through t() for translation.

Function attribute definitions are missing:
http://drupal.org/node/1354#functions

getresponse_comment_insert()
Those lines seams not to be used:
$name = urlencode($comment->name); /* nazwa */
$mail = urlencode($comment->mail); /* mail */

if (!getresponse_is_login() AND $comment_on AND !empty($webform_id)) {
Durpal modules use "&&" not "AND"

function getresponse_is_login() {
if ($GLOBALS['user']->uid == 0) {
The Drupal way would be
global $user
if ($user->uid == 0) {

$found = preg_match('#<' . $element_name . '(?:\s+[^>]+)?>(.*?)' . '#s', $xml, $matches);
To be on the secure side use:
preg_quote($element_name)

Constants shoukd be prefixed with module name
define('ELEMENT_CONTENT_ONLY', TRUE);
define('ELEMENT_PRESERVE_TAGS', FALSE);

protected function makeSafe($string) {
Why you not use check_plain() or
http://ch2.php.net/manual/en/function.htmlspecialchars.php

function curlRss() {
http://ch2.php.net/manual/en/function.file-get-contents.php
For better compatibility to shared hoster?

class GetResponseRSS {
Prefix all your methodes with public / protected / private

You should place in module description (http://drupal.org/sandbox/GetResponse/1408798) and README.txt
a link about, what is getResponse.com

Then you can go on and further descripte what thats module provides.

I have had to start testing this module and read code to find out what this module is good for.

You schould write a step by step starter tutorial.
register at getResponse.com
create webForm
configure webform id in drupal admin settings
place drupal block

For block, a plain html fallback for people with disabled js would also be an realy nice feature.

Your are a little inconsequent with your quotes. Sometimes single and sometimes double quotes.
t('

Subscribe via Web Form

'),
t("(leave empty to disable)"),

variable_get('getresponse_styleid', 1);
Use constantes insted directly 0 and 1

define('GETRESPONSE_STYLE_DRUPAL', 0);
define('GETRESPONSE_STYLE_WEBFORM', 1);

getresponse_is_login()
should be renamed into
getresponse_is_logged_in()
to be more self explaining.

Why you do first GET then POST

      getresponse_add_contact($comment->name, $comment->mail, $webform_id, 'GET');
      getresponse_add_contact($comment->name, $comment->mail, $webform_id, 'POST');

Simply POST should to the job?!

Function attribute definitions are still missing:
http://drupal.org/node/1354#functions

example: (or see patch)

/**
 * Send visitor name and email to GetResponse Webform.
 * 
 * @param string $name
 *   real name of the visitor
 * 
 * @param string $email
 *   e-mail address of the visitor
 * 
 * @param string $webform_id
 *   numeric webForm id from getresponse.com
 * 
 * @param string $method
 *   POST or GET
 */
function getresponse_add_contact($name, $email, $webform_id, $method) {
......

  $form['right']['f2'] = array(
    '#type' => 'fieldset',
    '#title' => t('GetResponse RSS'),

Better change f2 to something more saying like "rss"

  if (!empty($_POST) && form_get_errors()) {
    drupal_set_message(t('The settings have not been saved because of the errors.'), 'error');
  }

Dont do that in form, it will never called. You have the getresponse_admin_validate() for error messages.
Dont access $_POST directly. Use form api.

Actualy you show the comment "Sign up to our newsletter!" checkbox all times.
But if admin has choosen "Anonymous posters may not enter their contact information" for the content type,
than there is no e-mail field for the visitor in comment form.
See patch for fix.
Addtional there could be a form validator which is checking if form e-mail was not given but newsletter checkbox set.

Hello,

I've done quick look to you module.

.module file

hook_menu()

'access arguments' => array('administer getresponse'),
You do not define hook_permission() and administer getresponse permission.
It allows only super user (uid = 1) to configure your module (because Drupal will not check permission). But no one else will have the rights to use it.

getresponse_is_logged_in()

Drupal has it's own function for checking it: user_is_logged_in()

getresponse_admin()

As far as I know there is not such thing as hook_admin(). You should probably add another description in function comment.

You should use l() function for creating links instead of pure html.

getresponse_block_view()

While having your block not cached there is a possibility to add scripts by drupal_add_js().
Please read http://api.drupal.org/api/drupal/modules%21block%21block.api.php/functio...

Your $style_id can have only two values. You can use else instead of elseif.

getresponse_form_comment_form_alter()

'#title' => variable_get('getresponse_comment_label', 'Sign up to our newsletter!'),

You should use t() while outputing something.

The hook is called hook_form_FORM_ID_alter() (or hook_form_FORM_BASE_ID_alter() you should check which one) no hook_form_comment_form_alter()

getresponse_comment_insert()

getresponse.feed.inc

You should probably use theme function instead of writing html code directly. theme_item_list would be a good choice in my opinion.

Overall

Please read about keeping consitency with quotes.

Summary

I'm changing it to needs work beacause of permission issue although I'm still pretty new to module reviewing so if someone thinks otherwise please, feel free to change into needs review. Other issues are not RTBC blocker in my opinion :)

module file

1. Line 135: You cannot put html tags inside t(). You should concatenate tags and text separately.
Same on line 159. Check all t() functions for that same issue.

2. Line 281: As pointed on comment #17 you should use drupal_add_js on that same function instead. The path can contain variables and will load the javascript when the block is beeing viewed.

3. Line 315: You are initializing the variables webform_id et comment_on with values '' (empty string) and '0'. Then checking if true and if !empty. Those two tests will be always true.
PhP documentation for empty() and how PHP converts types during IF conditions.

install file

4. You should also delete the variables that are declared on line 296 of the .module file. Check also for other created variables on the module file.
Maybe you can store an array on that variable so you know its name and can delete it on uninstall.

Example to store array inside a variable:

variable_set("myvariable", array_values($form_state['values']['names_fieldset']));

feed.inc file

5. Files with classes should be referenced on the .info file.

6. There are strings without translation on lines 65 and 69.

needs work

getresponse.css

Some of the CSS rules are too generic and are likely to conflict with other themes and modules. There are likely to be other forms with IDs like #edit-actions. The use of !important in these rules makes them particularly evil.

Use class names / containers to make these more specific to your module's markup.

The CSS rules are surely not application blockers. Anything else?

PAReview is still OK (no errors).

Two really minor considerations (the rest seems ok):
1. Maybe the description in the info file could be shorter? Something like Provides integration with GetResponse email marketing service maybe. Then again, the documentation says links are allowed and max length is 255 so maybe is just me, but it also says that one line descriptions are preferred.

2. I'm not sure about the RSS feed and links to your facebook / twitter / etc. accounts on the settings page. Again, I am not saying it's bad, I just don't know if there is apolicy about that, because it might be considered advertisement.

GetResponse,

Signing all your comments with "GetResponse Team" makes it look like you're using a group account. That's currently disallowed on drupal.org. There's a discussion at http://drupal.org/node/1863498 about allowing organization accounts, but that would still require individuals to be doing all commits. Please confirm a single person is doing commits under your GetResponse account, or create separate accounts for others if that's not already the case.

In getresponse_admin_validate() you're using is_numeric() - be aware that this will return true for all numeric values, including floats. You probably want ctype_int() or something else if you want to ensure positive integer values.

Since you're altering the comment form as well, you might want to let your users know on the project page. Or perhaps that's already clear because they are using GetResponse - I don't know much about that service.

Those are pretty minor issues though, marking RTBC from me.

Hey,

we fixed this issue, now we accept only integer value. Please confirm did we have removed all the issues?

Regards,
GetResponse Team

Nice work, team!

I gave the GetResponse account (Peter Gzela) project maintainer access, and he can grant other developers all powers in this and other projects he creates. The maintenance done in their own names will count favorably toward individual applications should you choose to make them.

@mlncn Please use the approval template from https://groups.drupal.org/node/184389#promote

Thanks for your contribution, GetResponse!

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.