You are viewing a plain text version of this content. The canonical link for it is here.
Posted to log4php-dev@logging.apache.org by ku...@apache.org on 2009/05/23 21:42:42 UTC
svn commit: r777996 -
/incubator/log4php/trunk/src/main/php/LoggerAppender.php
Author: kurdalen
Date: Sat May 23 19:42:41 2009
New Revision: 777996
URL: http://svn.apache.org/viewvc?rev=777996&view=rev
Log:
reverted factory method (it's actually used in LoggerAppender::singleton() to instansiate appenders)
Modified:
incubator/log4php/trunk/src/main/php/LoggerAppender.php
Modified: incubator/log4php/trunk/src/main/php/LoggerAppender.php
URL: http://svn.apache.org/viewvc/incubator/log4php/trunk/src/main/php/LoggerAppender.php?rev=777996&r1=777995&r2=777996&view=diff
==============================================================================
--- incubator/log4php/trunk/src/main/php/LoggerAppender.php (original)
+++ incubator/log4php/trunk/src/main/php/LoggerAppender.php Sat May 23 19:42:41 2009
@@ -81,6 +81,21 @@
}
/**
+ * Factory
+ *
+ * @param string $name appender name
+ * @param string $class create an instance of this appender class
+ * @return LoggerAppender
+ */
+ public static function factory($name, $class) {
+ $class = basename($class);
+ if(!empty($class)) {
+ return new $class($name);
+ }
+ return null;
+ }
+
+ /**
* Singleton
*
* @param string $name appender name
Re: svn commit: r777996 - /incubator/log4php/trunk/src/main/php/LoggerAppender.php
Posted by Christian Grobmeier <gr...@gmail.com>.
> But if you need to apply some more logic in order to create the instance
> either LoggerReflectionUtils::createObject() (as a general mechanism) or a
> specific implementation that deals with only LoggerAppender-specific stuff
> in LoggerAppender::factory() is valid options. Currently both
> implementations looks the same, but typically in this case you would like to
> limit which classes can be instantiated (only instanceof LoggerAppender)
> which is logic that resides in a LoggerAppender::factory() method.
I understand what you mean now. But LoggerAppender does only provide a
general mechanism, not a limitated mechanism which just allows
LoggerAppenders to be created. Its public, everybody can use this
method to create objects of any kind.
IMHO we should either we limit this method to what you described or
leave it as general method, but then move it to a general class.
Otherwise we have this code duplicated at least 5 times or so and of
course have to test it 5 times.
Even when we check for types, its worth considering getting this
method out from this class - I am thinking on Spring example, where I
learned that object creation is not necessary the concern of the class
itself but of an external creator.
Its similar with singletons. All the implementations are exactly the
same, just creating an object, putting it into an instance-array and
returning if it already exists. There is nothing specific. Sometimes I
even feel that creation of multiple objects shouldn't be dissallowed,
like in LoggerHierarchy. Even log4j doesn't use this class a singleton
(at least this is not coming out from the javadocs)
Cheers,
Christian
Re: svn commit: r777996 - /incubator/log4php/trunk/src/main/php/LoggerAppender.php
Posted by Knut Urdalen <kn...@php.no>.
Christian Grobmeier wrote:
> can we use LoggerReflectionUtils::createObject
> instead of all those factory methods? Its duplicated code over all log4php,
> so it makes sense to use it from one point.
>
Normally I would do something like this instead of using a separate
factory method:
--- src/main/php/LoggerAppender.php (revision 777996)
+++ src/main/php/LoggerAppender.php (working copy)
@@ -108,7 +108,7 @@
if(!empty($name)) {
if(!isset($instances[$name])) {
if(!empty($class)) {
- $appender = self::factory($name, $class);
+ $appender = new $class($name);
if($appender !== null) {
$instances[$name] = $appender;
return $instances[$name];
Which is the fastest way ;)
But if you need to apply some more logic in order to create the instance
either LoggerReflectionUtils::createObject() (as a general mechanism) or
a specific implementation that deals with only LoggerAppender-specific
stuff in LoggerAppender::factory() is valid options. Currently both
implementations looks the same, but typically in this case you would
like to limit which classes can be instantiated (only instanceof
LoggerAppender) which is logic that resides in a
LoggerAppender::factory() method.
Just some thoughts around this :)
Knut
Re: svn commit: r777996 - /incubator/log4php/trunk/src/main/php/LoggerAppender.php
Posted by Christian Grobmeier <gr...@gmail.com>.
Hi,
can we use LoggerReflectionUtils::createObject
instead of all those factory methods? Its duplicated code over all log4php,
so it makes sense to use it from one point.
Cheers
Christian
On Sat, May 23, 2009 at 9:42 PM, <ku...@apache.org> wrote:
> Author: kurdalen
> Date: Sat May 23 19:42:41 2009
> New Revision: 777996
>
> URL: http://svn.apache.org/viewvc?rev=777996&view=rev
> Log:
> reverted factory method (it's actually used in LoggerAppender::singleton() to instansiate appenders)
>
> Modified:
> incubator/log4php/trunk/src/main/php/LoggerAppender.php
>
> Modified: incubator/log4php/trunk/src/main/php/LoggerAppender.php
> URL: http://svn.apache.org/viewvc/incubator/log4php/trunk/src/main/php/LoggerAppender.php?rev=777996&r1=777995&r2=777996&view=diff
> ==============================================================================
> --- incubator/log4php/trunk/src/main/php/LoggerAppender.php (original)
> +++ incubator/log4php/trunk/src/main/php/LoggerAppender.php Sat May 23 19:42:41 2009
> @@ -81,6 +81,21 @@
> }
>
> /**
> + * Factory
> + *
> + * @param string $name appender name
> + * @param string $class create an instance of this appender class
> + * @return LoggerAppender
> + */
> + public static function factory($name, $class) {
> + $class = basename($class);
> + if(!empty($class)) {
> + return new $class($name);
> + }
> + return null;
> + }
> +
> + /**
> * Singleton
> *
> * @param string $name appender name
>
>
>