[D7] Social Timeline

Hey

It's worth having a look here: http://ventral.org/pareview/httpgitdrupalorgsandboxledzepp941937020git there's a few little things it's picked up.

I'd also encourage you to give us a bit more detail of how it will work, why people would want to use it.

Mike

Fixed the git clone code

Copy README.txt content to the Project page: http://drupal.org/sandbox/ledzepp94/1937020

The module installs variables on the database (on variable table). These variables should be deleted when the module is unistalled. You have to create a social_timeline.install file and add the hook_unistall() function. Example:

/**
 * Implements hook_uninstall().
 */
function social_timeline_uninstall() {
  db_delete('variable')->condition('name', 'social_timeline%', 'LIKE')->execute();
  cache_clear_all('variables', 'cache_bootstrap');
}

Hi
module features sounds great. I installed it successfully. I filled my twitter username in setting form and saved it than I enable the block of social timeline. But It's loading and loading nothing showing there. Please check the function call there.

For saving admin setting use system_setting_form. It's reduce your line of code.

Have a look at attachment, containing some formatting issue after coder review.

Once you correct them, assign it back for review. Also would suggest you to review other proejct application and get a review bonus.

Automated Review
No Problems!

Manual Review

• Needs more information on the Module Page. The contents of README.txt a really good, so you can copy them into your module page.
• Commenting is good!
• Is it possible to use variable_del() in the hook_uninstall implementation rather than the db_delete()?
• In the .info file I think the dependencies line is incorrect. It should just say libraries. To depend on a particular version I think you need to implement hook_requirements.

The .info file dependency is based on forcing libraries 2.x as seen here:

http://drupal.org/node/1183456

Oh! Cool! Then I have learnt something new!

I'll mark it as RTBC and then we have to wait for a committer to come and give it one final review I think. Fancy giving mine a review in the mean time? #1957164: [D7] Commerce Booking

Wrong permission usage is a blocker, see the automated review:

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/social_timeline.install
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   15 | ERROR | Whitespace found at end of line
  --------------------------------------------------------------------------------
  

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/social_timeline.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 8 WARNING(S) AFFECTING 8 LINE(S)
  --------------------------------------------------------------------------------
    19 | WARNING | The administration menu callback should probably use
       |         | "administer site configuration" - which implies the user can
       |         | change something - rather than "access administration pages"
       |         | which is about viewing but not changing configurations.
   119 | WARNING | #options values usually have to run through t() for
       |         | translation
   128 | WARNING | #options values usually have to run through t() for
       |         | translation
   129 | WARNING | #options values usually have to run through t() for
       |         | translation
   130 | WARNING | #options values usually have to run through t() for
       |         | translation
   630 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   637 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   638 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
  --------------------------------------------------------------------------------
  

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.

You can't set your own issue to RTBC.

Hi,
I went through your module and found some issues :

.module file
> line 519
Please create a separate .js file if there are some variables to be passed to jquery. You can do this by using
drupal_add_js(<path-to-js-file>, 'setting')
and collect your variables in js file by using Drupal namespace as in :
Drupal.setting.<your-variable-name>
inline option for drupal_add_js() is used for small functionalities. Please refer https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_add_js/7

> line 633
As a Drupal Practice, do not use $form['input'], instead use $form['values'], please refer http://pareview.sh/pareview/httpgitdrupalorgsandboxledzepp941937020git

I'll look at this now in the Project applications sprint.

----
Top Shelf Modules - Crafted, Curated, Contributed.

• You have a couple security problems in $form_state reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxledzepp941937020git. Using $form_state['values'] should be the same as 'input'.
• In social_timeline_uninstall() I don't think you need to clear the cache manually with cache_clear_all('variables', 'cache_bootstrap');
• In social_timeline_admin_form() you can use $limit = drupal_map_assoc(range(1, 100)) to create the option list.
• You can shorten variable_get('social_timeline', NULL); to just variable_get('social_timeline');, NULL is already the default.
• Can you break up social_timeline_admin_form() somewhat? That's a pretty long function. Perhaps some of the gets or formatting of data could be moved out?
• Use the multibyte-safe drupal_strtolower() instead of php's strtolower().

Setting to "needs work" for the security issue in $form_state, but otherwise this looks RTBC.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Looks pretty good to me. One thing I would recommend: Because the majority of your code is the admin form (and the submit handler and theme function for the admin form, etc.), I suggest putting all that admin code into a separate social_timeline.admin.inc, and declaring a 'file' => 'social_timeline.admin.inc' element in your hook_menu() so that code will be loaded only when the admin visits the admin form, and not on every single page load. Not a release blocker, but a very good practice you should follow to help minimize memory usage, make pages load faster, unclutter your module, and make it easier to understand and maintain.

Thanks for making those fixes!

----
Top Shelf Modules - Crafted, Curated, Contributed.

Thanks for your contribution!

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.