[D6] SocialMediaFeed

Thank you for taking the time to submit your project for review. Before it can have a full code review, there are quite a few things that need to be addressed.

Projects submitted to this queue are meant to be release ready and demonstrate that you are ready to manage a project up to the highest standards. We all want quality modules on d.o and getting them means taking time and care to ensure that every module you create now and in the future will meet or exceed those standards. Please see Best practices for creating and maintaining projects, and Tips for ensuring a smooth review. Review process for Full Project Applications: What to Expect

CRITICAL - Your module contains mixed licences

All code on d.o is GPLv2. You have included code that is copyright to other people and using things like the MIT license. These must be removed. Please look into using the Libraries API/module to enable inclusion of things like this. Dual license doesn't count.

CRITICAL - No validation of user input

Validation function contains the rather cryptic dsm, not-translated, message:
drupal_set_message("Your feed settings have been saved. Data has been change accordingly.");
These values are used DIRECTLY in HTML output without any kind of processing. Only 'administer site configuration' can edit, but use input must always be processed. Administrator is subject to XSS attacks, just like anyone else. Strings also used directly in HTTP requests.

CRITICAL - No parsing of inputs into database

Sanitize or validate all inputs that go anywhere near database. Just becasue you THINK you're talking to facebook/whatever, doesn't mean you are. They have a fixed return format, validate it, verify values are sane, and escape anything going into the database appropriately.

Project page

A bit sparse. See Tips for a great project page. Also no documentation, or screenshots.

Similar module

Are there any modules that do similar things to this one? Please include them in your application and project page, or at least address the issue.

readme.txt

.. is meant to be README.txt. It should also be line wrapped to 80 characters for people to easily read on a text console.

yoursite.com

example.com, example.net, example.org are the domain to be used in an examples.

missing EOL characters

causes annoying git message "No End of line"

CVS $Id$ tags

d.o doesn't use CVS so these tags are not needed

Tabs and indenting

Please only use 2 spaces for indenting and absolutely no tabs. Indenting is also all over the shop. Please be consistent and follow the coding standards style.

non-unix line endings

please change all files to unix style line endings

.install file full of "TODO: please describe this X!"

???

// Implementation of hook_install

Please doxygen/phpdoc style comments for function headers. eg

/**
 * Implementation of hook_install().
 */

If your module defines any variables, remove them in hook_uninstall().

For D6 modules, hook_uninstall should also be used to remove any database tables created by the module. (This occurs automatically in D7.)

Inaccurate or copied comments .. please comment properly.

/**
 * @file
 * Module for fetching data from signup db table.
 * This module provides block content retrieved from a
 */

- no it doesn't.

prems: administer site configuration

Configuration of your module is contingent on another modules perms. Provide your own for greater flexibility and granularity.

use the correct page callback on menus

This is pointless. Just use drupal_get_form as callback.

function socialmediafeed_admin_settings() {
        return drupal_get_form('socialmediafeed_settings_form');
}

Use {} around tables

The following will not work with anyone using table prefixes.

$query = "TRUNCATE TABLE socialmediafeed_post";
$sql = db_query($query);

This can also be compressed to one line. No need to keep separate.

Not enough use of t().

Lots of untranslated strings.

module uses curl without checking its available

Not everyone has curl installed. Please add it as a requirement on project page and use hook_requirements to ensure it doesn't get installed without it. Alternatively, look into using drupal_http_request.

Master Branch

It appears 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.

Please use parameterised db_query

Many instances where you are using db_query as a normal function instead of using its built in quoting and safety systems. eg.
$query = "SELECT * FROM socialmediafeed_post ORDER BY time DESC LIMIT $displaycount";
This one is stupendously unsafe .. you're actually letting a foreign site dictate exactly what to put in your database without any kind of validation.

$query = 'INSERT INTO ';
$query .= $table;
$query .= ' SET ';
..
$query .= implode(', ', $placeholders);
db_query($query, $fields2[0], $fields2[1], $fields2[2], $fields2[3], $fields2[4], $fields2[5], $fields2[6], $fields2[7], $fields2[8]);

Please look at db_placeholders and db_query documentation.

hook_block

Feeds seem to all be stuck in one block. Perhaps it might be a better idea to allow the admin to split the blocks up so they can have them where they please.

hook_init

You're including code that should be in hook_init straight into the global scope of the module. Bad.

