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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

  • mikeytown2 committed e720d58 on authored by mgriego
    Issue #2299071 by mgriego: Parent panes can be mistakenly tagged as ESI'...
mikeytown2’s picture

Status: Needs review » Fixed
FileSize
854 bytes

Thanks for the patch. It has been committed. Let me know if you encounter other issues and have patches ready to go.

mgriego’s picture

Will do, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.