Re: [aur-dev] [PATCH 1/2] refactor apc code and move to aur.inc

On Sun, May 29, 2011 at 1:59 PM, elij wrote:

On Sun, May 29, 2011 at 7:27 AM, Lukas Fleischer 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.