Closed (won't fix)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
12 May 2012 at 10:45 UTC
Updated:
18 Jan 2013 at 12:24 UTC
Introduction
============
posterous spaces module helps to publish the node contents to blog hosted at posterous.com.
currently node title,body,tags are posted to posterous.com
Drupal 7
project link :http://drupal.org/sandbox/svnindia/1573504
git link : http://drupalcode.org/sandbox/svnindia/1573504.git
Comments
Comment #1
patrickd commentedwelcome,
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
Comment #2
svnindia commentedHi Patrick
Thanks for your valuable suggestions...
Will do the changes and get the review bonus for the next review.
Thanks,
svnindia
Comment #3
pgogy commentedHello,
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
Comment #4
pgogy commentedNot sure if the bugs in the checkboxes are "needs work" or "needs review" so leaving at needs review
Comment #5
sashi.kiran commentedHi 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.
Comment #6
svnindia commentedpgogy and sashi,
Will check and do the changes you suggested..
Thanks for reviewing..
Thanks,
svn
Comment #7
svnindia commentedHi,
* The Bug in the node type checkbox fixed.
* The Coding style issues fixed.
Ref link : PAReview
Thanks,
svnindia
Comment #8
misc commentedManual review:
Comment #9
svnindia commentedThanks @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.
Comment #10
svnindia commentedComment #11
Milena commentedPlease, 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:
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.
Always use t() to output strings (of course not in hook_menu()).
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.
Comment #12
svnindia commentedHi 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.
Comment #13
svnindia commentedComment #14
caesius commentedWhere 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"
Typo in code; $taxonmy -> $taxonomy. Also, you should use $tags = implode(',', $taxonomy['#items']) here:
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().
Comment #15
svnindia commentedHi,
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
Comment #16
klausiClosing 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 :-)
Comment #16.0
klausilinks added