Serve css and javascript as separate, cacheable requests

[dreadnaut]

Originally reported on Google Code with ID 157

[Low priority and probably not now]

We can reduce page size and loading time by serving css and javascript in separate
requests. We can instruct the browser to cache these files sending the correct headers.
(Although the browser will still re-request them on a manual F5/Ctrl-F5 refresh.)

This might actually work for the favicon as well, but there might be better solutions
for that.

Attached is a small proof of concept.

Reported by dreadnaut on 2012-11-23 12:30:25

<hr> * Attachment: willcache.php

[Christopher Kramer]

Yes, seems to make sense, especially for the stylesheet which is comparably huge.

Would work for the favicon, but maybe serving it from google is a nicer solution here.
Google sets proper caching headers (esp. a good etag) so regarding caching, both solutions
work. And favicons are often cached by browsers even if the cache-tags are set poorly.
(They are often not even refreshed with CTRL-F5.)

The only problem I can see here at the moment is if somebody uses phpLiteAdmin under
https, a browser might warn that some resource (the favicon) is being loaded with normal
http. Not a big problem though, we could use:
https://phpliteadmin.googlecode.com/svn-history/r300/source/themes/phpliteadmin.ico
Google even uses SPDY and probably has quite good servers ;-) , so I guess this request
should be processed faster than a request from your own server.

Reported by crazy4chrissi on 2012-11-23 21:07:56 - Status changed: Accepted

[dreadnaut]

I can think of a few ways to implement this:

a) simple output of the file + headers for caching (Cache-control, possibly Expires)
   good: source code keeps the highlighting

b) file as strings, so we can also evaluate a hash of the contents to use as Etag

c) buffered output, so we can also evaluate a hash of the contents to use as Etag and
the source code keeps being highlighted :)

If we don't need Etags, a) is enough, but if we want to return that header too, I'd
like c) over b), although I admit output buffering would be there just to keep the
js/css sources outside a string, for highlighting.

Reported by dreadnaut on 2012-11-23 22:55:47

[Christopher Kramer]

Hmm... I don't see a reason why we should calculate an eTag using a hash.
There are lots of other good ways to come up with an eTag. But I'd consider "Last Modified"
a lot easier in this case, as php can simply use the last modified date of the phpliteadmin.php
file here.
I'd go for a) with
 Expires or Cache-Control (not both!)
+ Last modified (instead of eTag)

Alternatively, we could use the phpLiteAdmin version-number as a simple eTag, but this
would cause problems while development and if users use a development-version. Revision-Numbers
would be a very good eTag as well, but we'd need to make Subversion insert it automatically
(which imho is possible).

Reported by crazy4chrissi on 2012-11-23 23:40:34

[dreadnaut]

Here is a first patch which uses Cache-control and Etag with filemtime().

Note:
- highlighting breaks anyway (and in horrible ways), since the surrounding <style>
and <script> tag are gone; I used HEREDOC strings in this patch;
- I had to move around some initialization lines: we need PAGE defined, but we should
probably not start a session;
- overall, we remove almost 10KB from the final HTML source, about 40% of the home
page for example.

A bit disappointed by the loss of highlighting, hadn't thought about that :|

Reported by dreadnaut on 2012-11-24 00:48:17

<hr> * Attachment: resources.diff

[dreadnaut]

Here's the complete output() method, yesterday I forgot to compare etags.

    // outputs the specified resource, if defined in this class
    public static function output($resource)
    {
        if ($resource != __FUNCTION__ && method_exists(__CLASS__, $resource)) {

            // also use last-modified time as etag; etag must be quoted
            $etag = '"' . filemtime(PAGE) . '"';

            // check for headers for matching etag
            if (isset($_SERVER['HTTP_IF_NONE_MATCH']) && $_SERVER['HTTP_IF_NONE_MATCH']
== $etag) {
                header('HTTP/1.0 304 Not Modified');
                return;
            }

            header('Etag: ' . $etag);

            // cache file for at most 30 days
            header('Cache-control: max-age=2592000');

            call_user_func(__CLASS__ . '::' . $resource);
        }
    }

