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

[elij]

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

>>               $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.