FIX: Repeated template lookups were not cached (fixes #4626)

[kinglozzer]

Hopefully the test makes a bit more sense than my wall of text in #4626. Basically, <% with $Foo %>, <% loop $Foo %> and {$Foo.Bar} are not cached.

As far as I can see in git history, they never have been explicitly cached. However, they were somewhat accidentally cached in 3.1. This is what happened in 3.1:

• <% if $Foo %> looks up Foo and then caches the result
• <% with $Foo %> would not be cached, but as the result was already in the cache from the <% if %> it would be pulled out anyway
  Refactor ViewableData::obj caching #3985 changed this behaviour, meaning that now if $cache is set to false, it avoids the cache - even if Foo is already present in the cache.

I don’t like using regular expressions to add a fourth parameter to obj() calls, but I can’t see any way to avoid it. Its method signature is different to XML_val() and hasValue(), yet it looks like the template parser expects it to be identical.

cc @hafriedlander

[tractorcow]

This looks like risky code. You're using a regexp to hack in an additional parameter to a method call?

[tractorcow]

Ok, I see you've addressed this in your PR description. I guess we should get @hafriedlander to look at it.

[kinglozzer]

Yeah, not ideal :(. I’m sure it’s possible to achieve the same output some other way, I’m just not anywhere near familiar enough with the parser to do that sort of refactoring!

[tractorcow]

I think we should fix this in 4.0. Also see #3924 for some of the work @dhensby has been doing with casting helpers.

Essentially we should REMOVE $forceReturnedObject, always return an object, and make both ViewableData::obj and ViewableData::*_val have the same method signature. Does that remove the need to hack in the extra param and special case for $method === 'obj'?

Alternatively, we could just revert the caching logic for 3.2. I would prefer not changing it again at this stage, though, since it's actually working predictably.

[tractorcow]

Tentatively setting milestone for this issue to 4.0 (even though it's a pr against the 3.2 branch).

[kinglozzer]

Essentially we should REMOVE $forceReturnedObject, always return an object, and make both ViewableData::obj and ViewableData::*_val have the same method signature. Does that remove the need to hack in the extra param and special case for $method === 'obj'?

Yes, making both method signatures identical would solve the issue (and remove the need for either hack). We can definitely fix this (for master) a lot more cleanly if we do it that way.

I would prefer not changing it again at this stage, though, since it's actually working predictably.

The API in ViewableData is more predictable but the way it works in “template-land” is, if anything, less predictable than 3.1 as a result. I agree that we probably shouldn’t revert that commit, though.

My concern is that this is potentially a BC breaking issue when upgrading to 3.2 as this might cause unexpected repeated queries etc with no notice of what’s happening or why: it took me some time to track down why my example wasn’t working in 3.2 but was working in 3.1.

It’s not a critical issue - it’s possible to work around it, at least for my example - but it’s a pain and is potentially a (minor) performance issue depending on what data is being fetched, so I’d like to see this fixed for 3.2 in some way if at all possible.

[tractorcow]

Hm, it might be a bit late for 3.2.0, since it's in rc2 at the moment. =(

[sminnee]

Shall we rebase this against master and merge it in, @tractorcow @kinglozzer?

[kinglozzer]

I think it’s best to wait for #3924 to be merged before looking at this again. Either way this won’t be merged into 3.2, so closing

[tractorcow]

I'm in favour of removing $forceReturnedObject in master. :)