On Sun, May 29, 2011 at 1:59 PM, elij <elij...@gmail.com> wrote: > On Sun, May 29, 2011 at 7:27 AM, Lukas Fleischer > <archli...@cryptocrack.de> wrote: >> On Sat, May 28, 2011 at 04:17:09PM -0700, elij wrote: >>> + if(EXTENSION_LOADED_APC) { >>> + $ret = apc_fetch(APC_PREFIX.$key, $status); >>> + if ($status) { >>> + return $ret; >>> + } >>> + } >>> + return $status; >>> +} >> >> I'd prefer to change get_cache_value()'s signature to return the status >> indicator and pass the actual result by reference. That way, it could be >> used as follows: >> >> ---- >> if (get_cache_value('foo', $foo)) { >> do_something $foo >> } >> ---- >> >> That just feels much more common and convenient. Any objections? > > That seems a bit unconventional, considering the existing codebase and > common php practices. While it might make for the occasional nice > conditional-if test, I think it is more of a leaky abstraction than > the existing method. > > I made an attempt to match the api signature for apc_fetch and > memcache::get, so that I had to change as little calling code as > possible (while making the api somewhat expected). > > The version as written behaves like the existing apc_fetch: > http://www.php.net/manual/en/function.apc-fetch.php > > And purposefully similarly to memcache::get (if no bool reference is > passed in to get_cache_value): > http://us.php.net/manual/en/memcache.get.php > > Memcache::get returns false on failure, which can be problematic if a > falsey value was stored in memcache, and you were trying to get it out > (and test that retrieval succeeded). Passing a bool by reference fixes > that case. > > You _can_ use a convention like this: > > if(!($foo = get_cache_value('bar'))) { > // do stuff > } > > As get_cache_value on failure *also* returns a falsey value, but this > runs into the memcache api problem of what happens if you want to > retrieve a falsey value. If you know that either you never store a > falsey value for that key, or if your conditional test is appropriate > assuming a falsey value stored (eg. if the condition is false due to > failure or retrieval of a falseyness, the expected behavior is the > same), then that convention works fine. > > Passing a data container by reference (your suggestion) would also > work fine, but I don't see that very often in practice. I am probably > not very current on php conventions though, and I am using the php > documentation as a reference for 'best practices for api signatures'. > Which may be a fools errand to some extent. ;) > > Do you make use of the 'pass data container by reference' convention > regularly, or see it commonly used? > >>> function updates_table($dbh) >>> { >>> - global $apc_prefix, $apc_ttl; >>> - $key = $apc_prefix . 'recent_updates'; >>> - if(!(EXTENSION_LOADED_APC && ($newest_packages = apc_fetch($key)))) { >>> + $key = 'recent_updates'; >>> + if(!($newest_packages = get_cache_value($key))) { >> >> Any reason to use an additional variable for the key here? > > Nope. That was just how it was, and I missed changing it to inline. > Note the diff.
Actually, I just looked at the file, and the key is used twice (slightly later in the same function). At the time I probably thought it was better to have a variable than multiple instances of the same string. >>> $q = 'SELECT * FROM Packages ORDER BY ModifiedTS DESC LIMIT >>> 10'; >>> $result = db_query($q, $dbh); >>> >>> @@ -43,26 +13,23 @@ function updates_table($dbh) >>> while ($row = mysql_fetch_assoc($result)) { >>> $newest_packages->append($row); >>> } >>> - if (EXTENSION_LOADED_APC) { >>> - apc_store($key, $newest_packages, $apc_ttl); >>> - } >>> + set_cache_value($key, $newest_packages); >> >> Looks like you introduced a whitespace mistake here. > > Ah yes. I have vim set to use tabs on php files, but the inc files do > not end in php... :/ > Which as an aside is not a good practice. If for some reason there was > a server configuration error, the inc files would be served up as > plain text. It is best practice to have config files (such as > config.inc) end in .php so they will be rendered in the off chance the > file is exposed. The include files should probably be renamed. > > I can fix this one line of whitespace and resubmit if desired. >