drupal_add_js(drupal_get_path('module', 'socialmediafeed') . '/js/jquery.mousewheel.js');
drupal_add_js(drupal_get_path('module', 'socialmediafeed') . '/js/jScrollPane.js');
drupal_add_js(drupal_get_path('module', 'socialmediafeed') . '/js/socialmediafeed.js');
drupal_add_css(drupal_get_path('module', 'socialmediafeed') .'/css/socialmediafeed.css');

theme function theme_socialmediafeed_feed_structure

theme functions should be purely output. All the logic you have in this function should be in the preprocess function which puts everything into variables that are then used in the output.

commit messages

Please review Commit messages - providing history and credit. These are quite amusing considering the order.
Thu Oct 20 01:31:55 2011 -0400 39abaa7 Second commit.
Wed Oct 19 23:04:09 2011 -0400 bbe4b1a Third commit.
Wed Oct 19 11:22:21 2011 -0400 2ed144a Initial commit.

coder module on minor

Please run your module through the Coder module set on "minor" to catch everything and fix all the issues.

A small review.

When you have addressed all these issues, please set the issue back to 'needs review'. If you have any questions, please don't hesitate to ask.

master branch

Please review step 5 of Moving from a master to a major version branch. I recommend against the "removing remote master" path, but instead just cleaning out the master and pointing people to the correct location in the README.txt file.

commit messages

Please review Commit messages - providing history and credit. "Made mods to pass inspection." is not that descriptive or useful.

CRITICAL - input validation

Looking through the commit changes and the validation you have added, it seems you have missed the mark a bit or don't understand any of the security implications of trusting user input. Just throwing wrapper functions around things is not enough. You actually have to look at the input and verify it's valid and only then submit it to the database.

For example - the socialmediafeed_settings_form()'s validate and submit functions. The validation function:

function socialmediafeed_settings_form_validate(&$form, &$form_state) {
  $message = t("Your feed settings have been saved. Data has been change accordingly.");
  drupal_set_message(check_plain($message));
}

This does NOTHING, and even lies to the user since nothing has been saved at this point.
Also, this has a great example of useless validation; you are running check_plain() on a static string. That dsm should be cut down the following, and actually moved to the end of the submit handler:

drupal_set_message(t('Your feed settings have been saved. Data has been change accordingly.'));

The validation function should be going through every user input, making sure that it's valid, and if it's not, calling form_set_error() on it so that the user is presented with an error and and oppertunity to change their input.
The submission function isn't even called until this validation function has been passed without form_set_error being called.
eg. socialmediafeed_facebook_profile_id should no doubt be an string of digits and letters only, so if someone enters "Frank Sinatra", your validation function should reject based on this and tell them that they need to be entering only digits for that field.

CRITICAL - database validation

You're still inserting into the database based entirely on what a foreign website tells you to. At least vailidate it and use drupal_write_record to make sure you follow the local schema. At present you just attempt to insert whatever the xml tells you to, key values and all!

drupal_add_js

By limitation of the D6 architecture, your drupal_add_js calls will not happen when the _theme call is not made - ie. when someone turns on block caching

spare files ..

.DS_Store somehow made it into the repository .. make sure you only commit files related to the project

empty class

class Twitter_Post - If you need an object but don't want to create a full class, just use stdClass.

$post = new stdClass();

.. but considering you just turn it into an array 10 lines later .. what's the point of using a class at all???

$vars = get_object_vars($post);

non localised function names

All functions used in your module must start with "socialmediafeed_", or "_socialmediafeed_" for private functions. This is prevent collisions with other modules in the global namespace.

stuff that hasn't been touched

There are still plenty of bits of code in there that need fixing ...

    'page callback' => 'socialmediafeed_admin_settings',
..
function socialmediafeed_admin_settings() {
    return drupal_get_form('socialmediafeed_settings_form');
}

function socialmediafeedflush() {
  // Flush the all feed data
  $query = "TRUNCATE TABLE socialmediafeed_post";
  $sql = db_query($query);
}

(function name is also not correct)

Indenting is still a bit all over the place, but at least there aren't any tabs in there any more. You still need to fix these up.

Please revist all the points from my previous post to make sure you have addressed them all, along with all of the ones in here before changing back to 'needs review'.

• There is a ".DS_Store" file in the repository, remove it.
• readme.txt should be README.txt, see http://drupal.org/node/447604
• jquery.mousewheel.js: appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
• socialmediafeed_requirements(): there should be no new line between function and doc block.
• "if (in_array ('curl', get_loaded_extensions())) {": there should be only one space after "if"
• socialmediafeed_schema(): do not translate DB field descriptions, this only generates overhead for translators.
• socialmediafeed_perm(): Hook doc block missing, see http://drupal.org/node/1354#hookimpl
• socialmediafeed_settings_form(): indentation errors, did coder not detect this?
• All functions need to have proper doc blocks.
• Why do you need cURL? Does drupal_http_request() not work?

  foreach ($socialmediafeed_vars as $key => $value) {
    // We validate the value from the form. The key is defined by the module so no need to validate that.
    variable_set($key, filter_xss($value));
  }

