Skip to content

Loading…

Page caching with querystring #2349

Closed
royduin opened this Issue · 19 comments

7 participants

@royduin

Page caching doesn't work proper with a querystring. If I've got this url: http://website.com/page and I've page caching enabled and I go to http://website.com/page?query=string then the cache from http://website.com/page will be loaded instead of with the querystring.

The solution is already given on StackOverflow: http://stackoverflow.com/questions/6194714/codeigniter-caching-issue-when-dealing-with-query-string-parameters the solution: http://www.okvee.net/articles/%E0%B8%9B%E0%B8%A3%E0%B8%B1%E0%B8%9A%E0%B9%81%E0%B8%95%E0%B9%88%E0%B8%87-codeigniter-cache-%E0%B9%83%E0%B8%AB%E0%B9%89%E0%B8%A3%E0%B8%B1%E0%B8%9A-querystring

Hereby I just ask to implement this in a coming version.

@bayssmekanique

Some developers have certainly come to use this as a feature, so the option to include query strings should not be implemented as a default, but rather as an option.

I can certainly envision many uses for this, and I use a modified Output class which performs a similar query string hashing, but I'm not certain if that is a supported use. Maybe a more experienced member of the community can clarify, but it has been my understanding so far that only slashed URI segments are supported in core classes, and that traditional URI query strings are left for the developer to handle. Any clarification on this topic would be helpful!

@devsathish

I was expecting this feature, after launching the app to prod we came to know about this behavior!! Looks weird!!

@ster ster added a commit to ster/CodeIgniter that referenced this issue
@ster ster add querystring to page caching. #2349 ac41ca6
@ster ster added a commit to ster/CodeIgniter that referenced this issue
@ster ster add querystring to page caching. #2349 6d61a05
@ster ster added a commit to ster/CodeIgniter that referenced this issue
@ster ster Fixed code style. Update changelog. #2349 af14e26
@narfbg

Resolved via #3384.

@narfbg narfbg closed this
@ghost Unknown pushed a commit to goreilly/CodeIgniter that referenced this issue
@ster ster add querystring to page caching. #2349 a9ba230
@ghost Unknown pushed a commit to goreilly/CodeIgniter that referenced this issue
@ster ster add querystring to page caching. #2349 3ee1771
@ghost Unknown pushed a commit to goreilly/CodeIgniter that referenced this issue
@ster ster Fixed code style. Update changelog. #2349 ab0465f
@vlakoff

What if $_SERVER['QUERY_STRING'] === '0'? It may happen. I'd suggest:

isset($_SERVER['QUERY_STRING']) && $_SERVER['QUERY_STRING'] !== ''

Then, it seems we could avoid code triplication by adding a method that provides the whole $uri, maybe even the full $cache_path.

@narfbg

Technically, you're right. But are you really concerned about a possible '?0' false positive? :)

@vlakoff

Yes :)

An app using a query string consisting of just a number is conceivable.

@vlakoff

Also, delete_cache() needs to be adjusted, as it receives the URI to be removed. Moving the new line like so should be fine:

if (empty($uri))
{
    $uri = $CI->uri->uri_string();
    empty($_SERVER['QUERY_STRING']) OR $uri .= '?'.$_SERVER['QUERY_STRING'];
}

Then, the improvements I suggested above would perfectly fit.

@narfbg

Sure, "conceivable" is why I said you're technically right.

But other than that ... PHP itself is not built to work with such query strings and CodeIgniter, even less so. For example, as far as the Input class is concerned, these are completely ignored. And when you narrow it down even more, to an application depending on an actual '0' value with nothing else in the query string (no sanity check must be assumed too) - you'd have to very carefully craft this behavior with the sole purpose of complaining about CI not supporting it. :)

@narfbg

Good catch with the last comment.

dfcca20

@vlakoff

Another thing, we should consider bayssmekanique's comment. Some developers surely depend on the previous behaviour (whether they were working-around it, or using it as a feature).

@narfbg narfbg reopened this
@narfbg

Right ... at the very least, it should be noted in the upgrade notes.

@vlakoff

What if someone floods with millions of different query strings to overflow the server hard drive? :trollface:

@vlakoff

My previous comment was serious :) Not only attackers, but also regular visitors. Think of hostings with only a few hundreds of MBs.

Considering this, and the above points, maybe we should make it opt-in? Something like this:

/*
|--------------------------------------------------------------------------
| Cache Recognize Query String
|--------------------------------------------------------------------------
|
| Set this to TRUE if you want to use different cache files depending on the
| URL query string.  Please be aware this might result in numerous cache files.
|
*/
$config['cache_recognize_query_string'] = FALSE;

Just some thought.

@ivantcholakov

I can figure out another way, but it needs performance tests.

The file name of the cached page can consist two md5 sums derived from the URI string (1) without and (2) with the query string, Then, before creation of a new file, the file system could be scanned for how many files are created under the first (1) md5 sum. If these files are more than 50 for example, they all can be deleted. After that the new particular cache file can be created.

This way is more intense about file system operations. Maybe it could be optimized somehow.

@narfbg narfbg closed this in a704aa7
@vlakoff

I think ivantcholakov is on the right way. Improving the idea, something like:

'/cache/' . sha1($uri_without_querystring) . '/' . $timestamp.'_'.sha1($querystring)

Then implement a rotation system, using glob(). For example see Monolog's RotatingFileHandler.

@narfbg

What's the point?

@vlakoff

Not letting anyone easily trigger write of hundreds of MBs on the server?

Or if the app appends some kind of query string that differ every time, and you have thousands of visitors. Each day.

It can break way too easily.

Long story short, if such a rotating system were written, would you merge it?

@narfbg

I'd be more concerned about the writes resulting in a DoS, not the storage. But sure, if you can write a sane patch - go for it.

@vlakoff

I had a look at it, and came across several solutions, but they are way too complicated. By design, this page caching mechanism should only be used for very simple cases. There're much better alternatives for more advanced caching needs. So, let's just move on.

@ghost Unknown pushed a commit to goreilly/CodeIgniter that referenced this issue
@narfbg narfbg Add 'cache_query_string' configuration option
Close #2349
449d48b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.