[D7] Uncache

Automated Review

Please fix the pareview issues
http://pareview.sh/pareview/httpgitdrupalorgsandboxjimafisk2525998git

Manual Review

Individual user account

[Yes: Follows] the guidelines for individual user accounts.

No duplication

[Yes: Does not cause] module duplication and/or fragmentation.

Master Branch

[Yes: Follows] the guidelines for master branch.

Licensing

[Yes: Follows] the licensing requirements.

3rd party assets/code

[Yes: Follows ] the guidelines for 3rd party assets/code.

README.txt/README.md

[ No: Does not follow] the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

[No: Does not follow] the guidelines for project length and complexity.

Secure code

[ No: List of security issues identified.] Please follow this url : https://www.drupal.org/writing-secure-code

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. (*) Major finding, needs work

Please use db_select instead of db_query().https://api.drupal.org/api/drupal/includes!database!database.inc/functio...
Please use l() instead of <a> tags. https://api.drupal.org/api/drupal/includes%21common.inc/function/l/7

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.

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

This review uses the Project Application Review Template.

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.

The module has a bug with deleting pages which are not nodes. Otherwise this looks great. Posting review below.

Automated Review :
Coder found no issues with the project.

Manual Review:

Individual user account
Yes, Follows
No duplication
Yes,Follows
Master Branch
Yes: Follows
Licensing
Yes: Follows.
3rd party assets/code :
Yes, follows
Code long/complex enough for review
Yes, follows
Security Code:
Yes,follows

Automated Review
No issue.

Manual Review
Seems OK.

Hi jimafisk,

The module has a bug with deleting pages when site is running inside the folder(Not at root).

'remove' => l(
t("Remove this page from Uncache"),
url("/admin/config/development/uncache/delete"),
array('query' => array('path' => $path))
),

It should be :
Global $base_url;
'remove' => l(
t("Remove this page from Uncache"),
url($base_url."/admin/config/development/uncache/delete"),
array('query' => array('path' => $path))

Automated Review:
No problems found.

Manual Review:
Module works great! Code is clean and easy to understand. Nice work.

Next up in my queue.

Automated Review

Review of the 7.x-1.x branch (commit 3c0c5a0):

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

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. Very similar to CacheExclude, which is well established. However, duplication is not a blocking issue.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

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

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

(*) There is a security problem with this module. Assigning to @th_tushar for PAR security training.

Coding style & Drupal API usage

A seprate permission would be nice for this, so that site builders can assign this to non-admins.

In uncache_page_listing_table(), why would the table never exist if the module is installed? Comment needed.

uncache_page_listing_table(), you don't need to pass the $base_url to url(), that is the magic of that function.

In uncache_form_submit(), you don't need to plain the inputs, only when you render them. See https://www.drupal.org/node/28984

Your top-level logic for preventing caching will likely be problematic in the long run. That logic should be in a hook_init(), and use drupal_page_is_cacheable().

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.

Security problem is the blocker. If @th_tushar dosn't find it in a few days, I'll post it.

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

This review uses the Project Application Review Template.

Hi @jimafisk,

I have manually reviewed your code, found the below issues,

In uncache_admin_settings() function,

All user facing text should be passed through Drupal's t() function.

'#markup' => '<br /><br /><h2>Pages that are currently being Uncached</h2>', should be '#markup' => '<br /><br /><h2>' . t('Pages that are currently being Uncached') . </h2>',

In uncache_page_listing_table() function,

The values fetched from the database should be check plained in case of plain text or filtered in case of html content.

'alias' => $alias, should be 'alias' => check_plain($alias), as it is a path.

Use Drupal's Form API to remove the uncached pages from the list. This should be done using Drupal's confirm_form(). See Avoid cross-site request forgeries (CSRF).

Remove $base_url from url() function.

In uncache_delete_page() function,

This should be done using Drupal's Form API. Move the validation to form validation function.

In uncache_form() function,

'#title' => 'URL', should be '#title' => t('URL'), as it is a user facing text.

In uncache_form_submit() function,

$input = check_plain($form_state['values']['uncache_page']); should be $input = $form_state['values']['uncache_page'];

User input text should be saved as is, it should only be check plained or filtered while displaying back to user.

Move the validation logic to form validation handler function and use form_set_error() function to display form errors.

Below user facing text should be passed through t() function.
$alias = $alias == "" ? "not set" : $alias; should be $alias = $alias == "" ? t("not set") : $alias;

Hi jimafisk,

Just some performance optimizations not really bug:

1. on the admin page this function called twice uncache_get_all_uncached_paths which means it makes two DB selects, you could avoid it if you would use some caching in your function, for example:

function uncache_get_all_uncached_paths($force_load = FALSE) {
  static $result;
  if(!isset($result) || $force_load) {
    $result = db_select("uncache_pages", "uc")
      ->fields("uc", array("path"))
      ->execute()
      ->fetchAll();
  }
  return $result;
}

on second call it will provide the result from the static variable and so you can save one DB query, but if you still need always get the exact real result you can call your function with TRUE parameter.

2. You can save again another DB query if you get rid of this condition:
sites/all/modules/uncache/uncache.module:255

if (db_table_exists('uncache_pages')) {

I think it is unnecessary, because if once you have installed the module the table will exist. (disable module won't remove the table).

3. On frontend you could save code amount if you would move the admin related codes (forms, settings, etc) to an uncache.admin.inc file. In this way that parts would be included only on backend.

I hope my suggestions can help you to make your module better.

manual review:

1. "url("/admin/config/development/uncache/delete")": paths should not start with "/" for url().
2. uncache_confirm_delete_validate(): i think this validation function should be removed. I should always be able to delete any path.
3. uncache_confirm_delete_submit(): why is there a "nl" prefix in the path? Should that be removed?
4. uncache_form(): doc block is wrong, thsi is not hook_form(). See https://www.drupal.org/coding-standards/docs#forms on how to document form building functions. Same for uncache_form_validate() and uncache_form_submit().
5. uncache_form_validate(): uncache_get_all_uncached_paths(): why are you loading all paths here? It would be enough to just query for the one path? Same for the code in the global scope which is executed on every single page request - if you have many paths then querying for the current path makes more sense.
6. uncache.module: having code in the global scope is dangerous and makes the execution unpredictable. Instead, you should use hook_init() or a similar hook.
7. "$GLOBALS['conf']['cache'] = FALSE;": use the API function drupal_page_is_cacheable() instead: https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/drupal...

Although you should definitively fix those issues they are not critical application blockers, looks RTBC to me otherwise. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

Thanks for your contribution, Jim!

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.