[D7] Social Media Aggregator

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

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.

Just had a look at the module now, have to say it took me longer than it probably should have to realise what it does!

The automated test does fail on that one issue. Looking at the entity module (inside entity.api.php) that has the extension .api.php not api.inc. Inside that file it does declare the hooks using its modules prefixed name so perhaps the automated review isn't a false positive? Minor issue anyway and its good to see an api helper file.

In your facebook submodule, looking through the use of theme('image'), do the facebook images get alt text from anywhere? Just to help the screen readers out perhaps, even if its not content manageable.

All in all though pretty good. :)

Now you have renamed the .api.php file it needs to move out of the includes folder and your admin.inc file should be moved in to there.

The module looks great, the only thing is that the path in hook_menu should be changed to includes/social_media_aggregator.admin.inc

After some testing with twitter ans facebook this module works as designed.

Automated Review

Good :)

Manual Review

Module duplication is a problem on drupal.org. How does this differ from the other modules that provide similar features? You should elaborate on this in the description above and on the project page.

social-media-aggregator-item-theme.tpl.php, you are printing directly from $variables[]? If these are defined in the hook_theme, aren't they directly available? Personally, I would renamed this to social-media-aggregator-item.tpl.php

The docblock for social_media_aggregator_permission() is wrong.

Some of your helper functions need proper docblocks.

You have a few places where you are calling theme() and placing it into markup (eg, social_media_aggregator_facebook_build()). Proper render arrays with a #theme definition would be better.

social_media_aggregator_facebook_get_page_id() shoudl really use drupal_http_request() instead of curl.

In social_media_aggregator_instagram_user_search(), you don't need two empty() checks, just the !empty($cache->data), or the explicit expiration check. cache_get() will expire items for you.

In social_media_aggregator_instagram_user_search, I think you have a logic error if the item is cached. PHPStorm is saying that $account_id on line 101 is unused. You also have some undefined return paths.

In social_media_aggregator_instagram_load_master_account(), you don't need to wildcard the query if you are just returning one field.

This module looks very well written, am I may be a little dense here, but where is the actual atom content being sanitized? By my trace, social_media_aggregator_field_formatter_view() invokes the callback in the submodule, eg, social_media_aggregator_facebook_build(). This will get the posts, eg social_media_aggregator_facebook_get_posts(), which will normalize them via social_media_aggregator_facebook_normalizer(), but that's it. I am not seeing any of the text filtering API functions being called anywhere. Perhaps you need to add a formatter setting for an input format and then use that with check_markup() in social_media_aggregator_field_formatter_view()? Please don't remove the security tag, we keep that for statistics and to show examples of security problems. If I am shown to be wrong, I will remove it.

Scott, if you think this is ready for another review, then change the status back to Needs Review. I likely can't take another look until Tuesday, but someone else may be able to in the meantime.

Docblocks. It looks like all of the helpers had one, but there were no @param or @return lines.

Render array. Yeah, you don't need to explicitly return markup. This lets other modules alter things before they get rendered. Poke around for some examples; it makes sense once you see some.

curl(). Leave a comment about that for future reference.

The output filtering / sanitization was the only blocker. I would comment as appropriate where another module is handling the sanitization.

Scott, could of quick things about the changes you made.

I think your use of check_markup() everywhere isn't quite right. See https://drupal.org/node/28984 I think that only the post itself would need to be rich text and need check_markup(), the rest can likely be check_plain().

You are also doing the sanitization too deep; someone may want to get the raw normalized data. The Drupal way is to sanitize on output. Since some of your dependencies sanitize for you, the build functions in each submodule are probably the best place. This needs to be addressed.

Some side notes (non-blockers).

social_media_aggregator_field_formatter_view() doesn't explicitly need to render via theme(), it can return a render array w/ the #theme index in it. In general, #markup is only for simple things.

Long term, you may want to implement hook_views_data() to make the normalized data available to Views.

Your usage is still wrong. You can't call check_markup on the normalized $item. You need to sanitize each element in the $item.

You have

function social_media_aggregator_facebook_build($identifier, $quantity, $offset = NULL) {
  $build = array();
  $items = social_media_aggregator_facebook_get_posts($identifier, $quantity, $offset);
  if (!empty($items)) {
    foreach ($items as $item) {
      $build[] = array(
        '#markup' => theme('social_media_aggregator_item', check_markup($item)),
      );
    }
  }

  return $build;
}

It should look something like:

function social_media_aggregator_facebook_build($identifier, $quantity, $offset = NULL) {
  $build = array();
  $items = social_media_aggregator_facebook_get_posts($identifier, $quantity, $offset);
  if (!empty($items)) {
    foreach ($items as $item) {
      $item['account_name'] = check_plain($item['account_name']);
      $item['account_handle'] = check_plain($item['account_handle']);
      $item['content'] = filter_xss($item['content']);
      // and so on...

      $build[] = array(
        '#markup' => theme('social_media_aggregator_item', $item),
      );
    }
  }

  return $build;
}

Also also please excuse me for not catching your check_markup() usage. That should only be used if you allow the user to select an input format (like the body field on nodes). Otherwise, use filter_xss(). See https://drupal.org/writing-secure-code

Calling theme() directly and stuffing it into #markup still isn't the best, but that is not a blocking issue.

Almost there. Sorry this keeps going back and forth, but security issues can't be skipped.

social_media_aggregator_twitter_build:54, remove the check_markup() around the $tweet. You filtered each element that goes to the screen, so it is safe.

social_media_aggregator_instagram_build:88, remove the check_markup() around the $post. You filtered each element that goes to the screen, so it is safe.