Reported by dreadnaut on 2012-11-24 15:57:01

[dreadnaut]

Reported by dreadnaut on 2012-11-27 20:13:14 - Labels added: Maintainability

[dreadnaut]

For the moment I've put this on hold: it has potential and I think we should include
it at a certain point.

However, due to the "string" problem which makes code less readable (no syntax highlighting),
I would wait until some way of using external files at least in debug mode (js mainly/
for css we have themes/ configuration?) is in place —which in turn should probably
wait for a more structured code.

Reported by dreadnaut on 2013-02-16 12:07:32

[dreadnaut]

Getting back on this, now that have external configuration. I'll add a method to specify
external files to be used instead of internal resources (js/css). With this, we can
add a couple of line to our local configuration:

Resources::useExternal('js', 'devel.js');

and work on the external readable source code. Once work is done, we store the minified
code as a string in the Resources class.

We should keep the non-minified source for js and css on the repository, where should
I put them?

Reported by dreadnaut on 2013-03-02 11:45:58

[dreadnaut]

Here we are, attached is a patch against r348.

I've used the YUI compressor¹ to minify the resources. I choose this tool because it's
cross-platform, works on both js and css and it's freely available².

Overall we trim ~5KB from the source code and ~10KB from the html sent to the browser
on each page load.

[1] YUI compressor docs:
http://yui.github.com/yuicompressor/

[2] Download from:
http://yuilibrary.com/download/ (stable, inside the YUI Build Tool)
https://github.com/yui/yuicompressor/ (development, under build/)

Reported by dreadnaut on 2013-03-02 16:20:48

<hr> * Attachment: resources-r348.diff

[dreadnaut]

Sorry for spamming this issue, but do you have a moment to a look at the patch above?
It's fairly large, so I'd like to commit it (or not, if it needs fixing) before too
much stuff changes and I need to rebase it :-p

Reported by dreadnaut on 2013-03-08 14:10:24

[Christopher Kramer]