$query = "TRUNCATE TABLE socialmediafeed_post";

The project application/review queue is not meant to be a massive hurdle. If you follow the instructions for submitting your project, use the Drupal API where you can, and heed the advice of other reviewing your code, it should be a fast and painless process. Everyone wants quality modules when they come to d.o, and for that to happen, everyone creating new projects has to ensure that they are the best they can be and they follow the best practices for security, and coding. We want your current and future contributions to be the best they can be!

Please ensure you address all of the issues raised in all posts. Either in code, or in text.

please go through my comments in #5 one more time, at least 2 of them still apply.

BPI,

Regarding #10 ... did you forget to 'git push'? I checked the first three items from #5, and all three issues are still present.

(And I supsect that .DS_Store should be 'excluded', not 'included'.)

@jthorson You've been bitten by the "master being full of old files" bug!

@BPI
Based on commit 6a8a4db84ed0b9277bccac0e0ce9a03b12e682b6, Sun Nov 6 21:09:40 2011 -0500

master branch

If you're not using master, please clear it out and replace it with a single README file informing the user of where to find the real files in git, such as the release branches.
Moving from a master branch to a version branch.

security issues

security flaws, still present. Database inserts use field names from foreign source. You already have a schema for the table which means you don't have to trust a foreign site, no matter what it is.

table handling

Prefixed table handling issue, still present.

3rd party code

GPL violations, still present.

indenting

Indenting is meant to make the code more readable. There are sections of the code that are incorrectly indented such that reading the code because quite confusing. Such line 442, which makes it look like the code which calls the function above is actually part of the function.

This is pretty close to ready, but it does have a few issues. The first two are should be considered functionality bugs, and the others are Coding Standards issues.

hook_menu

access arguments for administration of your module should be based on local permissions to allow greater granularity. you seem to have already included a permission of 'administer feed', but are using 'administer site configuration' instead. name of permission should be unique to your module too, so probably 'administer social media feed' instead.

socialmediafeed_facebook

local function socialmediafeed_facebook_insert does not handle table prefixes. use drupal_write_record instead on the $post array ie drupal_write_record($table, $post); and it's done. this saves having to build the whole query yourself, and
ditto twitter
ditto youtube
ditto foursquare

css/socialmediafeed.css

uses tabs instead of spaces. please use 2 spaces in place of every tab

js/socialmediafeed_form.js

uses tabs instead of spaces. please use 2 spaces in place of every tab

readme.txt vs README.txt

On *nix systems, case is significant. Please change the case of this file to README.txt

hook_schema

descriptions of schema tables and fields do not need to be translated

socialmediafeed_youtube

indenting has gone wonky. the top of local _insert function, prior to the if (!$myterms) is indented once more than it needs to be.

No further blockers, but these recommendations still stand for the module and can be dealt with at some point. Setting to RTBC for review by someone else.

highly recommended

block cache will kill CSS

Adding of CSS should be in hook_init if it's included on all pages as the block cache will mean that this code is not called when the block is cached. .. which is disabled by default for your module (not specifying a block cache method means no cache), BUT it also includes the CSS on all calls to the function instead of just when viewing the block. switch statement not indented properly. I'd still allow the block cache to be used if configured to enabled by the admin.

merely recommended

README is still lower case.

no idea what is going on here, but it's still lower case in the repository. rename it with

git mv readme.txt README.txt

.. and then commit it. It's still lower case in all branches. Windows will not rename a file on the drive via the GUI if you keep the same name and only change the case.

oops! no eol chars

readme, css, js and info file now missing EOL character at end of file. this is a new one after the reworking of the tab spacing

module name

In the info file, you could probably change the name of the module to 'Social Media Feed' instead of 'SocialMediaFeed' since this is a human readable field displayed on the modules page which can be word wrapped.

There are still files other than README.txt in the master branch, make sure to remove them. See also step 5 in http://drupal.org/node/1127732
Review of the 6.x-1.x branch:

• README.txt is missing, see the guidelines for in-project documentation.
• 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:

