Tumblr Connect

This actually needs to be reviewed before being set RTBC.

Looking through the code, some remarks. You do not necessarly need to resolve all of this to make this a full project.

- See http://drupal.org/node/1354#classes on how to correctly document classes and interfaces. In short, the idea is that you document the interface and just reference from the implementation to the interface, similiar to a hook implementation. Note that core isn't really following this yet, but people are working on it..

- $id = db_insert('tumblr_connect_batch')->fields($batch_command->toArray())->execute();
chained method calls should be written on a separate line with intendation of two spaces. Example:

$id = db_insert('tumblr_connect_batch')
  ->fields($batch_command->toArray())
  ->execute():

- public function getAllScheduled() {
In this function, you are querying field_data_body directly. That table might not exist, a site could for example use MongoDB for a field instead. Have a look at http://api.drupal.org/api/drupal/includes--entity.inc/class/EntityFieldQ.... Or maybe you can simply load the nid's and then use node_load_multiple(). That is slower, but it can be cached if performance is an issue.

- public function getAllNodes() {
You seem to be using four spaces instead of two in that function.

- version = VERSION
You shouldn't add this line to your .info file, the packing script will do that automatically

- // $Id$ (in .install)
You don't need this line anymore, git doesn't use it as you can see.

- drupal_uninstall_schema('tumblr_connect');
This is done automatically, this line must not be in your uninstall hook.

- $items['admin/config/tumblr_connect/setting'] = array(
you are creating your own category here. This shoud be avoided unless there is no other category that matches because the categories will be useless if every module adds their own categories. "Web services" looks like a perfect match to me... :)

- function getTypesAllowed($reset = FALSE) {
That function should be renamed to tumblr_connect_get_allowed_types() or something along those lines. a) prefix with your module name and b) use underline instead of camelCase for functions. There are a few more functions like that. a) is especially important.

- function tumblr_connect_block_view($delta = '') {
You should either remove the switch or use case 'tumblr_followers_block', having a switch with just default is pointless :)

- $form['drupal_types'][$key] = array(
You can simplify that line by using type 'checkboxes', then you can just pass in $types as '#options' and you have your list of checkboxes.

-

Wow, that's quite a commit ;)

Some *very* minor stuff about the db statements:

+      $id = db_insert('tumblr_connect_batch')
+                      ->fields($batch_command->toArray())
+                      ->execute();

Many calls are similar. That looks like a bit more than two spaces to me :)

Something like this should be enough:

$id = db_insert('tumblr_connect_batch')
  ->fields($batch_command->toArray())
  ->execute();

   public function delete($nid) {
     try {
-      $result = db_delete('tumblr_connect_batch')->condition('nid', $nid)->execute();
+      $result = db_delete('tumblr_connect_batch')->condition('nid', $nid)
+                      ->execute();
     } catch (Exception $e) {

condition() should be moved to a separate line too.

   public function getAllNodes() {
     $result = db_query('SELECT DISTINCT nid from {tumblr_connect_batch}');
     $list = array();
-    if (! $result) {
+    if (!$result) {
       throw new TumblrConnectDatabaseException(t('Unable to select all tumblr connect batch nodes'));
     }
     else {
-        while ($obj = $result->fetchObject()) {
-            $list[] = $obj->nid;
-        }
+      while ($obj = $result->fetchObject()) {
+        $list[] = $obj->nid;
+      }
     }
     return $list;
   }

You simplify this quite a bit using fetchCol().

    $list = db_query('SELECT DISTINCT nid from {tumblr_connect_batch}')->fetchCol();
    if (empty($list)) {
      // throw exception here.
    }
    return $list;

-    $obj = db_query('SELECT id from {tumblr_connect_config}')->fetchObject();
+    $obj = db_query('SELECT id from {tumblr_connect_config}')
+      ->fetchObject();

If you only have a single method call, you *can* leave it on the same line, but it's fine like this too I. This message is purely informal :)

Otherwise, awesome job on the added documentation and other cleanups.

Hello,

I have tested the new version of this module and I have few comments about it (didn't try new version yet):

1. Sincronization works pretty well: update is fast (immediate), and when a node is deleted it is also deleted in tumblr
2. It's easy to config.
3. I've created a content type with a image field on it and tried to see what happens. Unfortunately it is posted as text, although the desired behaviour would have been to post it as an image. Same thing with different content types containing one field with videos and links (one field per content). I think it would be great to have some kind of mapping so tumblr would recognize certain node types and fields as to improve tumblr's integration and presentation.

Thanks again and keep improving!

PS: I edited the post in order to remove something I said about a supposed error which is not true. Sorry for the inconvenience

You're right lorinpda,

I'm still getting used to sandboxes ;) sorry for the inconvenience, I didn't want to change the topic of conversation.

I have created a new issue in this sandbox: http://drupal.org/node/1097122

Let's wait to have this module aproved.

+1

+1

@lorinpa: Someone with the required permissions needs to make your project public. You could try to join #drupal-contribute (See http://drupal.org/irc) and ask if someone can do it.

@+1 posters: That's really not helpful to anyone, if you have tried the module and are using it, then say that. (e.g.: I have tested this module on my site and it is working fine or something like that).

@c-c-m Thanks for the review. Reviews are extremely helpful. However, I will say that I am not inclined to act to approve based solely on RTBC status set by a member who does not have "git vetted user" permission. @Berdir, would you go along with the RTBC? Barring that, another review from a vetted user would help here. Thanks.

Yes, the code looks good to me. I haven't actually used and verified that it works, but c-c-m did that. So I think that's fine...

Well, with that, this is approved and git vetted user is granted.

I dropped a minor patch into your issue queue.

Welcome, lorpinda, and thanks for your patience with this process and for your contributions to the community.

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

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