[D7] SN FB Dynamic Cover Photo

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 Pgautam,

- Please consider providing blank line before and after every conditional and control statements block.
- If an array is increasing 80 characters in a single then consider each array key/value as a separate line.
- URLs like "https://graph.facebook.com/testsocialbank?access_token" should be a part of admin configuration to make future changes to URL without modifying the code.
- die("Access token couldn't be acquired."), is something which strictly should not be a part of your code however you may use drupal message API or watchdog to make aware with what happened with the request.

sn_fb_dynamic_cover_photo.admin.inc

- sn_fb_dynamic_cover_photo_admin_settings_form(), please parse #description in t() method.
- dropdown_first_value and dropdown_second_value, please change field names to something more understandable.
- Do not user CURL instead use drupal_http_request method to call remote URL.
- Watchdog message is without parsing using t() method.

.module file
- remove MENU_NORMAL_ITEM as this is default in drupal 7.
- Any HTML tag like "P" should not be there in code.

Please resolve these issues.

Thanks

Some of the code review comments:

sn_fb_dynamic_cover_photo.install:
• Line #44, 51 and 58: Field descriptions should be different

sn_fb_dynamic_cover_photo.module:
• Line #12 and 13: As per developer API, image dimensions should be 851px by 315px but MACROs that we used defined width as 850px and height as 316px.
• Lie # 100: Can we have a wrapper function for all curl requests
• Line # 292: We should have an error scenario handling in case json /result is not proper i.e. json_decode fails
• Line # 157: Unused variable
• Line # 236: Where are we using variable $dir_images_type
• Line # 268: Unused variable $album_name

sn_fb_dynamic_cover_photo.admin.inc:
• Line # 14: Unused variable
• Line # 83: Don’t share app id and secret as default value
• Line # 50: Instead of using text-field with readonly attribute, try to use markup. Readonly attribute can be a security issue
• Line #96: is it possible to show only those content type which have image field associated with them

I can see most of my review comments as fixed in latest checkin.

This issue is not fixed? See https://drupal.org/node/532400

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxpgautam2047401git

I'm a robot and this is an automated message from Project Applications Scraper.

My Review comments have been incorporated. Changing status to 'reviewed & tested by the community'

Thanks,
Gaurav

PAReview: 3rd party code

facebook.php, base_facebook.php, and fb_ca_chain_bundle.crt 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.

Or perhaps you could integrate with one of the many existing Facebook modules, I would assume at least one of these would provide the kind of FB connectivity code you're looking for?

If you require curl to be installed, please implement a hook_requirements() to check for that. I'm not sure about image_gd_save() - not all servers have GD installed and may have imagemagick instead? Not sure if that function would fail.
Don't use die() if possible, just return from the function, log an error with watchdog() and perhaps give the user a nice message.

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

Hi,

Manual review:
1) Function getaccesstokenfromdb() on line #204 in "sn_fb_dynamic_cover_photo.module" doesn't exist.
2) After removing above function error, I got below warning:
"Warning: imagejpeg(): Invalid 2nd parameter, it must a filename or a stream in image_gd_save() (line 272 of D:\xampp\htdocs\drupal\modules\system\image.gd.inc)."

Thanks

Added git repository and sand box link

Awesome!

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

manual review:

1. sn_fb_dynamic_cover_photo_schema(): all DB tables should be prefixed with your module's name, or abbreviation.
2. What does the "sn" prefix mean and why is it needed? Please add a comment on the project page.
3. sn_fb_dynamic_cover_photo_schema(): many descriptions are wrong.
4. sn_fb_dynamic_cover_photo_getaccesstokenfromdb(): words should be separated with "_" in function names.
5. sn_fb_dynamic_cover_photo_cron()): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075 . Also elsewhere.
6. sn_fb_dynamic_cover_photo_curl_request(): please add a comment why you can't use drupal_http_request().
7. "variable_get('sn_fb_dynamic_cover_photo_app_id')": all variables defined by your module need to be removed in hook_uninstall().
8. "'#description' => 'Image Field from selected node type',": all user facing text must run through t() for translation, please check all your strings.
9. sn_fb_dynamic_cover_photo_first_dropdown_options(): drupal_map_assoc() and array_keys() can be used instead of the foreach loop.
10. README.txt misses instructions about the Facebook library?
11. sn_fb_dynamic_cover_photo_include_fb_files(): the function drupal_get_path() will always exist, why the check?

Although you should definitely fix those issues they are not critical application blockers, so ...

Thanks for your contribution, pgautam!

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.