Dynamic Cache

Hello, and thank you for applying for a CVS account. I am adding the review tags, and some volunteers will review the code, pointing out what it needs to be changed.

What is used as code for the proposed module could be used to create a custom index.php that would replace (for sites that needs it) the default index.php file Drupal comes with.

Yes, it's working well for us right now on two sites in which we need to be able to display pages that shouldn't be cached to specific anonymous users.

We implemented it last week and so far haven't seen any problems.

Thanks Brian!

I will report here a little checklist:

• The code needs to follow the coding standards; check in particular the format used for the control structures, and the name given to PHP variables, Drupal persistent variables, functions defined from the module.
• Files available from third-party sites should not be included within the module/theme. This is particularly true for files that are not licensed under GPL License v2, but it is also true for files that are licensed under the same license used by Drupal.
• The license file should not be included as well; the packaging script already include that file. In any cases, the code for modules/themes committed in drupal.org repository needs to be released under the same license used by Drupal; any compatible license is not allowed.
• Check the code passes the Coder validation.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

Rebooting an old thread. This module saved us several times, for example when trying to detecting incoming SSO via cookies, logged out as an anonymous user. Any more work on this? Would love to see a project page.

Would you mind if I create the project page? I have Git access with several other projects, and can clean up the module. Not sure how much future development it would need, but I could of course make you a co maintainer.

Bump.

This module does what it does very nicely. We use it on a site where depending on a particular session variable we want anonymous users to skip the cache. Prior to this module, we had to disable caching site-wide. Now our average response times are cut in half or less for anonymous users!

I'd like to see it available to the community. Let me know if you want assistance rolling this into a project.

Please see http://drupal.org/node/916552#comment-4147170 for how to move this forward.

brian_c do you just need to create a sandbox?

I am moving the issue to the right queue.

Hi,

The project is to small to get full project access:

• The project has less than five function or class method definitions
• The PHP/Javscript source code is less than 120 lines long (including comments and whitespace).

But you could apply just to get this project out as an full project. You could read more in this discussion: http://groups.drupal.org/node/195848

I did an automatic code review: http://ventral.org/pareview/httpgitdrupalorgsandboxbrianc1413374git - there some issues to take care about. Please feel free to ask if you have any questions.

Not sure what this could be combined into, but I use it on production to get around logging in SSO users.
My colleague borrowed this code to create a UserAgent cache bypass. Perhaps this module could be a generalized Cache ByPass module with a few components/uses.

You don't need to combine this module with existing ones, but you could not get full git access to create projects with a small module as an application, but you could try to have this promoted to a single project on a drupal.org.

Hi Brian,
I'm using this code in a large production already, and it works very nicely.
I've also created a sandbox project called "anonymous theme" that uses this code and would be very happy to see it published!
see:
http://drupal.org/sandbox/nevos/1507962

BTW: Here is the output of the Coding Standards sniffer...

dynamic_cache.module:2:1: error - You must use "/**" style comments for a file comment
dynamic_cache.module:2:1: error - There must be no blank line following an inline comment
dynamic_cache.module:5:1: error - Inline comments must end in  full-stops, exclamation marks, or question marks
dynamic_cache.module:5:1: error - There must be no blank line following an inline comment
dynamic_cache.module:12:1: error - Whitespace found at end of line
dynamic_cache.module:13:5: error - There must be no blank line following an inline comment
dynamic_cache.module:14:1: error - Whitespace found at end of line
dynamic_cache.module:19:22: error - There should be no white space after an opening "("
dynamic_cache.module:19:44: error - There should be no white space before a closing ")"
dynamic_cache.module:20:1: error - Whitespace found at end of line
dynamic_cache.module:21:5: error - There must be no blank line following an inline comment
dynamic_cache.module:22:1: error - Whitespace found at end of line
dynamic_cache.module:24:45: error - Whitespace found at end of line
dynamic_cache.module:31:11: error - BREAK statements must be followed by a single blank line
dynamic_cache.module:34:11: error - BREAK statements must be followed by a single blank line
dynamic_cache.module:47:1: error - Whitespace found at end of line

