[D7] PHP Finder

Fixed the git clone URL in the issue summary for non-maintainer users.

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 @muthukumar sri,

Automated Review

No issues detected.

Manual Review

Based on https://www.drupal.org/node/1587704

1. Basic application checks

1.1 Ensure your application contains a repository and project page link.

• Your application does not have the link to the project. https://www.drupal.org/sandbox/muthukumarsri/2621270

1.2 Ensure your project is not a duplication.

No duplication.

2. Basic repository checks

2.1 Ensure the repository actually contains code.

Repo contains code.

3. Security Review

3.1 Ensure the project does not contain any security issues.

No security vulnerabilities.

4. Licensing checks

4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file.

Yes.

4.2 Ensure the repository does not contain any 3rd party (non-GPL) code.

Yes.

5. Documentation checks

5.1 Ensure the project page contains detailed information.

Yes.

5.2 Ensure the repository contains a detailed README.txt.

Readme does not follow the README Template.

5.3 Ensure the code contains a well-balanced amount of inline-comments.

The module lacks the inline comments, however what it's doing is pretty straightforward.

6. Coding standards and style

6.1 Run an automated review and ensure there are no major issues.

No issues detected.

7. API and best practices Review

7.1 Ensure you are using Drupals API correctly.

1. In your hook_menu() items' titles and descriptions don't use t() function.
2. You rely on text_with_summary field type but missing just text_long
3. Direct access to the field_config_instance table is not quite correct. Please check https://api.drupal.org/api/drupal/modules!field!field.info.inc/function/...
4. No need to execute extra query with countQuery() when you can immediately use fetchAll() or fetchCol() and proceed with iterating the results.
5. When you retrieve the info from field_data_FIELDNAME table you get the count of nodes, then iterate to get entity_id one by one. Not very effective - please have a look at https://api.drupal.org/api/drupal/includes!database!database.inc/functio...
6. Defining strings for one place usage like $search_php = 'php'; does not seem reasonable.
7. In php_finder_view_report() you check that views module enabled - use https://api.drupal.org/api/drupal/includes%21module.inc/function/module_...
8. Code in general looks very heterogenous, e.g. query usage uses chaining in some cases, but does not use it in other cases where applicable.

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.

Hi,

In Line 47, you can instead of querying the DB for getting node type, title and status, You can use node_load_multiple, as relying heavily on query is not a gud idea.

https://api.drupal.org/api/drupal/modules%21node%21node.module/function/...

You can also implement hook_help, so that users gets more idea about the module.

@manojbisht_drupal: I think the code only wants to query for nodes, so node_load_multiple() would be wrong here. An improvement would be to use EntityFieldQuery instead, but also not a hard requirement.

There is a security vulnerability in this module and as part of our git admin training I'm assigning this to th_tushar so that he can take a look. If he does not find anything I'm going to post the vulnerability details in one week. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Hi @muthukumar sri,

There is security vulnerability in the module, below are the line of code that may violate security,

In php_finder.pages.inc file, php_finder_node_report() function.

if (count($result) > 0) {
  foreach ($result as $result_node) {
    $op_label = "edit";
    $op_link = "node/" . $result_node->nid . "/edit";
    $op_path = l($op_label, $op_link, array('html' => TRUE));
    $rows[] = array(
      'data' => array(
        $result_node->title,
        $result_node->type,
        $result_node->status ? t('published') : t('not published'),
        format_date($result_node->changed, 'short'),
        $op_path,
      ),
      'class' => array('draggable'),
    );
  }
}

$result_node->title should be passed through filter_xss() or filter_xss_admin() function to sanitize the values fetched from the database.

$rows[] = array(
	'data' => array(
		$result_block->info,
		$op_path,
	),
	'class' => array('draggable'),
);

$result_block->info should be passed through filter_xss() or filter_xss_admin() function to sanitize the values.

$rows[] = array(
  'data' => array(
    $result_view->name,
    $result_view->description,
    $op_path,
  ),
  'class' => array('draggable'),
);

