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 "Ivan Habunek (JIRA)" <ji...@apache.org> on 2010/04/28 17:02:32 UTC
[jira] Commented: (LOG4PHP-108) Add HTML line break to
LoggerAppenderEcho output
[ https://issues.apache.org/jira/browse/LOG4PHP-108?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12861821#action_12861821 ]
Ivan Habunek commented on LOG4PHP-108:
--------------------------------------
I have a couple of issues with this patch.
Firstly, do you think it's a good idea to have addHTMLLineBreak default to true? This changes the current default behaviour. I mostly develop command line apps so this was not welcome on my part.
Secondly, you append <br/> to end of the mesage if this option is enabled. Wouldn't it be neater to replace all line breaks in the message with <br />? I often log multi-line messages so from my perspective this would be the better approach.
Also, I think you broke one of the tests with this patch:
1) LoggerRendererMapTest::testUsage
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
ERROR - test1,test2,test3
-<br />
+
D:\Dev\log4php\src\test\php\renderers\LoggerRendererMapTest.php:82
> Add HTML line break to LoggerAppenderEcho output
> ------------------------------------------------
>
> Key: LOG4PHP-108
> URL: https://issues.apache.org/jira/browse/LOG4PHP-108
> Project: Log4php
> Issue Type: Improvement
> Components: Code
> Reporter: Florian Platzer
> Priority: Minor
> Fix For: 2.1
>
> Attachments: LoggerAppenderEcho.patch
>
> Original Estimate: 0.08h
> Remaining Estimate: 0.08h
>
> When using LoggerAppenderEcho the output will be printed in one line (in HTML).
> So automatically add a line break or add a new property for user's control
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to
LoggerAppenderEcho output
Posted by Christian Grobmeier <gr...@gmail.com>.
> On 3 May 2010 07:46, Christian Grobmeier <gr...@gmail.com> wrote:
>> Are you working on this? If not I'll do. I think we should do this
>> change soon before we all get confused with this error. Others
>> reported it too, but for me everything is still fine :-)
>
> I'll do it for practice. :-) Will try to have it done today.
Thats great thanks, I will try to apply it as soon as you send it
Cheers
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to
LoggerAppenderEcho output
Posted by Ivan Habunek <iv...@gmail.com>.
On 3 May 2010 07:46, Christian Grobmeier <gr...@gmail.com> wrote:
> Are you working on this? If not I'll do. I think we should do this
> change soon before we all get confused with this error. Others
> reported it too, but for me everything is still fine :-)
I'll do it for practice. :-) Will try to have it done today.
Ivan
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to
LoggerAppenderEcho output
Posted by Ivan Habunek <iv...@gmail.com>.
Hi everybody. Remember this thread? X)
Well, I'm back on this problem once again. So far it has been proposed
that a long term solution would be either:
a) to stop using LoggerAppenderPool
Personally I think we could do without it, in fact my rewrite of the
XML configurator does not use an appender pool at all. I believe the
same could be achieved for other configurators easily.
b) make LoggerAppenderPool non-static, and instantiate it by the configurators
This will work as well, but I think it's not the most elegant solution.
Now, obviously my vote is for a. Do you have any preferences?
As a quick-fix, I will implement clearing of the LoggerAppenderPool in
LoggerHierarchy::resetConfiguration(). This soulution will do until we
agree on a more permanent solution.
Regards,
Ivan
On 4 May 2010 08:39, Ivan Habunek <iv...@gmail.com> wrote:
> On 4.5.2010 7:03, Christian Grobmeier wrote:
>>
>> It still works for me. But the running order might be different on
>> different os. I heard from some other guy who has this failure.
>
> It is definitely a problem related to the order in which the tests run. As a
> matter of interest, which OS are you using? I tested on WinXP and Debian.
>
>> First, LoggerAppenderPool is only used by Configurators, I think we
>> should move it in this package and not on leave it in the root
>> package.
>>
>> Second, I don't see that LoggerAppenderPool needs to be statically
>> available. If we create a new LoggerAppenderPool object in each
>> configurators constructor and use the object then this should be
>> solved without any additional reset calls or clear methods.
>>
>> What do you think?
>
> I've been working on the XML configurator and, frankly, I don't see the need
> for LoggerAppenderPool at all, at least not in the way it is currently used.
> As you said, it is only used by the configurator classes, and if this is the
> only place it is meant to be used, then it can easily be replaced by a
> private variable within the configurator class.
>
> Maybe I missed something? Otherwise, your ideas look OK. :)
>
> Regards,
> Ivan
>
>
>
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to LoggerAppenderEcho
output
Posted by Ivan Habunek <iv...@gmail.com>.
On 4.5.2010 7:03, Christian Grobmeier wrote:
> It still works for me. But the running order might be different on
> different os. I heard from some other guy who has this failure.
It is definitely a problem related to the order in which the tests run.
As a matter of interest, which OS are you using? I tested on WinXP and
Debian.
> First, LoggerAppenderPool is only used by Configurators, I think we
> should move it in this package and not on leave it in the root
> package.
>
> Second, I don't see that LoggerAppenderPool needs to be statically
> available. If we create a new LoggerAppenderPool object in each
> configurators constructor and use the object then this should be
> solved without any additional reset calls or clear methods.
>
> What do you think?
I've been working on the XML configurator and, frankly, I don't see the
need for LoggerAppenderPool at all, at least not in the way it is
currently used. As you said, it is only used by the configurator
classes, and if this is the only place it is meant to be used, then it
can easily be replaced by a private variable within the configurator class.
Maybe I missed something? Otherwise, your ideas look OK. :)
Regards,
Ivan
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to
LoggerAppenderEcho output
Posted by Christian Grobmeier <gr...@gmail.com>.
Hi,
> I think I know why this works for you. Are you by any chance running
> phpunit only on that one test class?
no, only in dev mode i run single tests. But before committing i am
running mvn test AND phpunit . for all tests.
It still works for me. But the running order might be different on
different os. I heard from some other guy who has this failure.
> It took me a little while, but I know why this happens. The test which
> fails uses Logger::resetConfiguration() to clear the previous settings
> and appenders. However, the resetConfiguration() method does not reset
> the LoggerAppenderPool. Next time configure() and initialize() is
> called, instead of creating new appenders, the pool will use existing
> ones. In this case, the htmlLineBreaks option is left turned on by a
> previous test which uses the same appender name, and that is why the
> test fails.
>
> Just to prove the point, I implemented a method to clear the appender
> pool (LoggerAppenderPool::clear()). When I edit
> LoggerRendererMapTest::testUsage(), and add
> LoggerAppenderPool::clear() before applying the new configuration, the
> test passes as expected.
>
> In conclusion, i think that:
> * LoggerAppenderPool should have a clear() method
> * this method should be called by Logger::resetConfiguration()
>
> If you have the time, have a look at this problem, I'd like to finish
> the echo appender issue first.
Interesting. I looked into the code and think we should do something here.
First, LoggerAppenderPool is only used by Configurators, I think we
should move it in this package and not on leave it in the root
package.
Second, I don't see that LoggerAppenderPool needs to be statically
available. If we create a new LoggerAppenderPool object in each
configurators constructor and use the object then this should be
solved without any additional reset calls or clear methods.
What do you think?
Like this:
public function configure(LoggerHierarchy $hierarchy, $url = '') {
$pool = new LoggerAppenderPool();
$pool->getAppenderFromPool("name");
Cheers
Christian
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to
LoggerAppenderEcho output
Posted by Ivan Habunek <iv...@gmail.com>.
On 3 May 2010 07:46, Christian Grobmeier <gr...@gmail.com> wrote:
> Are you working on this? If not I'll do. I think we should do this
> change soon before we all get confused with this error. Others
> reported it too, but for me everything is still fine :-)
I think I know why this works for you. Are you by any chance running
phpunit only on that one test class?
If you only run phpunit on this one class ("phpunit
renderers\LoggerRendererMapTest.php") you will *not* get the error.
You have to run all tests by running "phpunit" without parameters.
It took me a little while, but I know why this happens. The test which
fails uses Logger::resetConfiguration() to clear the previous settings
and appenders. However, the resetConfiguration() method does not reset
the LoggerAppenderPool. Next time configure() and initialize() is
called, instead of creating new appenders, the pool will use existing
ones. In this case, the htmlLineBreaks option is left turned on by a
previous test which uses the same appender name, and that is why the
test fails.
Just to prove the point, I implemented a method to clear the appender
pool (LoggerAppenderPool::clear()). When I edit
LoggerRendererMapTest::testUsage(), and add
LoggerAppenderPool::clear() before applying the new configuration, the
test passes as expected.
In conclusion, i think that:
* LoggerAppenderPool should have a clear() method
* this method should be called by Logger::resetConfiguration()
If you have the time, have a look at this problem, I'd like to finish
the echo appender issue first.
Regards,
Ivan
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to
LoggerAppenderEcho output
Posted by Christian Grobmeier <gr...@gmail.com>.
On Sat, May 1, 2010 at 8:26 AM, Ivan Habunek <iv...@gmail.com> wrote:
> On 30.4.2010 23:10, Christian Grobmeier wrote:
>>
>> Agreed.
>
> Then I propose to change the setting to be called "useHTMLLineBreaks" to
> better reflect the new behaviour. Then update the EchoAppender to convert
> any line breaks to <br /> if this setting is set to true.
OK
>>> Also, I think you broke one of the tests with this patch:
>>> 1) LoggerRendererMapTest::testUsage
>>> Failed asserting that two strings are equal.
>>
>> That one is strange - it works fine for me. Did you update it?
>
> Strange, I just tried it out on windows and linux with code freshly checked
> out of the trunk. I get the error in both cases. Nevermind, we'll have to
> update tests anyway.
Are you working on this? If not I'll do. I think we should do this
change soon before we all get confused with this error. Others
reported it too, but for me everything is still fine :-)
Cheers
Christian
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to LoggerAppenderEcho
output
Posted by Ivan Habunek <iv...@gmail.com>.
On 30.4.2010 23:10, Christian Grobmeier wrote:
> Agreed.
Then I propose to change the setting to be called "useHTMLLineBreaks" to
better reflect the new behaviour. Then update the EchoAppender to
convert any line breaks to <br /> if this setting is set to true.
>> Also, I think you broke one of the tests with this patch:
>> 1) LoggerRendererMapTest::testUsage
>> Failed asserting that two strings are equal.
>
> That one is strange - it works fine for me. Did you update it?
Strange, I just tried it out on windows and linux with code freshly
checked out of the trunk. I get the error in both cases. Nevermind,
we'll have to update tests anyway.
Ivan
Re: [jira] Commented: (LOG4PHP-108) Add HTML line break to
LoggerAppenderEcho output
Posted by Christian Grobmeier <gr...@gmail.com>.
Hi,
actually I have thought on it a while and hoped somebody would comment
it. Thanks Ivan for doing it finally.
> Firstly, do you think it's a good idea to have addHTMLLineBreak default to true? This changes the current default behaviour. I mostly develop command line apps so this was not welcome on my part.
There is a second reason for defaulting to "false". Because it was
"false" before. Since its nam is echoappender i thought it would be
more selfexplainary with br in it, but you double my doubts and I
would tend to "false" again, as you do.
> Secondly, you append <br/> to end of the mesage if this option is enabled. Wouldn't it be neater to replace all line breaks in the message with <br />? I often log multi-line messages so from my perspective this would be the better approach.
Agreed.
> Also, I think you broke one of the tests with this patch:
> 1) LoggerRendererMapTest::testUsage
> Failed asserting that two strings are equal.
> --- Expected
> +++ Actual
> @@ @@
> ERROR - test1,test2,test3
> -<br />
> +
That one is strange - it works fine for me. Did you update it?
Cheers,
Christian
>
> D:\Dev\log4php\src\test\php\renderers\LoggerRendererMapTest.php:82
>
>
>
>> Add HTML line break to LoggerAppenderEcho output
>> ------------------------------------------------
>>
>> Key: LOG4PHP-108
>> URL: https://issues.apache.org/jira/browse/LOG4PHP-108
>> Project: Log4php
>> Issue Type: Improvement
>> Components: Code
>> Reporter: Florian Platzer
>> Priority: Minor
>> Fix For: 2.1
>>
>> Attachments: LoggerAppenderEcho.patch
>>
>> Original Estimate: 0.08h
>> Remaining Estimate: 0.08h
>>
>> When using LoggerAppenderEcho the output will be printed in one line (in HTML).
>> So automatically add a line break or add a new property for user's control
>
> --
> This message is automatically generated by JIRA.
> -
> You can reply to this email to add a comment to the issue online.
>
>