added image versioning along with related config options etc

[leeovery]

Hi,

Im sorry if Ive done this wrong as its my first pull request. I rebased my feature branch onto the tip of develop and have pushed that to my repo.

Basically Ive added versioning for images. This is something I use quite a lot as my images are big so I cache them for 10 years (!). If the image is changed then the filename will automatically change too and hence will force the browser to re-request it.

Obviously this needs a .htaccess modification so Ive left the default to false so it doesnt break anything. The necessary lines for the .htaccess file are included in the casset config file.

Also you could pass in a bool param to Casset::img to force versioning on a per-img basis but I didnt add this at this stage.

Hope thats helpful. Let me know what you think.

Lee (leekudos)

[canton7]

Interesting idea... I'm going to give it a bit of thought before accepting it in its current form though.

My main concern is one of trying to keep Casset from becoming a monolith, although I'm not too successful at this I'll admit. In this case, you're forcing the user to use one particular scheme for cache invalidation. What if they want to use a scheme such as filename.png?mtime=blah?

I had the same issue with ShonM's need to use Casset to parse SASS files. He wanted to build a full-blown parsing systen into Casset, which I felt was overkill. In the end (after quite a lot of thought), we came up with the post_load callback, which solves the problem very cleanly.

I'm just wondering whether callbacks are the way to go here, as well. If we allow the user to modify the URL of any requested asset (giving them enough information of course), then they could implement a system such as this very simply. Code that implements your system, and the associated .htaccess file, could then go into a subfolder as a suggested use of this callbacks system.

What do you think?

Also:

• Your patch will find the filemtime of a remote image (one on another server), which will fail.
• You've based your work on quite an old commit in develop (hit the "Forks" button just under the search box to see this graphically). A good workflow for submitting patches is:

# Start a new feature branch, based off the tip of develop
git checkout -b feature/img_versioning develop
# Work...
git add; git commit; etc...
# If develop has moved forwards while you've been working on the patch, update your local develop and rebase the feature onto it
git checkout develop
git pull
git checkout feature/img_versioning
git rebase develop

The feature branch is for your convenience. If you don't use a feature branch, and I merge the patch with small changes, you're going to get a conflict next time you pull.

Thanks! Antony

[leeovery]

Thanks for the reply.

I'll properly digest the info when I'm back in the office.

Just a quick note on using ?mtime=whatever, I read somewhere this caused issues with a version of IE. Ive tried to find the article but can't at the moment. It would be preferable though as it wouldn't require any htaccess edits.

I like your callback idea though.

This is an essential thing for me but I'm sure there's a better way of doing it!

Cheers.

Lee Sent from my iPhone

On 30 Sep 2011, at 14:47, Antony Male reply@reply.github.com wrote:

Interesting idea... I'm going to give it a bit of thought before accepting it in its current form though.

My main concern is one of trying to keep Casset from becoming a monolith, although I'm not too successful at this I'll admit. In this case, you're forcing the user to use one particular scheme for cache invalidation. What if they want to use a scheme such as filename.png?mtime=blah?

I had the same issue with ShonM's need to use Casset to parse SASS files. He wanted to build a full-blown parsing systen into Casset, which I felt was overkill. In the end (after quite a lot of thought), we came up with the post_load callback, which solves the problem very cleanly.

I'm just wondering whether callbacks are the way to go here, as well. If we allow the user to modify the URL of any requested asset (giving them enough information of course), then they could implement a system such as this very simply. Code that implements your system, and the associated .htaccess file, could then go into a subfolder as a suggested use of this callbacks system.

What do you think?

Also:

• Your patch will find the filemtime of a remote image (one on another server), which will fail.
• You've based your work on quite an old commit in develop (hit the "Forks" button just under the search box to see this graphically). A good workflow for submitting patches is:

# Start a new feature branch, based off the tip of develop
git checkout -b feature/img_versioning develop
# Work...
git add; git commit; etc...
# If develop has moved forwards while you've been working on the patch, update your local develop and rebase the feature onto it
git checkout develop
git pull
git checkout feature/img_versioning
git rebase develop

The feature branch is for your convenience. If you don't use a feature branch, and I merge the patch with small changes, you're going to get a conflict next time you pull.

Thanks! Antony

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/pull/5#issuecomment-2249546

[canton7]

Good point, I think I've read something similar somewhere. Either way, I think it should be a decision for the user, not our decision.

It would be nice if we allow the same functionality to be applied to js/css assets, as well. Let's assume that our user has pre-minified their assets, and is using casset with both combine and min set to false. In this case, they might well want to set long expire times on all of their assets, and use your system for expiry.

