[D7] POWr Social Media Icons

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

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.

Hello, and welcome to the review process!
Looking at your user name, it looks like you are using the account on behalf of the organization. But please note drupal.org accounts are for individuals.

Take a look at the pareview results and correct them.

powr_social_media_icons.info
- A 'version' is not necessary. It will be added by drupal.org packaging script.
- block module is a required module by Drupal core. It's not necessary to explicitly add it as a requirement.

powr_social_media_icons.module
- I think it's perfectly safe to define the 'generate_powr_token' function without checking it first.
- Consider using hook_page_build to add the JS snippet. Even better, make the block contents a renderable array and add the the JS in #attached property.
- You can use DRUPAL_CACHE_GLOBAL for caching instead of per-role because the block isn't going to make use of permissions or make a difference on user role.

Hi mate,

Good work mate.
I did a review of your code and found the following issues (See below):

Issues:
1. GIT URL: The one that you have supplied is specifically for your login. Can you please change it to:
git clone --branch master http://git.drupal.org/sandbox/benross/2193167.git powr_social_media_icons

2. I also found that you were working directly off your master branch so decided to head over to pareview.sh to do an automated review. Please refer to the report below: http://pareview.sh/pareview/httpgitdrupalorgsandboxbenross2193167git
Can you please make fix all the errors as suggested by pareview before you change the status back to Needs Review.

3. LINE 42
if ($local_mode) {
$js_url = '/';
$js_url .= '/localhost:3000/powr_local.js';
}
Do we still need this? If yes, please include powr_local.js with module and modify code to load this file directly off module directory.

