Issue #2854: Permit ViewableData children to avoid caching #2855

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@g4b0

Useful for frontend Session work. The issue: #2854

@halkyon
SilverStripe Ltd. member

There's already an argument $cache = false to obj() on ViewableData. Could that not be re-used? Perhaps that's used for something else though.

In any case, if we have a property for disabling caching, I'd rather see it as $cached = true, the naming of "avoid" isn't consistent with the way caches are enabled and disabled in other places in the framework.

@g4b0

Using $cached = false doesn't solve because a call to $product->Quantity (#2854) anywhere in the code will cache the result. If then I store the $product in session, and in a new request I change the Quantity value, there is no way to get the new value, since the cached version is always returned.

I agree with you with the naming convention, pull request is coming..

@g4b0 g4b0 commented on the diff Feb 18, 2014
view/ViewableData.php
@@ -354,7 +361,7 @@ public function renderWith($template, $customFields = null) {
public function obj($fieldName, $arguments = null, $forceReturnedObject = true, $cache = false, $cacheName = null) {
if(!$cacheName) $cacheName = $arguments ? $fieldName . implode(',', $arguments) : $fieldName;
- if(!isset($this->objCache[$cacheName])) {
@g4b0
g4b0 Feb 18, 2014

Once cached there's no way to get the original value, also if it changes

@g4b0

Changed $avoidCaching in $cached

@simonwelsh

Tests?

@simonwelsh simonwelsh added the 3.1 label Mar 15, 2014
@g4b0

How can I unit test Session behaviour through a cli script? Anyway, it's just a single line of code checking with OR a variable that defaults to false (not true), so the normal code flow results unchanged.

Setting $cached=false simply skip caching, so the result will be always the same, maybe a little bit slower.

@simonwelsh

You're not testing Session behaviour, you're testing ViewableData behaviour.

You'd test that having the flag set to true causes obj() to load from the cache and that setting it to false causes it to be recalculated.

@g4b0

Ok, just pushed the test

@g4b0

Any news about including this PR? I just upgraded my project to SS 3.1.5 and I really don't like to hack the core...

@tractorcow

Here's a minor complaint...

if an object has $this->cached = false, should it still be caching the result of obj (even though that cached value is never accessible while cached = false). If we re-enable $this->cached = true, should we expect those supposedly uncached values to be accessible?

@g4b0

If $this->cached = false the object will never be cached, if we re-enable $this->cached = true then the first time it will be rendered by the template it will be cached, and then the actual cache behaviour will work.

If we put $this->cached = false again in the next execution the $obj will be generated and not picked from the cache, and so on...

This PR is mainly addressed for those using ViewableData objects in sessions, indeed its default is the actual behaviour, that is not session safe. Have a look here #2854 (comment) for more details.

[edit]
I re-read your comment, maybe now I undestand it better :)
Are you speaking about this line of code https://github.com/g4b0/silverstripe-framework/blob/viewable-data-cache-issue/view/ViewableData.php#L386 ?

Maybe it should be modified in

if($cache && $this->cached) $this->objCache[$cacheName] = $value;
@tractorcow

Better way: Refactor out the caching part of ViewableData:: obj into its own function. It'll make it more readable, and you can still override it if necessary.

Less api please. :)

@g4b0

I don't think I have a deep enough knowlege of ViewableData to refactor ViewableData:: obj. Any help from the core dev team?

@g4b0

Hey, any news about this PR?

@tractorcow

@dhensby has been working on an updated ViewableData for 4.0. Maybe see whether he can incorporate this into his pr?

@tractorcow

This PR probably couldn't go into 3.1 anymore... since we've adopted semantic versioning any new api should probably go into 3.2 (3 branch) at the least.

@dhensby
SilverStripe Ltd. member

@g4b0 I need to look at this a bit. I'm not sure I fully understand what the problem is here (mostly because I haven't familiarised myself with this issue).

NB: the ViewableData stuff I'm working on is here #3924

@dhensby dhensby self-assigned this Feb 24, 2015
@g4b0

@dhensby the problem is simple: if you store a DataObject in session (for example an Ecommerce Cart), cached values will override real values.
An example in my issue: #2854

@tractorcow

It would be better to allow caching to be overridden, rather than simple disabled or enabled as needed via a flag. That way you could implement a better caching mechanism in your subclass, if the default isn't useful enough.

@tractorcow

Also, refactoring the caching into a protected method could make this PR still eligible for 3.1 (since it's a simple refactor, rather than a new mechanism).

If you can't break the rules then bend them as far as you can. :D

@g4b0

It's not so easy, since SS internals calls ViewableData::obj that caches the DO whit it's own logic, so which method are you thinking to refactor?

@tractorcow

Check out #3955

Its preferable to customise behaviour using code, rather than flags with explicit behaviour coded in behind it, as the developer has a lot more control over how the overriding class behaves. It some cases it may not be sufficient to simply turn caching on or off, but rather it may be necessary to customise other elements of the caching mechanism (e.g. the caching store, lifetime, etc).

@tractorcow

Moved this PR to the 3 branch (3.2) #3985

@dhensby dhensby closed this Sep 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment