[PEPr] Proposal for PHP::phpfastcache

[Khoa Bui]

Khoa Bui (http://pear.php.net/user/khoaofgod) proposes PHP::phpfastcache.

You can find more detailed information here:
http://pear.php.net/pepr/pepr-proposal-show.php?id=692

[Bertrand Mansion]

Hi,



I only looked quickly at the code.



I think that there is room for improvements. The 'one class does it all'
concept is bad here because it reduces your code reusability,
extendability, flexibility and creates maintenance problems. It makes it
painful to add a new backend (redis, apcu, tokyotyrant, mongodb, ...) or
modify an existing one.



You could improve it by looking at the current Cache_Lite or Cache
packages, and investigating design patterns like Bridge and maybe Chain of
responsability in case you want to keep the possibility to have multiple
backends.



http://en.wikipedia.org/wiki/Bridge_pattern

http://en.wikipedia.org/wiki/Chain_of_responsibility_pattern



You could allow backends to register themselves as available upon loading.
You seem to have defined your own order of priority for backends, this
should maybe left as a parameter for the user.



You use static functions for set/get. This is also wrong because it doesn't
allow to have more than one cache. You should provide a factory method
instead, maybe a singleton method if necessary. See the Log package, it is
a good example of an API you could use.



In conclusion, your code probably works fine, but it is poor design.

[Michael Gauthier]

After looking at the code, I agree with Bertand. This class is doing too
much. For example, emailing error messages does not belong in a cache
library.



There is definitely room in PEAR for a cache library, but the code needs to
be split into multiple reusable classes rather than one monolithic static
class.



Serializing cache data is potentially a security issue as a vulnerability
in any backend now has the potential to execute code on the server.

[Khoa Bui]

hi guys, I'm Khoa.



I agree with Bertrand Massion too, my code need improve design.



I just created new plan for next version.



I just looked at other classes, and I see I have to learn more from them on
patterns design.



Right now, my code is work good, but it is painful to add a new backend or
modify an existing one.



Thanks for all advises,



I will propose my next version on couple months soon.

[Michael Gauthier]

Post by Khoa Bui
hi guys, I'm Khoa.
I agree with Bertrand Massion too, my code need improve design.
I just created new plan for next version.
I just looked at other classes, and I see I have to learn more from them on
patterns design.
Right now, my code is work good, but it is painful to add a new backend or
modify an existing one.
Thanks for all advises,
I will propose my next version on couple months soon.

Sounds great. I look forward to reviewing the new version.

Cheers,
Mike

[Bertrand Mansion]

Post by Khoa Bui
hi guys, I'm Khoa.
I agree with Bertrand Massion too, my code need improve design.
I just created new plan for next version.
I just looked at other classes, and I see I have to learn more from them on
patterns design.
Right now, my code is work good, but it is painful to add a new backend or
modify an existing one.
Thanks for all advises,
I will propose my next version on couple months soon.

Hi Khoa,

If you need help with design patterns, api and stuff, don't hesitate to ask on the list.
There is definitely a room for a good caching library in PHP that works with 5.3+ and modern backends.

Regards,

--
Bertrand Mansion
Mamasam

[Khoa Bui]

thanks Mansion for some example on the comment.
I have created the list of what I need to improve already.

Yeah, I like to learn new thing too. PHP 5.3 is amazing on new things, but I want to make it compatible with PHP 5.1+ - It hard cause 5.1 is less magic function.
However, I will make a new version first, before I ask u guys for help on stuff.

Today is a day I learn many new things. Hehehehe. From your comment, I know MongoDB , and this is first time I see a NoSQL database, interesting.

Thanks Dude, I will not change my project status to Votes until I finish my next version.


________________________________
From: Bertrand Mansion <***@mamasam.com>
To: Khoa Bui <***@yahoo.com>
Cc: PEAR developer mailinglist <pear-***@lists.php.net>
Sent: Monday, May 13, 2013 2:56 PM
Subject: Re: [PEAR-DEV] [PEPr] Comment on Caching::phpfastcache

Post by Khoa Bui
hi guys, I'm Khoa.
I agree with Bertrand Massion too, my code need improve design.
I just created new plan for next version.
I just looked at other classes, and I see I have to learn more from them on
patterns design.
Right now, my code is work good, but it is painful to add a new backend or
modify an existing one.
Thanks for all advises,
I will propose my next version on couple months soon.

Hi Khoa,

If you need help with design patterns, api and stuff, don't hesitate to ask on the list.
There is definitely a room for a good caching library in PHP that works with 5.3+ and modern backends.

Regards,

--
Bertrand Mansion
Mamasam

[Lester Caine]

Post by Khoa Bui
Yeah, I like to learn new thing too. PHP 5.3 is amazing on new things, but I want to make it compatible with PHP 5.1+ - It hard cause 5.1 is less magic function.
However, I will make a new version first, before I ask u guys for help on stuff.

While keeping compatibility with 5.1 may seem a good idea, I'd strongly suggest
that you treat it like PHP4 and pretend it does not exist. One of the problems
with clean new code is throwing in stuff which has no place going forward.
Personally I wish they had simply called PHP5.3 '6' and had done with it. In
reality it's that different in areas that it should be considered a major
version. PHP5.1 is no longer supported, and should be treated as such.
--
Lester Caine - G8HFL
-----------------------------
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

[Khoa Bui]

Khoa Bui (http://pear.php.net/user/khoaofgod) has initiated the call for votes on Caching::phpfastcache.

Please review the proposal and give your vote here:
http://pear.php.net/pepr/pepr-proposal-show.php?id=692

[Bertrand Mansion]

Bertrand Mansion (http://pear.php.net/user/mansion) has voted -1 on the proposal for Caching::phpfastcache.

Proposal information:
http://pear.php.net/pepr/pepr-proposal-show.php?id=692
Vote information:
http://pear.php.net/pepr/pepr-vote-show.php?id=692&handle=mansion

Comment:

I vote -1 because your code is not ready yet, in my opinion.



Have a look at :



- accessors methods to set/get options in drivers

- call_user_func_array instead of eval()

- You might use __invoke() instead of your first function

- use an autoloader for drivers or you can load all drivers at once instead
of hidding require_once in methods, and get rid of isExistingClass...

- follow pear coding standards

- use an interface for drivers and maybe a common abstract class

- spelling mistakes in sqlite driver

- isExistingClass ? heard of
http://php.net/manual/en/function.class-exists.php

- define property and methods scopes explicitly

- rename $this->method to $this->driver or storage

- don't die(), use Exceptions or trigger_error

- have each driver define the options it needs, not the main class

- clean up your code at the bottom of the class

- You might have to rename your class if you want it to fit PEAR standards
(there is already 2 cache packages in pear)

I haven't looked at the drivers code, there might be things to fix there
too.

[Khoa Bui]

Oh yeah.
Thanks Bretrand again for your comments. Now i Have a list to work on it ^_^

Will get back to u guys again, when I am ready again with this one.

Next time, I will send you the code first, before I post on pear, Bretrand. Will you look at it next time for me?


________________________________
From: Bertrand Mansion <***@mamasam.net>
To: PEAR developer mailinglist <pear-***@lists.php.net>
Cc: Bertrand Mansion <***@mamasam.net>; Khoa Bui <***@yahoo.com>
Sent: Monday, August 5, 2013 4:51 PM
Subject: [PEPr] -1 for Caching::phpfastcache


Bertrand Mansion (http://pear.php.net/user/mansion) has voted -1 on the proposal for Caching::phpfastcache.

Proposal information:
http://pear.php.net/pepr/pepr-proposal-show.php?id=692
Vote information:
http://pear.php.net/pepr/pepr-vote-show.php?id=692&handle=mansion

Comment:

I vote -1 because your code is not ready yet, in my opinion.

Have a look at :

- accessors methods to set/get options in drivers
- call_user_func_array instead of eval()
- You might use __invoke() instead of your first function
- use an autoloader for drivers or you can load all drivers at once instead
of hidding require_once in methods, and get rid of isExistingClass...
- follow pear coding standards
- use an interface for drivers and maybe a common abstract class
- spelling mistakes in sqlite driver
- isExistingClass ? heard of
http://php.net/manual/en/function.class-exists.php
- define property and methods scopes explicitly
- rename $this->method to $this->driver or storage
- don't die(), use Exceptions or trigger_error
- have each driver define the options it needs, not the main class
- clean up your code at the bottom of the class
- You might have to rename your class if you want it to fit PEAR standards
(there is already 2 cache packages in pear)
I haven't looked at the drivers code, there might be things to fix there
too.

[Bertrand Mansion]

Hello Khoa,

I am a bit short on time so I'd prefer if you post your code on Pear list to have others review it as well. My suggestions are not always feasible, so you'll have to see for yourself.

Regards,

Bertrand

Post by Khoa Bui
Oh yeah.
Thanks Bretrand again for your comments. Now i Have a list to work on it ^_^
Will get back to u guys again, when I am ready again with this one.
Next time, I will send you the code first, before I post on pear, Bretrand. Will you look at it next time for me?
Sent: Monday, August 5, 2013 4:51 PM
Subject: [PEPr] -1 for Caching::phpfastcache
Bertrand Mansion (http://pear.php.net/user/mansion) has voted -1 on the proposal for Caching::phpfastcache.
http://pear.php.net/pepr/pepr-proposal-show.php?id=692
http://pear.php.net/pepr/pepr-vote-show.php?id=692&handle=mansion
I vote -1 because your code is not ready yet, in my opinion.
- accessors methods to set/get options in drivers
- call_user_func_array instead of eval()
- You might use __invoke() instead of your first function
- use an autoloader for drivers or you can load all drivers at once instead
of hidding require_once in methods, and get rid of isExistingClass...
- follow pear coding standards
- use an interface for drivers and maybe a common abstract class
- spelling mistakes in sqlite driver
- isExistingClass ? heard of
http://php.net/manual/en/function.class-exists.php
- define property and methods scopes explicitly
- rename $this->method to $this->driver or storage
- don't die(), use Exceptions or trigger_error
- have each driver define the options it needs, not the main class
- clean up your code at the bottom of the class
- You might have to rename your class if you want it to fit PEAR standards
(there is already 2 cache packages in pear)
I haven't looked at the drivers code, there might be things to fix there
too.

[Stelian Mocanita]

Stelian Mocanita (http://pear.php.net/user/stelianm) has voted -1 on the proposal for Caching::phpfastcache.

Proposal information:
http://pear.php.net/pepr/pepr-proposal-show.php?id=692
Vote information:
http://pear.php.net/pepr/pepr-vote-show.php?id=692&handle=stelianm

Comment:

Along with the issues/recommendations Bertrand exposed in his vote, I would
also like to add:



- getOS is defined twice

- a lot of file operations without any error handling (fopen, opendir,
etc)

- not entirely sure why you need to call an external api call in this
particular case, but since you have it there, file_get_contents is #1
blocking and #2 it emits and E_WARNING.



As a side note, when you link to github, redirect me there and do not ask
for my e-mail.