Local combo loader for YUI #17

Closed
wants to merge 6 commits into
from

Conversation

3 participants
@dpobel
Contributor

dpobel commented Mar 16, 2012

This increases the frontend performances when YUI2 or/and YUI3 is used by the diminution of HTTP requests; even in a local setup, this is noticeable.

The combo.php script is inspired by the combo.php from https://github.com/yui/phploader.

The last commit is a bit scary/tricky. Actually, this is required because the 2.8.2 files are internally tagged as 2.8.2r1 [1] so when the loader builds the path it adds "2.8.2r1" instead of "2.8.2"...

[1] https://github.com/dpobel/ezjscore/blob/local_combo_loader_yui/design/standard/lib/yui/2.8.2r1/build/yuiloader/yuiloader.js#L1075

@dpobel

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 16, 2012

Contributor

I forgot to add that in virtual host mode a new rewrite rule is needed. Obviously, this needs to be documented for upgrade and new install.

Contributor

dpobel commented Mar 16, 2012

I forgot to add that in virtual host mode a new rewrite rule is needed. Obviously, this needs to be documented for upgrade and new install.

@gggeek

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 16, 2012

Contributor

The combo.php sure looks like it is a fast piece of code, but I'm not too happy about the proliferation of frontend controllers. Could we provide an alternative way of triggering the same functionality without extra rewrite rules, a bit like ezjscore does with index_ajax?
Also the name "combo" does not indicate it is a frontend controller. All other existing ones are called index_something.php

Contributor

gggeek commented Mar 16, 2012

The combo.php sure looks like it is a fast piece of code, but I'm not too happy about the proliferation of frontend controllers. Could we provide an alternative way of triggering the same functionality without extra rewrite rules, a bit like ezjscore does with index_ajax?
Also the name "combo" does not indicate it is a frontend controller. All other existing ones are called index_something.php

@gggeek

View changes

combo.php
+ else if ( COMBO_DEBUG === true )
+ {
+ $error = true;
+ $code .= "/* ERROR: {$file} does not exist */";

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 16, 2012

Contributor

Open to XSS?

@gggeek

gggeek Mar 16, 2012

Contributor

Open to XSS?

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 17, 2012

Contributor

I don't think so, but it's true that a bit of escaping wouldn't hurt

@dpobel

dpobel Mar 17, 2012

Contributor

I don't think so, but it's true that a bit of escaping wouldn't hurt

@gggeek

View changes

combo.php
+$error = false;
+foreach ( $yuiFiles as $f )
+{
+ $file = COMBO_YUI_BASE . str_replace( '../', '', $f );

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 16, 2012

Contributor

There might be directory traversal attacks that bypass this replace filter (eg by putting a NUL char between the two dots). I think it is safer to use php's realpath() function to check if the requested file is within the combo yui base

@gggeek

gggeek Mar 16, 2012

Contributor

There might be directory traversal attacks that bypass this replace filter (eg by putting a NUL char between the two dots). I think it is safer to use php's realpath() function to check if the requested file is within the combo yui base

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 17, 2012

Contributor

I don't think so neither, if you put NUL between the two dots, the path becomes wrong:
Tested with from my home on my linux box:

This works normally:
php -r '$f = "../tigrou/.bashrc" ; var_dump( file_get_contents( $f ) );';
This does not work:
php -r '$f = ".\0./tigrou/.bashrc" ; var_dump( file_get_contents( $f ) );';

@dpobel

dpobel Mar 17, 2012

Contributor

I don't think so neither, if you put NUL between the two dots, the path becomes wrong:
Tested with from my home on my linux box:

This works normally:
php -r '$f = "../tigrou/.bashrc" ; var_dump( file_get_contents( $f ) );';
This does not work:
php -r '$f = ".\0./tigrou/.bashrc" ; var_dump( file_get_contents( $f ) );';

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

I did not test it, but the first attack that comes to my mind would be on windows servers, by using '..' in the path.

Will do more research on the security of using pure str_replace to avoid directory traversal attacks.

Last nitpick: A directory called "hello.." is unexpected, but it would be filtered out by your code, while it is in theory acceptable.

@gggeek

gggeek Mar 17, 2012

Contributor

I did not test it, but the first attack that comes to my mind would be on windows servers, by using '..' in the path.

Will do more research on the security of using pure str_replace to avoid directory traversal attacks.

Last nitpick: A directory called "hello.." is unexpected, but it would be filtered out by your code, while it is in theory acceptable.

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 18, 2012

Contributor

Win what ? ;-) Ok this issue might be valid (I don't have any Windows PHP setup to test).
Since, this combo loader is done only for YUI files, "hello.." is not acceptable. It is done for YUI files only because we currently miss a system to properly declare custom YUI module so that it can be asynchronously loaded and so that it plays well with our override system and with the LoadFromCDN setting. The combo as it is done now, is only a first step to improve page loading time since we more and more use YUI.

@dpobel

dpobel Mar 18, 2012

Contributor

Win what ? ;-) Ok this issue might be valid (I don't have any Windows PHP setup to test).
Since, this combo loader is done only for YUI files, "hello.." is not acceptable. It is done for YUI files only because we currently miss a system to properly declare custom YUI module so that it can be asynchronously loaded and so that it plays well with our override system and with the LoadFromCDN setting. The combo as it is done now, is only a first step to improve page loading time since we more and more use YUI.

+/**
+ * Path the YUI files from ezsjcore
+ */
+define( 'COMBO_YUI_BASE', 'design/standard/lib/yui/' );

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

clearly state that ALL files below here are world-readable

@gggeek

gggeek Mar 17, 2012

Contributor

clearly state that ALL files below here are world-readable

+$yuiFiles = explode( '&', $_SERVER['QUERY_STRING'] );
+$cacheKey = md5( $_SERVER['QUERY_STRING'] . COMBO_YUI_BASE );
+
+header( "Cache-Control: max-age=315360000" );

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

expiry time should be configurable?

@gggeek

gggeek Mar 17, 2012

Contributor

expiry time should be configurable?

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 17, 2012

Contributor

useless

@dpobel

dpobel Mar 17, 2012

Contributor

useless

@gggeek

View changes

combo.php
+ }
+ apc_store( $cacheKey, $code, COMBO_APC_TTL );
+}
+echo $code;

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

