GRA4 Social Network for Drupal

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1911756
Project 2: http://drupal.org/node/1902954

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hello, trof!!

My notes

Notes here are from scanning the code

gra4_functions_callback.inc

  if ($str_current_signature !== $str_remote_signature) {
    // Not going to talk to such a server!
    die ('GRA4:Auth Failed|' . $str_remote_signature);
  }
  // If we here - SERVER KNOWS OUR SECRET KEY
  // GRA4_SET_SECRET
  // gra4 gives us new key.
  if (isset($_SERVER['HTTP_GRA4_SET_SECRET'])) {
    $str_set_secret = gra4_c_get_server_var('HTTP_GRA4_SET_SECRET');
    gra4_c_set_config_value('GRA4_secret', $str_set_secret);
    // Verify.
    if ($str_set_secret != gra4_c_get_config_value('GRA4_secret')) {
      die('GRA4:Unable to save configuration value GRA4_secret');
    }
  }

I don't know if die is the perfect function to call here.
If these are error messages you want to return, perhaps drupal_set_message would be enough. If you insist on using die(), take a look at drupal_exit().

gra4_functions_local.inc

function gra4_c_is_authenticated_user() {
  global $user;
  return ((isset($user->name) && ($user->uid != '0') && (isset($user->pass)) && ($user->status == '1')));
}

wouldn't user_is_logged_in() be good enough instead of implementing this function?

gra4_functions_local.inc

function gra4_c_is_local_admin() {
  global $user;
  if ($user->roles['3']) {
    return TRUE;
  }
  else {
    return FALSE;
  }
}

I see where you are going with this. You want to check if the user has the user role of administrator, right?
To make it easier to read, change it to this: if (in_array('administrator', array_values($user->roles)))

gra4_functions_web.inc

/**
 * Going to return $a_files_to_delete.
 */
function gra4_c_build_curl_post_data(&$a_files_to_delete) {
  // Here we going to keep formatted filenames to merge with POST  data.
  $a_file_ret = array();
  $a_field_names = array_keys($_FILES);
  for ($i = 0; $i < count($_FILES); $i++)
.....
..... 

Have you checked if the files api in drupal could help you in this function?

Good luck with your project! all the best.

/ adnasa

Changing status

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

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

Hi trof,

I've found a few things which I think will need fixing. The libraries stuff can be a bit of a pain so sorry about this but I think it's going to need to be done. I've been told off about the same thing before.

Use of third party code

I think you're going to need to use the libraries module to load up the GRA4 code that's provided by the GRA4 developers. It's against Drupal policy to use external code in a module so this is probably a strong requirement.

I did try to find where the GRA4 code comes from, but couldn't find that after a few minutes of google searching, so perhaps you could include a link to the repository in your README?

If you have the gra4 files in /sites/all/libraries/gra4/ then your libraries integration code would look something like

<?php
function gra4_libraries_info() {
  return array(
    'gra4' => array(
      'name' => 'GRA4',
      'vendor url' => 'http://example.com/',
      'download url' => 'http://example.com/',
      'version' => 'VERSION_STRING',
      'files' => array(
        'php' => array(
          'gra4_config.inc',
          'gra4_functions.inc',
          'gra4_functions_callback.inc',
          'gra4_functions_init.inc',
          'gra4_functions_local.inc',
          'gra4_functions_web.inc',
        )
      ),
    ),
  );
}

and then you would load them where required with libraries_load.

http://drupalcontrib.org/api/drupal/contributions!libraries!libraries.mo...

If you need to load a specific file rather than all of them at once you're probably going to need to set the gra4_libraries_info array up differently. See the documentation for details.

http://drupalcode.org/project/libraries.git/blob/refs/heads/7.x-2.x:/lib...
http://drupal.org/project/libraries

CSS Loading

I think the following section of code should probably go into an graf_init() function, unless you have a specific reason why not?
It might even be better off defined as a menu route using hook_menu, the purpose is just to serve CSS files from specific paths?

// Loading local CSS files.
$str_short_self = gra4_c_create_str_short_self();
$css_names = explode(',', $_gra4_global_config['include_css']);
for ($i = 0; $i < count($css_names); $i++) {

  if (gra4_c_get_server_var('REQUEST_URI') == $str_short_self . '/gra4' . $css_names[$i]) {
    $str_file_name = dirname(__FILE__) . $css_names[$i];
    $str_file_content = file_get_contents($str_file_name);
    header("Content-Type:  text/css;charset=UTF-8 \r\n");
    // Tell the user agent to cache this stylesheet for an hour.
    $age = 3600;
    header('Expires: ' . gmdate('D, d M Y H:i:s', time() + $age) . ' GMT');
    header('Last-Modified: ' . gmdate('D, d M Y H:i:s', @filemtime($str_file_name)) . ' GMT');
    header('Cache-Control: public, max-age=' . $age . ', must-revalidate, post-check=0, pre-check=0');
    header('Pragma: public');

    exit($str_file_content);
  }
}

Spelling errors

A minor point but worth fixing, eg my IDE flags up mis-spellings for attention so it's a little distracting having them pop up.
automaticaly
instalation
doublecheck (double check)
recomment
functionalty
instalaltion

Hi trof,

Your project contains several files with 3rd-party code. Your project must be free of any copyright statements. The 3rd-party code must be distributed as a library.

Where you implement hook_menu your doc block should say "Implements hook_menu()." Currently it says "Function gra4_menu()."

In line 28 of your module file you should wrap "Visibility settings" in a t() function.

You have at least four blocks of commented out code in the functions file that you can remove.

Do you need to include your functions file with every Drupal page load? That's what you are doing currently by calling require_once in your module file. If the file only needs to be loaded for the gra4 page, you could include it in hook_menu:

$items['gra4'] = array(
    'title' => 'GRA4 Social Network',
    'page callback' => 'gra4_get_gra4_content',
    'access callback' => TRUE,
    'expanded' => TRUE,
    'file' => 'gra4_functions.inc',
  );

The project fails automated reviews:

gra4_functions.inc

severity: criticalreview: security_12Line 138: the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead [security_12]

      $menu_path = mb_substr(gra4_c_get_server_var('REQUEST_URI'), mb_strlen($base_cms_path));

    severity: criticalreview: security_12Line 154: the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead [security_12]

        if (gra4_c_get_server_var('REQUEST_URI') == $str_short_self . '/gra4' . $css_names[$i]) {

    severity: criticalreview: security_12Line 224: the use of REQUEST_URI is prone to XSS exploits and does not work on IIS; use request_uri() instead [security_12]

      $str_params = mb_substr(gra4_c_get_server_var('REQUEST_URI'), mb_strlen($str_short_self));

    severity: criticalreview: security_0Line 382: Potential problem: trigger_error() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_0]

          trigger_error("FATAL ERROR: Unable to write to '$str_file_name' to save the configuration.  You have to change the permissions for '$str_dir_name' manually.", E_USER_ERROR);

    severity: criticalreview: security_0Line 390: Potential problem: trigger_error() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_0]

        trigger_error("FATAL ERROR: Unable to save configuration to '$str_file_name' . You have to change the permissions manually.", E_USER_ERROR);

