Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong default category #519

Closed
wants to merge 5 commits into from
Closed

Wrong default category #519

wants to merge 5 commits into from

Conversation

KES777
Copy link
Contributor

@KES777 KES777 commented Aug 26, 2015

details here
#518

@miyagawa
Copy link
Member

Do you have a test to verify this fix?

@KES777
Copy link
Contributor Author

KES777 commented Aug 27, 2015

I have check seven times and it is actually has no matter what value is there. So I can write:
`$self->logger( Log::Log4perl->get_logger('asdfasdfasdf') );
And every thing work fine, because of none use this category setted up. this is useless code (for one exception)
I will explain later.

@miyagawa
Copy link
Member

Well, by test I mean the unit test, such as the one we already have: t/Plack-Middleware/log4perl.t. If you can add a new test in that file to verify this fix, I will get it merged.

If that is difficult for you, let me know.

@KES777
Copy link
Contributor Author

KES777 commented Aug 27, 2015

ok. I will do week or two later.

Corect me please if I wrong: it seems that 'category' option is useless at all, is not?

@KES777
Copy link
Contributor Author

KES777 commented Aug 27, 2015

I correct myself: enable 'log4perl' is not requred for enable 'Debug', [qw/log4perl/]

So I fix error with '0' category and contribute to DOC.

use Modern::Perl;
use HTML::Mason::PSGIHandler;

my $app = sub {
    my $env =  shift;
    return [
        '200',
        [ 'Content-Type' => 'text/plain' ],
        [ "Hello World" ],
    ];
};

use Plack::Builder;
my $b =  builder {
    enable 'Log4perl';
    $app;
}

In this program without use Log::Log4perl (but it is not requried to have it at all) I get error:

Can't locate object method "get_logger" via package "Log::Log4perl" at /home/user/perl5/lib/perl5/Plack/Middleware/Log4perl.pm line 23

You use Log::Log4perl in your module. I think you should use Log::Log4perl instead of requre ...
or check Log::Log4perl instead of $self->conf and then requre if it not available yet.

@KES777
Copy link
Contributor Author

KES777 commented Aug 30, 2015

If you can add a new test in that file to verify this fix, I will get it merged.

will you merge?

@miyagawa
Copy link
Member

miyagawa commented Sep 5, 2015

This PR has a lot more changes by just fixing the default category bug. You could make the code change as minimal, or I will cherry pick only the changes I think is fine, but that will take me a bit of time.

@KES777
Copy link
Contributor Author

KES777 commented Sep 5, 2015

Oh, I am sorry. Different MR was mixed here.
I will try to revert those and make this PR clear.

@KES777
Copy link
Contributor Author

KES777 commented Sep 7, 2015

Open new MR

@KES777 KES777 closed this Sep 7, 2015
@KES777 KES777 deleted the master branch September 7, 2015 11:40
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling cd44fc5 on KES777:master into ** on plack:master**.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants