[D7] Acquia Purge

I am SqyD / Paul Krischer, maintainer of Purge and I approve this module. This module will allow Purge 2.x to be vendor neutral and provide a solid tested base for anybody that would like to implement a platform specific purge solution that integrates with Drupal.

Typo

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

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.

Checklist from this page: http://drupal.org/node/1587704

1. Basic application checks

1.1 Ensure your application contains a repository and project page link: ok
1.2 Ensure your project is not a duplication: ok
1.3 Ensure you don't have multiple applications: ok

2. Basic repository checks

2.1 Ensure the repository actually contains code: ok.
2.2 Ensure you are working in a version specific branch: NOT OK

Although master branches are commonly used in the Git world, the Drupal community uses major version branches (e.g., 7.x-1.x, 6.x-1.x) instead, since master does not indicate what Drupal version the project is compatible to.

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

2.3 Ensure your project contains a minimum of handwritten code: ok

3. Security Review

3.1 Ensure the project does not contain any security issues: ok

4. Licensing checks

4.1 Ensure the repository does not contain a ‘LICENSE.txt’ file: ok
4.2 Ensure the repository does not contain any 3rd party (non-GPL) code: ok

5. Documentation checks

5.1 Ensure the project page contains detailed information: ok
5.2 Ensure the repository contains a detailed README.txt: ok
5.3 Ensure the code contains a well-balanced amount of inline-comments: ok

6. Coding standards and style

6.1 Run an automated review and ensure there are no major issues: NOT OK.
It appears you have some coding style issues: http://ventral.org/pareview/httpgitdrupalorgsandboxnielsvm1871170git. You should get them fixed.

7. API and best practices Review

7.1 Ensure you are using Drupals API correctly: My recommendations below:
File acquia_purge.install

• Line 38: when adding links, the drupal l() function should be used instead of writing the html markup.

File acquia_purge.module

• Line 35: you are getting the url's path by stripping the domain. A better approach would be using the php parse_url function: http://fr2.php.net/parse_url. Domains can have multiple forms, such as http://www.domain.com:8080/path and you might not be able to detect this using the current method.
• Line 379:
  

  $uri = sprintf('http://%s%s%s', $domain, $base_path, $path);
  

  Not sure if this is the best way of doing this. You should maybe use the output of parse_url or the drupal url() function instead. It's better for handling the trailing slashes or other problems.

• Line 174: Function _acquia_purge_get_domains_add_acloud(): You should probably try to figure out if there is a better way of doing that. You might not always have the permission to read through those vhost .conf files. In my opinion, a configuration page where the admin user cand enter the ServerName and ServerAlias would be a more robust approach.
• Line 239:

  $site = explode('.', $site);
  

  Again parse_url can be used instead of explode.

File: acquia_purge.tlds.inc

• Keeping the list up to date, although it seems like an easy task, at some point it might fail for someone ... It might be worth thinking of a more reliable strategy ...

File: acquia_purge.rules

• Function _acquia_purge_action_purge():
  

  /**
   * Action callback to the "Purge a path from Varnish on Acquia Cloud" rule.
   */
  function _acquia_purge_action_purge($configured_paths) {
    $paths = array();
  
  
    // As users can enter these paths, we are treating them as tainted input.
    foreach (explode("\n", $configured_paths) as $configured_path) {
      $path = array();
  
      // We split the trimmed path on / and reassemble the path to work around
      // starting or trailing slashes and other nasty things.
      foreach (explode('/', trim($configured_path)) as $path_indice) {
        if (!empty($path_indice)) {
          $path[] = $path_indice;
        }
      }
  
      // Reassemble the path and add it to the paths collection.
      $paths[] = implode('/', $path);
    }
  
    // Relay the purging of these paths to our API helper and we are done here.
    acquia_purge_purge_paths($paths);
  }
  

  Instead of exploding and imploding those paths, you can simply use something like this, in order to check for starting or trailing slashes.

  // Strip trailing slashes
  if(substr($string, -1) == '/') {
      $string = substr($string, 0, -1)
  }
  // Or a better idea.
  $string = rtrim($string, '/');
  $string = ltrim($string, '/');
  

PS: If you wish to speed this process up, there is this program: Review Bonus

Hi Niels,

I have attached a .patch file that addresses the coding style issues. You could maybe give me commit right for this module and next time I could just commit the changes.

I am currently using the 7.x-1.x-beta2-queuing-elb-support branch on our Acquia subscription, and it works for the things with which we need. I do have some comments and observations. To give some background, here is our setup:

www.example.com resolves to Akamai.
Akamai does origin pull from origin.example.com but it MASKS the origin with a host header change to "www.example.com"
Because the purge module must point to a domain on the balancer (i.e. it can't point to www.example.com), it has to be pointed to origin.worldvision.org which will leave any Varnish cache for "www.worldvision.org" uncleared when Akamai does its origin pull, thus giving Akamai an outdated Varnish cache.

The 7.x-1.x-beta2-queuing-elb-support branch has allowed us to mitigate this which clears both Varnish caches.

Some observations:

1. The queuing takes quite a bit of time, sometimes clearing up to 1600 items ten at a time. If I change this value, is there a max that the ACQUIA_PURGE_QUEUE_TRESHOLD should not exceed?
2. The queue will sometimes state a negative percentage (such as -20%) but I cannot seem to reproduce consistently. I did not see an open bug, and won't open one until I can reproduce consistently.
3. Each purge adds an individual log to Watchdog. That can be 1600+ entries every time someone hits publish. Any way to combine these into one log? (I know that would be difficult with queuing).
4. We plan on having Akamai caches purged at the same time for the same URLs, eventually. Any plans on integration with the Akamai module?

You have a couple minor errors listed here: http://pareview.sh/pareview/httpgitdrupalorgsandboxnielsvm1871170git.
Thanks for addressing possible duplication on your project page.
You should remove the master branch.
lol @ the function name _acquia_purge_are_we_on_acquiacloud() :)
Ouch, you have to grep through /etc/apache2/conf.d ? This is a very specific use module, and it's coming from Acquia, so if you're ok with it I am too.

Great addition!

----
Top Shelf Modules - Crafted, Curated, Contributed.

This will apparently also require an Acquia Purge Workbench Moderation module, as the message is sent when saving to draft or needs review. I'd consider this low priority, since it doesn't cause data to be lost or website visitors to get inappropriate content, but just wanted to get the issue recorded.

If I may respectfully suggest a wording change:

"Changes have been made that require pages to be refreshed from your caches and load balancers. Please wait..."

It appears this message is not AJAX-driven, so "Please wait..." could be confusing, leading to someone waiting for the message to change. Also, depending on who gets permissions (I anticipate showing this message to all publishers), some of this comes across as jargon. So, I suggest:

"There have been changes to content, and this content needs to be refreshed throughout the system. There may be a delay before the new content appears to all website visitors."

This message might need to be adjusted depending on whether it only shows to the user who triggered the action or to all users who are logged in at the time who have permission to see the message.

It's been a month since RTBC, and none of those look like blocking issues, so...

Thanks for your contribution, nielsvm!

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.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Just for findability, this appears to have moved to https://drupal.org/project/acquia_purge

Made the sandbox URL clickable.