[D7] Backlinkseller

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.

Please fix the code issues, check below list of errors:

EDIT: removed pareview.sh blob.

@sandykadam: Please don't change the title on the projects you review! This is the second one I've seen that you changed recently. Also, please DON'T paste the entire pareview output here - link to it instead, like was done in #1.

@abogomolov: you have to set the issue status back to "Needs review" when you fix problems and want people to take another look at your project.

@abogomolov

1) In .module file in _backlinkseller_affiliate_block_content() can you use drupal standard functions to create link & image instead of hardcoding html tags, you can use l() check https://api.drupal.org/api/drupal/includes%21common.inc/function/l/7#com...

2) In .install file use drupal standard function for copy file_copy()
2.1) Again hardcode html tag of href use url() or l()
2.2) Use placeholder for strings in t()

3) In backlinkseller.inc file use standard way of coding to handle errors.
3.1) Use try/catch block instead of suppressing error by '@'
3.2) Instead of fsockopen use drupal_http_request().

if ($block_name === 'backlinkseller-affiliate') {
    $block = array(
      'subject' => '',
      'content' => _backlinkseller_affiliate_block_content(),
    );
  }

  if ($block_name === 'backlinkseller') {
    $block = array(
      'subject' => '',
      'content' => _backlinkseller_advertisements_block_content(),
    );
  }

- No need of === operator here, you can use simple equality operator here.
- you can use switch/case here instead of multiple if/else.
- Please fix coder module reports:-

Line 31: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_fapi_title]
    '#description'   => t('This ID you can find on the top of the Backlinkseller Code. (<em>$BACKLINK_SELLER["ID"] = "<strong>0123456789.9876543210</strong>";</em>)'),

In regards to backlinkseller.inc, you say you don't want to change it because it's "external code from the provider".

Drupal.org does not in general allow third-party code in the repository, and does not allow non-GPL code in the repository. If this is external code from the provider, like you said, then it should not be distributed with your project. Instead, it should be downloaded separately at install time and you should be using the Libraries module.

Regardless, naming the function the same as the module (just plain "backlinkseller") is a bad idea. It should be renamed to something more descriptive.

Likewise, using sockets to implement your own minimally-functional version of the HTTP protocol is a bad idea. What if, for example, a web site uses a proxy server? You have no proxy handling in your HTTP implementation, no redirect handling, etc. etc. You should instead be using drupal_http_request() or cURL for HTTP requests, as mentioned in #6.

