Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for caching with file extensions #152

Merged
merged 4 commits into from
Aug 19, 2016
Merged

Add support for caching with file extensions #152

merged 4 commits into from
Aug 19, 2016

Conversation

jasonvarga
Copy link
Contributor

Hey there.

I'd like to be able to add file extensions to the cached images so they can be served directly.

In our app (Statamic, in this case), we'll have an option to pre-generate the images and use this approach. Obviously we'd lose the dynamic URL based nature of Glide, but this would be a switch people could flick to increase performance.

On heavier sites, loading an image directly is considerably faster than bootstrapping the app.

@reinink
Copy link
Contributor

reinink commented Aug 19, 2016

So the only issue I see here is when you specify a different file encoding in your image manipulations (?fm=jpg). The source file may be a png, but the outputted image may be a jpg.

I think we could check for this though. We'd need to check the $params array, as well as any defaults or presets. Something like this:

  1. Use fm if present in current $params list
  2. Use fm if a preset (p) is being used and fm is set for that preset ($this-> presets)
  3. Use fm if set in the defaults ($this-> defaults)
  4. Use the format of the source file (pathinfo($path)['extension'])

@jasonvarga
Copy link
Contributor Author

Ah yes of course. Forgot about fm. I'll get that sorted.

@reinink
Copy link
Contributor

reinink commented Aug 19, 2016

Thanks man! I'd help you but I'm out of time for this week.

@jasonvarga
Copy link
Contributor Author

No worries. I just pushed up the changes to support that.


if ($this->cacheWithFileExtensions) {
$ext = isset($params['fm']) ? $params['fm'] : pathinfo($path)['extension'];
$cachedPath .= '.'.$ext;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this a single line?

$cachedPath .= isset($params['fm']) ? $params['fm'] : pathinfo($path)['extension'];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely. Done.

@reinink
Copy link
Contributor

reinink commented Aug 19, 2016

This is looking great. Next question. Is this 1.0.2 or 1.1.0? It's not a bug patch, so technically by semver this is a minor version adding functionality in a backwards-compatible manner. So let's maybe do 1.1.0.

@jasonvarga
Copy link
Contributor Author

Sounds good to me!

@reinink reinink merged commit 7325554 into thephpleague:master Aug 19, 2016
@reinink
Copy link
Contributor

reinink commented Aug 19, 2016

Tagged: https://github.com/thephpleague/glide/releases/tag/1.1.0

@jasonvarga
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants