[PHPTAL] [PATCH] Shouldn't skip over __get() just because __isset() was defined…

[Kornel Lesiński]

On Tue, 28 Jun 2011 18:44:49 +0100, Terin Stock <terinjokes at gmail.com>  
wrote:

> isset() isn't defined for lazy-loading properties, they're either set or  
> not set.

IMHO lazy-loading is an implementation detail that should not be visible  
to isset().

The state you have is "property is there and it has a value, but the value  
is still in the database", but your __isset() says "property is not there  
or has no value", which is a different thing.

> Not "not currently set, but could be in the future". Therefore, is it  
> correct or incorrect to define it one way or another in our  
> application's objects?

If you're saying that property is not set (yet), then PHPTAL will not use  
it (yet). I don't think it's safe for PHPTAL to assume that every unset  
property will appear when __get() is used.

> In the current implementation, PHPTAL makes an active attempt at  
> stopping me from having __isset defined in a way logical for the rest of  
> my application.

It may be convenient for your application, but I don't think that's good  
solution for all applications that use PHPTAL. I believe consistency with  
isset() and ability to avoid double-checking with __get() is more  
important.

I think your application should use different method for exposing  
lazy-loading state (e.g. isLoaded($property)) and use __isset() for  
reporting existence of properties whether they're loaded or not.

> My patch fixes that, and doesn't change anything if you have decided to  
> implement __isset the other way.

Thank you for your patch. It's not fun to reject patches, I'm sorry for  
that.

You're of course free to use modified version of PHPTAL if you wish.

-- 
regards, Kornel Lesiński