Page 3 of 6

Re: [Beta] HTTP Guest Cache

Posted: Tue Feb 19, 2013 7:48 pm
by Kot Matroskin
Hi Haravikk,

I've got some questions related to latest version of your mod.

First of all, do you know that installation package contains some unnecessary files like ._mod_http_guest_cache.php and ._acp_mod_http_guest_cache.php? I don't know what are the files, suppose it is something from your MacOS. Apple likes to add trash anywhere.

Next one from functions.php.

Code: Select all

	if ($_SID == '' && $session_id === false && empty($_EXTRA_URL) && !$params_is_array && !$anchor/* BEGIN mod_http_guest_cache */ && (!isset($mod_http_guest_cache) || !$mod_http_guest_cache->is_cacheable())/* END mod_http_guest_cache */)
It is better to rewrite for improved comprehension and further modifying. Like below:

Code: Select all

	if ($_SID == '' && $session_id === false && empty($_EXTRA_URL) && !$params_is_array && !$anchor
/* BEGIN mod_http_guest_cache */ 
&& (!isset($mod_http_guest_cache) || !$mod_http_guest_cache->is_cacheable())
/* END mod_http_guest_cache */)
Your variable names are too long and too similar, because largest part of each variable is "$mod_http_guest_cache_...". Of course, that is your coding style and your choice, but... I'd changed it somewhere. ;)

Really, in some cases using of additional variables is unnecessary. See your code below:

Code: Select all

		static $mod_http_guest_cache_param = null, $mod_http_guest_cache_param_val = null;
		if ($mod_http_guest_cache_param === null)
		{
			$mod_http_guest_cache_param = $mod_http_guest_cache->get_param();
			$mod_http_guest_cache_param_val = $mod_http_guest_cache->get_param_val();
			if (is_null($mod_http_guest_cache_param_val))
			{
				$mod_http_guest_cache_param_val = 1;
			}
First of all I don't understand why you used "static" keyword here (and in other places within functions)? I thought it makes sense in classes only, for providing access without instantiation of the class.

So, you can re-factor it in the following way. Move your "configurations" variables (like $mod_http_guest_cache_param) into class private fields, initialize its in class constructor, return values from get_param() like you does it right now. So, anytime these properties will have correct values (calculated from settings or from their default values). So, your get_param() method will be simplified to the following:

Code: Select all

public function get_param()
{
  return $this->_cache_param;
}
and code above from functions.php can ever be omitted, because all initialization will be performed within the class. And you can use

Code: Select all

$params .= $mod_http_guest_cache->get_param() . '=' . $mod_http_guest_cache->get_param_val();
instead of current

Code: Select all

$params .= "$mod_http_guest_cache_param=$mod_http_guest_cache_param_val";
Also it will help you to avoid declaring global $config within each method of your class. It is necessary to declare it just once: in class constructor.

I can't understand some workarounds from the following method (and other similar methods):

Code: Select all

	function get_param_val()
	{
		static $param_val = 'null';
		if ($param_val === 'null')
		{
			$param_val = request_var($this->get_param(), -1);
			$param_val = ($param_val === -1) ? null : $param_val != 0;
		}
		return $param_val;
	}
Why do you need static here?

Why 'null' instead of null and isset() check, which is much faster than string comparison?

Why you've set value, but on next line inspects it (if ($param_val === 'null'))?

At a glance this method easy simplified to the following:

Code: Select all

	function get_param_val()
	{
		$param_val = request_var($this->get_param(), null);
		return isset($param_val) ? (bool) $param_val : null;
	}
Or ever you can simply write return request_var($this->get_param(), null);

Also you've checked cron by analyzing $run_cron variable. Not sure that it is correct, because it means that cron will work for registered users only. I did this check by the following:

Code: Select all

if (!isset($cron_type) || !$cron_type)
$cron_type is real variable contains necessary cron call if it specified. If not, the variable is empty or not set.

Re: [Beta] HTTP Guest Cache

Posted: Tue Feb 19, 2013 8:32 pm
by Haravikk
Kot Matroskin wrote:First of all, do you know that installation package contains some unnecessary files like ._mod_http_guest_cache.php and ._acp_mod_http_guest_cache.php? I don't know what are the files, suppose it is something from your MacOS. Apple likes to add trash anywhere.
Ah yeah, I thought I'd run the script to get rid of those but I must have missed something; it's safe to just ignore those and I'll try to remember and remove them in future, or upload a "clean" version of the archive later on.
Kot Matroskin wrote:Your variable names are too long and too similar, because largest part of each variable is "$mod_http_guest_cache_...". Of course, that is your coding style and your choice, but... I'd changed it somewhere. ;)
Ah, I mostly do that to make sure there's zero possibility of a clash in case another mod uses the same variable name for something else.
Kot Matroskin wrote:First of all I don't understand why you used "static" keyword here (and in other places within functions)? I thought it makes sense in classes only, for providing access without instantiation of the class.
Actually, static as a keyword existed before PHP added static variables for non-instantiated classes. In the context I've used it the static keyword refers to a variable that persists between calls to the same function, so the first time you call the append_sid() method those variables are set to null, but next time the method is called they have the correct values already stored for use. The saving's probably a bit minimal but it's one of PHP's handy little features that I like to use, see here for details, I think it might answer your other questions?

Basically it's a handy way to add a global variable to a single function/method, that's initialised first time round and then retains the value on subsequent calls.
Kot Matroskin wrote:Also you've checked cron by analyzing $run_cron variable. Not sure that it is correct, because it means that cron will work for registered users only. I did this check by the following:

Code: Select all

if (!isset($cron_type) || !$cron_type)
$cron_type is real variable contains necessary cron call if it specified. If not, the variable is empty or not set.
I think both solutions are about the same really; the $run_cron parameter of page_footer() specifies whether the test for cron-jobs should be performed in the first place, so setting it to false on HTTP cacheable pages seemed the right thing for me to do, it should have the same end result in practice I think?

Re: [Beta] HTTP Guest Cache

Posted: Wed Feb 20, 2013 6:18 am
by Kot Matroskin
Haravikk wrote:
Kot Matroskin wrote:Also you've checked cron by analyzing $run_cron variable. Not sure that it is correct, because it means that cron will work for registered users only. I did this check by the following:

Code: Select all

if (!isset($cron_type) || !$cron_type)
$cron_type is real variable contains necessary cron call if it specified. If not, the variable is empty or not set.
I think both solutions are about the same really; the $run_cron parameter of page_footer() specifies whether the test for cron-jobs should be performed in the first place, so setting it to false on HTTP cacheable pages seemed the right thing for me to do, it should have the same end result in practice I think?
Who will call cron jobs in case when only guests are online on your forum?

Re: [Beta] HTTP Guest Cache

Posted: Wed Feb 20, 2013 12:32 pm
by Haravikk
Kot Matroskin wrote:Who will call cron jobs in case when only guests are online on your forum?
I don't think that it should be a problem; no HTTP caching is performed on posting.php so any time someone changes something the cron job(s) should run normally. If no-one's posting then there isn't really anything that the cron-jobs desperately need to do anyway I don't think.

Re: [Beta] HTTP Guest Cache

Posted: Wed Feb 20, 2013 1:35 pm
by Kot Matroskin
Cron jobs perform much tasks. E.g. cleanups database, cache, performs other custom actions (configured by mods).

Re: [Beta] HTTP Guest Cache

Posted: Wed Feb 20, 2013 5:13 pm
by Haravikk
Kot Matroskin wrote:Cron jobs perform much tasks. E.g. cleanups database, cache, performs other custom actions (configured by mods).
I get what you're saying; I suppose if headers aren't sent by the time page_footer() has been called then I can still undo any cacheable headers that were set if a cron-job happens to be due, it's not the prettiest solution either but it should at least let the occasional cron job execute when pages are being refreshed.

Like I say though it shouldn't really make a difference to a board in practice, as someone must be logging in or posting now and then so cron jobs will still execute, otherwise the site is dead in which case it doesn't really matter :)