Looks good. I just announced a feature freeze (https://groups.google.com/forum/?fromgroups=#!topic/phpliteadmin/zMuAA6agfAQ),
which would mean this should be committed into an 1.9.5 branch. But if you commit it
soon, I think it's also okay if you commit it into 1.9.4 as we still have some time
for testing.

Only problem I see is that it makes css and Js very hard to change. So we need be make
sure this does not decrease maintainability.

But I think for the 1.9.5 branch, we should split the phpliteadmin.php file into multiple
files for development and write a build-script that bundles everything back into one
file anyway. This build script then could do the JS/CSS-compression from a standalone
file which is not compressed.
This would be another argument for introducing this in 1.9.5.
I think there is no ticket for the split & build idea yet, although I have it in mind
for some time. I guess I need to open one.

Reported by crazy4chrissi on 2013-03-09 00:24:59

[Christopher Kramer]

Ah. I think the use of Heredoc syntax for static class variables has been added in PHP
5.3.0:
"As of PHP 5.3.0, it's possible to initialize static variables and class properties/constants
using the Heredoc syntax"

So I guess this might not work with PHP 5.2, which we still support. Have you checked?

Reported by crazy4chrissi on 2013-03-09 13:00:21

[dreadnaut]

heredoc: argh, it doesn't :|  I'll work around it, no big deal.

Re your post #11: js/css is not much difficult with the latest patch, since one can
add two lines in the config file and work on external files.

Resources::useExternal('css', 'devel.css');
Resources::useExternal('javascript', 'devel.js');

Of course, this will be better with a split file + build system.

I'll fix the heredoc issue then, add an option for base64, and post a final patch later
today.

Reported by dreadnaut on 2013-03-10 10:37:43 - Status changed: Started

[dreadnaut]

Here's the 5.2 compatible version as a patch agains r352. It can still be improved upon,
but it should be fine for 1.9.4 —I'll have to change it anyway once we split the code
in multiple files.

As soon as I commit this, I can finalise the patch for Issue 185.

Reported by dreadnaut on 2013-03-10 22:15:01

[dreadnaut]

Sorry, forgot the attachment.

Reported by dreadnaut on 2013-03-10 22:15:43

<hr> * Attachment: resources-r352.diff

[Christopher Kramer]

Okay, thanks. Some things I noticed:

- useExternal() is not used at the moment, and I wonder how it would be used. Because
you would need to call it before Resources::output() so it makes sense. (Because $_resources
is not kept across two requests). Of course possible, but at the moment we check for
stuff like $theme below.
- you seem to like "echo $var1, $var2" instead of "echo $var1.$var2". Although this
is fine, I would prefer to keep things consistent and at the moment, string concatenation
is used everywhere instead of multiple echo parameters. (Please not another micro optimization
discussion here, if you think that's worth it start another issue and we can discuss
all the micro optimization stuff there.)
- you prefix $_resources with "_" because it is private (?), which is fine but we also
should do this consistently if we want to do it. At the moment we don't, like in the
Authorization class. And if we do it, we should also do it for methods I think. And
I am not sure if this still is common coding convention, I used to do this at a time
when "private" didn't exist in php yet :-D
- the approach with the user-function-call is okay, but we should introduce a prefix
for them. If somebody creates a resource "output", things would go horribly wrong (looping
endlessly). A pity we also don't have anonymous functions available in PHP before 5.3.
- But I am also not sure if the Heredoc syntax is the best thing here. I mean it is
a long string, but Heredoc is like a double quoted string and I think single quoted
strings would be better so we don't get problems with characters like $,{} or \n which
could occur in JS or CSS and could accidentally interpreted as PHP. Especially something
like {$...}  could easily end up in JS (prototype).
End then, why do we do <?php echo ... ?> inside of the heredoc? It breaks the JS as
it does not get interpreted as PHP like this!
A pity Nowdoc syntax isn't available as well *arg*. Maybe we should simply use a normal
single quoted string?

Reported by crazy4chrissi on 2013-03-11 09:47:53

[dreadnaut]

- useExternal: see comments #8 and #11 above; it will probably go away if we go split-file.
- coding conventions: sorry, I was only aware of "use tabs, not spaces" so I assumed
anything would go, as long as it was readable; I can switch to concatenation (although
it seems unnecessary here)
- private/_: the code in Authorization is not mine, I just moved it in there, while
I wrote this class from scratch; I'd like to use underscores, yes, but we should write
these things down for the future :)
- function prefix: good idea
- nowdoc would indeed be the best...

Anyway, I don't want to get the perfect implementation out now: I'd like to release
a 'working' version and iterate on it in 1.9.5, once we have the build system in place.
Quite a lot is going to change then, and I'd love to go for gzdeflated resources. These
would be base64 encoded and therefore compatible with any string syntax.

Reported by dreadnaut on 2013-03-11 10:42:42

[Christopher Kramer]

Regarding useExternal: Why should we use this at all? If there is an external file (e.g.
a css-theme), why should we refer to a php script that reads the css-file rather than
pointing directly to the css-file? I mean this way we could enforce caching by setting
the etag, but I think we should rely on servers properly set up to allow caching of
css and js.

Reported by crazy4chrissi on 2013-03-11 10:44:22

[Christopher Kramer]

(Did not see your previous post when posting post 18).
Coding conventions: Yes, we should document stuff. I just always looked at how it was
done and tried to do it similarly. It's not me who wrote PLA initially either ;-)

Okay. No need to make the implementation perfect now. But please have a look on this
PHP-code within the heredoc. It should be replaced by something that works, because
the current patch breaks the help-popup feature.

Reported by crazy4chrissi on 2013-03-11 10:47:42

[Christopher Kramer]

Reported by crazy4chrissi on 2013-03-11 11:23:37 - Labels added: Target-1.9.4

[dreadnaut]

