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

[elij]

On Sun, May 29, 2011 at 1:59 PM, elij <elij.mx at gmail.com> wrote:
> 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.

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