4. LINE 76 function powr_social_media_icons_help($path, $arg) {
Strings should be passed through t() functions for translations. Please head over to https://api.drupal.org/api/drupal/modules%21help%21help.api.php/function... for some examples

5. LINE 132 '#markup' => t('
Customize POWr Social Media Icons right in the page:

Just view your page to see the POWr Social Media Icons plugin. Click the settings icon to open the POWr Editor.'),
Please remove
tags out of t() method.

Thanks mate.

Great work and thanks for your contribution.

Cheers,
Ash

Hi there,
I'm sorry for not being able to participate actively since my first comment.
I see that you have done a lot of work, but some minor things need to be improved I guess. They indeed are minor things and not necessarily blockers IMHO.

powr_social_media_icons_help function

All of the current minor issues reported by pareview are from this function. PHP has heredoc and nowdoc that you can put a text that expands over multiple lines. Nowdoc is introduces in PHP 5.3, but Drupal 7 requires only 5.2.
If possible, consider using Heredoc for the help text.
Alternately, you could improve the t() function. Also, note that t() function has modifiers @, % and ! which respectively check_plain(), check_plain and wraps with em tags and leave the code as-is.

A few changes include:
<a target='_blank' href='http://www.powr.io/tutorials/how-to-add-social-media-icons-plugin-to-your-drupal-site'> " . t('Full tutorial here.') . "</a>
to:
t('<a target="_blank" href="@url">Full tutorial here</a>', array('@url' => 'http://www.powr.io/tutorials/how-to-add-social-media-icons-plugin-to-your-drupal-site')
It's perfectly fine to use simple HTML tags inside the t() function. See a lot of examples here.

Full snippet below (or here):

$text  = '<h1>' . t('Add POWr Social Media Icons to your site and edit right in the page.') . '</h1>';
$text .= '<br />';
$text .= '<h1>' . t('<a target="_blank" href="@tute-url">Full tutorial here.</a>', array('@tute-url' => 'http://www.powr.io/tutorials/how-to-add-social-media-icons-plugin-to-your-drupal-site')) . '</h1>';
$text .= '<br />';
$text .= '<h1>' . t('How to install:') . '</h1>';
$text .= '<ol>';
$text .= '<li>';
$text .= t('Enable the POWr Social Media Icons Module.');
$text .= '<i>' . t('Note: you may need to clear your caches after installing: Configuration->Development->Clear All Caches.') . '</i>';
$text .= '</li><li>';
$text .= '<strong>' . t('To use as a block: ') . '</strong>';
$text .= t('visit %structure -> %blocks and drag the %POWr to your desired block.', array(
            '%structure' => t('Structure'),
            '%POWr' => 'POWr Social Media Icons',
            '%blocks' => t('Blocks'),
            ));
$text .= '</li><li>';
$text .= "<b>" . t('To use within ANY post, article, or other content: ') . "</b> " . t('simply write the Shortcode in any text:') . "<b>[<i></i>powr-social-media-icons label=" . '"' . "your label" . '"' . "]</b>. " . t('You also have instant access to all other POWr plugins, full list at') . " <a href='http://www.powr.io' target='_blank'>POWr.io</a>";
$text .= '</li><li>';
$text .= t('View your site and your Social Media Icons will appear right in the page. Click the gears icon and you can customize right in the page.');
$text .= '</li>';
$text .= '</ol>';
$text .= '<br />';
$text .= '<h1>' . t('Examples') . ':</h1>';
$text .= '<img src="https://s3-us-west-1.amazonaws.com/powr/plugins/socialMediaIcons/screenshot-1.jpg" style="height:300px;" />';
$text .= '<img src="https://s3-us-west-1.amazonaws.com/powr/plugins/socialMediaIcons/screenshot-2.jpg" style="height:300px;" />';
$text .= '<img src="https://s3-us-west-1.amazonaws.com/powr/plugins/socialMediaIcons/screenshot-3.jpg" style="height:300px;" />';
return $text;

How the JS is added

I'm sorry even though you mentioned you used hook_page_build, I cannot see it used anywhere in the module.
Suggesting to use hook_page_build may seem like a personal preference, but there are few reasons behind this:

- hook_init is not executed on cached pages. drupal_add_js() calls won't be called in a form rebuild or when a page is served from cache (unless the page is statically cached, for example by the Boost module). Without adding the JS, the functionality of the module is not useful at all, right ?
- hook_boot is called on all pages, even the cached ones, but it's too early to make the necessary calculations.
- It's soon to be removed from Drupal 8. It's all about to render-able stuff and this approach is not really compatible with it.

hook_page_build or alter, in the other hand, solves all problems I mentioned above.

Even better, change the JS to look up the token from Drupal.settings, add the JS to a file and add in the module's .info file as a script. (Bonus: drupal will take care to not include the file multiple times)

Block view callback

Why implement hook_block_view_alter when you can do the same in hook_block_view ?

Thanks for the hardwork, Ben.

About that hook_page_build, I'm sorry being not specific about that. I actually meant to avoid drupal_add_js call and attach the JS using javascript.
So, the the following first snippet can be changed in to the second variant:

// Write js.
  $js = "(function(d){
    var js, id = 'powr-js', ref = d.getElementsByTagName('script')[0];
    if (d.getElementById(id)) {return;}
    js = d.createElement('script'); js.id = id; js.async = true;
    js.src = '$js_url';
    js.setAttribute('powr-token','$powr_token');
    js.setAttribute('external-type','drupal');
    ref.parentNode.insertBefore(js, ref);
  }(document));";

  // Add js.
  drupal_add_js($js, array('type' => 'inline'));

to:

// Write js.
  $js = "(function(d){
    var js, id = 'powr-js', ref = d.getElementsByTagName('script')[0];
    if (d.getElementById(id)) {return;}
    js = d.createElement('script'); js.id = id; js.async = true;
    js.src = '$js_url';
    js.setAttribute('powr-token','$powr_token');
    js.setAttribute('external-type','drupal');
    ref.parentNode.insertBefore(js, ref);
  }(document));";
  $page['page_bottom']['#attached']['js'][] = array(
    'data' => $js,
    'type' => 'inline',
    'every_page' => TRUE,
  );

Also change the function signature:

/**
 * Add powr js.
 */
