The Juicebox XML cache module has been receiving some testing and is probably getting close to a fairly stable point. This module provides some performance advantages (addressing the points from #1906860: Notes on performance when core page caching is disabled.) while also allowing Juicebox to work with non-standard field instances (addressing the points from #2290443: Panels compatibility when individual fields are added to a pane and #2006830: Support for views row field formatter (Views Not saving Juicebox Configuration and reverting to default when saved).

It's probably time to start considering the steps to get it added to this main Juicebox module (both 7.x-2.0 and 8.x-2.0).

Comments

rjacobs’s picture

Version: 7.x-2.x-dev » 8.x-2.x-dev

It'll probably be best to do this in D8 first and then backport.

rjacobs’s picture

Status: Active » Needs review
StatusFileSize
new43.29 KB

Here's a draft patch for the local testbots.

Status: Needs review » Needs work

The last submitted patch, 2: juicebox-add_xml_cache-2380741-2.patch, failed testing.

rjacobs’s picture

Status: Needs work » Needs review

Test failures are from changes just committed in #2382533: Attach assets only via the asset library system, which adds a new issue that has to be delt with for this module.

rjacobs’s picture

StatusFileSize
new45.31 KB

Head changed, so here's an updated patch.

Status: Needs review » Needs work

The last submitted patch, 6: juicebox-add_xml_cache-2380741-6.patch, failed testing.

rjacobs’s picture

Status: Needs work » Needs review
StatusFileSize
new52.5 KB

Yeah, so reverse-engineering the way D8 core adds views-based tests turned out to be a bloody nightmare (we need a view to test a case where a gallery display is dependent on the cache). I think I finaaaaaalllly figure it out. Here's a new patch.

  • rjacobs committed 3856420 on 8.x-2.x
    Issue #2380741 by rjacobs: Add Juicebox XML cache feature.
    
rjacobs’s picture

Version: 8.x-2.x-dev » 7.x-2.x-dev
Status: Needs review » Active

Ok, committed. Now back to D7.

rjacobs’s picture

Status: Active » Needs review
StatusFileSize
new34.97 KB

Here's a backport for D7.

Status: Needs review » Needs work

The last submitted patch, 11: juicebox-add_xml_cache-2380741-11.patch, failed testing.

rjacobs’s picture

Status: Needs work » Needs review
StatusFileSize
new35.34 KB

Hummm, let's try that again.

Status: Needs review » Needs work

The last submitted patch, 13: juicebox-add_xml_cache-2380741-13.patch, failed testing.

rjacobs’s picture

Status: Needs work » Needs review
StatusFileSize
new35.38 KB

Well bloddy hell, I simply cannot replicate locally whatever error the testbots are spitting back. It's clearly related to the new test logic and not the changed code. I guess I'll have to figure out some creative way to debug this.

Status: Needs review » Needs work

The last submitted patch, 15: juicebox-add_xml_cache-2380741-15.patch, failed testing.

rjacobs’s picture

Status: Needs work » Needs review
StatusFileSize
new35.34 KB

Might have been a stupid oversight regarding the fact that the testbots force non-clean URLs. Trying again...

rjacobs’s picture

Status: Needs review » Postponed

Alright, so there are working versions of this available for both D7 and D8. However, the more I experiment with this cache logic the more I feel a bit uneasy about it. The concerns that come up include:

  • There is no way to safely expire some cache values. Galleries that are dependent on the cache have to store their related XML as CACHE_PERMANENT. This is because we we can't regenerate the XML cache while any other caches (Drupal and external) are holding on to a cache of the gallery's page, and we have no control over the duration of those other caches. This means these entries will sit the bin for a really long time, and anytime a gallery's images or conf is changed the cache will just grow without a constraint.
  • Related the point above while we have obsolete cache values in the bin we need to do something special to validate them to ensure that they cannot be used. Without doing this we may be exposing old revisions of entity data to people using old query strings (cache ids) in their XML URL. Without some validation I suppose this could be considered an access bypass. While implementing some kind of validation may be possible, it could be pretty complex to handle correctly.

So I'm just a bit uneasy about things and will probably continue to look into some other ideas.

  • rjacobs committed 7a873b6 on 8.x-2.x
    Issue #2401389 by rjacobs: Add sub-request XML fetching for non-standard...
rjacobs’s picture

Status: Postponed » Closed (won't fix)

I'm going to go with a "sub-request" concept to solve this problem, as outline in #2401389: Consider alternate ways to store and generate gallery XML. It should be much cleaner to do it that way. As a result, this can be closed.