Exchange Links

welcome,

I wonder if there's also a similar module doing this? (Could not find something after searching ~5min, maybe you could?)

Please take a moment to create a README.txt that follows the guidelines for in-project documentation.

Please take a moment to make your project page follow tips for a great project page.

It make's no sense to create a tag yet, you should only create tags if you're going to create a release (what is not possible yet).

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxanwarmax1428472git

You can also get a review bonus and we will come back to your application sooner.

regards

Hi,

Please see result of automated review from ventral pareview and try to fix errors and warnings.
On the link, you should "repeat review" before change back status of the task to "need review".

Manual review:
- Every text display to users should be in t() function as in exchange_linkspage.inc.
- Be aware that html code should not be in t() -> Eg: Line 19 in exchange_links.module.
- Every comments should end by a "." , "?" or "!".
- Remove unnecessary comments as at line 90 in exchange_links.install.

I installed your module, and went looking for the configuration page, and couldnt find it. then i looked at your code, and noticed that your main configuration settings page is a MENU_LOCAL_TASK.

  $items['admin/config/search/exchange-links/settings'] = array(
    'title' => 'Settings',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('exchange_links_settings_form'),
    'access arguments' => array('administer exchange links'),
    'type' => MENU_LOCAL_TASK,
    'weight' => 10,
    'file' => 'exchange_links.admin.inc',
  );

All you have to do is add separate menu item to be displayed on the configuration page, and then set the settings page as "MENU_DEFAULT_LOCAL_TASK" to make it be the first page that is shown.

  $items['admin/config/search/exchange-links'] = array(
    'title' => 'Exchange Links settings',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('exchange_links_settings_form'),
    'access arguments' => array('administer exchange links'),
    'type' => MENU_NORMAL_ITEM,
    'weight' => 10,
    'file' => 'exchange_links.admin.inc',
  );
  $items['admin/config/search/exchange-links/settings'] = array(
    'title' => 'Settings',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('exchange_links_settings_form'),
    'access arguments' => array('administer exchange links'),
    'type' => MENU_DEFAULT_LOCAL_TASK,
    'weight' => 10,
    'file' => 'exchange_links.admin.inc',
  );

Also, in the settings page you have the default text of the html code set as "

<a href="http://www.yoursite.com" title="Site Title">Site Title</a>

which comes from your .install file

 variable_set('exchange_links_html_code', '<a href="http://www.yoursite.com" title="Site Title">Site Title</a>');

This is also shown on the admin.inc file.

  $form['your_linking_details']['exchange_links_html_code'] = array(
    '#type' => 'textfield',
    '#title' => 'HTML Code',
    '#required' => TRUE,
    '#description' => 'Enter HTML code here. Example: <a href="http://www.yoursite.com" title="Site Title">Site Title&amplt;/a>',
    '#default_value' => variable_get('exchange_links_html_code', '<a href="http://www.yoursite.com" title="Site Title">Site Title</a>'),
  );

In your .pages.inc you have:

 $form['FPV_related_links'] = array(
    '#type' => 'item',
    '#markup' => '<div>&nbsp;</div>' . get_exchange_links() . '<div>&nbsp;</div>',
  );

...
/**
 * Implementation of get_exchange_links()
 */
function get_exchange_links() {

Your functions names should start with the name of your module to avoid namespace issues. So it should probably be something like :

function exchange_links_list() {

Manual Review :-

• exchange_links.module:- Line 186. It's not a hook, so you should use the @return syntax to describe the returned data of that function. Refer Doxygen and comment formatting conventions.
• exchange_links.install:- hook_uninstall, You should use variable_del, instead of db_query and then no need to clear cache as well.
• exchange_links.pages.inc:- function exchange_links_form(). Wrap #title, #description within t() function
• exchange_links.pages.inc:- function exchange_links_form(). Line 68 If there is a link use l() within t().

Thanks for your reviews, just make sure that you pick applications that did not get a review in a long time. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. Get a review bonus and we will come back to your application sooner.

manual review:

1. exchange_links_install(): no need to set variables on installation, as you can use default values with variable_get() anyway.
2. exchange_links_uninstall(): why do you need the delete query on the vocabulary table?
3. "'access callback' => TRUE,"; a path in hook_menu() is unprotected? Are you doing that on purpose?
4. exchange_links_form(): all user facing text must run through t() for translation. Also make sure to use placeholders for dynamically inserted variables.
5. exchange_links_form(): don't create table markup yourself, use theme('table', ...) instead.
6. "'#value' => 'Add my link',": again, t() missing. Check all your strings in your module.
7. exchange_links_get_exchange_links(): doc block does not make sense. This is not a hook, why the "Implements"? See http://drupal.org/node/1354#functions . Also elsewhere.
8. exchange_links_get_exchange_links(): don't create list markup yourself, user theme('item_list', ...).
9. exchange_links_admin_links_form_submit(): use drupal_write_record() (schema validation) here instead of db_insert().
10. Same in exchange_links_view_links_form_submit() instead of db_update().
11. "module_invoke_all('exchange_links_operations'": if your module invokes its own hooks your should document them in an *.api.php file. See http://drupal.org/node/161085#api_php
12. exchange_links_overview(): $roles is never used.
13. Please fix all doc blocks of your functions, they are useless right now.

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

Please add all your reviews to the issue summary, also the new ones.

manual review:

1. exchange_links_install(): just remove the variable_set() calls here.
2. exchange_links_uninstall(): The extra delete query on the taxonomy_vocabulary table is not necessary.
3. exchange_links_form(): doc block is wrong, this is not a hook, this is a form builder function. See http://drupal.org/node/1354#forms Same on exchange_links_form_validate() and others. Please check all your function documentation.
4. 'Play fair link back!': all user facing text must run through t() for translation. Please check all your strings. Same in exchange_links_form_validate().
5. exchange_links_overview(): The output of this function is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as site title or category I get a nasty javascript popup. This allows an attacker to pass malicious script code to the site administrator. You need to sanitize user provided input before printing it. See http://drupal.org/node/28984

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

Reviewed one more project.

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

manual review:

1. exchange_links_overview(): this is still vulnerable to XSS. Taxonomy term names are also user provided data and need to be sanitized.
2. Same for exchange_links_form(): all the user provided text settings retrieved with variable_get() should run through the appropriate sanitization function.

Security issues are blockers, but otherwise this looks nearly ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Added Manual Reviews of other projects

manual review:

1. exchange_links_form(): $link_category is only used on #options for a select form element, which sanitizes automatically. so you don't need the check_plain() on the items, double escaping is bad. See http://drupal.org/node/28984
2. exchange_links_form(): this is still vulnerable to XSS. The related links listing also includes the category which is not sanitized in exchange_links_get_exchange_links().

So unfortunately I need to bump this back one more time. Almost there! Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Added review links

This now seems to pass all tests at ventral

Took a look, and this seems like RTBC for me.

Thanks for your contribution, anwar_max! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.