We ran into a problem when setting up a testbed with this module with our site. Our site makes heavy use of Panels, and some of the common page components (ie sidebars) are constructed with mini-panels. To test this module, I enabled ESI caching on one of the globally-cacheable panes in the sidebar mini-panel. This caused us to lose some of the styling on the entire sidebar mini-panel. When I investigated this, I discovered that the outer div for the sidebar that's added when it's used as a pan in the Panels page was missing.
After a good bit of investigating, I discovered that what was happening was that the theme('panels_pane'...) function wasn't being called for the sidebar mini-panel when it was being added to the page because the esi_theme_is_esied() function detected the esi tags inside the sub-pane, which caused it to skip the rest of the pane processing (and, hence the theme wrapper) for the sidebar pane.
I've attached a patch that fixes this problem by modifying the esi_theme_is_esied function to also check that the bit of text that it's looking for is in the correct position in the content, so that it doesn't bail out for an ESI'd sub-pane.
One comment I would make about this is that it only works for the included theme_esi_tag function. If that function is overridden, then the esi_theme_is_esied function really should be overridden as well... Perhaps it would be a good idea to allow esi_theme_is_esied to invoke a hook that lets modules return whether they think the content is ESI'ed or not.
This issue is for the 6.x-2.x version of the module. I have NOT checked to see if the same issue exists in the 7.x version. If it does, I assume this would need to be forward-ported.
Comment | File | Size | Author |
---|---|---|---|
#2 | esi-2299071-1-more-restrictive-checking.patch | 854 bytes | mikeytown2 |
esi-themed-check-6x.patch | 818 bytes | mgriego |
Comments
Comment #2
mikeytown2 CreditAttribution: mikeytown2 commentedThanks for the patch. It has been committed. Let me know if you encounter other issues and have patches ready to go.
Comment #3
mgriego CreditAttribution: mgriego commentedWill do, thanks!