EMF GraphicMail

welcome,

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.

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow tips for a great project page. Also create a README.txt that follows the guidelines for in-project documentation.

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/httpgitdrupalorgsandboxmja1452484git

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

regards

corrected git/project url

Hi,

I didn't install the module because I don't meet the requirements so I just looked through the code - looks good!

Couldn't see much wrong:

Probably a good idea to have a README.txt in the major branch saying "See major version branches" as per http://drupal.org/node/1127732

emf_graphicmail.module

1. Would prefer to see a specific permission, hook_perm() (or inherited from EMF module?).
2. Line 17: t() in emf_graphicmail_menu() for title & desc
3. Line 68: I'm assuming that there is a reason for not using drupal_load_include instead of @require_once http://api.drupal.org/api/drupal/includes%21module.inc/function/module_l...
4. Line 71: $options - would it be wise to have check_plain() or something run on them? Can we be sure they are ok?
5. Line 125: Prefer standard if/else structure as more readble: http://drupal.org/coding-standards#controlstruct

Good luck.

Why recommending installing the module in the plug-ins directory of emf project ? Your module isn't (technically) incorporated in EMF so I would recommend installing it in the usual sites/*/modules directory.

There is no need for the plug-in module to reside inside the EMF source tree and doing so can be a pain to the site further maintenance (updates, ...).

