Campaign Monitor Integrated (CMI)

You need to create a sandbox project, and provide here the link to the project.

Sandbox was created, you can find it at: http://drupal.org/sandbox/Prosite/1093244

That is quite some code... Looked through it, here are some things I found. There are some possible security issues you need to take care of and you should also follow the Coding Style closer, see below for more details.

To fix this, I suggest you make separate commits with git, that will make it easier for me to review the changes :)

- I suggest you add a simple README file to your master branch to inform that the code is in another branch because master is currently checked out by default (will be possible to change that soon I hope) and this might be confusing to someone checking out the module

- Have a look at http://drupal.org/project/libraries to see how you can integrate libraries in a better way. The problem with placing them in your module directory is that with the correct procedure to update a module (which is, remove old directory and then add new one, drush does this always), you will have to get them again

- // $Id$ is not necessary anymore, git doesn't use that

- Please follow the Drupal coding style, run http://drupal.org/project/coder over your module to see the problems. For example, there are many unecessary spaces, for example "if ( ! _cm_integrated_check_wrapper( ) ) {" should be like "if (!_cm_integrated_check_wrapper()) {".

- Intend by two spaces, not four. Coder will check that too

- require_once 'campaignmonitor_createsend/csrest_clients.php'; That doesn't work, either use drupal_get_path('module', 'your_module') to get the path to your module or use libraries..

- db_query("INSERT INTO {cm_integrated_statistics} SET cid='%s'", $params['cid'] );
I think that is MySQL specific SQL, you should rewrite that using VALUES

- db_query( "INSERT INTO {cm_integrated_campaign_lists} VALUES ". implode( ", ", $lists) );
This is MySQL specific too, to be compatible with PostgreSQL, you will have to execute multiple insert queries. Note that the new database abstraction layer in Drupal 7 provides an API to support multi-row inserts on databases that support it with a fallback to multiple queries for the others. Same for segments

- IN (". implode(',', array_keys($campaigns)) .")". This is not secure. Use http://api.drupal.org/api/drupal/includes--database.inc/function/db_plac... instead. There are others too.

- The UPDATE query in cm_integrated_db_campaign_send seems to be missing a WHERE condition for the $nid? Or should it really update all? Then what is $nid for? :)