Re: [Beta] HTTP Guest Cache

Posted: Sun Feb 24, 2013 2:42 pm
by Kot Matroskin
Hi Haravikk,

I've modified Cache Guests Pages mod by adding ETag/If-None-Match http headers support. It means that user will get 304 Not Modified status for unchanged pages in their browser cache. Or obtains that pages from caching proxies.

I've removed support of your mod from my code, because I have to operate with same HTTP headers too.

Re: [Beta] HTTP Guest Cache

Posted: Sun Feb 24, 2013 3:25 pm
by Haravikk
Kot Matroskin wrote:I've modified Cache Guests Pages mod by adding ETag/If-None-Match http headers support. It means that user will get 304 Not Modified status for unchanged pages in their browser cache. Or obtains that pages from caching proxies.

I've removed support of your mod from my code, because I have to operate with same HTTP headers too.
It's great to hear you've added that support, though I'm not sure it specifically requires you to remove support; if the HTTP guest cache is installed (and enabled) then would it not make sense to just override ETag support?

i.e:
  • If an If-None-Match header arrives then ignore HTTP guest cache.
  • Otherwise, if page is cacheable (same check that added support to your mod before should be fine) then use $mod_http_guest_cache->set_headers(), otherwise set ETag.
Essentially the aim of HTTP guest cache is to offload a cacheable page entirely from the server (no requests get sent to the server at all so long as the cached file is considered fresh), so it would make sense to override ETag support since it still uses requests but tries to cut response time by only checking that a resource is still valid.

Obviously it's up to you if you want to include support in your mod yourself or leave it to me to provide an optional install for compatibility, but I still don't think the two mods are mutually exclusive.

