When using ImageAPI-Optimize in conjunction with Drupal's responsive image styles, a wrong content-length header is sent. I think this happens because in line 165 of this file:
/core/modules/image/src/Controller/ImageStyleDownloadController.php
it creates a header 'Content-Length' => $image->getFileSize(). I found that *at this point* the file size determined is in fact correct. But later, ImageAPI-Optimize will make the image smaller without adapting the content-length header accordingly. Disabling the Content-Length altogether (see attached patch) solves the issue but is only a band-aid solution for obvious reasons.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | imageapi_optimize-2822239-wrong-content-length-headers.patch | 706 bytes | steven jones |
| #9 | imageapi_optimize_wrong_content_length-2822239-9.patch | 648 bytes | stmh |
| #2 | image_content_length_disable.patch | 1.54 KB | rgpublic |
| image_content_length_disable.patch | 819 bytes | rgpublic |
Comments
Comment #2
rgpublicSorry, missed one spot where the header also needs to be disabled. Updated patch.
Comment #3
steven jones commentedLooking at the code path for this, the filesize is calculated after ImageAPI Optimize has done its thing, so I'm not sure why you're seeing this issue.
Something to step through with a debugger for sure. Maybe there's a static cache somewhere.
Comment #4
stmh commentedI can confirm the bug.
I started debugging the issue today and I can report, that getimagesize returns the wrong file size. This is because php caches stat-informations, so the proper fix would be to clear the stat-cache after executing an external command. See attached patch for imageapi_optimize which will replace all former patches.
For some background-information see https://lyte.id.au/2014/05/01/what-the-hell-php/
cheers,
Stephan
Comment #5
stmh commentedComment #8
steven jones commented@stmh Good work!
I suspect that we'll want this for all our processors. Maybe we should lift this code into a more generic method, or move it into the pipeline code so that it gets applied after the processors in the pipeline.
Comment #9
stmh commented@steven-jones I implemented your suggestion and moved the cache clear after ::applyImage. See attached patch.
Comment #10
steven jones commentedI've moved the code slightly, so that it's after applying each processor in a pipeline, as it's possible that a processor itself would read something from the stat cache and expect the results to be valid.
As we don't have tests for the pipeline/processor framework for now we won't bother adding tests for this patch yet. But will need to loop back around and add them in a follow-up.
Comment #11
steven jones commentedComment #12
steven jones commentedThanks everyone!