is it worth to implement here sending compressed output if client supporots it?
And etag/304 responses? Maybe could share some code with index_cluster...

@gggeek

gggeek Mar 17, 2012

Contributor

is it worth to implement here sending compressed output if client supporots it?
And etag/304 responses? Maybe could share some code with index_cluster...

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 17, 2012

Contributor

to me it's not the job of a PHP script. With a well configured web server (apache2 + mod_deflate) it's done for free.
Etag and 304 responses are useless with an Expiry of 10 years.

@dpobel

dpobel Mar 17, 2012

Contributor

to me it's not the job of a PHP script. With a well configured web server (apache2 + mod_deflate) it's done for free.
Etag and 304 responses are useless with an Expiry of 10 years.

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

It is true that the webserver can do all of this on its own: etags, expiry dates, compression. The main advantage of replicating this functionality within php is that we are less dependent on webserver configuration, so we make deployment easier.

As for the 10-years expiry: I guess that it is ok as long as there is no change in the js/css files that are served via the combo. This forces the css/js developer tho change the name of css/js files every time they patch them. While this might happen automatically whenever yui release a new version, I am thinking also about developers wanting to use the combo to load their own stuff

@gggeek

gggeek Mar 17, 2012

Contributor

It is true that the webserver can do all of this on its own: etags, expiry dates, compression. The main advantage of replicating this functionality within php is that we are less dependent on webserver configuration, so we make deployment easier.

As for the 10-years expiry: I guess that it is ok as long as there is no change in the js/css files that are served via the combo. This forces the css/js developer tho change the name of css/js files every time they patch them. While this might happen automatically whenever yui release a new version, I am thinking also about developers wanting to use the combo to load their own stuff

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 18, 2012

Contributor

AFAIK, we don't do GZip content negotiation for "normal" pages nor in index_cluster.php when serving plain text files so there's nothing new here. Again this can be added later. For expiry, as said just above, it's only able to server YUI files, so the name change if there's a source code change. That's why it's useless for now to change this.

@dpobel

dpobel Mar 18, 2012

Contributor