However we decide to go about this, Casset will support this soon.

[leeovery]

I use a system for css and js at the moment with very long expire times.

I actually started to patch casset so the mtime was part of the js/css filename as opposed to a param but I ditched it. I didnt like editing the htaccess file to make it work. Then I started the image versioning work and ended up in the same place! lol

I really think it would be better though as the ? thing does cause issues in some browsers.

Perhaps the default is its appended to the url via a GET param (as it is currently with js/css). If desired the .htaccess lines could be added and the 'better' option could be enabled. This way the user is making the decision.

Also a good htaccess file could be included with caching enabled amongst other things.

[canton7]

You've probably realised this, but the combined/minified asset filenames are based on the mtime of the last-modified file which is included, meaning that the filename changes whenever one of the assets from which it is composed changed.

Just following up the callback idea a bit more, what do you think to something like this:

<?php
// In your config...
'filename_callback' => function($filepath, $type, $remote) {
    if ($remote || $type != 'img')
        return $filepath;

    $path = pathinfo($filepath);
    $mtime = '.'.filemtime(DOCROOT.$filepath).'.';
    return str_replace('.'.$path['extension'], $mtime, $filepath.$path['extension']);
}

[leeovery]

Yea I did realise :)

It's the same as I do myself in my code currently. Most logical and simple answer to problem.

I like the callback idea. How would that be triggered? Would it trigger for all assets?

I'll take a look at code later. Still out and about.

Thanks.

Lee Sent from my iPhone

On 30 Sep 2011, at 17:09, Antony Male reply@reply.github.com wrote:

You've probably realised this, but the combined/minified asset filenames are based on the mtime of the last-modified file which is included, meaning that the filename changes whenever one of the assets from which it is composed changed.

Just following up the callback idea a bit more, what do you think to something like this:

<?php
// In your config...
'filename_callback' => function($filepath, $type, $remote) {
   if ($remote || $type != 'img')
       return $filepath;

   $path = pathinfo($filepath);
   $mtime = '.'.filemtime(DOCROOT.$filepath).'.';
   return str_replace('.'.$path['extension'], $mtime, $filepath.$path['extension']);
}

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/pull/5#issuecomment-2251211

[canton7]

It would trigger for all assets, yes. The $type argument allows users ignore asset types they don't care about, though (the same approach as was taken for the post_load callback).

For images, it would be called from the same place you called version_img from. For other assets, probably line 1021, where it would override the ?mtime bit that's currently i place.

[leeovery]

FYI I've basically finished work on adding the callback. It's working fine but have been trying to get closures working with arguments in config. Submitted a pull request to fuel/core but it was rejected. The design was made to not allow closures in config with arguments. They are only meant to dynamically assign a value.

With this in mind the casset config needs updating for the calllbacks. Having them in config wont work if they need arguments which they obviously do.

I'll make the chance in my next pull request for you :-)

[canton7]

Hmm, that's odd. As you can see, the post_load callback works just fine from the config.

I'm halfway through implementing this callback myself, so don't worry too much. If we both end up implementing it, I'll take the best of the two.

[leeovery]

Haha. Already done it for images. Perhaps you do the other assets first and I'll get the pull request in tonight.

The postload callback wont work with 1.1 with arguments but it depends how's it's written in.

How you putting a closure in config?

They are simply meant to return a value not a closure (a function).

Lee Sent from my iPhone

On 2 Oct 2011, at 15:15, Antony Male reply@reply.github.com wrote:

Hmm, that's odd. As you can see, the post_load callback works just fine from the config.

I'm halfway through implementing this callback myself, so don't worry too much. If we both end up implementing it, I'll take the best of the two.

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/pull/5#issuecomment-2262223

[canton7]

Hmm it might be a 1.1 thing. I'll check in a little

[leeovery]

Check out the "value" method in fuel/core/classes/fuel.php.

It calls the closure to get the value. No way for parameters to stay assigned. Not tested this but maybe stacking two closures, one inside the other, would work. Horrible idea though.

I think using the set_ method in casset is better to set the callback.

[leeovery]

This is the reply I got from jelmer:

"This was by design: closures in config are meant to be executed in order to get the actual value, it wasn't meant to be able to pass along closures. Your "fix" isn't that, it actually adds functionality but more importantly it adds ambiguity: closures without args are always executed but add a single var and it will be a weirdly regenerated closure. That's not a good thing.

We made the call to only allow closures in config that are meant to dynamicly assign a value, not closures as config values."

Lee Sent from my iPhone

On 2 Oct 2011, at 16:12, Antony Male reply@reply.github.com wrote:

Hmm it might be a 1.1 thing. I'll check in a little

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/pull/5#issuecomment-2262452

