[D7] dx_cache module

Hi danmatthews,

I cant access your git , please edit version control access at this page with the following:
Go to your project page , at the Version control menu select the version 7.x-1.x , check the Non-maintainer? checkbox and click Show.
Copy that and edit this page.

Also edit the description page of your module , is hard to have a code review without to know how and what does your module exactly.

PAReview.sh comes up clean (http://ventral.org/pareview/httpgitdrupalorgsandboxdanmatthews1864818git).

I did spot one issue: in several locations, you are using db_query("DELETE.... You should be using a query builder object with db_delete instead (see http://drupal.org/node/310081).

Also, one nitpick on your README.txt; you say

$myvalue = dx_cache_get('my.key'); // Returns the value if valid,
false if expired.

Perhaps you should also indicate that it will also return false if the key doesn't exist (just a thought; not a big deal).

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 :-)

Multiple Applications

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

Project 1: http://drupal.org/sandbox/danmatthews/1728120

Project 2: http://drupal.org/sandbox/danmatthews/1864818

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.

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

Manual Review:
1) Table name is wrong in dx_cache_cron() function.
$results = db_query("DELETE FROM {ox_cache} WHERE expires < %d", time());

Table names should be enclosed in {curly_brackets} in db_query in dx_cache_has function. Ideally you should use db_select() instead of db_query.

Will be good if you also write hook_help.

Regards,
Sadashiv.

Nice little tool.

• Coder Sniffer has found some issues with your code: http://ventral.org/pareview/httpgitdrupalorgsandboxdanmatthews1864818git

Automated review

Everything in order, no errors.

Manual review

1. Sync your README with the project page. You've got a lot of useful data there that is of great use for those just navigating to your module page.
2. I'd also advise to check_plain() the data, see Writing secure code.

Do NOT run check_plain() on data that you are storing, only run it when you output things.

"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." http://drupal.org/node/28984

Good idea Re README, but I think the module is generally RTBC.

I think you should change the priority to bump up and the status should remain needs review.

Refer application prioriy on http://drupal.org/node/894256

Project Review:

Automated Test results:

• No errors found from http://ventral.org/pareview/httpgitdrupalorgsandboxdanmatthews1864818git.

Manual Review:

• Project installed without errors via Drush installation of drupal + project.
• In this section of code:
  

      'expires'    => isset($ttl) ? (time() + $ttl) : NULL,
      'created_at' => time(),
  

  
  consider defining time() somewhere once before you set the values in the array. This way time() is atomically used once in this function and there is no chance that expires vs created_at are off by a second or two when using this library with alot of data, or lots of calls at once.

Eg,

  $timestamp = time();
  ...
    'expires'    => isset($ttl) ? ($timestamp + $ttl) : NULL,
    'created_at' => $timestamp,

... you're not doing any big work between these two time() calls, but this approach is more futureproof and atomic.

oops forgot to change status in #19.

What I would add is to use http://api.drupal.org/api/drupal/includes%21bootstrap.inc/constant/REQUE... instead of time(), however this is a really minor issue, plus the module is fairly simple.

I'd say this is ready to be published, marking it as RTBC.

Git default branch is not set, see the documentation on setting a default branch.

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

1. dx_cache_set(): instead of delete/insert you could us db_merge()?
2. dx_cache_schema(): primary key is missing.
3. README.txt: data should NOT be sanitized when saved to the database, only when it is printed to HTML on output. You should remove that and please read http://drupal.org/node/28984 again. "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."

But that are not application blockers, so ...

Thanks for your contribution, danmatthews!

I updated your account to let you 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 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.