- cm_integrated_db_get_campaign( NULL, "c.nid = {$nid}" That looks insecure too. At the very minimum, you should make sure $nid is cast to an integer. And better, rewrite that to pass in an array of key => value pairs and then use placeholders in your query builder function

- The delete and insert queries at the end of cm_integrated_db_update_statistics() need to be updated according to the above too

- You should use variable_del() in hook_uninstall

- I suggest you re-format your hook_install similar to http://api.drupal.org/api/drupal/modules--node--node.install/function/no... for example. That is much easier to read IMHO.

- Same for FAPI arrays, see http://api.drupal.org/api/drupal/modules--node--node.pages.inc/function/... for an example

- cm_integrated_filter_build_where will need to be checked for security/updated where API too

- $status_info = "". $all_status[$campaign['campaign_status']] .'';
I suggest you rewrite that by adding the class directly on the td element, you can do that by not using a string in $rows but an array where 'data' contains the string and then specify class. See http://api.drupal.org/api/drupal/includes--theme.inc/function/theme_table/7

- Make sure the values displayed in cm_integrated_manage() are protected against CSS. Use check_plain() for all values that have been entered by users at some point.

- $output .= "

";
I suggest you either use drupal_set_message() or display that information inside your table. Drupal 7 has direct support for that using the empty param, in D6, you have do it yourself, see http://api.drupal.org/api/drupal/modules--node--content_types.inc/functi... for an example.

- Drupal modules usually place their page callbacks either in module.admin.inc or in module.pages.inc, not custom things like settings or manage.

- 'access arguments' => array('administer site configuration'),
I suggest you define your own permission instead...

- You have some "interesting" links in there that you might want to remove like 'http://testsandbox.iblogger.org/sites/default/files/cm/cm1.html';

- You should probably use theme('table') in theme_cm_integrated_node, as described above, you can define custom classes too with that.

- Use http://api.drupal.org/api/drupal/modules--system--system.module/function... in your settings form, that will automatically save all submitted form values with variable_set(), just make sure that the form element name matches with the variable name.

- drupal_set_message( implode( '
', $messages) ); Maybe loop over messages and display them separately?

You have a number of unnecessary/wrong tags and branches now, compare http://drupalcode.org/sandbox/Prosite/1093244.git with the branch/tag naming guidelines at http://drupal.org/node/1015226

Note that it doesn't make sense to create release tags for your project yet, you should only do that once you have a full project and can create release nodes (more details can be found on the linked page above). Additionally, I strongly suggest to start with an alpha or beta release instead of a stable one. Once you have published a stable release, you will have to work together with the security team and release a security update for your module in case someone finds a security issue. Among other things...

So you should remove all your unecessary tags and branches (local and on the server) and instead continue to develop on the 6.x-1.x branch.

Right now, it's impossible for me to revew the changes you did, because you commited them as new files in a new branch, so I can't see what you actually did without going through all files again.

If you need any help with Git, I suggest you join the IRC channel #drupal-gitsupport, you usually get very quick help in there. See http://drupal.org/irc. I'll be there in a few minutes.

Changing priority according to the new priority guidelines.

There are already a bunch of Campaign Monitor related modules out there:
http://drupal.org/project/campaign_monitor
http://drupal.org/project/campaignmonitor
http://drupal.org/project/emf

Although on first glance they do not have the feature to create Campaign Monitor newsletters from nodes, it would still be a good idea to submit that functionality as a patch to one of those, to avoid endless duplication. It's not in the end user's interest to have 4 different Campaign Monitor modules, all doing slightly different things.

Hi Sven,

About the modules you mentioned:
The first two are only supporting subscribing to the CM newsletter, the EMF module is not only for CM, but offers a framework for other mail clients to.
Next features in the CMI module will be adding and choosing templates for the mail campaign, integrated reports of campaigns, ...
Because this CMI module has lot's of code, i think it's better to have this as a separate module. People can always use one of the other Campaign monitor modules to handle there subscriptions, but this can also be done on theme level.

Grtz,
Donny

Talked it through in detail on IRC

Issues

• Space indent (4 instead of 2) - This is a result of a new eclipse install, settings have since been set and in future updates this is no longer an issue
• long elseif statement chains - are on the issue list to be changed to switch blocks
• A few spots of html through the module - This is also on the issue list
• Some commented out code - Will be removed from future versions
• Incomplete uninstall

Due to a recent API change at campaign monitor this version no longer works. An updated version will be released after development is done.

New version can be found on the 6.x-1.x branch and can be reviewed.
There was a huge code clean up and some extra new functionalities have been added to the module.

See http://ventral.org/pareview/httpgitdrupalorgsandboxprosite1093244git

Switched back to needs review, so in-depth reviews won't be blocked by coding standart issues.

Currently working on the coding standard issues, update will follow soon

All Code sniffer issues are fixed now in the 6.x-1.x branch

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• 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:

• "'access arguments' => array('administer site configuration'),": this permission is too generic, create your own.
• cm_integrated_nodeapi(): line 260: this line is way too long and hard to read, please break it up a little.
• "0 => 'N/A',": this should be run through t(), too.
• _cm_integrated_is_campaign_exits(): why do you query for count(*) if you are not interested in the count?
• cm_integrated_db_create_campaign(): you should use drupal_write_record() instead of SQL insert queries.
• cm_integrated_db_get_campaign(): long query on one line, try to break it up to make it more readable. See http://drupal.org/node/2497 and indentation. Same in cm_integrated_db_update_campaign_statistics().
• _cm_integrated_db_build_where(): no need to use check_plain() here, the DB API will sanitize it for you.
• _cm_integrated_send_node(): again very long code lines, please break the arrays up a little.

Sorry for the long delay!

I had a raw look:

1. Again some issues found by pareview ;)
2. there are a lot of 'id's in your schema using varchar type, are they really chars or actually integers?
3. require_once "{$cm_integrated_path}/cm_integrated.api.inc";this is kind of inconvenient, either require_once './cm_integrated.api.inc'; or require_once dirname(__FILE__) . '/cm_integrated.api.inc'; should do the same
4. please split long array lists through multiple lines for better readability
5. specific permission names as 'manage newsletters' should at least contain your module name
6. don't put so much logic into a single hook_form_alter, whenever/wherever possible use hook_form_form_id_alter()
7. _cm_integrated_check_wrapper - why not using libraries API ?
8. maybe i've overlooked it but I think your submodule does not delete it's variables on uninstallation
9. does the main module take care of the submodule's variables? If yes, that should not be, otherwise your clashing your submodule's namespace by using _content suffix.
10. there's lots of php logic in some of your theme functions, you should try to put most of it into a preprocess function
11. make sure node title's are filtered (check_plained) when thei'r printed to the table

As my d6 knowledge is pretty limited and this is a huge bunch of code, I'll stop here.

I think we should do some basic security tests and get this approved (I've not seen all code but I've seen worse as full project ;D) because this is just too much to make a serious review on (I could spend a whole day).

regards.

Hi patrickd,
Thanks for the review.

Please see the points-wise comments

#1 Fixed http://ventral.org/pareview/httpgitdrupalorgsandboxprosite1093244git :)

#2 In the db schema, we have used some of the 'ids' as an integer but in case others, we can not use it eg: 'cid', it is provided by Campaign monitor in string format.

#3 Fixed.

#4 FIxed.

#5 Fixed.

#6 Actually in case of node add/edit form, form name is specific to the node type hence we are going with the hook_form_alter.

#7 We are already using library api, '_cm_integrated_check_wrapper' is just a wrapper to it, to check and display the status message (inside it is using library api).

#8 Yes, I have fixed it.

#9 No! now individual module deleting their own variables.

#10 We have implemented hook_theme, basically it finds the available templates and return the array for the theme function.

#11 Fixed.

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

You can now delete the master branch:

git branch -D master
git push origin :master

I had a final glance at your code and as it is quite much to review it is hard to make sure that I missed nothing but as far I can see this is pretty much ready.
RTBC, leaving the real fun to someone else ;-)

Thanks for your contribution, Prosite!

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 reviewers as well.