function powr_social_media_icons_page_build() {

to

/**
 * Attach powr js.
 */
function powr_social_media_icons_page_build(&$page) {

It seems the master branch is still used in the repo, and no version-specific branches are there.
See this nice guide about that: https://drupal.org/empty-git-master
It was reported in the pareview since the beginning.

Also, a few general points:
- You could actually attach the JS to the block itself, but the actual use of hook_page_build or hook_init hook is because the widget can convert special tokens into widgets, which require the js snippet to be present on every page.
- Even though you'd feel fine when using a very short commit messages, when the module develops, people fill find the commit messages very useful.

Other than the master branch problem, I believe the module good enough to go to the "Reviewed and tested by community" status. I'm just enthusiast reviewing the project applications but Klousi or such administrator will need to review and approve this application.

I will definitely be here to do what I can do get this passed to a full project though. Good luck!

"Needs work", because of the master branch thing. Everything else looks ok to me.

Great work, Ben. Thanks a lot for your contributions.
I'm not a git administrator on drupal.org so all I can do giving my thumbs up for the code as it worked perfectly in terms of functionality and the code has had a huge improvement over past days and I believe in the well standard now.

I'm setting status to RTBC since ashishupadhayay also has reviewed this. Hopefully chx, Klousi or other admin will take a final review and promote your account to a full one so you can publish the code in the official way.

Thanks again and I wish you all the best!

PS: consider reviewing some other applications. After 3 review, tag this thread with PAReview: review bonus tag so administrators will give priority to this application.

Yeah, I will recommend going that path as well. Do a review on new projects based on your experience with drupal modules and then tag your post with PAReview: review bonus tag... Kluasi has decided to focus his time on project applications with a review bonus.

https://groups.drupal.org/node/214938

I will do a review this weeekend and if everything works as expected will give your module a thumbs up.

Thanks

Cheers,
Ash

Blocking API issues:

1. Please remove variables in a hook_uninstall(), like powr_token
2. Token names should be prefixed with the name of your module, so powr_token should be powr_social_media_icons_token

Non-blocking issues:

• You can remove .gitignore
• In README.txt, "the Blocks is" should be "the Block module is"
• Also in README, are these tokens meant to be different? [powr-social-media-icons label="your label"], [powr-Social Media Icons label='MY LABEL']
• I know other projects call them shortcodes, but in Drupal they're generally referred to as "tokens"
• In generate_powr_token(), you can use drupal_random_bytes() to create that string
• You really don't need to use powr_social_media_icons_page_build at all, that injects your library onto every page on the site. Instead call drupal_add_js() inside of hook_block_view_alter, then it will only be loaded when your block is being displayed
• In powr_social_media_icons_page_build(), you can use drupal_add_js() instead of a custom javascript to include your library

Ayesh's suggestion

change the JS to look up the token from Drupal.settings, add the JS to a file and add in the module's .info file as a script.

is the best route to go. If you don't want to include your js as a file, you can still use drupal_add_js() and Drupal.settings for much cleaner code.

Hey Guys,

Thanks for all the suggestions, very helpful.

I have an update ready for this but am having trouble logging into the account that originally created this module (password reset does not seem to be sending emails and haven't heard a response from help @ drupal). Is it possible for an admin to grant me access to update this module as ben @ powr.io?

Appreciate the help!

In the hopes of moving this forward in the meantime, I've attached the latest module .zip. Here are responses to the comments from kscheirer (thanks for your help!):

Updated issues
- This module can be used in conjunction with other POWr modules that share the same powr_token. So I think it is best to keep the powr_token even after uninstall. That is the only variable that will persist.
- Similarly to above, the powr_token can be shared among other POWr modules, so I think it is best to keep it as it.
- .gitignore removed
- Readme text changed to "the Block module is"
- Added language for "tokens" in addition to "shortcode"
- Now using drupal_random_bytes()
- The reason powr_social_media_icons_page_build is so that shortcodes/tokens can be used in any text (which the JS can interact with). So this is intentional
- Now using drupal_add_js()

Finally, if any admin is willing, I'd love to have a quick call to talk a little more about the POWr library in general, and what might be the best way to get it more accessible for Drupal users. The POWr library has been installed close to 1M times on other CMS platforms and I'd love to make it easier to find and use for Drupal users.

Manual Review

File: README.txt
Line exceeds 80 characters; contains 85 characters at 38

In response to kandy-io:

That readme character issue is a link which needs to be 85 characters, so as I understand that is ok.

I need a help in understanding.
So is it Javascript that replaces the powr tag with appropriate share tag? Isn't that a security issue? Someone can post a comment such as [powr-social-media-icons], and it suddenly changes to social icons?

If this is the case, I'm afraid this can be taken as a security risk. While no sane person would post such comment, it is still possible for anyone to do so. In cases like this, you need to add a new filter, and a processing handler so users can enable this filter only for permitted users, and replacements will happen only when absolutely necessary.

Also, regarding the powr_token variable: As far as I can see, you need this only to be unique for each site, and there is no real reason to store it in the database.
Use either or both of these:
drupal_get_hash_salt()
This value will be the hash of the $databases variable, so it is unique for each site installation, even if you copy the site to a different server.

drupal_get_private_key()
This is what you probably need. This value is stored in the database, and is unique for each installation. Value gets carried when you copy the site to other servers because this is a variable stored in the DB, just like your powr_token.

However, do not forget to use the hash of above return values. Google Analytics module recently had a security release because it exposed these keys.

Hi Ayesh,

Good to hear from you again. A few responses for you:

- The script can be prevented from replacing text in any section by adding a powrIgnore class. There is also no way that a user (for example someone posting a comment in your example) can actually post any content within POWr since they will not have access. We're live on a few hundred thousand sites with this scheme and it's never been an issue. So I think it's reasonable to keep as is, at least for now.
- Awesome idea to use the built-in keys. Uploading a new version using that.

Hope we can move toward getting this pushed live in the near future if possible. Many thanks for your help.

Best,
Ben

I will try my to provide another review. I'm sorry I do not have any rights to grant you with git access. I
m just volunteering to maintain the queues. I have personally met some drupal.org admins. I know how had they work. There is no way this post is not getting their attention. It just takes some.

Ok, thanks Ayesh, I really appreciate your time and insights.

Please update your git command:
From: git clone --branch master http://git.drupal.org/sandbox/benross/2193167.git powr_social_media_icons
To: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/benross/2193167.git powr_social_media_icons

I can not git clone your project because you have deleted master branch.

Hi ben@powr.io
In
powr_social_media_icons_help

Missing return statement, you should return empty string, or "not found page" to make sure you do not get any error.

It is not a blocker, just a minor issue, please fixed it if you have free time.

I'm sorry it has been a long time. If you can review other applications, 3+ would work best, you can get the Review Bonus. I know a few drupal.org admins personally, but I don't think it would be OK to call them outside drupal.org.

Your best bet probably is the Review Bonus. I used to review applications of others even before I had git access for my own account (I had no modules to publish openly at that time), and since I had more reviews, my application was approved fairly quickly.

Assigning to myself. I'm a GIT admin now, so I will be able to approve this application. First, I will do a manual review and if everything is OK, I will set the status to "Reviewed and tested by community". Applications usually stay a few days in this status, and then I will approve it .

Thanks for your patience.

Automated Review

Found some minor bugs http://pareview.sh/pareview/httpgitdrupalorgsandboxbenross2193167git
Please add spaces before and after Concat operator as mentioned in automated report

Manual Review

Individual user account

Yes: Follows

No duplication

Yes: Does not cause.

Master Branch

Yes: Follows. No Master Branch.

Licensing

Yes: Follows.

3rd party assets/code

Yes: Follows, no 3rd party code.

README.txt/README.md

Yes: Follows the template https://www.drupal.org/node/2181737

Code long/complex enough for review

Yes: Follows

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. (*) Please consider kscheirer suggestion in #18 to remove powr_social_media_icons_page_build().
   It cause exceeding external script loading on the pages where user don't need a social links block (as result - page load performance issue).
   For example, on my previous version of the site, I used to social icons only for contact page.

   I also suggest to replace powr_social_media_icons_block_view_alter() with powr_social_media_icons_block_view().

   So, the end code for .module could looks like:

   /**
    * @file
    * Module file for powr_social_media_icons.
    */
   
   /**
    * Help Page.
    */
   function powr_social_media_icons_help($path, $arg) {
   .. no changes
   }
   
   /**
    * Implements hook_block_info().
    */
   function powr_social_media_icons_block_info() {
   .. no changes
   }
   
   /**
    * Implements hook_block_configure().
    */
   function powr_social_media_icons_block_configure($delta = '') {
   .. no changes
   }
   
   /**
    * Implements hook_block_view().
    */
   function powr_social_media_icons_block_view($delta = '') {
     $block = array();
     if ($delta == 'powr_social_media_icons') {
       $js_url = '/';
       $js_url .= '/d1w86dhf197kq6.cloudfront.net/powr.js';
       $powr_token = 'drupal_' . sha1(sha1(drupal_get_private_key() . drupal_get_hash_salt() ));
       $js_url .= '?powr-token=' . $powr_token.'&external-type=drupal';
   
       // Actually write the content here:
       $block['subject'] = '';
       $block['content']['#markup'] = "<div class='powr-social-media-icons' label='Drupal_$delta'></div>";
       $block['content']['#attached']['js'] = array($js_url => array('type' => 'external'));
     }
     return $block;
   }
   
   

   Tested on my local instance, works fine.
   Marking as (*) because it is easy to fix before release and really important for site performance.

2. Minor code style issues reported by automated testing

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.

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.

@POWr.io, if you want to force your project to high priority review list, please do 3 manual review (as I did for your project and follow links in PA Robot #1 comment).

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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