[D7] D2D

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxshakty1889370git

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 shakty,

First, you should commit a 7.x-1.x branch and put all your code there. Having a "master" branch is discouraged.
Please see the Release naming conventions to understand why.

You should also remove the LICENCE.txt file. It will be added by the drupal's packaging script.

.modulefile

I am not sure to understand why you have some manu require_once there. You can use drupal_load_include() to load those files only when you need them.
Also, all those files that are include doesn't seems to follow the drupal coding standards. (function commented using inline comments, function that doesn't have any comment at all, ...).
In a more general way, it seems to really lacks comments on those files, it is really hard to understand what those files are doing. For example, in the d2d.manage_groups.inc file, you don't have a single line of comment.

d2d_example/d2d_example.info

using files[] in .info files are only used by drupal's class autoloader, so you can remove those 2 lines in your file.

files[] = d2d_example.module

A few other coding standars erros in others files, but this should be easy to fix with the pareview help.
files[] = d2d_example.install

Hello shakty!

manual review:

The code needs a better documentation. An example:

d2d.friendship.inc

/**
 * Returns a human understandable text describing the current friendship state.
 */
function d2d_friendship_outgoing_describe($arguments, $time_insert, $time_next_send, $time_invalid, $max_send_trials, $instance, &$persistent_state) {

The documentation must explain the arguments that recieves the function and the return value. You have a lot of function in the same situation.

same file, line 19:

switch ($instance['friendship_state']) {

It's a good idea check if the key friendship_state exists to prevent errors. It happens in a lot of functions.

if (isset($instance['friendship_state']) {
switch ($instance['friendship_state']) {

It's necessary include all the files at the top of .module? The files have to be included only when are necessary.

About module_load_include vs require_once.

module_load_include is better use it when you want to load files of anothers modules (you can't know their location), but if you want include your own module files you can use require_once. module_load_include check the module route and if is the same module this is a extra-job.

With

module_load_include('inc', 'd2d', 'includes/d2d.constants');

drupal have to "discover" the route to the module d2d that is your module. Then, include the file with a require.

require_once realpath(__DIR__) . '/includes/d2d.constants.inc';

Do the same, but Drupal doesn't have to check the route.

d2d/_example/d2d_example.module

You can use hook_menu instead hook_menu_alter.

d2d/_example/includes/d2d_example.info.inc and d2d/_example/includes/d2d_example.remote_control.inc

don't have the functions documented.

README

Use README.txt instead README.md

Good luck!

Hello.

I have to correct one of the points of my review. Sorry. In the review process of a module of mine told me that instead utilizase module_load_include required, so I adopted that point as a correction to make.

My module has passed the review process, but the person who enabled the publication of the project (cweagans) commented as follows on the subject:

For this bit:

 389 /**
 390  * include the block functions file
 391  */
 392 require_once realpath(__DIR__) . '/pay_with_a_tweet.blocks.inc';
 393 
 394 /**
 395  * include the tokens functions file
 396  */
 397 require_once realpath(__DIR__) . '/pay_with_a_tweet.tokens.inc';

1) To load module include files, you should use module_load_include; and
2) If you're including this unconditionally, there's no reason to split them out into another file. It is, in fact, a bit slower, since you're adding a couple of disk reads/file open operations into the mix. You could probably just copy the contents of those files into your .module file and you'd be fine.

I understand that, in this case, it is better to follow the recommendation of a person who can enable publishing projects.

Hi all,

I would like just to share that we have been using d2d successfully with the following modules to exchange information between different drupal instances:

• https://drupal.org/project/patterns_client
• https://drupal.org/project/patterns_server

In d2d_install() you don't need to set defaults for your variables, you can do that when you read the value with variable_get('somevar', 'defaultvalue');
You also don't need the drupal_uninstall_schema() call, Dupal 7 will remove hook_schema for you automatically.

How does this module compare with the Services? I believe that module also support xml-rpc in a secure fashion. The Deploy module is built on top of that to push content around from one server to another.

Setting to needs work for the above question, I did not do a full code review.

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

Thanks for the additional information!

You have a large number of style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxshakty1889370git. Some of those will be spurious, but reducing that number would be great. There are a couple notices not to use form_state['input'], which is a security issue. And remove the master branch from the repo.

Setting to "needs work" for security and lots of drupal code style issues. Otherwise, I think is this ready and is not a duplication.

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

Thanks for those updates!

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

No further action is required, but the best thing you can do is get a Review Bonus by reviewing other applications. That will get you to the top of the list of projects to get reviewed (and hopefully approved). Only manual reviews count, just using http://pareview.sh is not enough.

You won't be able to promote your project until you receive "git vetted user" status, there's 1 level of review left.

It's been a month without any problems reported, so I'm promoting this myself as per https://drupal.org/node/1125818.

Thanks for your contribution, shakty!

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.

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