AFAIK, we don't do GZip content negotiation for "normal" pages nor in index_cluster.php when serving plain text files so there's nothing new here. Again this can be added later. For expiry, as said just above, it's only able to server YUI files, so the name change if there's a source code change. That's why it's useless for now to change this.

@gggeek

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

Lat question: apart from being faster/less resource intensive, does combo.php have any other advantage upon using the ezjscore packer for the same purpose?

Contributor

gggeek commented Mar 17, 2012

Lat question: apart from being faster/less resource intensive, does combo.php have any other advantage upon using the ezjscore packer for the same purpose?

@dpobel

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 17, 2012

Contributor

Could we provide an alternative way of triggering the same functionality without extra rewrite rules, a bit like ezjscore does with index_ajax?

Did you notice that index_ajax.php was removed lately and when it was there it required a custom rewrite rule ?
See http://doc.ez.no/Extensions/eZ-Publish-extensions/eZ-JS-Core/Installation

Also the name "combo" does not indicate it is a frontend controller. All other existing ones are called index_something.php

"combo" is the usual name in the YUI world. When you set ezjscore.ini/eZJSCore/LoadFromCDN to enabled, the combo used is "http://yui.yahooapis.com/combo?". the idea is to be consistent with that

Lat question: apart from being faster/less resource intensive, does combo.php have any other advantage upon using the ezjscore packer for the same purpose?

both have a different purpose. The packer packs (!) (and optionaly minify) files together to speed up "static loading" in an HTML page while the combo.php speeds up the dynamic and asynchronous loading when you use the YUI loader.

Contributor

dpobel commented Mar 17, 2012

Could we provide an alternative way of triggering the same functionality without extra rewrite rules, a bit like ezjscore does with index_ajax?

Did you notice that index_ajax.php was removed lately and when it was there it required a custom rewrite rule ?
See http://doc.ez.no/Extensions/eZ-Publish-extensions/eZ-JS-Core/Installation

Also the name "combo" does not indicate it is a frontend controller. All other existing ones are called index_something.php

"combo" is the usual name in the YUI world. When you set ezjscore.ini/eZJSCore/LoadFromCDN to enabled, the combo used is "http://yui.yahooapis.com/combo?". the idea is to be consistent with that

Lat question: apart from being faster/less resource intensive, does combo.php have any other advantage upon using the ezjscore packer for the same purpose?

both have a different purpose. The packer packs (!) (and optionaly minify) files together to speed up "static loading" in an HTML page while the combo.php speeds up the dynamic and asynchronous loading when you use the YUI loader.

@dpobel

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 17, 2012

Contributor

just to make the last sentence in the above comment a bit more clear, here are the waterfalls of JS files without and with the patch while loading /content/view/full/2 in the admin interface with the default settings (empty browser cache):

without the patch

without the patch

with the patch

with the patch

Contributor

dpobel commented Mar 17, 2012

just to make the last sentence in the above comment a bit more clear, here are the waterfalls of JS files without and with the patch while loading /content/view/full/2 in the admin interface with the default settings (empty browser cache):

without the patch

without the patch

with the patch

with the patch

@gggeek

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

Did you notice that index_ajax.php was removed lately and when it was there it required a custom rewrite rule ?
See http://doc.ez.no/Extensions/eZ-Publish-extensions/eZ-JS-Core/Installation

No, my bad, I did not notice it. But it is still mentioned in the instructions page that you link to ;-) . I guess that ezjscore docs will have to be versioned by the time ezp 4.7 is released (and btw the changelog file within ezjscore itself seems to be stuck at version 1.3.0.alpha1, it would benefit of an update, too)

Contributor

gggeek commented Mar 17, 2012

Did you notice that index_ajax.php was removed lately and when it was there it required a custom rewrite rule ?
See http://doc.ez.no/Extensions/eZ-Publish-extensions/eZ-JS-Core/Installation

No, my bad, I did not notice it. But it is still mentioned in the instructions page that you link to ;-) . I guess that ezjscore docs will have to be versioned by the time ezp 4.7 is released (and btw the changelog file within ezjscore itself seems to be stuck at version 1.3.0.alpha1, it would benefit of an update, too)

@gggeek

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

both have a different purpose. The packer packs (!) (and optionaly minify) files together to speed up "static loading" in an
HTML page while the combo.php speeds up the dynamic and asynchronous loading when you use the YUI loader.

