[D7] Advanced Page Expiration

A few quick ones:

• You have a couple pa review errors: http://pareview.sh/pareview/httpgitdrupalorgsandboxsteel-track2280899git
• I see a bunch of variable_get and variable_set, however there is no .install file, which should variable_del any of these variables on uninstall.
• in hook_init:
  • You could use user_is_anonymous instead of the global user variable
  • You could use current_path instead of the $_GET['q']
  • May as well initialize max_age inside the first conditional since it won't be called unless it's met
• Same notes as above for several other functions
• Could use a bit more documentation for the hook implementations to say what they're doing.
• Missing README.txt
• If the varnish module is required, add it as a requirement in your .info (not sure if it would be)
• in ape_check_cacheable, you mention checks in order of cost. PHP actually short circuits logical operators, so you could put this into one return statement if you wanted.

Nothing that's really release blocking, but there's some notes. I use varnish a lot too so am interested in this module, I like the idea.

I second point 3, allowing cache-control headers in drupal_goto. In a high traffic environment that may have a lot of 'legacy redirects' that can't be deeper in Apache, it's good to have cacheability on 301's at least coming from Drupal.

Hi, thanks for your contribution.

Best practice mandates that you should not use raw $_GET or $_POST in modules. Use the Drupal API, read more here:
https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_ge...

Additionally it is poor practice for the Project Applicant to set RTBC themselves, for reasons I'm sure you can imagine. That aside, the RTBC status is set only after a thorough review and test has been conducted by one or more experienced developers and they or another experienced developer have deemed it ready to be RTBC. User "skh" provided only a "quick" review and unfortunately hasn't followed up.

It looks like this module is being ignored, sadly, and I suspect it's because only people with very specific setups will be able to test it. Is it possible for you to set up a server environment with a base Drupal install, and allow a git admin / Drupal reviewer to install the module and test it? Only suggesting this as it may help your project move along.

Cheers
Matt

Right, then that looks fine.

I would suggest doing 3 reviews of other modules (as it looks like you've got a good grasp on development) and then we can get "PAReview: Review Bonus" on this module, meaning it will get some attention from Drupal admins + better developers than I, hopefully who will move it past RTBC or provide some good feedback.

Hi,

Automated Review
there are still some outstanding errors.
http://pareview.sh/pareview/httpgitdrupalorgsandboxsteel-track2280899git

Manual review

README.txt/README.md
(*) No: Follows the guidelines for in-project documentation and the README Template.

hook_help() is missing in your module.

Great job so far.

Automated Reviews

The Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

ape.module

severity: normal review: style_control_spacing
Line 141: Control statements should have one space between the control keyword and opening parenthesis 

      if(!isset($max_age)) {

Line 144: Control statements should have one space between the control keyword and opening parenthesis [style_control_spacing]

      if(is_null($max_age) && !is_null($new_age)) {

ape.install
  @file block missing (Drupal Docs) [comment_docblock_file]

Manual Reviews

The README.txt file do not follow the guidelines for in-project documentation. Pleas check the README.txt Template for the guidlines.

Hi Trak,

Update your project git path on your created issue by the following.

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/steel-track/2280899.git

I'd be happy to RTBC this if the following are either addressed or have a mitigation.

2 minor nits

1. testApeHeaders has not declared visibility
2. Add a link to the module configure page from ape.info

public function testApeHeaders() {

configure = admin/config/development/ape

ape_page_delivery_callback_alter does not check on the status of the response.

Curling both a 404 response and a 403 response as an anonymous user gave a response of Cache-Control: public, max-age=60
I'm not sure whether this is the desired intention although at the same time agree that most reverse proxy accelerator solutions (looking at you Varnish) will be configured to:

• Disregard/cache for a specific time 404s/403s
• Passthru any requests with sessions which equivocate to logged in users.

With this in mind I was looking to see whether this was a known response that you had an understanding of and perhaps could add to the README so users were aware that it came with 403/404 caching too and that it was the responsibility of their reverse proxy to deal with that.

301/302 redirects aren't being cached.

The reason for this is because a drupal_goto() never makes its way through drupal_deliver_page() - this is pretty logical as we're just delivering a header() and telling the browser to look under another rock. Because drupal_deliver_page() doesn't get executed during the redirect step, the ape_page_delivery_callback_alter() function never gets called. This is what sets the header for reverse proxies and if it doesn't get called, the headers won't get added.

By altering ape_drupal_goto_alter() to the following, those headers get set and the reverse proxy will see them in their shining glory.

function ape_drupal_goto_alter(&$path, &$options, &$http_response_code) {
  $max_age = NULL;
  if ($http_response_code == 301) {
    $max_age = variable_get('ape_301_lifetime', 0);
  }
  elseif ($http_response_code == 302) {
    $max_age = variable_get('ape_302_lifetime', 0);
  }
  ape_cache_set($max_age);
  ape_set_cache_header($max_age);
}

I think you forgot to commit the ape_test directory/test module.

Really nice effort - your changes are solid, the module works and will be highly useful to many sites. Marking as RTBC.

No issues found here, nice work and some good suggestions from previous reviewers.

Thanks for your contribution, steel-track!

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.