[D7] Share Link

Hi,

Please review the automated test error and fixed it.

http://pareview.sh/pareview/httpsgitdrupalorgsandboxharmonicnewmedia2784...

Also update the git clone URL with correct one.

Thanks
Dilip

Hi lionslair, what do you mean by short name?

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,

DrupalPractice has found some issues with your code, please review following URL:

http://pareview.sh/pareview/httpsgitdrupalorgsandboxharmonicnewmedia2784...

Thanks

Here's an initial review as requested.

1. module_load_include('inc', 'sharelink', 'sharelink'); You shouldn't use this in a global context, simply require_once the files if they belong to your module.
2. 'title' => t('Share Link'), - Don't use t() to translate title in hook menu.
3. A few of your menu callbacks have access TRUE, you should add a basic access callback or permission of some kind.
4. In sharelink_delete_multiple() i see you manually cleaning up files, if you had the correct file_usage entries i'd have expected those managed files to be cleaned up by Drupal?
5. The formatting of many files such as sharelink.form.inc isn't inline with Drupal's formatting.
6. Many of the functions are camel case such as _sharelinkGetAnnouncementsFromDatabase rather than Drupal's preferred sharelink_get_announcements_from_database
7. There is a lot of HTML string building, i'd recommend trying to refactor into theme functions with corresponding templates where possible.
8. $tz_perth = new DateTimeZone('Australia/Perth'); - Should this be picked up from the users/site timezone?

9. if (isset($sharelink_settings['license_key']) && !empty($sharelink_settings['license_key'])) {
   if (isset($sharelink_settings['graph_range']) && !empty($sharelink_settings['graph_range'])) {
   if (isset($sharelink_settings['stock_code']) && !empty($sharelink_settings['stock_code'])) {
   

   empty() does an isset() check as well so the isset() call is redundant.

10. $data = curl_exec($ch); - Consider using drupal_http_request()
11. return 'http:'. SHARELINK_SOURCE . _sharelinkGetLicense() . "/announcements"; - Looks like this builds a http url with the license key included? Can you use https?
12. In _sharelinkCreateUrlFriendlyFilename you are using _sharelinkShuffleString to md5 hash a part of the filename? Are you trying to hide something there, checkout drupal_hmac_base64() which is also URL safe.

13.   if ($return !== false) {
        return $return;
      } else {
        return false;
      }
    

    This entire chunk be replaced with return $return; Although, looking at the code above, it's initialised to array() so I don't see how it would ever be false anyway.

Hi lionslair

Automated Review

By PAReview.sh:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxnisith2696411git

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
No: Follows the guidelines for in-project documentation

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

(+) Please add hook_help

(*) - I found some issue about translate in t():
some cases have t(), and some cases don't have t()

 $html = '<p>' . t('If you move servers or change domain names please !contact_us to organise an updated license key.', array('!contact_us' => l(t('contact us'), 'http://www.sharelink.com.au'))) . '</p>';

  if ($json === FALSE) {
    $html .= "<div>There was an error checking the license. Please try again later.</div>";
  } else {

    $stock_code = $json->symbol;

- Should use base_path() instead "/":

<img src="/<?php print drupal_get_path('module', 'sharelink'); ?>/images/sharelink_uninstall.png" alt="sharelink uninstall" />

  <p>Then go to the uninstall tab and continue to uninstall. This will remove all components of sharelink on your site.</p>
  <img src="/<?php print drupal_get_path('module', 'sharelink'); ?>/images/sharelink_uninstall_remove.png" alt="sharelink uninstall remove" />

you can use:

<img src="<?php print base_path(). drupal_get_path('module', 'sharelink'); ?>/images/sharelink_uninstall.png" alt="sharelink uninstall" />

  <p>Then go to the uninstall tab and continue to uninstall. This will remove all components of sharelink on your site.</p>
  <img src="<?php print base_path(). drupal_get_path('module', 'sharelink'); ?>/images/sharelink_uninstall_remove.png" alt="sharelink uninstall remove" />

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 walk through are recommendations.
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.

Thanks
Nhan

@mrken201016: looks like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be "needs work"?

Hi @lionslair

http://pareview.sh/pareview/httpsgitdrupalorgsandboxharmonicnewmedia2784...

EDIT: removed long pareview.sh dump.

Please fix code review firt

Thanks
Nhan

@mrken201016: please don't post the full output of pareview.sh, it just clutters up the issue. A link is enough :)

Hello lionslair,

Reviewers here have done great work ,, and you followed that so I'll do a review to see if this is a RTBC.

Automated Review

Nothing :)

Manual Review

Individual user account

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

No duplication

[Yes:Does not causes] 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

[No: Does not follow] 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. / No: List of security issues identified.]

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

Have you considered renaming your project to something more specific? like ASX share link(ASX_share_link) it'd be better to know what the project is about? and what the share link about exactly even when users search for this in Drupal.
1. you could use the (configure = path to your project's configuration page) in your info file which helps a lot using your module your path I think is (admin/config/sharelink)
2. README file needs some enhancement .. I think Configuration section needs to be separated from Installation section which you dont have there.
3. good job on writing rich help tpl :) ,, but you could put those phrases in t() .. it would be perfect :)

I see no blockers here so I think this is a RTBC :)

Thank you

I will update 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!

Thank you, 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 go the dedicated reviewer(s) as well.