Re: services_libravatar caching

Hello Stelian,


> Changes are now in my fork, please have a look.
> >> 3. Please move DriverCacheLite.php to Driver/CacheLite and rename
> >> the class accordingly.

1. I'm a bit disappointed that you don't know about the PEAR
Class-to-filename conventions. They are even publicly accepted via
PSR-0:
> $filename = str_replace($classname, '_', '/') . '.php'

So Services_Libarvatar/Cache_Driver should be in
Services/Libravatar/Cache/Driver.php

For including the files, you assume that the file with its full is in
include path:

> require_once 'Services/Libravatar/Driver.php';

No __DIR__ and "../" at all.

Btw, all classes in that package need to be prefixed with
Services_Libravatar - so 

2. There should be no default driver set, or at least a "null" driver
   that does no caching at all. Currently Cache_Lite is required;
   something that I would not want at all.

3. protected $dnsCache has "@var mixed". Why mixed? Shouldn't all
   drivers implement the Services_Libravatar_Cache_Driver interface
   Or is null allowed? Replace "mixed" with
   "Services_Libravatar_Cache_Driver"

4. "@param int $time - length in seconds". Please remove the "-", it's
   not necessary and not used anywhere else.

5. "@return bool;" Please describe what the return value means, e.g.
   "true if saving succeeded, false if not"

6. "@return null;" This needs to be "@return void", to indicate no
   return value. You also do not need to add "return null;" to that
   code; PHP does that automatically anyway.

7. "Class available since Release @package_version@"
   This would always update the package version, which is not what we
   want :) The cache driver is a new feature, so the next
   Services_Libravatar version containing it will be 0.3.0. Set this
   version hard in there.

8. I'd like to be able to change the settings of the Cache_Lite object.
   So please either allow passing a Cache_Lite object to the Driver
   constructor, or make a getCacheLite() method in that driver.



-- 
Regards/Mit freundlichen Grüßen
Christian Weiske

-=≡ Geeking around in the name of science since 1982 ≡=-

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAlCNgkoACgkQFMhaCCTq+CMlDwCgtvnV2j9xZZcfhOIMPpUaVviR
nmsAn1zc0oAnEh1JydsfYjdoyA6LLQEp
=Y1qt
-----END PGP SIGNATURE-----