function backlinkseller_help($path, $arg) {
function backlinkseller_menu() {
function backlinkseller_block_info() {
See last bullet point https://drupal.org/coding-standards/docs#drupal and use Implements hook_foo().

'type' => MENU_NORMAL_ITEM,
hook_menu defaults to MENU_NORMAL_ITEM so defining a type is not necessary.

For your hook_block_view, please use $delta as your function variable name: https://api.drupal.org/api/drupal/modules%21block%21block.api.php/functi...

Array formatting should follow this standard: https://drupal.org/coding-standards#array. This means all your hook_menu arrays, etc should be reformatted.

function _backlinkseller_advertisements_block_content() {
...
    $backlinks = _backlinkseller_fetch_links($config);
    if (!empty($backlinks)) {
      $result = variable_get('backlinkseller_html_before_ads');
      $result .= $backlinks;
      $result .= variable_get('backlinkseller_html_after_ads');
    }

There is no string sanitization for $backlinks. This could lead to lots of bad things, including XSS.

.install file:
copy($source, $target);
Consider using Drupal's File API or simply serving up the image from the module itself. https://api.drupal.org/api/drupal/includes%21file.inc/7

variable_set('backlinkseller_site_url', $base_url . '/');
This won't work on sites where the base isn't a slash. I see logic that assumes slash in several places of the module.

.inc file:

function _backlinkseller_fetch_links($config) {
...
    if (strpos($response->data, '<response>') === FALSE || strpos($response->data, '</response>') === FALSE) {
      $result = '<!-- INVALID_RESPONSE -->';  // <== Un-used variable.
    }
    
    $result = str_replace(array('<response>', '</response>'), '', $response->data);

Unused variable.

.admin.inc file:
Consider using https://api.drupal.org/api/drupal/modules%21system%21system.module/funct...

Very close.

admin.inc:

backlinkseller_settings_form()
  ...
      '#attributes' => array(
        'required' => 'required',
        'aria-required' => array('true'),
      ),

Are these necessary? I've not seen them used before and I don't see any consistency in the rest of the required form elements.

'#description' => t('This ID you can find on the top of the Backlinkseller Code. ($BACKLINK_SELLER["ID"] = "0123456789.9876543210";)'),
Move the example into a replacement so translation is clean.

Using system_settings_form() and backlinkseller_settings_form_submit() at the same time are in conflict.
Never reformat data when storing it i.e. check_plain() on backlinkseller_id. This should happen at page render time. Its part of the golden rule of data in Drupal.

$module_path = base_path() . drupal_get_path('module', 'backlinkseller');
    $img_path    = $module_path . '/assets/images/';
  
    $form['affiliate']['backlinkseller_affiliate_banner'] = array(
      '#type' => 'radios',
      '#title' => t('Banner'),
      '#description' => t('Choose a banner for the affiliate program.'),
      '#default_value' => variable_get('backlinkseller_affiliate_banner', 1),
      '#options' => array(
        1 => '<img src="' . $img_path . 'banner-1.gif" />',
        2 => '<img src="' . $img_path . 'banner-2.gif" />',
        3 => '<img src="' . $img_path . 'banner-3.gif" />',
      ),
    );

Use theme_image.

$installation_root = $_SERVER['DOCUMENT_ROOT'] . base_path();
Use DRUPAL_ROOT. See index.php for an example.
I still don't understand why the images can't be served up from the module itself. Why does it need to be copied to public files?

.inc:

  $result = NULL;
  $scheme = 'http://';
  $host   = 'channel8.backlinkseller';
  $tdl    = '.de';
  $port   = 80;
  $path   = '/channel/';
  $query  = array(
    'id=' . $config['ID'],
    'page=' . urlencode($config['PAGE_URL']),
    'ip=' . ip_address(),
  );

  if ($config['DEBUG_FLAG']) {
    $query[] = 'debug=1';
  }

Use drupal's url() function to build the url.

.module:

$image_base_path = base_path() . variable_get('file_public_path', conf_path() . '/files/');
    $url             = 'http://www.backlinkseller.de/?referrer=' . $affiliate_id;
    $image           = theme('image', array(
      'path' => $image_base_path . md5('backlinkseller') . '.gif',
      'alt' => variable_get('backlinkseller_affiliate_banner_alt', ''),
      'attributes' => array(),
    ));

Get rid of the extra whitespace. No need for an empty attributes array. Build the URL using url(). Actually, use l() to do the whole thing. The image path for the image shouldn't include base_path(). See theme_image.

$config['ID']         = variable_get('backlinkseller_id');
    $config['DEBUG_FLAG'] = (bool) variable_get('backlinkseller_debug_flag');

Whitespace.

    $request_uri = request_uri();
    if (substr($request_uri, 0, 1) == '/') {
      $request_uri = substr($request_uri, 1);
    }

    $request_uri = str_replace(variable_get('backlinkseller_site_url'), '',
      $request_uri);

    $page_url = variable_get('backlinkseller_site_url') . $request_uri;

Use url(). To get a link to the homepage, use .

  switch ($variables['block_html_id']) {
    case 'block-backlinkseller-backlinkseller':
      $variables['block_html_id'] = variable_get('backlinkseller_html_block_id');
      break;

    case 'block-backlinkseller-backlinkseller-affiliate':
      $variables['block_html_id'] = variable_get('backlinkseller_affiliate_html_block_id');

Pass the html id through drupal_html_id() to confirm they are unique.

I'm not suggesting you should remove ARIA. In fact, it makes sense to add. However, there wasn't consistency within the module for the use of aria-required. I only saw it on a couple of the required form elements.

I don't think that the system_settings_form() is needed if you have a submit handler.

$image_base_path = variable_get('file_public_path', conf_path() . '/files/');
    $url = 'http://www.backlinkseller.de/?referrer=' . $affiliate_id;
...
   $result = l($image, $url, array(
      'html' => TRUE,
      'attributes' => array(
        'title' => variable_get('backlinkseller_affiliate_link_title', ''),
      ),
    ));

I think this should use l() for creating the URL. l() internally calls url() so it accepts all the same parameters.

$request_uri = request_uri();
    if (substr($request_uri, 0, 1) == '/') {
      $request_uri = substr($request_uri, 1);
    }

    $page_url = variable_get('backlinkseller_site_url') . $request_uri;
    $config['PAGE_URL'] = $page_url;

Use url() to build a url. To provide a url to the homepage, use url('<front>')

Not required, but maybe good to note on the project page and the README that some search engines do not allow the use of link selling and therefore this would be a violation of their terms of service.

Moving back to needs work for conflicting calls to submit handler and system_settings_form() and use of url/l. The rest of the stuff isn't a blocker but highly encouraged.

I assume this url is a query string parameter. Use url to build up the query string, etc.

Marking as RTBC now that url is now used. However, there isn't any need to pass variables in the query string through check_plain since it gets urlencoded. I also saw some extra whitespace re-added in that last commit but that isn't a release blocker.

-    $image = theme('image', array(
+    $image           = theme('image', array(

Everyone can change most statuses. But only someone else should move from needs review to RTBC. PHPStorm (depending on the version) has built in support for Drupal. https://drupal.org/node/1962108

Still a couple coder findings @ http://pareview.sh/pareview/httpgitdrupalorgsandboxabogomolov1645208git-.... However, these are not blockers. Assigning to mpdonadio in case he gets a chance to do a final review.

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

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests?

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. You have to get a review bonus to get a review from me.

manual review:

1. backlinkseller_install(): no need to set variables on installation as you can make use of default values with variable_get() anyway.
2. backlinkseller_install(): I don't think you need to build $target, just use 'public://' . md5('backlinkseller') . '.gif';
3. "'#default_value' => check_plain(variable_get('backlinkseller_id')),": check_plain() is wrong here, since the form API #default_value element will automatically sanitize the value. Make sure to read https://www.drupal.org/node/28984 again.
4. "variable_set('backlinkseller_html_block_id', check_plain($form_state['values']['backlinkseller_html_block_id']));": do not run check_plain() here: "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database." from https://www.drupal.org/node/28984 . Also elsewhere.
5. _backlinkseller_fetch_links(): Doc block should not be in all caps, see https://www.drupal.org/coding-standards/docs#functions
6. _backlinkseller_advertisements_block_content(): usually this screams a bit XSS, but since the high level "administer site configuration" permission is required to input the variables in the first place, this is not a security issue and fine as is.
7. _backlinkseller_fetch_links(): "$result = str_replace(array('', ''), '', $response->data);": why is that necessary? Please add a comment.

The check_plain() usage is a blocker right now, you need to know when to use sanitization and when not. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Unassigning myself as klausi got to it before me.

@abogomolov , when you have made changes, set the status back to Needs Review. That is what tells the reviews that something is ready. I went ahead and did this.

XSS issues are still there.

Please see screenshots below:

Added following content on Setting page:
URL:admin/config/advertisement/backlinkseller/settings

Added following content to Affilates Page:
URL:admin/config/advertisement/backlinkseller/affiliate

Enabled both the Backlinker blocks:

Getting following two nasty JS popup. You need to sanitize this before rendering, make sure to read https://www.drupal.org/node/28984 again.

Adding PAReview: security tag to this thread. please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Assigning to myself for next review, which will hopefully be tonight.

Automated Review

Review of the 7.x-2.x branch (commit a663b91):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. You have to get a review bonus to get a review from me.

Source: http://pareview.sh/ - PAReview.sh online service

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

(*) No: Follows the guidelines for 3rd party code. Unless you can point me to a place where it says usage of banner-1.gif, banner-2.gif and banner-3.gif is OK, as it seems third-party (http://www.backlinkseller.de/).

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. backlinkseller_settings_form_validate(): To validate url, IMO a better option might be to use valid_url() to keep it all Drupal friendly :)
2. Instead of using substr() better to use drupal_substr().
3. '#default_value' => variable_get('backlinkseller_id'), setting default value would be good.
4. (+) backlinkseller_uninstall(): $_SERVER['DOCUMENT_ROOT'] still exists, rather use DRUPAL_ROOT.
   

   $target = $_SERVER['DOCUMENT_ROOT'] . base_path() . variable_get('file_public_path', conf_path() . '/files/')
               . md5('backlinkseller') . '.gif';

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.

I am not seeing any blocking issues except 3rd party code. All issues mentioned by other reviewers have been addressed. I also tested this module from XSS prospective and it is working fine. Comments and docs are also looks good now. Project page is very well organised.

It looks RTBC to me that I'll do after getting answer on banner images.

Thanks!

Changes looks good.

Thanks!

manual review:

1. _backlinkseller_affiliate_block_content(): the check_plain() is not needed here since theme_image() will escape the alt attribute with drupal_attributes() for you.
2. _backlinkseller_advertisements_block_content(): the first check_plain() here is not needed since you are not printing $backlinkseller_id to HTML. You should only escape variables if you are actually printing them. Same in _backlinkseller_fetch_links(), not printing $query to HTML, so no check_plain() needed.

But that are not critical application blockers, so ...

Thanks for your contribution, abogomolov!

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.