• did you try drupal_http_requst() instead of cURL?
• socialmediafeed_settings_form(): This function does not implement hook_form()! This a form callback, see http://drupal.org/node/1354#forms
• socialmediafeed_youtube(): why do you define functions inside of functions?

Sorry, I can only find time for applications with a review bonus, see #1410826: [META] Review bonus.

In the function socialmediafeed_facebook, you first ask for an access token from graph.facebook.com but not once do you check that the result is valid or has been trimmed correctly. If this has an error, it will just continue one throwing errors and attempt to access resources that do no exist.

In socialmediafeed_twitter you don't catch any errors potentially caused by new SimpleXMLElement($buffer->data) (or check that there is something to parse in the first place) and again the errors are ignored.

Ditto youtube. foursquare.

Errors and timeouts do happen. Stick a firewall rule in place to block the traffic going to one or all of these places and see what kind of fun errors turn up, or just code with failure as an option.

It looks like it is pretty much ready to go. There is one rather large outstanding issue though. The use of die() has adverse flow on effects and should not be used for this kind of situation (no other cron tasks will happen, none of the exit functions are called, silent failure).

While this might work, it should not be used at this level. SimpleXMLElement will also throw exceptions and throw errors if it does not get its way.

$sxml = new SimpleXMLElement($feedurl->data) or die('Error creating a SimpleXML instance');

SimpleXMLElement will throw an excepting and raise E_WARNINGs for every error it encounters in the XML. It needs a try/catch around it and to have the errors suppressed. If it doesn't end up with valid XML at the end (in the catch), then no further processing should happen.
http://www.php.net/manual/en/simplexmlelement.construct.php

libxml_use_internal_errors(true); 
try{ 
  $xmlToObject = new SimpleXMLElement($notSoWellFormedXML); 
} catch (Exception $e){ 
  // Retreat!
  return;
} 

I ran the coder review over the module just for fun to see if someone would be bouncing it for that and there are quite a few things it has picked up. Since the code is readable and this is well into the manual review process, these are in no way blocking issues, I leave them as an advisory of things that could be fixed up.
http://ventral.org/pareview/httpgitdrupalorgsandboxblueprintinteractive1...

Anyway, the handling of the XML looks to be the last sticking point as the rest look good. Sorry I missed the error handling on the XML before as I did not check how it handled errors until it had the die() next to it.

The die is gone, and the handling of the result looks good enough beyond that. Nothing else was changed beyond that so the rest of the module still stands and ready to RTBC.

There are still a lot of coding standards issues in there that you should look at at some point.

For future reference

git commit messages and attribution

Please refer to Commit messages - providing history and credit about giving yourself some credit and properly formatting commit messages.

From here, another git admin will have to do one more final review the module and if they find no issues, set it to "fixed" and grant git vetted status. If they find any issues, it'll be set to "needs work" with appropriate instructions.

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. Get a review bonus and we will come back to your application sooner.

manual review:

• socialmediafeed_settings_form(): you could use system_settings_form(), then you don't have to save varaibles yourself in the submit function.
• theme_socialmediafeed_feed_structure(): do not print link markup yourself, use the l() function instead.
• Same for images: use theme('image', ...) instead.
• theme_socialmediafeed_feed_structure(): the incoming data from the various services is untrusted user provided input. I don't see sanitization anywhere, you are just printing the data as is. Even if the services do some sort of sanitization already you should run text and titles through the appropriate functions, see http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-...

Hi @BPI, I've ran pareview.sh and it's still seeing some errors:

• Remove all .DS_Store files from your repository.
• 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. Get a review bonus and we will come back to your application sooner.

(attached a .txt file with the rest of the output)

Make sure you have pushed your latest changes to the repository and that you use the latest -dev of drupal code sniffer so you see all the errors.

1. about point number 1, keep in mind that you can add your own processing to system_settings_form using $form['#submit'][] = 'your_callback', so potentially you could make it easier to maintain by not having to set all variables.

3. according to http://api.drupal.org/api/drupal/includes!theme.inc/function/theme_image/6:

$path: Either the path of the image file (relative to base_path()) or a full URL. If this is a full URL, $getsize must be set to FALSE or nothing will be returned.

so you should be able to use theme('image') for external images as long as you set $getsize to FALSE.

I'd also recommend the changes as identified in comment #30.

However, the sanitization issues from #28 appear to have been addressed ... and perhaps excessively in some cases. ;)

Given this, bumping back to RTBC.

@kiamlaluno: Please provide this message if you approve an application (see http://groups.drupal.org/node/184389 ):

Thanks for your contribution, BPI! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.