$result_view->name should be passed through filter_xss() or filter_xss_admin() function to sanitize the values fetched from the database.

Also, as the Drupal node data is being fetched from the database, it would be efficient to use EntityFieldQuery.

Changing the status to "Needs work". Please fix the above issues and and change the status back to "Needs review".

Hi,

I have enabled this module in my local instance, and I have placed Twitter Widget ID in it, but it is not loading my twitter.

Also please mention in your documentation, how to get twitter widget ID.

Hi,

Sorry for above comment, this was meant for another project.

Really sorry for the same.

@th_tushar: while filter_xss() would also be fine in this context I would recommend check_plain() instead. Node titles for example should never be interpreted as markup, so check_plain() for escaping is appropriate.

Fixed the git clone URL in the issue summary for non-maintainer users.

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

Hi, I reviewed your module and can make one notice: it's good practice to use function t() for text strings in the interface.

Timeout when invoking pareview.sh for https://git.drupal.org/sandbox/muthukumarsri/2621270.git at http://pareview.sh/pareview/httpsgitdrupalorgsandboxmuthukumarsri2621270git

Do you have any third-party files committed? 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access.

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

Silly bot problems, sorry.

Hi muthukumar sri,

Please fix the issues pointed in #15 of this issue.

Changing the status to "Needs work".

Fixed the git clone URL in the issue summary for non-maintainer users.

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

fixing tags

Hello,

Below are a few issues that I found:

Blockers:
1. On line 83/122/165 of php_finder.pages.inc, you use the variable $more, if no results are fetched from the database, this can result in an undefined error.
2. In php_finder.pages.inc there is some text, this text should be wrapped in a t() function to have them translatable.

Other issues / remarks:
1. Is there a reason you mix single and double quotes? I think it's good practice to always use single quotes.
2. I want a second opinion on this, but I would keep them lowercase, so they only get translated once (capitalize), then wrap in strtoupper() if you want to have them upper case..

    $header = array(
      'title' => t('TITLE'),
      'type' => t('TYPE'),
      'status' => t('STATUS'),
      'changed' => t('UPDATED'),
      'operation' => t('OPERATION'),
    );

3. All helper functions (non hook functions) should start with an underscore, followed by the module name.
4. I don't know the reason for the ;id in the info file?
5. Tip: Instead of setting $output and returning it at the end, you can remove every else and just return. Example (php_finder_view_report):

/**
 * Page callback for for find php in view.
 */
function php_finder_view_report() {

  // Check views enabled.
  if (module_exists('views')) {
    // Get views for using php_code format.
    $php_in_view = db_select('views_display', 'vd');
    $php_in_view->fields('vv', array('vid', 'name', 'description'));
    $php_in_view->leftJoin('views_view', 'vv', 'vv.vid = vd.vid');
    $php_in_view->condition('vd.display_options', '%' . db_like('php') . '%', 'LIKE');
    $result = $php_in_view->execute()->fetchAll();
    if (count($result) > 0) {
      foreach ($result as $result_view) {
        $op_label = "edit";
        $op_link = "admin/structure/views/view/" . $result_view->name . "/edit";
        $op_path = l($op_label, $op_link, array('html' => TRUE));
        $rows[] = array(
          'data' => array(
            filter_xss($result_view->name),
            $result_view->description,
            $op_path,
          ),
          'class' => array('draggable'),
        );
      }
      $header = array(t('VIEW NAME'), t('DESCRIPTION'), t('OPERATION'));

      return theme('table',
        array(
          'header' => $header,
          'rows' => $rows,
          'attributes' => array('id' => 'text-format-order'),
        )
        );
    }
    return "No views available";
  }
  return "Views not enabled";
}

For the rest it seems to be fine. But setting it needs work for the small issues.

Hi,

about 1, I looked better at the code, and it is fine, but still I would initialize the $rows array.

about 2, dont use t('ABOUT') instead, strtoupper(t('about')), translated strings are case sensitive.

This to avoid having ABOUT and about in the translation tables.

Removed one automated review.

manual review:

