Serve css and javascript as separate, cacheable requests
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
Comments (31)
-
-
reporter 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 -
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 -
reporter 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
-
reporter 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 -
reporter Reported by
dreadnaut
on 2012-11-27 20:13:14 - Labels added: Maintainability -
reporter 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 -
reporter 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 -
reporter 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
-
reporter 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 -
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 -
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 -
reporter 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
-
reporter 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 -
reporter Sorry, forgot the attachment.
Reported by
dreadnaut
on 2013-03-10 22:15:43<hr> * Attachment: resources-r352.diff
-
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 -
reporter - 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 -
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 -
(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 -
Reported by
crazy4chrissi
on 2013-03-11 11:23:37 - Labels added: Target-1.9.4 -
reporter | 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
-
reporter 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
-
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 -
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 -
reporter | 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
-
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 -
reporter | 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 -
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 -
reporter This issue was closed by revision r353.
Reported by
dreadnaut
on 2013-03-13 19:49:29 - Status changed:Fixed
-
reporter 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 -
Great, thanks. This means 350 LOC less we need to scroll through ;-)
Reported by
crazy4chrissi
on 2013-03-13 19:59:50 - Log in to comment
Reported by
crazy4chrissi
on 2012-11-23 21:07:56 - Status changed:Accepted