[canton7]

Makes sense I guess. Still a little confused as to how the post_load callback managed to work, then...

Anyway, lunch, then time to look at things properly

[leeovery]

It'll work if assigned using the set method in casset obviously. But not via the config.

I fixed it but it's not what they want which I understand.

Be best to remove the config callback and just set using the method. Causes confusion otherwise.

I'm busy until later but will send across my work when in home. Take it or leave it. I like helping :-)

Lee Sent from my iPhone

On 2 Oct 2011, at 16:26, Antony Male reply@reply.github.com wrote:

Makes sense I guess. Still a little confused as to how the post_local callback managed to work, then...

Anyway, lunch, then time to look at things properly

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/pull/5#issuecomment-2262512

[canton7]

Confirmed as a 1.1 thing. I'll remove the existing post_load stuff now

[leeovery]

You could code it so dev could name a function/method in config (one that is defined elsewhere in app). Then if callback is not null and is a string, dynamically call the method etc. If closure then call as closure.

Lee Sent from my iPhone

On 2 Oct 2011, at 17:48, Antony Male reply@reply.github.com wrote:

Confirmed as a 1.1 thing. I'll remove the existing post_load stuff now

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/pull/5#issuecomment-2262860

[canton7]

Take a look at 45bad4aab484030bcb5e140326cc5efd0e97d057

[leeovery]

Will check when I'm home.

All done?

Lee Sent from my iPhone

On 2 Oct 2011, at 18:06, Antony Male reply@reply.github.com wrote:

Take a look at 45bad4aab484030bcb5e140326cc5efd0e97d057

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/pull/5#issuecomment-2262944

[canton7]

Take a look at the feature/filepath_callback branch. Think I've covered everything there...

[leeovery]

Awesome! Looked through your code and your way was better than mine. I was basically finished too but it wasn't quite as thorough as yours!

Thanks for the mention too :-)

Ill checkout this branch and give it a whirl tomorrow morning.

Thanks for spending the time on this. Im sure itll help more than just me.

Might be worth mentioning in your README that the .htaccess line needs to be above the index.php removal lines suggested by FuelPhp. Otherwise it'll fail as the uri with mtime doesnt point to a real file or dir (due to the mtime string obv). Those not used to fault-finding .htaccess will wonder why this is happening and will assume its Casset. I know I would have several years ago.

[canton7]

Added!

Should we have 'php' listed as a filetype for an example .htaccess do you think? Methinks it might too easily erroneously match someone's php file...

I'll let you give it a spin before merging it in, as there's a fair chance I'll have screwed it up somewhere.

[leeovery]

Remove php. Don’t even know why it was there. I wrote that line ages ago.

I do use this with swf files too but that’s for another day :)

Not sure if you are UK based but I'm getting tired now. Had a late night. Will checkout tomorrow morning and give it a good testing!!

[canton7]

Cool, removed!

Casset will support swf files (in the same way that it supports images) at some point, but this requies a bit of restructuring to allow people to specify arbitary dirs. That is, renaming 'css_dir', etc, to all sit under some 'dirs' key... Or maybe keeping 'css_dir' et al but adding some 'extra_dirs' key... Not sure at this point. get_filepath() was written with this in mind, anyway.

That's a good point actually -- get_filepath doesn't call the filepath callback. Oops! Give me a min...

[canton7]

Done.

I'm rebasing all this time by the way (I like to keep feature branches clean, and they're not normally public), just keep this in mind if you're pulling.

[leeovery]

All looks good to me!

Ive tested with all assets with combine switched on and off.

FYI, Im using this as the callback to satisfy my needs.

Casset::set_filepath_callback(function($filepath, $type, $remote) {
    if ($remote) return $filepath;
    $ext = pathinfo($filepath, PATHINFO_EXTENSION);
    return str_replace('.'.$ext, '.'.filemtime($filepath).'.'.$ext, $filepath);
});

Thanks again for integrating this into Casset.

[canton7]

Bear in mind your callback should be being called for all assets (js, img, etc) -- is this the case?

[leeovery]

yes its calling and working for imgs, js and css. All confirmed as working.

Am I right in thinking this isnt required for js and css. The filename will change soon as anything is changed in that group (or whatever). Correct?

[canton7]

Grand.

Yeah that's correct. It used to just append the last mtime, but that got changed as part of the feature patchset. I added that functionality in case someone comes up with a use for the callback that we haven't thought of, though.

I'll get this merged in tonight.

[leeovery]

Awesome.

Ill amend my callback to exclude non-img assets.

[canton7]

Merged the branch. Closed. Thanks for your help :)