1. info file: package should be removed, see "In general, this property should only be used by large multi-module packages, or by modules meant to extend these packages, such as Fields, Views, Commerce, Organic Groups, and the like." from https://www.drupal.org/node/542202#package
2. php_finder_node_report(): instead of manual building your $result_mine array you could use array_merge() to make one result array where append all node IDs?
3. php_finder_node_report(): as I said filter_xss() is not ideal for node titles, because they are usually escaped with check_plain().
4. "$op_label = 'edit';": all user facing text must run through t() for translation. Same for "return 'No content available';" and elsewhere. Please check all your strings.
5. php_finder_node_report(): do not call theme() here, just return a render array. Drupal core will render it later for you. See https://www.drupal.org/node/930760 . Also elsewhere.
6. "db_like('php')": this call does not make sense, since there are no special characters in the static string 'php'. The db_like() call can be removed in this case.
7. php_finder_view_report(): this is vulnerable to XSS exploits. If I enter <script>alert('XSS');</script> as view description than I will get a nasty javascript popup on this page. You need to sanitize user provided text before printing, make sure to read https://www.drupal.org/node/28984 again. This is a limited vulnerability because it requires an attacker to have the "administer views" permission, but it should be fixed in any case.

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

Can you please explain what your module does that is not already covered by Security review?

Also: Please take a moment to make your project page follow the Project page template and it may be a good idea to also read tips for a great project page.
In particular, you must add the section "Similar projects and how they are different", making sure that it:

1. acknowledges the existence of similar projects (such as Security review); and
2. briefly explain how they are different.

And while you're at it, please take a moment to make your README.txt follow the guidelines for in-project documentation and the README.txt Template.

Manual review:
- * Several places use a variable that may not be defined.
- Other non-blocking feedback.

php_finder.pages.inc

php_finder_node_report()

  $nid_set = array();
  if (count($result_get_fields) > 0) {
    foreach ($result_get_fields as $result_field) {
      $table_name = 'field_data_' . $result_field->field_name;
      $field_format_name = $result_field->field_name . '_format';
      $get_node_ids = db_select($table_name, 'n');
      $get_node_ids->fields('n', array('entity_id'));
      $get_node_ids->condition($field_format_name, 'php_code');
      $result_get_node_ids = $get_node_ids->execute()->fetchCol(0);
      if (count($result_get_node_ids) > 0) {
        $nid_set[] = $result_get_node_ids;
      }
    }
  }

  // Merge all inner array values to the same array.
  if ($nid_set) {
    $result_mine = call_user_func_array('array_merge', $nid_set);
  }
  else {
    $result_mine = array();
  }

  if (count($result_mine) > 0) {
    // Removes duplicate node ids.
    $unique_nids = array_unique($result_mine);

Lines 32-48: I think combining the results of each query here could be done simpler. What about adding the IDs of the nodes into one array instead of using a multi-dimensional array for $nid_set? I don't think the array_merge() or the $result_mine variable would be necessary.

Line 49: Could you call node_load_multiple here instead of doing a custom database query?

Line 54:

    if (count($result) > 0) {
      foreach ($result as $result_node) {
      

It shouldn't be necessary to use count() on the array if using foreach - on an empty array it will skip the foreach.

Line 83: $rows may not be defined.

php_finder_block_report()

Line 121: $rows may not be defined.

php_finder_views_report()

$result_view->name is passed through check_plain() on line 151 but not 147.
Line 167: $rows may not be defined.

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.

manual review:

1. project page: you still have not explained the differences to https://www.drupal.org/project/security_review there. Please follow https://www.drupal.org/node/997024 for your project page.
2. "call_user_func_array('array_merge', $nid_set);": why do you need the call_user_func_array() here? Just call array_merge() directly?
3. The admin pages do not respect node access when querying the DB tables, but that might be ok in this case since you really want to always list all nodes I guess. See https://www.drupal.org/docs/7/api/database-api/dynamic-queries/query-alt...

Otherwise looks good to me.

Thanks for your contribution, muthukumar!

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.

Assigning issue credits.