In my opinion the two things are not very different: "take a set of css/js files and pack them together so that they can be loaded in a single http call". The fact that the http request for that can be initiated either by yui code or by a link that ezjscore itself put in the html is largely irrelevant. Having common functionality implemented in a single place, with possibly more flexibility, is imho better than having it implemented in many different places, every time with a lot of limitations and a single, dedicated scope.

Contributor

gggeek commented Mar 17, 2012

both have a different purpose. The packer packs (!) (and optionaly minify) files together to speed up "static loading" in an
HTML page while the combo.php speeds up the dynamic and asynchronous loading when you use the YUI loader.

In my opinion the two things are not very different: "take a set of css/js files and pack them together so that they can be loaded in a single http call". The fact that the http request for that can be initiated either by yui code or by a link that ezjscore itself put in the html is largely irrelevant. Having common functionality implemented in a single place, with possibly more flexibility, is imho better than having it implemented in many different places, every time with a lot of limitations and a single, dedicated scope.

@gggeek

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

One more nitpick.
This might not be valid anymore, but in a recent past reverse proxies did not obey caching headers in http responses whenever the requested url contained query strings (at least, the squid conf did that out of the box). Bad usage, fostered in the 1st place by badly coded web apps - and a commonly used tricks, so that many devs append ?foo=baz to the urls of their dynamically-generated css/js, and have it working regardless of web server config (which might have an expiry header added to all css responses).
To avoid this, we could use the path_info instead, same as we do with the rest of eZP.
Of course, that would mean changing a bit the yui combo-loader code - and of course, it should have to be done in a smart way, so that it supports both the ez-combo-url-syntax and the yui-on-cdn one... but maybe it's just a matter of setting the proper comboBase property (http://developer.yahoo.com/yui/docs/YAHOO.util.YUILoader.html)?

Contributor

gggeek commented Mar 17, 2012

One more nitpick.
This might not be valid anymore, but in a recent past reverse proxies did not obey caching headers in http responses whenever the requested url contained query strings (at least, the squid conf did that out of the box). Bad usage, fostered in the 1st place by badly coded web apps - and a commonly used tricks, so that many devs append ?foo=baz to the urls of their dynamically-generated css/js, and have it working regardless of web server config (which might have an expiry header added to all css responses).
To avoid this, we could use the path_info instead, same as we do with the rest of eZP.
Of course, that would mean changing a bit the yui combo-loader code - and of course, it should have to be done in a smart way, so that it supports both the ez-combo-url-syntax and the yui-on-cdn one... but maybe it's just a matter of setting the proper comboBase property (http://developer.yahoo.com/yui/docs/YAHOO.util.YUILoader.html)?

@gggeek

This comment has been minimized.

Show comment Hide comment
@gggeek

gggeek Mar 17, 2012

Contributor

"combo" is the usual name in the YUI world. When you set ezjscore.ini/eZJSCore/LoadFromCDN to enabled, the combo used is "http://yui.yahooapis.com/combo?". the idea is to be consistent with that

