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

Caching not working with External image, isImageAlreadyCached problem #137

Open
wdebusschere opened this issue Jul 11, 2016 · 8 comments
Open
Assignees
Milestone

Comments

@wdebusschere
Copy link

Warning: GenericErrorHandler 2: filemtime(): stat failed for /Users/xxx/Sites/www.domain.com/workspace/hosting26.externalserverxxx.com/pictures/folders/2825AL.jpg on line 121 of file /Users/xxx/Sites/www.domain.com/extensions/jit_image_manipulation/lib/class.jit.php

Throws this error with function isImageAlreadyCached
$parameters['last_modified'] = @filemtime(WORKSPACE . '/'. $parameters['image']);

@wdebusschere wdebusschere changed the title External image isImageAlreadyCached problem Caching not working with External image, isImageAlreadyCached problem Jul 11, 2016
@nitriques
Copy link
Member

Which version of Symphony ? And jit ? I have a black image (which is a problem) but not the same as yours...

@wdebusschere
Copy link
Author

Symphony 2.6.7
JIT Integration 2.0.2
The error only appears in the log
Image status is always 200.. while the Image status of local images is 304 on a refresh.

@nitriques
Copy link
Member

Hum... ok thanks. Still can't reproduce but still having a black image from cache

@nitriques nitriques added the Bug label Jul 20, 2016
@nitriques nitriques added this to the 2.1.0 milestone Jul 20, 2016
@nitriques nitriques self-assigned this Jul 20, 2016
@nitriques
Copy link
Member

I've boiled it down to transparent png, which gets their transparency ripped out when the file is save from cache, but I do not know how this happens...

nitriques added a commit that referenced this issue Jul 28, 2016
The way we handled HEAD request was

1. Different from the GET request, since it lacked some checks
2. Way to general for our needs

This commit refactors the old things with a single brand new function
that will

1. Make sure the request is a 200
2. Return only cache related headers, since that's the goal

Re #136
Cc #138
Cc #130
Cc #137
nitriques added a commit that referenced this issue Jul 28, 2016
This commit cleans up the code and fixes a bunch of issues.

* Cache validation

There was a bug, #137, which would made the cache unusable with external
images. This has been fixed. The function also makes sure to validate
that the original file, either remote or local, has not changed since
the last modification of the cache file itself. If the cache file is no
longer valid, it gets deleted. This also fixes #136.

* Last modified value

There was a problem describe in #138 where a second request to a url
with proper cache heading would still send a 200, because the
Last-Modified value was read from a different source. This commit
changes this by doing the following.

1. Always use a local mtime: either the original local file or our
tempfile created from the http request.
2. If cache is enabled, always use the cache file mtime, even when the
request creates the cache file. This insure repeatable results for all
subsequent requests.

This brings a side effect that is needed:
before, we used to touch the cache file when found valid and we must
need to stop this to be able to get repeatable results.

---

Throughout the process, values are "exported" into the parameter array
when certain condition are met. This insure that the values sent to the
user will be the right one.

Sorry for the big commit, I did my best to split things up :)

Fixes #137
Fixes #136
Fixes #138
@nitriques
Copy link
Member

@wdebusschere Latest integration code should fix this. If you could confirm, that would be neat!

@nitriques
Copy link
Member

@wdebusschere Lots of work has been made yet again, if you could confirm please. Thanks (but no rush)

@wdebusschere
Copy link
Author

wdebusschere commented Aug 2, 2016

@nitriques

Perfect. + no more errors in the Symphony log
1st image = the remote image
2nd image = the same, but local version

screen shot 2016-08-02 at 11 11 24

@nitriques
Copy link
Member

SUPER! Thanks!

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

No branches or pull requests

2 participants