Add support to the PSR LoggerInterface #105
Conversation
| @@ -18,7 +18,8 @@ | |||
| "jms/metadata": "1.*", | |||
| "jms/parser-lib": "1.*", | |||
| "jms/aop-bundle": ">=1.0.0,<1.2-dev", | |||
| "jms/di-extra-bundle": "1.3.*" | |||
| "jms/di-extra-bundle": "1.3.*", | |||
| "psr/log": ">=1.0,<2.0" | |||
There was a problem hiding this comment.
a hard requirement on logging ? seriously ?
There was a problem hiding this comment.
Yes as it replaces the LoggerInterface of the HttpKernel Symfony component. Why does this sound like a bad idea to you?
There was a problem hiding this comment.
The point is, is the logger a required dependency of the library ?
There was a problem hiding this comment.
Yes as it is used in many use statements.
But if the dependency is not included here it will be by the FrameworkBundle wich requires the HttpKernel component wich required the psr/log. I thought it was better to require it here to make it more explicit.
There was a problem hiding this comment.
use statement are not defining a dependency. You can put anything into a use statement. It does not trigger the autoloading.
There was a problem hiding this comment.
and the question is whether it is a required dependency or an optional one
There was a problem hiding this comment.
So it's not a dependency as Logger is always optional.
|
This requires branching the bundle as it will makes the logging incompatible with Symfony 2.1 |
|
I appreciate the effort, but I'm wondering if there is a problem with the current code? If there is one, maybe we can find a solution that does not require a BC break. |
|
There is absolutely no problems with the code. I just did it because I was reading the code and as the LoggerInterface of Symfony will be deprecated I thought updating it would be a good thing. There is no problem to close this right now if you want :) |
|
No, let's keep it open, we will need it eventually, but I'll probably not merge it for some time. |
|
As you want. Tell me if more needs to be done. |
|
@stof I removed the dependency to I wonder if I should change the min version of the framework bundle to >=2.2 |
|
Currently the tests fail because of the PSR LoggerInterface. Best solution imho is to create a new branch for 2.1. |
|
@ricbra tests are not failing because of this change. They are failing because of reaching the github api rate limit and trying to download a file |
|
I'm sorry for being unclear, I was trying to say the tests for this bundle are failing without this change. When I do a fresh clone, So my conclusion is that these problems are caused by the changes in the LoggerInterface. I don't see how this is related to github api rate? Certainly not when running the tests locally? |
|
ah, I thought you were talkign about the failures on Travis. and this issue is in fact a bug in Symfony 2.2 where the NullLogger misses the |
This PR was merged into the master branch. Commits ------- b1d1168 Fixed the NullLogger to implement the HttpKernel interface again Discussion ---------- Fixed the NullLogger to implement the HttpKernel interface again | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no (reverting one introduced by mistake) | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | n/a This BC breaking mistake has been reported in schmittjoh/JMSSecurityExtraBundle#105 (comment)
|
closed in favor of #202 (comment) |
No description provided.