Re: [Beta] HTTP Guest Cache

Posted: Sun Feb 24, 2013 6:14 pm
by Kot Matroskin
Your words are reasonable. My solution provides better page actuality (owing to write-through caching), but it requires call to server at the least for returning 304 status. Maybe someone will consider it as unnecessary load, who knows...

So, let's come to an agreement. You'll release stable version of your mod, and I'll include supporting it into my code. :) Hope it will force you to submit your sources to modification database. ;)

Re: [Beta] HTTP Guest Cache

Posted: Mon Feb 25, 2013 6:00 am
by mandrake88
Let me see if i understand properly how this mod work. If enter to the forum as guest, the 2nd time i will see an cached version direct from my local cache?

Re: [Beta] HTTP Guest Cache

Posted: Mon Feb 25, 2013 12:38 pm
by Haravikk
mandrake88 wrote:Let me see if i understand properly how this mod work. If enter to the forum as guest, the 2nd time i will see an cached version direct from my local cache?
Not just your local cache, but also any intermediate HTTP caches such as Squid proxy and (when setup correctly) CloudFlare. The aim being that so long as the cached copies are still considered fresh they can be served up to guests without any need to contact your server at all.


For general purpose caching Kot Matroskin's Cache Guests Pages mod may give bigger benefits overall, however my mod is what you need if you know your host has a suitable HTTP cache to take full advantage of it. You may of course still see improvement without one, since guests won't produce duplicate requests, and it can also help to throttle bots such as search-engine spiders, since most won't refresh a page that has given an expiration date until it has passed.
Kot Matroskin wrote:So, let's come to an agreement. You'll release stable version of your mod, and I'll include supporting it into my code. Hope it will force you to submit your sources to modification database.
I'm hoping to have a finished version soon, though there are a couple of extra features I need to finish, and I'm having some inconsistent results with CloudFlare that I'm trying to get resolved (some sites cache the pages fine once a custom rule is setup, while others never cache any of the pages regardless of headers and rules. I'm not sure if it's my mod's fault or something weird with CloudFlare, but I'm trying to get it resolved before I look at producing a final release :)

Re: [Beta] HTTP Guest Cache

Posted: Mon Feb 25, 2013 1:06 pm
by Kot Matroskin
I'm hoping to have a finished version soon
Let me know asap, also let me know if you'll have to change headers call format. I used the following, let me know if I'll need to change something:

Code: Select all

				/* BEGIN compatibility with mod_http_guest_cache */
				global $mod_http_guest_cache;

				if (isset($mod_http_guest_cache) && 
					$mod_http_guest_cache->is_cacheable() && 
					method_exists($mod_http_guest_cache, 'set_headers'))
				{
					$mod_http_guest_cache->set_headers();
				}
				else
				/* END compatibility with mod_http_guest_cache */

Re: [Beta] HTTP Guest Cache

Posted: Mon Feb 25, 2013 2:30 pm
by Haravikk
Kot Matroskin wrote:
I'm hoping to have a finished version soon
Let me know asap, also let me know if you'll have to change headers call format. I used the following, let me know if I'll need to change something:
That's perfect; I extracted my header changes into the set_headers() method so that I can keep it completely consistent even if I do need to make changes, they shouldn't have any impact on your compatibility code and should work without issue if my mod isn't installed.

Re: [Beta] HTTP Guest Cache

Posted: Thu Mar 21, 2013 10:19 am
by Kot Matroskin
Haravikk,

your 0.2.0 version doesn't work with latest version of [RC] Cache Guests Pages mod, moreover it causes fatal error by endless redirect with increased URL parameters string.

As far as I investigated root of the issue based in the following string in functions.php file:

Code: Select all

redirect(append_sid($current_page['page'], $current_page['query_string']));
First of all $current_page['page'] is not correct parameter for append_sid(), because it contains page URL with parameters, but instead it should be file name only. Try to use $current_page['page_name'] instead.

Described above may cause URL parameters increasing (e.g. index.php?param=1 redirected to index.php?param=1&param=1).

What related to continuous redirect, that was caused by hook_cgp.php functionality from CGP mod. This file removes SID for guests pages at the begin of append_sid(), so your code below never executed: $params .= "$mod_http_guest_cache_param=$mod_http_guest_cache_param_val";. Of course, may be I should modify my hook_cgp.php... but why not pass your &cache=1 in code below, instead of modifying append_sid() function?

Example illustrates what I mean (just example, requires to care about some small things like & and ? symbols):

Code: Select all

redirect(append_sid($current_page['page_name'], $current_page['query_string'] . '&cache=1' ));

Re: [Beta] HTTP Guest Cache

Posted: Thu Mar 21, 2013 11:08 am
by Kot Matroskin
btw, your installation still contains some unnecessary files like ._mod_http_guest_cache.php, ._acp_mod_http_guest_cache.php, etc. Apple products are evil. ;)