[D7] OExchange

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,

I did not found issues on http://pareview.sh/pareview/httpgitdrupalorgsandboxvikramjeet2177409git

On Coder (https://drupal.org/project/coder) these items are the only ones I found. The last one I think it could be a false positive:

image_shift.js
File: @file block missing (<a href="http://drupal.org/node/1354#files">Drupal Docs</a>)

oexchange_share.js
File: @file block missing (<a href="http://drupal.org/node/1354#files">Drupal Docs</a>)

oexchange.inc
Line 49: Using the e flag in preg_match is a possible security risk. For details see http://drupal.org/node/750148
      preg_match("/\<title\>(.*)\<\/title\>/", $output->data, $title);

When I installed the module, this notice showed to me:

Notice: Use of undefined constant COMMENT_NODE_CLOSED - assumed 'COMMENT_NODE_CLOSED' in oexchange_install() (line 25 of /var/www/drupal7/sites/all/modules/contrib/oexchange/oexchange.install).

The constant COMMENT_NODE_CLOSED is from comment module, which I don't have it enabled. Appears to have a little dependence that does not interfere in the module functionality, and could be solved easily, with only a module_exists() verification.

Hi Vikram

1. In the file oexchange_share.js, you are referring to jQuery as $, while under the file image_shift.js you're referring it as jQuery. Is there any specific reason for this? If not, please change jQuery to $ in image_shift.js to maintain consistency. You can also do the other way round, but as per convention $ is used more than jQuery, so I would suggest the former.
2. Under oexchange_share.js, you're using this expression 7 times $('.drupal_share_button'). So, its a good practice to cache this in a variable like: var $drupalShareBtn = $('.drupal_share_button'); Then refer the above variable everytime instead of that expression.
3. Replace the expressions such as $(this).is('[description]'), $(this).is('[title]'), etc where you're trying to check if that attribute exists with a function which is specifically for that use case - hasAttr.$(this).hasAttr('title')
4. Also, please cache $(this) as well in a variable instead of creating an object every time. In your case, $(this) will result in the same result from this expression as well $('.drupal_share_button'). So $(this) can be replaced with $drupalShareBtn which I have expalined above.

Thanks
Raman

On second thought, point 4 can be invalid in certain cases, since there can be multiple elements with class .drupal_share_button on the page. But, you still must cache $(this) as var $this = $(this); so an object is not created every time.

manual review:

1. project page is too short. What does the module do? Does it add sharing buttons? Only to nodes? on all pages? what can be configured? See https://drupal.org/node/997024
2. oexchange_uninstall(): you don't need to foreach() over the DB result, just use fetchCol() on the result to get a whole column as array.
3. oexchange_share_form(): I guess you should also set a message like "Please login to share this link" if a user is anonymous? Why is the user_access() check performed here? That permission should be set in hook_menu().
4. PHP Notice: Undefined index: en in oexchange_node_view() (line 96 of oexchange.module).
5. oexchange_node_view(): do not use global $language_content, use entity_language() or $node->language instead.
6. oexchange_node_view(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS title');</script> as node title then I will get a nasty javascript popup. You need to sanitize user provided text before printing. Make sure to read https://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems. oexchange_share_form() looks a bit suspicoius as well, as you are printing stuff from the user supplied $_GET to HTML.
7. oexchange_node_view(): contains a lot of custom markup, this should be generated in a theme function, so that it can be easily overridden.

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

1. More info needed on project page.
2. Use 'access arguments' => array('share content'), in your hook_menu
3. Rename oexchange_admin_form.inc => oexchange.admin.inc to follow the psuedo standard naming standard for admin forms.
4. Consider using field_delete_instance in case any of the fields created by the module end up being reused on on non-oexchange node types.
5. Not all variables used in the module are removed in the hook_uninstall. i.e. oexchange_image16

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.