Do not dump assets when not changed #457

Open
wants to merge 3 commits into
from

Projects

None yet

4 participants

@stijnkoopal

Assets are copied on every request, which induces a great performance penalty. This PR makes sure that assets are only dumped when they do not exist or when they are modified.

Fixes widmogrod/zf2-assetic-module#74

@stof stof and 1 other commented on an outdated diff Jun 18, 2013
src/Assetic/AssetWriter.php
);
+
+ if (!file_exists($target) || filemtime($target) < $asset->getLastModified()) {
@stof
stof Jun 18, 2013 Collaborator

$asset->getLastModified() will not use the deep mtime resolution, and so will break the dumping when the change is in an imported file instead of the main asset file

@roelvanduijnhoven
roelvanduijnhoven Jun 19, 2013

How can we turn instead to using deep mtime resolution?

@data219
Contributor
data219 commented Jun 18, 2013

Dont you think such new behaviour should not become default for everybody?
We use exactly the same mechanism but I am not fully happy with our solution.
At the moment we are using a NonOverwritingAssetWriter that simpliy extends the AssetWriter and does what this changes do but without a mtime check. Since we use the cache busting worker we dont need it, mtime is brought into output filename via the cache busting worker and thus new filename. You dont need any mtime logic in this, thats what the cache busting worker is for.
And please make it an extenstion of AssetWriter instead of patching the original. This enhancement is quite nicer if you can choose to use it or not (and in such cases I vote for an extension instead of a config flag to enable it)
If you like I can show you some of my half ready code and we can work on a even more optimized solution together =)

@roelvanduijnhoven

We do not use caching busting at all. So the solution you described does not work in our instance. Still, seeing your implementation might be useful!

The problem with making our own implementation is that the AssetWriter has no common interface. Likewise extending this class can not easily be done (other then simply replacing the complete writeAsset method).

Let's make sure these performance boosts will come available to everyone.

@data219
Contributor
data219 commented Jun 20, 2013

i dont see your problem...
ok there is no interface defined, but whats the problem with overwriting a method in an extension? i replaced the write method and it works like a charm. simply added the file_exists().

Are you looking at an older version of the AssetWriter???

Have a look:

´´´php
class NonOverwritingAssetWriter extends AssetWriter
{
protected static function write($path, $contents)
{
if(efs_filefunctions::file_exists($path)) {

  return;
}

parent::write($path, $contents);

}
}
´´´
(Sorry, companies php code style yet)

In fact I like this. wasnt aware that the write method was introduced and path calculations are already separeted from actual writing.

So @roelvanduijnhoven, what do you think about that snippet?
One could add an additional mtime check... but I suggest to keep that configurable since every fstat has a cost in many enviroments and people using cache busting wont need any mtime checks (since that is the cache bustings job)

@stijnkoopal

I introduced the notion of a NonOverwritingAssetWriter as you kindly suggested. You are right that this patch actually does not belong in the original implementation. Furthermore, I added some tests.

@roelvanduijnhoven

Seems pretty good to use! What do you think of this @kriswallsmith?

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