But since this is still set via an ini parameter (which means it's easy to pick a name we like for the local combo file), I'd prefer to have eZ be consistent with itself rather than with external systems

Contributor

gggeek commented Mar 17, 2012

"combo" is the usual name in the YUI world. When you set ezjscore.ini/eZJSCore/LoadFromCDN to enabled, the combo used is "http://yui.yahooapis.com/combo?". the idea is to be consistent with that

But since this is still set via an ini parameter (which means it's easy to pick a name we like for the local combo file), I'd prefer to have eZ be consistent with itself rather than with external systems

@andrerom

This comment has been minimized.

Show comment Hide comment
@andrerom

andrerom Mar 17, 2012

Owner

just to make the last sentence in the above comment a bit more clear, here are the waterfalls of JS files without and with > the patch while loading /content/view/full/2 in the admin interface with the default settings (empty browser cache):

Yes, but we only use local files because sales people wanted us to optimize for offline use at some point, this is not the case if you enable loading from CDN server is it?

As for 4.7, this arrived to late, so lets have some discussions on integrating this when the certification dust settles a bit.

Owner

andrerom commented Mar 17, 2012

just to make the last sentence in the above comment a bit more clear, here are the waterfalls of JS files without and with > the patch while loading /content/view/full/2 in the admin interface with the default settings (empty browser cache):

Yes, but we only use local files because sales people wanted us to optimize for offline use at some point, this is not the case if you enable loading from CDN server is it?

As for 4.7, this arrived to late, so lets have some discussions on integrating this when the certification dust settles a bit.

@dpobel

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 18, 2012

Contributor

@gggeek for sharing code between the packer and the combo, there's also a performance issue. To use it you need the whole ezp stack because it depends on several classes (eZDesignResources, eZINI, ...) and like in index_cluster.php we need the best performances otherwise the combo is completely useless or even counter productive. That's also why the combo.php is done in such a "plain PHP" way.

@andrerom yes, the idea here is too get the same result in terms of HTTP requests (and then performances) no matter is you enabled or not the loading from CDN setting. When LoadFromCDN form is disabled, the performance becomes to be an issue because of the wider use of YUI in the admin interface.

Contributor

dpobel commented Mar 18, 2012

@gggeek for sharing code between the packer and the combo, there's also a performance issue. To use it you need the whole ezp stack because it depends on several classes (eZDesignResources, eZINI, ...) and like in index_cluster.php we need the best performances otherwise the combo is completely useless or even counter productive. That's also why the combo.php is done in such a "plain PHP" way.

@andrerom yes, the idea here is too get the same result in terms of HTTP requests (and then performances) no matter is you enabled or not the loading from CDN setting. When LoadFromCDN form is disabled, the performance becomes to be an issue because of the wider use of YUI in the admin interface.

@andrerom

This comment has been minimized.

Show comment Hide comment
@andrerom

andrerom Mar 18, 2012

Owner

@dpobel I got that, my point was merrily that instead of complicating this we could try lobbying for returning it to being on by default. But then I think there needs to be a easier way to disable these things, maybe a "Enable offline mode (reduced performance)" during setup wizard?

If we do that, we clearly state that CDN is preferred for performance and we don't have to start loading all assets over php like this (http servers are pretty good at dealing with static assets)..

Owner

andrerom commented Mar 18, 2012

@dpobel I got that, my point was merrily that instead of complicating this we could try lobbying for returning it to being on by default. But then I think there needs to be a easier way to disable these things, maybe a "Enable offline mode (reduced performance)" during setup wizard?

If we do that, we clearly state that CDN is preferred for performance and we don't have to start loading all assets over php like this (http servers are pretty good at dealing with static assets)..

@dpobel

This comment has been minimized.

Show comment Hide comment
@dpobel

dpobel Mar 18, 2012

Contributor

@andrerom To me, the use of the public CDN is not always the best solution. It's nice for files that might already be in the browser cache like jQuery but for YUI files it's a bit different due to the loading of only the necessary modules (and those differ from a site to another). Using a public CDN also involves an other DNS request and a connection initialization that might be a slower than expected. For instance, the ping to my server is about 30ms while YUI's CDN is about 55ms at the moment. And from a mobile connection, the difference is probably even bigger.
In addition, the yui.yahooapis.com does not support HTTPs so it's not usable in a big range of Intranet/Extranet/Admin interface.

Contributor

dpobel commented Mar 18, 2012

@andrerom To me, the use of the public CDN is not always the best solution. It's nice for files that might already be in the browser cache like jQuery but for YUI files it's a bit different due to the loading of only the necessary modules (and those differ from a site to another). Using a public CDN also involves an other DNS request and a connection initialization that might be a slower than expected. For instance, the ping to my server is about 30ms while YUI's CDN is about 55ms at the moment. And from a mobile connection, the difference is probably even bigger.
In addition, the yui.yahooapis.com does not support HTTPs so it's not usable in a big range of Intranet/Extranet/Admin interface.

@andrerom

This comment has been minimized.

Show comment Hide comment
@andrerom

andrerom Sep 19, 2012

Owner

Closing as we're about to move system-extension into eZ Publish 4.x repo, and this pull makes more sense to re consider after that is done.

Owner

andrerom commented Sep 19, 2012

Closing as we're about to move system-extension into eZ Publish 4.x repo, and this pull makes more sense to re consider after that is done.

@andrerom andrerom closed this Sep 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment