Posterous SPACES

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 the tips for a great project page (you may use html tags on your project page for nice formatting). Also make sure your README.txt follows the guidelines for in-project documentation.
Also all functions should be prefixed with your module/theme name to avoid name clashes.

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

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

Hi Patrick

Thanks for your valuable suggestions...

Will do the changes and get the review bonus for the next review.

Thanks,
svnindia

Hello,

Some suggestions

'access arguments' => array('administer site configuration'),

Is a broad permission - you can use hook_permissions to make your own

I'd also suggest a hook_help as well

Perhaps consider some validation on the fields?

When I check the Node Types check boxes - they don't stay checked?

Thanks

Pat

Not sure if the bugs in the checkboxes are "needs work" or "needs review" so leaving at needs review

Hi Sanjith,
Its a nice piece of code. Can u please make the module more descriptive, try to add description before every function that u have used..
It will make things easier to analyse.

pgogy and sashi,

Will check and do the changes you suggested..

Thanks for reviewing..

Thanks,
svn

Hi,

* The Bug in the node type checkbox fixed.
* The Coding style issues fixed.
Ref link : PAReview

Thanks,
svnindia

Manual review:

• Your module is dependent on curl, but you never check if cur is present on the server, but maybe you could use drupal_http_request instead?
• Also, I think you should filter your input from the forms, before using it, see http://api.drupal.org/api/drupal/includes!common.inc/group/sanitization/7

Thanks @MISc for reviewing.

* I have checked the pre-instillation condition cURL availability. I would like to use cURL because in feature enhancement file posting to posterous will be done.

* The posterous SPACES configuration form field values are now sanitized.

Please check the updated code for review.

Please, remove any files from your master branch and add default branch in Version Control tab.

.info file

Your description should start with a capital letter.

.install file

You have some foreign keys in both of your tables. Use proper schema structure to add them as foreign keys.
Check also core user.install.
Uid is not defined as big or unsigned there:

 'uid' => array(
 'type' => 'int',
 'not null' => TRUE,
 'default' => 0,
 'description' => "User's {users}.uid.",
 ),

Consider naming your tables with your module name. It would be posterous_spaces_account instead of posterous_account.

Do not place empty functions in your module. You can safely remove hook_install().

.module file

Instead of hook_form_alter you can use hook_BASE_FORM_ID_alter().
http://drupal.org/node/1673856

You do not need to do check_plain($form_state['values']['posterous_profilename'])
FORM API makes sure that values are safe.

Remove any commented code from your module.

Do not validate on hook_submit, choose validate. What's more you have your form placed in the other file that form submit function. It's little chaotic. Consider naming your admin form posterous_spaces.admin.inc (following coding standards) rather than posterous.pages.inc

Do not remove user input.
$form_state['values']['posterous_email'] = '';
User can make a small typo. He should see what he entered to easily correct it.

'#title'=>'Publish to posterous',

Always use t() to output strings (of course not in hook_menu()).

$tags != ''

It should probably be empty($tags), to better readability, but it is not a bug :)

Automatic review

Look also at this issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxsvnindia1573504git

Questions

Why do you save your posterous node in the Drupal database? I did not see that you ever query for that data. It looks like unneccessary table.

Hi Milena,

Thanks for reviewing our module.

I have updated/modified the code as per your suggestions.

Questions:
Why do you save your posterous node in the Drupal database? I did not see that you ever query for that data. It looks like unneccessary table.
Answer: To give the actual link to the posterous blog post page. In the future development the blog files are posted to posterous and the embed code will be used in our drupal node. For these purpose we need the response.

Where is the .admin.inc file?:

"Fatal error: require_once() [function.require]: Failed opening required '/var/git/working-copies/abielefe/d7_sandbox/htdocs/sites/all/modules/custom/posterous_spaces/posterous_spaces.admin.inc' (include_path='.:/usr/local/php/php-5.2.17b/lib/php') in /var/git/working-copies/abielefe/d7_sandbox/htdocs/includes/menu.inc on line 514"

posterous_spaces.module
 21 function posterous_spaces_menu() {
...
 30 'access arguments' => array('administer posterous spaces'),
>>> 'file' => 'posterous_spaces.admin.inc',
 32 );

Typo in code; $taxonmy -> $taxonomy. Also, you should use $tags = implode(',', $taxonomy['#items']) here:

posterous_spaces.module
 85 $taxonmy = field_view_field('node', $node, 'field_tags');
 86 $tags = '';
 87 foreach ($taxonmy['#items'] as $items) {
 88 if ($tags != '') {
 89 $tags .= ',';
 90 }
 91 $tags .= $items['taxonomy_term']->name;
 92
 93 }

Here you should put spaces around brackets and parentheses (for some reason the automated review does not catch this). Also, you should put the chained methods on db_update on separate lines so that it's easier to read. Finally, you do not actually do anything with the exception -- at the very least you should do a drupal_set_message().

posterous_spaces.module
102 try{
103 $curlpost = curl_init($posterousurl);
104 curl_setopt($curlpost, CURLOPT_POST, TRUE);
105 curl_setopt($curlpost, CURLOPT_POSTFIELDS, $values);
106 curl_setopt($curlpost, CURLOPT_RETURNTRANSFER, TRUE);
107 curl_setopt($curlpost, CURLOPT_USERPWD, $global_username . ":" . $global_passwd);
108 $status = curl_exec($curlpost);
109 $status = json_decode($status);
110 db_update('posterous_spaces_node')->fields(array('status' => serialize($status)))->condition('nid', $nid, '=')->condition('vid', $vid, '=')->execute();
111 }catch(Exception $e){
112 // Error can be cached and processed here.
113 }

Hi,
Thanks for reviewing caesius.

I am planing to implement the new API calls for import the blogs from posterous to drupal node.
At that time will fix the above issues.

Thanks,
SVN

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 :-)

links added