dynamic_cache.install:2:1: error - You must use "/**" style comments for a file comment
dynamic_cache.install:2:1: error - There must be no blank line following an inline comment

Though the code is very short and simple, it seems that having this available as a proper module/project makes good sense. This would allow other module developers that depend on this functionality to simply define a dependency instead of incorporating their own (possibly inconsistent or hacky) methods of conditionally disabling the cache. Having this as a project would also provide a "go-to" place for exploring this concept in D7. Perhaps this could even grow into a module that manages a wide-range of simple, yet low-level, cache controls.

I suppose there are few issues in the sandbox queue that might warrant attention before moving too far forward with all this. At the moment there are 2 bug reports (I just added one, but I think it has a very easy solution):
http://drupal.org/project/issues/1413374

FYI: I'm involved in a D7 upgrade that will require this module. #1515474: D7 version

Likely I'll add a patch there.

It's looking like there is certainly some demand for getting this functionality out there as a full project, and as I noted above, this is the kind of thing that other modules could nicely leverage as a dependency.

Given that this has been an open case for quite some time, I'd like to offer whatever help I can to get this module to full project status while also staying in-line with brian_c's intentions. For example, I would be willing to:

1. Setup a full project for this module via my own git access, and give initial commit credit to brian_c.
2. Setup brian_c as a co-maintainer. I believe this access can now be granted by an existing maintainer without the candidate going through a formal co-maintainer application process? I see that brain_c is quite active in the Drupal community, and if this module cannot serve as a full project application (see comments #18-22) perhaps the co-maintainer approach would be the most efficient way to get this done.
3. Do some cleanup (readme file, coding standards, doxygen, etc.).
4. Address pending issues in the sandbox issue queues (we already have an RTBC patch for the only outstanding bug report).
5. Port the module to D7 (we already have a leg-up on this based on the work in http://drupal.org/project/ua_cache_bypass).
6. Explore ways to grow this concept. For example, they may be some room to discuss a dependency relationship with existing contrib modules like User-Agent Cache Bypass and/or the possibility to consolidate functionality from such modules. At the moment, the core "dynamic cache" concept seems like the lowest common denominator functionality, so I don't think it can be merged into any existing project the leverages cache control on hook_boot.

Of course if brian_c wants to keep moving this through as an app for individual project git access, or grow the module to the point where this can be an app for full git access, that's all good. I just wanted to suggest an alternative that might accomplish the same objectives a bit faster.

As a user of dynamic_cache and maintainer of UA Cache Bypass, I'm on board with rjacobs in #27. Perhaps someone could reach out to brian_c?

Hi Brian,

The decision is totally yours! My guess is that if you are willing to run-through at least points 3 and 4 from comment in #27 within your sandbox, and if no other issues crop-up, this application can probably be marked as "reviewed". After that it would be a matter of seeing how long it would take for one of the folks with git admin access to see this, give it a final review, and then hopefully decide to grant project-specific access to you. My understanding is that the number of people with git admin access (the ability to setup git access that is not associated with an existing project) is very small, so I think getting their attention is the tricky part. Someone please correct me if I am wrong in this summary.

Having someone who already has git access start the project and give you co-maintainer access could essentially lead to the same result (you would be a project maintainer, with the same rights as any other maintainer). It just requires someone who's willing to personally help out and kick-start things for you, and you can see there are a couple such people out there.

My personal feeling is that if you are willing to see it through the whole way yourself you'll probably learn more about the process and community, and we'll all benefit from that. So I think that's a great approach. I just wanted to somewhat 'bump' the issue by offering my own help.

Looking forward to reviewing your sandbox updates!

Thanks Brian,

A few comments. These are mostly minor points, and I don't think they are deal-breakers to getting dynamic_cache set as an official project, but I think responding to some of these points might make it more likely that you could leverage this git application for proper git access, as opposed to just single-project git access:

• The testing functionality in indeed a nice touch, and one that I think is a great asset. Some thoughts I had specific to this concept:
• I'm wondering if the testing functionality needs to be it's own module? Perhaps this kind of thing could just be included within the main module, and if you want to be able to toggle it on-or-off you could expose a "debug" or "testing" configuration variable in the admin interface. I know that having another module allows you to implement a separate hook_boot (at a lower weight) where you can toggle the cache setting, but I think you could accomplish the same thing by checking for a test condition in the beginning of dynamic_cache_boot()
• For the testing you are using a cookie to setup a condition where the cache is intentionally bypassed. Could this same thing be accomplished by just checking for a query string parameter? I just imagine it might be too easy to set the cookie for a test and then forget to unset it, meaning the cache is always bypassed for that user. A $_GET check could streamline the testing code a bit too I suppose. I could see some advantages to using a cookie, but I think it would depend on other ways the "testing" concept might be leveraged.
• One thing that would really boost the utility of the test would be to make possible to see test results site-wide, and not just on a specific testing page. After all, the way developers use the dynamic_cache functionality can vary quite a bit, and they may want to test different dynamic contexts/conditions that should trigger a cache bypass across a site. Having the option to report the cache status (with something like drupal_set_message) on any page would be handy... subject of course to some kind of "debugging" option being enabled. Making a test status block available would also be an option, though a more involved one to implement I suppose.
• At some point you might want to explore the creation of simpletest tests (http://drupal.org/project/simpletest) for this module. This can be a bit involved, but it something some people love to see.
• You have a "package" defined for the module in the .info file (which is also the module name). I'm not personally too picky about these kinds of things, but I think you would need to have a larger project to justify this, see; http://drupal.org/node/231036.
• You have a git branch of 6.x-1.0-rc1. However, I believe the convention is that this should be a tag within a 6.x-1.x branch (see: http://drupal.org/node/1015226). For a sandbox project I don't think this is an issue, but looking forward, if the project becomes official you'll want to adhere to the convention for identifying releases, etc.

Thanks for posting your changes.

Great, thanks for the replies,

I think the bottom line is that you have given great consideration to performance and scalability in this effort (certainly more so than I in a review capacity), plus you have a good sense of Drupal structures and APIs and appear to be leveraging the community in your efforts. In my option that should speak even louder than whether or not you meet some hard limit on the amount of code included with the application (you probably meet that limit now anyway if counting the test logic). I personally don't see any reason why this can't be a full git access application.

You have also addressed some of the "housekeeping" issues (coder, readme, etc). The only exception might be choosing which "package" to assign in the .info, but hey, I don't think that needs to stand in the way of promoting this effort.

So I'd like to just mark this as reviewed. We know several others have tested the core logic behind this module (and are using it in production), so review attention has been thorough.

Some more specific replies appear below:

------------

Regarding the extra module, this probably is a fine case where having tests in a separate contained module makes sense. I don't know the true relative cost of an extra "if" check in hook_boot, but your justification makes me agree that it should be avoided.

In terms of the cookie vs query, etc. A query parameter could toggle the logic of the test, but you are correct in that it could lead to different cache results for a given page request. Anyway, having the extra check to confirm you are on the test page while checking the cookie really resolves things (the issues was pretty nominal anyway).

Also in terms of tests, if you are willing to explore the usage of a block to report test results I think that would really be the best case (but my vote is to save that effort until this app process is complete so we can get this thing out there).

For the package assignment, how about "Performance and scalability" (I think this is one of the best practice suggested examples). Really, this module is all about performance, as it's a way for people to generally keep caching enabled while still having a way to conditionally bypass it.

Lastly, regarding the git branching and tagging, I don't think there are any "rules" about how you assign branch names for your own working purposes, but when it comes time to make public releases several Drupal systems depend on certain naming conventions (http://drupal.org/node/1015226 explains it all best). All this is surely something you would have sorted out on your own (when setting up a release after getting this promoted), but I figured it was worth pointing out now.

Thanks!

I'll echo rjacobs on the package assignment "Performance and scalability" is probably the way to go.

Otherwise, count this as a +1 to RTBC, particularly echoing rjacobs' sentiments on leveraging community, and the use of this module in production environments.

manual review:

1. 6.x-1.0-rc1 is a git tag name and should not be used for branches to avoid confusion.
2. dynamic_cache_test_page(): all user facing text should run through t() for translation.

But that are just minor issues, so ...

Thanks for your contribution, brian_c! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.