| But please have a look on this PHP-code within the heredoc. It should be replaced
| by something that works, because the current patch breaks the help-popup feature.

Yup, that's important. Another patch attached, which removes heredoc and adds base64+deflate,
shaving another 4KB from the source code.

Reported by dreadnaut on 2013-03-12 23:02:30

<hr> * Attachment: resources-r352.diff

[dreadnaut]

Ok, I think this is the final one: I removed the call_user_func() stuff (it was just
a workaround to use heredoc, also removed). I'll leave useExternal for this version,
but we won't need it after the source split.

Also, I'll commit the original css and js under a resources/.

Reported by dreadnaut on 2013-03-13 11:34:43

<hr> * Attachment: resources-r352.diff

[Christopher Kramer]

Hmm. Problem I see here is that we use defalte compression without checking HTTP-Header
"Accept-Encoding" in the request. So it would send a deflated response even if the
client does not support it. This not only breaks HTTP compliance but can also cause
practical issues. IE6 (you might have guesed IE causes problems again haha) probably
will get into trouble if behind a proxy: As far as I know, it does not support compressed
data from / through a proxy (i.e. does not say it accepts it in the request-header).
However, I don't know what it does if you send it nevertheless. And imho there are
virus scanners that remove the accept-encoding header from requests so they can scan
the response easily. Probably they might give a false alert if we send the response
compressed anyway.

So I would never break a standard like HTTP for the sake a few bytes. You just never
know what clients do. It will work out in most situations, but some users might end
up with no stylesheet at all for example.

So what I propose is to check the accept-encoding header and if compression is supported,
output the compressed resource. If not, inflate it and output that.
This will make it http compliant, work fast using compression if supported, and has
a fallback if not.

Reported by crazy4chrissi on 2013-03-13 16:09:44

[Christopher Kramer]

Ah. here I had read this about IE6 and anti virus software: https://developers.google.com/speed/articles/use-compression

Reported by crazy4chrissi on 2013-03-13 16:16:25

[dreadnaut]

| So what I propose is to check the accept-encoding header and if compression is
| supported, output the compressed resource. If not, inflate it and output that.

Done! Also, I switched from deflate to gzip encoding, again for IE6/7/8. Same compression,
just a few additional bytes of header.

Reported by dreadnaut on 2013-03-13 16:45:25

<hr> * Attachment: resources-r352.diff

[Christopher Kramer]

Okay, great. This looks very good.
Only one tiny thing I noticed:
// only use the inline stylesheet if an external one does not exist
We don't have any "inline" stylesheet at all anylonger, so I would say it should say
"default" instead of "inline" or something like this.

And I think we might consider placing "exit;" within output(). Because after output(),
no HTML should be echoed in any case. Somebody might add a call to output() in the
future but forget to add the "exit".

Reported by crazy4chrissi on 2013-03-13 17:31:42

[dreadnaut]

| And I think we might consider placing "exit;" within output().

I don't like that too much, as it moves flow control inside a class/function and the
main code becomes less readable:

if (isset($_GET['resource'])) {
    Resources::output($_GET['resource']);
    exit;
}

without the 'exit' you see a function call, but unless you know what happens *inside*
it you will expect the program to continue.

Other issues: might create trouble with testing and potentially with output buffering.

Reported by dreadnaut on 2013-03-13 17:48:42

[Christopher Kramer]

hmm, okay. Probably just add a comment before the declaration of output() that you usually
need to exit after calling this function.

Reported by crazy4chrissi on 2013-03-13 17:54:05

[dreadnaut]

This issue was closed by revision r353.

Reported by dreadnaut on 2013-03-13 19:49:29 - Status changed: Fixed

[dreadnaut]

Comments tweaked and code committed. We are left with about 5000 lines of code in the
main file.

Reported by dreadnaut on 2013-03-13 19:51:04

[Christopher Kramer]

Great, thanks. This means 350 LOC less we need to scroll through ;-)

Reported by crazy4chrissi on 2013-03-13 19:59:50