BTW, once promoted, if your module is installed with drush, it will reside in sites/*/modules, not int the plug-ins subdirectory of EMF.

Thanks for your comment, however I have to say I disagree. EMF is a specifically 'modular' module - i.e. it requires these plugins to operate. The fact that it ships with a Campaign Monitor and MailChimp plugin means that it works out of the box, but it would do nothing without the plugins.

Equally, my module will do nothing - not even install - without EMF.

I did not said EMF wasn't required. I said that it should not be recommended to end user to deploy it under emf module folder but under its own folder.

Your module doesn't (technically) have to be under emf module folder to operate.

Given that this is a D6 module, where the vast majority of module installs are still done via FTP, there is no maintainability problem - if you want to install a new version of EMF, just copy the new files over the old ones.

Well... maintainability has nothing to do with Drupal core version. Here, we're doing svn+rsync deployments since D5. Your just-copy-the-new-files-over-ftp doesn't take care of deleting deprecated & useless files which can be harmful in some cases. You'll then have to manually fine check your module update to detect files that have vanished remembering the exceptions you manually added. This is insane in large scale production environments. Plus there's no technical advantage of doing so.

Finally, ALL other EMF plugins follow this pattern. If you disagree with it, can I suggest you take that up with the EMF maintainers?

No. Campaign Monitor & MailChimp sub-modules lives in the EMF code base because they are provided with it. They're part of the same 'project'. Interspire E-mail Marketer & Addemar plugins are separate project and do not live in EMF codebase. The do not have to be installed under the emf folder and their documentation does not re command so either.
If you want to see your module live alongside Campaign Monitor & MailChimp plugins, you should not submit a project application for a module on its own but submit a patch to EMF maintainers to be included inside the EMF project itself.

(My 0.02€, but i'll be happy to read other fellows comments & thought on that topic)

Oh, and while you are doing so, could you ask one of them to take a look at my module please?! It has been in production use for months and they could approve it in seconds...

I can't check it against a real GraphicMail setup but I'll do a quick code review ASAP.

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
Review of the 6.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (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. You have to get a review bonus to get a review from me.

manual review:

1. "module_load_include('inc','emf_graphicmail','emf_graphicmail.api')": 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 'mymodule.inc';
2. emf_graphicmail_form_emf_admin_list_info_form_alter(): $datasets_data: do not store that sanitized. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database". From http://drupal.org/node/28984
3. "watchdog('emf_graphicmail', 'Error adding custom fields data for subscriber ' . $email);": do not concatenate variables directly into watchdog() calls, use placeholders instead.

The point is not to >store< data sanitized, but to sanitize data before it is >displayed<. There is no problem to save raw data in drupal and sanitizing it in the hook_form_alter() before displaying it to the user.

1. emf_graphicmail.module - line 129 - now is if(){ should be if () {

Hi guys, have used the Graphic Mail module before for D6 sites (works great thanks). Trying to implement for a D7 site. Can you please point me in the right direction for a download of D7 version or EMF Graphic Mail plugin for EMF. Thanks.

Hi guys, this is a followup to earlier mail. Have the Graphic mail module working. However, by default it also appears on the front page. This is despite correct setup in the block section. Thanks again.

This is a one-time approval process to get permission to promote it (and future projects) to a full project. No need to go through it again. As successful completion of the project application process results in the applicant being granted the "Create Full Projects" permission, there is no need to take multiple applications through the process. See http://drupal.org/node/1011698

Hi Mark,

Please fix this small error

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/graphicmail.class.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
43 | ERROR | Function comment short description must end with a full stop
--------------------------------------------------------------------------------

Thanks
Shanid kv

Minor coding standards are not application blockers. Please take an actual look at the source code.

Sure klausi
i will check source code

mja, I understand your frustration with the review process. It can can seem lengthy and nitpicky, but please don't take the code review personally. What feels nitpicky actually serves a valuable purpose. It ensures people who are granted permission to create modules (as mentioned, it's a one time approval) are at least familiar with Drupal coding standards. Coding standards are important in a collaborative environment. The Drupal coding standards aren't the same as (and in some cases contradict) other things I've coded, so I see where you're coming from that at times it seems a bit annoying and frustrating. But without standards, you could end up with nightmare code that no one could read. People who are reliant on modules need to be able to contribute to them and/or take over coding them if the maintainer disappears.

I'm going to submit my module for approval, and I fully expect to get the same critiques if I have any violations of coding standards. If people ignored them, I might not even be aware I violated a standard for Drupal. The review process is not just a means for others to criticize and block code from being released. It's a learning tool.

Here are the issues I found:

You still have a master branch. It should be deleted. See step 6 at Moving from a master to a major version branch.

Your README.txt file and the project page still recommend installing the module into the EMF module's plugins directory. It should instruct users to install it under sites/all/modules, or as most modules do, you can leave out where to install and assume users know where modules should be installed. Instructing users to place a module into another module's directory goes against best practice. (See EMF Interspire and EMF Addemar Plugin. Neither recommends placing their EMF plugin module's code into the EMF directory.)

emf_graphicmail.settings.inc

'#description' => t('Your Graphicmail API Username. See Graphicmail\'s <a href="http://www.graphicmail.co.uk/api/documentation/">documentation</a> for more info.'),

'description' => $t('Your Graphicmail API password has not been set yet. Please specifiy it on the <a href="@url">Graphicmail settings page</a>.', array('@url' => url('admin/settings/emf_graphicmail'))),

According to the t() documentation, you should avoid escaping quotation marks. But you've also got double quotes in there, so I'm not quite sure what's recommended for that. As such, I'd guess it's not something that should block promotion. But personally, I'd just avoid the whole issue by taking out the apostrophe and and writing "See the Graphicmail documentation" instead.

graphicmail.class.php
Potential XSS vulnerability. In unsubscribeUser() I can't tell where $email is being populated. It seems like it might be external or user input, but it's not sanitized before being sent to watchdog().

watchdog('emf_graphicmail', 'Could not remove dataset data for subscriber ' . $email . ' from dataset ' . $dataset_id . '. The user may not have had a dataset entry');

Should probably be:

watchdog('emf_graphicmail', 'Could not remove dataset data for subscriber @email from dataset @dataset_id. The user may not have had a dataset entry', array('@email' => $email, '@dataset_id' => $dataset_id);

emf_grapichmail.api.inc

drupal_set_message($e->getMessage(), 'error');
watchdog('emf_graphicmail', $e->getMessage(), array(), WATCHDOG_ERROR);

Unsanitized before output. Are you certain that $e->getMessage() for your exception on call_user_func_array will always return data safe to output to the screen? Or might it output the contents of or some part of the variables passed to the function, at least one of which seems like it might be data returned from an API call. If you're not certain, you should sanitize the data before outputting it.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that 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 :-)

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" (found some problems with the project) or "reviewed & tested by the community" (found no major flaws).

Changed module install path recommendation