social_media_aggregator_facebook_build:72, remove the check_markup() around the $item. You filtered each element that goes to the screen, so it is safe.

If you do those three things, this is RTBC.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  

  FILE: ...ocial_media_aggregator_facebook/social_media_aggregator_facebook.module
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   96 | WARNING | Potential security problem: SSL peer verification must not be
      |         | disabled
  --------------------------------------------------------------------------------
  

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. "drupal_alter('social_media_aggregator_sources_info', $social_media_sources);": that hook should also be documented in the *api.php file.
2. social_media_aggregator_field_formatter_view(): do not call theme() here and do not use #markup, just return a nested render array. See https://drupal.org/node/930760 . Same in social_media_aggregator_facebook_build() and elsewhere.
3. "return format_interval($difference, 1) . ' ' . t('ago');": do not concatenate variables to translatable strings, use placeholders instead like t('@time ago', ...).

So the cURL peer verification is the last security blocker right now, otherwise this would be RTBC to me as well. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Automatic review

65 | WARNING | Line exceeds 80 characters; contains 84 characters
65 | ERROR | Inline comments must start with a capital letter
66 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks

Still some length of line, capitalization and punctuation errors of some inline comments.

Manual review

• I miss better comparison between existing projects like TB Social Feed and Just Another Social Module (JASM)
• Testing this is problematic as it requires developer api access to all harvesting services. How about setting up a test installation for us to see the "finished" result? Or some screenshots that gives us an idea.
• Conceptually the idea is good, though I don't quite get if it's possible to, say, enable this field on the user entity, and thereby allowing individual users to stream their facebook feed on their profile? What I'm looking for is more concrete usage examples.
• Only basic testing is done. As the sub-modules requires development key in order to retrieve streams I stopped there.
• There are no issues in your sandbox, though the module has 68 commits.
• social_media_aggregator.info: The description = is almost exactly the same as the module's name. Browsing through the code it looks like this is more a field type than an API module. How about naming it "Social Media Aggregator" and as description say "Field type and API for collecting news feed from various social sites". Or separate it in two: One API and one field module.
• The field input form has no #description of the individual input elements. I have no idea what to put here in the entity edit form.
  

  function social_media_aggregator_field_widget_form(&$form, &$form_state, $field, $instance, $langcode, $items, $delta, $element) {
    $element += array(
      '#type' => $instance['widget']['type'],
      'source' => array(
        '#type' => 'select',
        '#title' => t('Source'),
        '#options' => social_media_aggregator_sources_list(),
        '#default_value' => isset($items[$delta]['source']) ? $items[$delta]['source'] : '',
      ),
      'identifier' => array(
        '#type' => 'textfield',
        '#title' => t('Identifier'),
        '#default_value' => isset($items[$delta]['identifier']) ? $items[$delta]['identifier'] : '',
      ),
      'quantity' => array(
        '#type' => 'textfield',
        '#title' => t('No. of Items'),
        '#default_value' => isset($items[$delta]['quantity']) ? $items[$delta]['quantity'] : '',
      ),
    );
    return $element;
  }
  

• README.txt: Not one word about this being a field type.

Summary

I like the idea, but the fact that it is a field type needs more focus. Project page, README, module descriptions, maybe even module decoupling.

Code test passes.

Well, almost. In your social_media_aggregator.api.php:29 I don't know what to do with the $source variable. As you're using drupal_alter() to provide this hook the thingy that should be altered need to be passed by reference. $source is conceptually also plural here:

function hook_social_media_aggregator_sources_info_alter(&$sources) {
  $sources['my_source'] = array(

PAReview.sh also complains about some type hinting. Have a looksie: http://pareview.sh/pareview/httpgitdrupalorgsandboxscotthooker2245205git

function social_media_aggregator_facebook_presave($item) {

should be

function social_media_aggregator_facebook_presave(array $item) {

On a general note, please don't change the meaning of an already posted comment after it is posted. Rather, post a new. Now my answer in #39 looks completely out of the blue as I replied to your #38 with the sound "Don't know why PAReview.sh complained".

@scotthooker: You haven't addressed points 2. and 3. in @klausi's manual review in #32.

As for my review, I think the overall what does this module actually do problem space is the most pressing one.

social_media_aggregator.info: The description = is almost exactly the same as the module's name. Browsing through the code it looks like this is more a field type than an API module. How about naming it "Social Media Aggregator" and as description say "Field type and API for collecting news feed from various social sites". Or separate it in two: One API and one field module.

Looking at your code in social_media_aggregator.module most of it is related to your field type. Say it like it is or split it up.

Adjusting the GIT clone URL so that the project gets downloaded in the directory with the project name.

Sorry for the delay, make sure to complete your review bonus again and this will get finished faster.

manual review:

1. social_media_aggregator_settings(): what is the unit for the setting? Seconds? Please add that to the description.
2. social_media_aggregator_field_formatter_view(): do not call theme() here and do not use #markup, just return a nested render array. See https://drupal.org/node/930760 . Same in social_media_aggregator_facebook_build() and elsewhere.
3. "return format_interval($difference, 1) . ' ' . t('ago');": do not concatenate variables to translatable strings, use placeholders instead like t('@time ago', ...).
4. What is the difference to https://www.drupal.org/project/scald or https://www.drupal.org/project/media ? Please add that to your project page.

But that are not critical application blockers, otherwise looks RTBC to me. The review bonus tag is already removed, you can add it again if you have done another 3 reviews of other projects.

Assigning to jthorson as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, scotthooker!

I updated your account so you can 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 stay 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.