change createObject

[Florian Semm <Fl...@gmx.de>]

Hi, 

i start the developing of a Log4PhpBundle for the Symfony2 framework. The basic appenders like AppenderFile and AppenderRollingFile work fine. Now I want to integrate the doctrine-service for the db logging. But there are some probleme with die LoggerReflectionUtils::createObject(). My appender is a full qualified classname with namespaces. So the createObject() execute basename('Foo\AppenderBar') and remove the namespace: AppenderBar is the return value. Of course the class AppenderBar  does not exist.

I remove this line (LoggerReflectionUtils:131) in the method and it works fine, with the basic appenders of log4php too. 

I don't want hack log4php. So is it possible to change/remove this line in the public repository?

FS

Re: change createObject

[Florian Semm <fl...@gmx.de>]

Am 03.10.2011 14:08, schrieb Ivan Habunek:
> I have removed the offending line in trunk.
>
> Regards,
> Ivan
>
> 2011/10/3 Ivan Habunek<iv...@gmail.com>:
>> On 3.10.2011. 7:54, Christian Grobmeier wrote:
>>> The only thing to think about is backwards compatibility. Two options:
>>>
>>> 1) don't care on bc and remove the line
>>> 2) add an optional param to createObject, like: createObject($class,
>>> [$usingNamespace]);
>> 3) Stop using createObject because it only obfuscates code? :)
>>
>> I'm not a fan of that method. Why is:
>> $obj = ReflectionUtils::createObject($class);
>> better than:
>> $obj = new $class();
>> ?
>> And it can silently do nothing if $class is empty (as twitter users would
>> say: #fail). :)
>>
>>> I would prefer #1, b/c would introduce a param for a php version we
>>> currently don't support.
>>>
>>> Ivan, what do you think?
>> I agree that we can do #1 right away. All tests pass without problem.
>>
>> Anyway, basename() is meant to be used on file paths, not on class names.
>> The only reason it works is because "\" is the delimiter for both namespaces
>> and the windows file system.
>>
>> Florian, just a heads-up. I'm in the middle of rewriting the whole
>> configurator part of log4php. In other words, there will be substantial
>> changes, and soon. If you're interested in a preview have a look here:
>> https://svn.apache.org/repos/asf/logging/log4php/branches/experimental/config-adapters
>>
>> Regards,
>> Ivan
>>
thanks for this preview.

I think I will create an experimental branch for the log4php-bundle too.


FYI:

Master branch on GitHub: https://github.com/floriansemm/Log4PhpBundle

FS

Re: change createObject

[Ivan Habunek <iv...@gmail.com>]

I have removed the offending line in trunk.

Regards,
Ivan

2011/10/3 Ivan Habunek <iv...@gmail.com>:
> On 3.10.2011. 7:54, Christian Grobmeier wrote:
>>
>> The only thing to think about is backwards compatibility. Two options:
>>
>> 1) don't care on bc and remove the line
>> 2) add an optional param to createObject, like: createObject($class,
>> [$usingNamespace]);
>
> 3) Stop using createObject because it only obfuscates code? :)
>
> I'm not a fan of that method. Why is:
> $obj = ReflectionUtils::createObject($class);
> better than:
> $obj = new $class();
> ?
> And it can silently do nothing if $class is empty (as twitter users would
> say: #fail). :)
>
>> I would prefer #1, b/c would introduce a param for a php version we
>> currently don't support.
>>
>> Ivan, what do you think?
>
> I agree that we can do #1 right away. All tests pass without problem.
>
> Anyway, basename() is meant to be used on file paths, not on class names.
> The only reason it works is because "\" is the delimiter for both namespaces
> and the windows file system.
>
> Florian, just a heads-up. I'm in the middle of rewriting the whole
> configurator part of log4php. In other words, there will be substantial
> changes, and soon. If you're interested in a preview have a look here:
> https://svn.apache.org/repos/asf/logging/log4php/branches/experimental/config-adapters
>
> Regards,
> Ivan
>

Re: change createObject

[Ivan Habunek <iv...@gmail.com>]

On 3.10.2011. 7:54, Christian Grobmeier wrote:
> The only thing to think about is backwards compatibility. Two options:
>
> 1) don't care on bc and remove the line
> 2) add an optional param to createObject, like: createObject($class,
> [$usingNamespace]);

3) Stop using createObject because it only obfuscates code? :)

I'm not a fan of that method. Why is:
$obj = ReflectionUtils::createObject($class);
better than:
$obj = new $class();
?
And it can silently do nothing if $class is empty (as twitter users 
would say: #fail). :)

> I would prefer #1, b/c would introduce a param for a php version we
> currently don't support.
>
> Ivan, what do you think?

I agree that we can do #1 right away. All tests pass without problem.

Anyway, basename() is meant to be used on file paths, not on class 
names. The only reason it works is because "\" is the delimiter for both 
namespaces and the windows file system.

Florian, just a heads-up. I'm in the middle of rewriting the whole 
configurator part of log4php. In other words, there will be substantial 
changes, and soon. If you're interested in a preview have a look here:
https://svn.apache.org/repos/asf/logging/log4php/branches/experimental/config-adapters

Regards,
Ivan

Re: change createObject

[Christian Grobmeier <gr...@gmail.com>]

Hello Florian,

> i start the developing of a Log4PhpBundle for the Symfony2 framework. The basic appenders like AppenderFile and AppenderRollingFile work fine. Now I want to integrate the doctrine-service for the db logging. But there are some probleme with die LoggerReflectionUtils::createObject(). My appender is a full qualified classname with namespaces. So the createObject() execute basename('Foo\AppenderBar') and remove the namespace: AppenderBar is the return value. Of course the class AppenderBar  does not exist.
>
> I remove this line (LoggerReflectionUtils:131) in the method and it works fine, with the basic appenders of log4php too.
>
> I don't want hack log4php. So is it possible to change/remove this line in the public repository?

Actually I don't see a reason to leave this in code. At the moment, a
potential user is calling it the class must be already loaded. So it
doesn't make sense to give the fullqualified as argument. Guess it is
only there b/c of historical reasons.

The only thing to think about is backwards compatibility. Two options:

1) don't care on bc and remove the line
2) add an optional param to createObject, like: createObject($class,
[$usingNamespace]);

I would prefer #1, b/c would introduce a param for a php version we
currently don't support.

Ivan, what do you think?

Cheers
Christian