gra4_functions_callback.inc

    severity: normalreview: style_function_spacingLine 47: Functions should be called with no spaces between the function name and opening parentheses [style_function_spacing]

        die ('GRA4:Auth Failed|' . $str_remote_signature);

    severity: normalreview: style_function_spacingLine 101: Functions should be called with no spaces between the function name and opening parentheses [style_function_spacing]

      exit ('GRA4:OK');

@trof,
I recommend you read the project application checklist: http://drupal.org/node/1587704
Especially see item 4.2. You have a lot of 3rd-party code in your module. If you don't take care of this issue, someone will likely remove your review bonus, since this issue has been pointed out a few times now in this thread.

The ventral.org site appears to be malfunctioning right now and that is why it is not showing you the errors. To save yourself time, I recommend installing the coder module because you can run it repeatedly more quickly than you can use ventral.org, and it doesn't require that you push files to the remote repository each time you want an automated review.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

I removed the review links from the issue summary that only contain automated review stuff.

Removed automated reviews.

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

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

manual review:

1. gra4_get_gra4_content(): $b_frame_needed is always TRUE? Why is it needed?
2. gra4_get_gra4_content(): doc block is wrong, see hook_menu() page router callback functions http://drupal.org/node/1354#functions . Same for many other functions that have useless doc blocks just repeating the function name. Please provide information what the function actually does.
3. gra4_functions.inc: do not execute gra4_c_callback() in the global scope, why is that needed?
4. gra4_functions.inc: *.inc files can never be called "over the web", since all Drupal web server configurations that I know only execute *.php files. So you can remove that.
5. gra4_c_fetch_web_data(): why do you need to cURL and cannot use drupal_http_request()? Also, if you depend on cURL you need to specify that in hook_requirements() like http://api.drupal.org/api/drupal/modules!simpletest!simpletest.install/f...
6. gra4_c_build_curl_post_data(): why do you unset $_POST? Please add a comment, that looks wrong?
7. gra4_c_set_config_value(): writing the config to the module directory is a very bad idea. It makes updating the module harder, since the folder cannot simply be replaced. And it is good security practice to write-protect code folders, so this will fail on many sites. Better but your configuration file into the private files directory that can be configured in Drupal.
8. gra4_c_is_local_admin(): the administrator role can be removed, so there is no guarantee that it exists in the Drupal installation. Also you should never check for a specific role, you should always check for a permission, for example "administer site configuration" is the highest level one.

I cannot say much about the rest of the module, since it does not really use Drupal's APIs and looks completely different to things that are usually created on drupal.org. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Add manual issue

I'm confused - the "library" seems to be specifically written for Drupal, so I think it should be developed on drupal.org. Aren't you affiliated with GRA4?

This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed (in this case there is not really much besides hook_help()). However, we can promote this single project manually to a full project for you.

But otherwise looks good to go to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to MiSc as he might have time to take a final look at this.

Trying to change tag again.

no objections for more than a week, so ...

Thanks for your contribution, trof!

I promoted this to a full project for you: http://drupal.org/project/gra4

Now that this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.

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 get 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.