Your question triggered some discussion here. Hopefully nobody minds me just
pasting it here FYI. It's raw and a bit chaotic but hopefully you get some
feeling what we were talking about.
Regards
Tomas Liubinas
[14:27:56 | Pakeitė 14:28:06] Tomas Liubinas: could anyone pls provide any
arguments for Christopher regarding oxSuperCfg?
[14:31:14] Tomas Liubinas: I can't because mostly I tend to agree with him.
[14:31:50] [ox] Sarunas Valaskevicius: I basically agree also. but.. unit tests
[14:32:08] [ox] Sarunas Valaskevicius: it would be hard to make unit tests
which use singleton directly
[14:33:28 | Pakeitė 14:34:02] [ox] Sarunas Valaskevicius: also - another
approach would be to pass config as param to class constructors (or
functions).. but then this may seem redundant and not very nice code, since
oxConfig does change, also oxSession and other classes.. would be not nice code
[14:33:58] Tomas Liubinas: why it is hard to use in unittests?
[14:34:28] [ox] Sarunas Valaskevicius: e.g.
func aa(){
oxConfig::getInstance()->asasd();
}
[14:34:35] [ox] Sarunas Valaskevicius: you can not mock it
[14:34:42] [OXID] Jürgen Gramenz: private and static...thats evil
[14:34:47 | Pakeitė 14:35:31] [ox] Sarunas Valaskevicius: except using
modConfig - which is also not really nice - some code for binding it (basically
unit tests code) resides in getInstance
[14:35:58 | Pakeitė 14:36:27] [ox] Sarunas Valaskevicius: and
func aa(){
$this->getConfig()->asasd();
}
can be mocked normally
[14:36:16] [ox] Sarunas Valaskevicius: this is the only reason I guess it was
implemented like this
[14:37:40 | Pakeitė 14:39:19] [ox] Sarunas Valaskevicius: private and static
not always evil (these are intended language constructs) - but it is evil in
eShop, yes :) (because of modules). (i.e. not overusing it, brings less complex
API [private], while static is for static data :) )
[14:39:37] [OXID] Jürgen Gramenz: no, try to test a real singleton (i mean not
only getInstance )...f.i. if you would like to test privacy of __clone() you'll
have to use reflection...the real heck is the static
[14:40:09] [OXID] Jürgen Gramenz: you cannot unset this... and this is a bit
tricky if you would like to test different cases....
[14:40:50] [OXID] Jürgen Gramenz: i mean....i would be glad if someone could
show me a real good unittesting of a singleton
[14:44:22] [ox] Sarunas Valaskevicius: btw - oxSuperCfg contains lots of static
[14:45:01 | Pakeitė 14:46:35] [ox] Sarunas Valaskevicius: it is composed of
static variables only :)
[14:47:47 | Pakeitė 14:48:25] [ox] Sarunas Valaskevicius: so it is basically
same getInstance, with a shortcut through this objects method - which is only
really needed for unit tests (plus it caches instance twice - in oxConfig and
in oxSuprtCfg)
[14:49:21] [ox] Sarunas Valaskevicius: " real good unittesting of a singleton "
- test object initialization and caching of instance - getInstance does not do
more
[14:49:54 | Pakeitė 14:49:59] [ox] Sarunas Valaskevicius: for other methods -
you can create object new and mock it, it is not needed to use getinstance in
unit tests
[14:50:53 | Pakeitė 14:52:23] Tomas Liubinas: imo it is no more dificult to
test than any other class having static method. And it looks like we can not
get away without using no static at all
[14:52:03] [ox] Sarunas Valaskevicius: yes - static is ok, if used for static
data.
another problem with singletons - they tend to be big. and yes - oxConfig is
huge.. (maybe too huge)
[14:52:03] [OXID] Jürgen Gramenz: please tell us about your testing strategies
for singeltons and/or static vars...i think it would be useful for efire people
[14:54:55] [ox] Sarunas Valaskevicius: like I said,
func aa(){
oxConfig::getInstance()->asasd();
}
is a problematic case, (described above), unless there would be setInstance :)
[14:55:15] Tomas Liubinas: this is how we mock it:
[14:55:38] Tomas Liubinas: oxConfig::getInstance():
[14:55:47] Tomas Liubinas:
/**
* Returns singleton oxConfig object instance or create new if needed
*
* @return oxConfig
*/
public static function getInstance()
{
if ( defined( 'OXID_PHP_UNIT' ) ) {
if ( isset( modConfig::$unitMOD ) && is_object( modConfig::$unitMOD
) ) {
return modConfig::$unitMOD;
}
}
if ( !self::$_instance instanceof oxConfig ) {
//exceptions from here go directly to global exception handler
//if no init is possible whole application has to die!
self::$_instance = new oxConfig();
self::$_instance->init();
}
return self::$_instance;
}
[14:57:30] Tomas Liubinas: etc :)
[14:57:50] [OXID] Jürgen Gramenz: hmm, looks like phpunit driven design ;)
[14:57:55] [ox] Sarunas Valaskevicius: this is the case " which is also not
really nice - some code for binding it (basically unit tests code) resides in
getInstance"
[14:58:28] [OXID] Jürgen Gramenz: but interesting way
[14:58:41 | Pakeitė 14:58:53] [ox] Sarunas Valaskevicius: ghmm.. maybe it
should be changed to setInstance($obj) method?
[14:59:06] [OXID] Jürgen Gramenz: dependency injection would be perfect
[14:59:10] [OXID] Jürgen Gramenz: but
[14:59:22] [OXID] Jürgen Gramenz: is it a singleton after this?
[14:59:53] [ox] Sarunas Valaskevicius: aha :) by making self::$_instance
protected it is possible even without using setInstance of unit code in
getinstance
[15:00:33 | Pakeitė 15:00:38] [ox] Sarunas Valaskevicius: class, extended in
unit tests, can have setInstance, and set to protected oxConfig::$_instance
[15:00:43 | Pakeitė 15:01:08] [ox] Sarunas Valaskevicius: so shop code would be
cleaner
[15:10:56 | Pakeitė 15:16:35] [ox] Sarunas Valaskevicius: ghmz.. passing
through constructor con - memory usage increase (+ a bit of time to assign it
locally) for every object created..
[15:20:00] Tomas Liubinas: I guess Erik will ask first of all why we using
sigeltons at all :)
[15:20:22] Tomas Liubinas: So the reason: It is not possible to fit all classes
under one hierarchy tree
[15:21:12] [ox] Erik Kort: @Tomas: yes .. I will ask .. but wait ..
[15:21:14 | Pakeitė 15:22:57] [ox] Erik Kort: BTW: Singleton is consider by
some very knowledgable people to be more of an anti-pattern then as an design
pattern .. I tend to agree with that .. You are putting 2 independent purposes
in a class. A) its intended purpose and B) the global once instantiation. You
are making basically a global variable again .. There are better ways for that.
Answer to testing: dependency injection solves _alot_ of testing problems, and
actually makes you classes more flexible at the same time ..
BTW2: Static functions should also only be used in exceptional cases.. as it is
procedural code again .. no real OO anymore .. you basically just have an
function namespace... if you use non-public static members to much to try to
achieve the OO a bit again, you lose your slight performance advantage you had
and your code gets to tightly coupled.. Meaning that it gets hard to refactor
:) .. see some of the efire static classes .. ....
its way over used in the PHP world because people think it is significantly
faster. Its actuall a bit faster .. not that much .. But most of the time
(>95%) of runtime is lost on other stuff .. (DB access, file access, network
access and in general amount of code in total to run)
BTW3 .. In eFire we dont make any new Singleton classes anymore .. We still
have legacy classes using them.. Same for static ... newer classes hardly get
static functions anymore. .. Legacy classes who are primerally like function
namespaces are still maintained in that design, but no newer....
[15:21:20] [ox] Dainius Bigelis: wow
[15:23:19] [ox] Erik Kort: BTW .. we should have discussed this in the
DEV-GENERAL mailing list :D
[15:24:34] [ox] Sarunas Valaskevicius: we can paste it., as this way discussion
is a lot faster
[15:24:42] [ox] Sarunas Valaskevicius: (no irc channel yet :) )
[15:28:58] [ox] Sarunas Valaskevicius: what about "passing through constructor
con - memory usage increase (+ a bit of time to assign it locally) for every
object created.." ? is it in 5% performance loss we should not care about ?
[15:29:09] [ox] Erik Kort: BTW .. I propose to _replace_ skype in the
development with IRC .. so we only have one tool for internal and external
chats ... What do you think ? (Nettalk is a good Win client for that ..)
[15:30:13] [ox] Erik Kort: @Sarunas: yes dont care about it .. because its 5%
OR LESS THAN 5% .. see the "(>95%) runtime" part of text
[15:30:46] [ox] Erik Kort: when you you have perfomance problems you profile..
you will always find way bigger peerfomance losses then the static calls ..
[15:31:40] [ox] Sarunas Valaskevicius: most of the time these bigger calls do
more functionality, and summing memory and time used for DI, could be quite
much not to notice
[15:32:09 | Pakeitė 15:32:36] [ox] Erik Kort: and if you actuall have found
one, its a case where recursivly millions of time a function is called ... That
you can make static then .. But I have seen one yet :) .. And to screw up the
OO design for that?, .. no thanks .
[15:33:53] [ox] Erik Kort: With good OO design comes good maintainability of
code .. THATS is directly saved time and therefore $$$ for the company ..
[15:34:06] [ox] Sarunas Valaskevicius: true
[15:34:35] [ox] Erik Kort: Besides, I enjoy programming more if it is good OO
code ;)
[15:36:58] Tomas Liubinas: this discussion is a good argumetn for irc
[15:38:57] Tomas Liubinas: well ok but here comes the practical side again.
[15:39:33] Tomas Liubinas: This non singelton approach coud work for java when
you fit all the classes under one class hierarchy tree
[15:39:48] Tomas Liubinas: but we simply can't do that
[15:39:53] [ox] Erik Kort: so .. in the end .. we have the fact where we have
singletons and statics in shop & efire ... But its functional working .. so we
have to live with it ... But when we all keep this in mind, we can slowly ,
without taking actuallly extra time (tasks), refactor that together when we
need to change that class for another reason .. When we need to refactor that
class any way .. First thing for easy transition is decoupled design and code
.. Dont underestimate this .... with decoupled design can gradually refactor
... in eFire we now have rougly >1.000.000 of code and its partially not
decoupled... Those are parts we cant really refactor gradually today ..
[15:40:05] Tomas Liubinas: one of the reasons 3rd party libs
[15:40:14] Tomas Liubinas: we need config eg in smarty plugins
[15:40:26 | Pakeitė 15:40:36] [ox] Erik Kort: @Tomas: why cant we do that ?
and why do we need one class hierarchy if we want to prevent singletons ?
[15:40:54 | Pakeitė 15:41:24] [ox] Sarunas Valaskevicius: smarty plugins can
get vars from view -> so it is not that big problem
[15:41:13] [ox] Sarunas Valaskevicius: i.e. it is possible
[15:41:38] [ox] Erik Kort: yes... its possible to not use singletons without a
one class hierarchy ..
[15:41:57 | Pakeitė 15:42:09] Tomas Liubinas: unless all the libs comes in
packages like com.smarty.compiler we can't inject the var deep down to every
instantiated object
[15:42:02] [ox] Erik Kort: I want irc now ! :D
[15:43:22 | Pakeitė 15:43:50] [ox] Sarunas Valaskevicius: did not really
understood Tomas..
for our smarty plugins we can use this.. are there other 3rd party libs where
it is needed ?
[15:44:16] [ox] Sarunas Valaskevicius: ghmm.. maybe editor? would need to
check..
[15:44:29] Tomas Liubinas: smarty plugin is called from template by (I guess)
eval'ing some code, it's a simple function the only way to get config there is
singelton (or any other static peace )
[15:44:39] Tomas Liubinas: the same is with almost any other 3rd party lib
[15:44:48] [ox] Sarunas Valaskevicius: no - it gets whole smarty with all its
params
[15:44:50] [ox] Erik Kort: BTW We can use an global registry .. (I'm not sure
what the design pattern name for this is) ... Something like Zend_Registry
will do ... You have same affects as singleton .. (as for global only once
existence of an object) .. without making the class itself untestable or
limited ..
[15:45:15] [ox] Sarunas Valaskevicius: e.g.
function smarty_function_oxgetseourl( $params, &$smarty )
{
[15:45:25] Tomas Liubinas: @Sarunas - I use smarty just for illustration, the
same is with any other 3rd party lib
[15:45:44 | Pakeitė 15:47:50] Tomas Liubinas: Wordpress? Jumla? Drupal?
[15:45:49] [ox] Sarunas Valaskevicius: any other 3rd party lib which inludes
oxid code ?
[15:45:59 | Pakeitė 15:46:19] [ox] Erik Kort: Just heard .. the design pattern
ist called "Registry" .. :D thnx Jürgen ..
[15:47:00] [ox] Sarunas Valaskevicius: how registry is it different from
globals ?
[15:47:00] Tomas Liubinas: @Sarunas, yes we can solve smarty case, but we do
not want to limit ourselves to partuicular 3rd party libs
[15:48:43 | Pakeitė 15:50:09] [ox] Sarunas Valaskevicius: @Tomas: ok, so we
dont have the problem now. thus - we should not solve it now (limit our actions
by problem which is not here) .. or no ?
[15:48:45] Tomas Liubinas: Would anybody mind if I paste this conversation to
dev-general?
[15:49:38] [ox] Danijel Matkovic: <-- votes for irc too (smirk)
[15:52:37 | Pakeitė 15:52:56] [ox] Erik Kort: BTW .. Whats the difference
between an registry and a true global variable or singleton then, you might ask
? ..
welll, with a registry you have the global access buts its encapsulated /
decoupled .. You have an interface to it. Depency injection does works here ..
setup will inject a test object or mock/stub whatever you want in the registry
right before testing the function ... and voila .. its testable ...
[15:57:52 | Pakeitė 16:00:13] [ox] Sarunas Valaskevicius: @Erik: ok, clear. but
the same result is with singleton, whose value you can set in unit tests?
[15:58:28] [ox] Sarunas Valaskevicius: e.g. by making self::$_instance
protected it is possible even without using setInstance of unit code in
getinstance
class, extended in unit tests, can have setInstance, and set to protected
oxConfig::$_instance
[16:07:52] [ox] Erik Kort: @Sarunas: Then you still have all of the OO design
weaknesses of the singleton pattern itself ..
To insert a dummy object in to the registry is a on liner .. derive a extra
class to test is more .. and that is more coupled to class implementation
itself ..
[16:08:14] [ox] Erik Kort: mom in telko .
[16:10:40] [ox] Sarunas Valaskevicius: derived extra class is not used more
than just seting instance.. so it has only one one-liner function (setInstance)
and is not used anywhere else. i.e. objects does not get to use this derived
class.
about class coupling - yes, agree
[16:32:32] Tomas Liubinas: ok
[16:32:36] Tomas Liubinas: so lessons learned:
[16:32:47] Tomas Liubinas: - Dependency injection
[16:32:51] Tomas Liubinas: - Registry design pattern
[16:33:16] Tomas Liubinas:
http://blog.astrumfutura.com/archives/340-The-Zend-Framework,-Dependency-Injection-and-Zend_Di.html
[16:40:58] [OXID] Jürgen Gramenz: wowow, dependency injection container ... a
dream architecture....class instantiation via di container and putting it into
the registry and then di via getting the intantiated object from the
registry.... but thats a hell of a work to refactor... there is already an
interesting di container: pimple http://github.com/fabpot/Pimple/tree/master
... the only thing is...its for php 5.3...but the idea behind it and the class
architecture is brilliant in my opinion
[16:41:34] [OXID] Jürgen Gramenz: maybe its some useful info
[16:48:18] Tomas Liubinas: @Sarunas: you are arguing about the same thing. The
fact is that you can't mock singelton. And yes you can modify it, but I guess
the Registry does exactly this.
[16:52:35] [ox] Sarunas Valaskevicius: actually, I was not arguing :)
just noting the diferences between two approaches if using for just unit
testing (because shop does not override oxconfig instance).
current impl is singleton, the question was - why would registry be better for
shop oxconfig class. (and got answer - coupling)
[16:53:50] [ox] Erik Kort: Yes ...Now dont get into a singleton jihad and rip
them out everywehre were you see them :) .... Slowly and easy .. refactor when
the need arise .. getting a singleton out is more a design issue then as an
testing issue .. if you have a singleton in shop who you need to test _now_ ..
and if its not about to refactored today, use Sarnunas way of testing .. .. If
you are refactoring something and this class architecural would change anyway
.... think of getting the singleton out of it. ... BTW We dont have a Registry
yet in shop, do we ? ..
[16:55:15] Tomas Liubinas: we don't
[16:58:33] [ox] Erik Kort: so lets keep that in mind and define what kind of
regsitry would be good for the shop .. .. Lets discuss that another time ;)
[17:04:28] Tomas Liubinas: ack
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of
> Christopher Simon
> Sent: Friday, August 14, 2009 4:24 PM
> To: [email protected]
> Subject: Re: [oxid-dev-general] oxSuperCfg
>
> Hi Tomas,
>
> I think Singletons are definetly the State of the Art method
> to implement this functionality.
>
> Improvements compared to inherited methods from "god class":
>
> + no methods where they shouldn't be ($oOrder->isAdmin() <- makes no
> + sense) free "namespaces" for method names and therefore:
> + intuitive interface $oOrder->getUser() should user which made the
> + order
>
> Could you explain why your decision did go "pro" oxSuperCfg?
>
>
> Tomas Liubinas wrote:
> > Hi Christopeher,
> >
> > This oxSuperCfg() is a result of long internal discussions.
> Personally
> > I would agree that the way it is implemented now is not
> perfect, and
> > sometimes as you say could be avoided at all. In most cases
> you could
> > use singelton getInstance() instead of dynamic (non static?)
> > ->getConfig(). However one could argue should we use
> singeltons at all?
> >
> > Regards
> > Tomas Liubinas
> >
> > -----Original Message-----
> > From: [email protected]
> > [mailto:[email protected]] On Behalf Of
> > Christopher Simon
> > Sent: Friday, August 14, 2009 12:35 PM
> > To: [email protected]
> > Subject: [oxid-dev-general] oxSuperCfg
> >
> > Hi,
> >
> > nearly every class is a child of oxSuperCfg.
> >
> > It can be confusing anyway. Here is a example:
> >
> > <snip>
> > $oOrder = oxNew('oxorder');
> >
> > $oOrder->load($oxid); // just for example, imagine that we have
> > retrieved the orders oxid from somewhere
> >
> > $orderUser = $oOrder->getUser();
> > </snip>
> >
> > What is returned by getUser()?
> >
> > It's not the user which made the order - like a lot of people would
> > expect - its the active user.
> >
> > Which makes no sense on this object.
> >
> > In general, all methods in oxSuperCfg are completly redundant:
> >
> > - getConfig(): returns result of singleton oxConfig::getInstance()
> >
> > - getSession(): returns result of oxSession::getInstance()
> >
> > - getUser(): returns oxUser::loadActiveUser which could be
> implemented
> > as static method on user object
> >
> > - isAdmin(): the result is defined as global function in
> oxfunctions,
> > and it's a config parameter named "blAdmin" anyway.
> >
> > the unit test functionality of these methods could be
> implemented in
> > this singletons or static functions. there is no need to have
> > oxSuperCfg, also because it's no "cfg" or "config" anyway.
> >
> > In addition, not every object needs those methods, but every object
> > has those methods, at the moment.
> > _______________________________________________
> > dev-general mailing list
> > [email protected]
> > http://dir.gmane.org/gmane.comp.php.oxid.general
> > _______________________________________________
> > dev-general mailing list
> > [email protected]
> > http://dir.gmane.org/gmane.comp.php.oxid.general
>
> _______________________________________________
> dev-general mailing list
> [email protected]
> http://dir.gmane.org/gmane.comp.php.oxid.general
>
_______________________________________________
dev-general mailing list
[email protected]
http://dir.gmane.org/gmane.comp.php.oxid.general
