[D7] Simple tweets

Manual Review

You have many issues related to coding standards + some errors.

You can see the full report her:
http://pareview.sh/pareview/httpgitdrupalorgsandboxdpush2650716git

PS: your git clone command is "git clone --branch 7.x-1.x https://git.drupal.org/sandbox/D.push/2650716.git simple_tweets"

Automated Review

No Reviews Found

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

[Yes: Follows] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[Yes: Follows] the guidelines for project length and complexity.

Secure code

[Yes: Meets the security requirements.]

Coding style & Drupal API usage

No issues found.

Suggestion

1. Please mention that 'Twitter-Post-Fetcher' library required.

I am not getting any result in the block. Can you please explain why?

This review uses the Project Application Review Template.

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.

Hi,

The file which u have defined is not as per the drupal naming standard, like u have defined file with admin.inc under includes folder in module folder. While it should be somewhat like: simple_tweets.admin.inc

That alone is surely not an application blocker, anything else that you found or should this be RTBC instead?

Hi,

I think there is some issue with images in twitter widget, as I placed block for simple tweets,in sidebar first. It is loading fine, until I enable the checkbox for Show Images. as Images are coming out of div, and it is looking weird.

Hi @D.push,

I have manually reviewed your code, below are the findings.

• In .install file, it should be variable_del('simple_tweets_id'); instead of variable_del('simple_tweets__id');
• In simple_tweets_block_view()function, libraries_load('Twitter-Post-Fetcher'); should be below if ($delta == 't-w-block') { line, so that the library gets loaded only if the block is rendered on a page.

Changing the status to "Needs work". Please flip the status back to "Needs review" once the above issues are fixed.

Automated Review

pareview.sh found some minor issues. While these should be fixed, it is not a blocker.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) includes/lib_api.inc, line 30+: As a best practice, never place code outside of a function! If you really want this code to be executed on every page request, there are hooks for that (e.g. hook_preprocess_page() or hook_init(), just to name a few options). That said, the best approach in this case would probably be to either implement hook_requirements() or to only show this message in a few distinct places, e.g. on your configuration page (place it in the page callback of the configuration page) and when your block is shown (place it in your implementation of hook_block_view()).
2. (*) module file, line 8: Don't use require_once(). Instead, use module_load_include(). Also, as a best practice, do not include files outside of functions. Your includes/lib_api.inc just contains one hook implementation (and a few lines of code, which should be in another hook, see above). Why do you need a separate file just for this one hook implementation? As a best practice and for performance reasons, move your hook_libraries_info() implementation into your module file and remove your includes/lib_api.inc file.
3. (+) add.js, line 8: As a best practice, prefix your behaviour names with your module name to prevent naming collisions with other contributed modules or core.
4. (+) add.js, line 19: To facilitate localization of your module, please use Drupal.t() to make your string translatable. See Translating strings in JavaScript for more information.
5. (+) hook_block_view() and admin.inc: As a best practice, please provide sensible default values for all your variable_get() (i.e. use the second parameter). Also, I guess for some of the settings, there just aren't any sensible default values (like the widget id). For those variables, consider checking, if the user already configured a value, before you render your block.
6. (+) info file: Please remove the quotes from the package name.
7. includes/admin.inc: As a best practice, please name your include files like module_name.something.inc, i.e. simple_tweets.admin.inc in this case.
8. hook_help(): Why do you check for a README.md file, if you don't have one? Also, consider using drupal_get_path() instead of dirname(__FILE__) to get the path to your module directory. Since you don't use markdown, you can also get rid of the check for markdown module. A check_plain() or a check_markup() in combination with filter_default_format() before showing the output of your README.txt file might also make sense just as a precaution.
9. hook_block_info(): Since your block is showing the same output on every page, the default cache setting for blocks (DRUPAL_CACHE_PER_ROLE) doesn't make much sense. Consider defining a suitable cache level here by specifying the cache key in your block definition.
10. hook_block_view(): Instead of calling libraries_load() here, as a best practice, add your library using #attached like this: $block['content']['#attached']['libraries_load'][] = array(Twitter-Post-Fetcher');. Also, you might want to check, if the library is actually installed, before rendering the block.
11. hook_block_view(): As a best practice, put all your settings in a bucket with your module name (e.g. Drupal.settings.simple_tweets.id). That way, you could also add all the settings at once. Untested example:
    

           $block['content']['#attached']['js'][] = array(
             'data' => array(
               'simple_tweets' => array(
                 'setting1' => variable_get('simple_tweets_setting1', 'default value'),
                 'setting2' => variable_get('simple_tweets_setting2', 'default value'),
               ),
             ),
             'type' => 'setting',
           );
          

12. add.js, line 25+: As a best practice, instead of calling Drupal.settings.setting to get your settings, use the settings parameter to get your settings: settings.setting
13. add.js: To make your code work with other contributed modules, that use Drupal's AJAX API, make sure to only call your code once per block, e.g. by adding a processed-simple-tweets class to your block after processing and checking for this class before doing anything. Also make sure, that you only apply changes to that part of the DOM present in the context parameter.
14. add.js: Since you don't use jQuery, you probably don't need to pass this to the closure.
15. add.js: By looking at the documentation of TwitterFetcher, I can't find an option Permalinks. Should this setting be assigned to the showPermalinks option instead, which is always set to false now? If not, why not provide a separate setting in your configuration form for showPermalinks?
16. admin.inc: Why do you provide a separate configuration page for your block instead of just using hook_block_configure() to provide a configuration form for your block?
17. admin.inc, line 9: There is no hook_form(). Instead, this is a form builder function for your form.
18. admin.inc, line 15: Maybe add a sentence on how to obtain this id to the description or link to a suitable documentation page provided by Twitter? It might not be obvious for all your users.
19. admin.inc, line 93: It might be a good idea to sort your options alphabetically by value so that users can easier locate their desired language. Also, it would be a good idea to do this dynamically to sort by localized values instead of the English strings.
20. admin.inc, line 95: Why is the key for French in upper case when the rest of the keys is all lower case? Is this a typo? Also, why is French the default language? While I don't have anything against French in particular, I guess a good default value would be English or, better yet, the default language of the Drupal instance.
21. admin.inc, line 169: There is no hook_validate(). Instead, this is a validation callback function for your form.
22. admin.inc, line 180+: Consider wrapping this code in an else clause of your widget id validation above to only perform the HTTP request, if the widget id has a valid format.
23. admin.inc: Shouldn't Premalinks be Permalinks? If so, maybe also rename the variable before a stable release for clarity.
24. Please fix the remaining issues identified by pareview.sh (see above).

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Revision reviewed: cbd7ead

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Assinging to myself to review that probably be tonight.

looking at this now.

manual review:

1. simple_tweets_form(): as already said: it might make sense to use hook_block_configure() instead, so that people just configure the JS settings with the block.
2. Widget ID: where do I get it from? The description of the field does not tell me that?
3. When enabling the block I get the following error in my browser JS console: TypeError: Drupal.behaviors.simple is undefined. You need to replace "Drupal.behaviors.simple.tweets" with "Drupal.behaviors.simple_tweets". Make sure to test your widget next time before you push changes.
4. simple_tweets_form_validate(): doc block is incomplete, this is a validation handler. See https://www.drupal.org/coding-standards/docs#forms

Otherwise looks good to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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 (here you get to ask fans of your project to help you!)

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.

unassigning from myself.