yireo / Yireo_NextGenImages

44 stars 26 forks source link

Module not compatible with Varnish #28

Closed rommelfreddy closed 1 year ago

rommelfreddy commented 2 years ago

Hello @jissereitsma

if found the issue, that the module will not work for esi blocks.

by loading the ESI blocks Magento will only render the block itselfs and is not using the method Magento\Framework\View\LayoutInterface::getOutput.

Example: The menu of Magento is a ESI Block (block with TTL). So Magento replaces it with the placeholder for varnish. So your module will not replace the images, cause in the second step (loading the ESI Block) only the function Magento\Framework\View\Element\AbstractBlock::toHtml is called.

-- Suggestion: Maybe it would be a good idea to process every block instead the whole output (event subscriber on view_block_abstract_to_html_after). So the changes by the module will be also cached by its parents block.

jissereitsma commented 2 years ago

Thanks for the suggestion. I have worked upon this with a new commit waiting to be released with the next major version of NextGenImages: https://github.com/yireo/Yireo_NextGenImages/commit/b32a758486fb84d645e87806cea26b92db137c16 The suggestion for listening to the event is indeed the way I went for, except for I don't want to apply it to any block (for performance reasons) but only those blocks with a ttl higher than zero (because those are turned into ESI blocks) and only when FPC is enabled. It works for me. But it needs to wait for the next major release before everyone else can enjoy.

jissereitsma commented 2 years ago

The new release has been made a couple of days ago: 0.3.0 - does that fix things for you?

ppeyman commented 1 year ago

The new release has been made a couple of days ago: 0.3.0 - does that fix things for you?

i checked it everything works well in varnish just base or main images doesn't replaced with webp extension

jissereitsma commented 1 year ago

@ppeyman Could you explain a bit more? If I add punctuation to your sentence, I understand that "everything works well in Varnish" so that the original bug is already crossed off the list. You seem to address a new problem here as well: "base or main images are not replaced". Could you post a new issue for this? And could you describe in that issue a bit on how to duplicate this? Is this Varnish-related or does this occur without Varnish and/or FPC as well?

jissereitsma commented 1 year ago

I'm closing this specific issue, because the original issue seems fixed.