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

LoggerInterface and Monolog #5968

Closed
fabpot opened this issue Nov 10, 2012 · 21 comments
Closed

LoggerInterface and Monolog #5968

fabpot opened this issue Nov 10, 2012 · 21 comments

Comments

@fabpot
Copy link
Member

fabpot commented Nov 10, 2012

As of today, we have a LoggerInterface declared in the HttpKernel component. This can be problematic for components that need a logger but do not depend on HttpKernel.

There are several options here to fix this "issue":

1/ Let the FIG group decide on an interface and rely on the FIG package that would declare this interface (keep in mind that we need to have something stable by the end of December for Symfony 2.2);
2/ Create a new Symfony component with just the LoggerInterface file;
3/ Remove the interface altogether and rely on Monolog instead;
4/ Create a Symfony Logger component that support both Monolog and the Zend Logger;
5/ Do nothing.

see #5911 for the start of the discussion.

I tend to prefer option 3 for the many reasons:

  • If we had created a Logger component when we decided to create Monolog (switching from the Zend Logger at that time), we wouldn't have this discussion about this today. So, I like to think of Monolog as being the Symfony Logger component;
  • There aren' that many other logger libraries in PHP and this is not the kind of problems that need to have the flexibility to be replaced by something else;
  • Monolog is flexible enough to cover all the use cases you will ever need;
  • If this is a must, we can also add support for the Zend Logger library within Monolog (@Seldaek agreed with this possibility);
  • Thanks to Composer, relying on an external library like Monolog is not a problem anymore.
@fabpot
Copy link
Member Author

fabpot commented Nov 10, 2012

Also, I forgot to mention that a Logger must always be an optional dependency.

@dlsniper
Copy link
Contributor

  1. is not really a option as people implementing components in their existing applications might not want to have the HttpKernel in their project, I know of at least one case.

  2. seems a bit over the hand as if you are using Symfony components with Zend components you could just bridge them to use either Zend/Logger or Monolog and if @Seldaek agrees with a bridge to Zend/Logger then it should be a problem anymore.

  3. While it would be the best solution, it seems unreliable as FIG should actually work and release this inside 2 months and given the way PSR1/2 got stuck with CS talks rather that check the other issues like Caching I don't really see a solution in this.

Which brings us to 2) and 3)

  1. Would only be good if people use the components separately and they want to bridge their existing loggers to Symfony2 one. I think this is the case of most existing applications that use logging with an internal library, I'm working on such a application right now and for me implementing a interface would be the easiest way to bridge the two of them. On the other hand it would look weird to have a 'component' that has just a interface declared :)

and

  1. Monolog is mature, used in Symfony2 Standard/Components already and people using them know what to expect from it/how to use it. If support for Zend Framework would exist as well then I think it's the best way to go.

TLDR 👍 for 3

@barnabywalters
Copy link

I am just having this problem with the Security component standalone (wanting to pass my Monolog instance to constructors which want an instance of Symfony LoggerInterface).

+1 for option three.

@sstok
Copy link
Contributor

sstok commented Nov 12, 2012

+1 for option three

@p16
Copy link

p16 commented Nov 28, 2012

+1 for option three

The "Symfony Logger component" is a good idea but I don't see a need for it right now.

@Seldaek
Copy link
Member

Seldaek commented Nov 28, 2012

Just FYI since not everyone here is following the PHP-FIG mailing list I guess, I started a proposal for a PSR LoggerInterface - if we get that then it'll be possible to rely on that instead of relying on monolog. It's a slightly more neutral approach. I'll do my best to push this forward before end of the year.

@p16
Copy link

p16 commented Nov 28, 2012

The PSR LoggerInterface sounds good.

I still think that the quickest and better solution (for this issue) is Monolog.
If the PSR LoggerInterface will be accepted, it will be implemented by Monolog, making easy to pass from Monolog to LoggerInterface.

@stof
Copy link
Member

stof commented Nov 28, 2012

@p16 but I don't think refactoring all component to depend on Monolog itself just in the meantime is worth it (they will still need to be refactored all to depend on the PSR interface). So if we wait for the PSR interface, I suggest keeping the existing interface in the meantime.

@Seldaek could you link to the discussion ?

@p16
Copy link

p16 commented Nov 28, 2012

@stof I agree with you, my only concern is how long it's going to take to have a PSR LoggerInterface.

@fabpot
Copy link
Member Author

fabpot commented Nov 28, 2012

The deadline to implement our choice for 2.2 is the end of december.

@dlsniper
Copy link
Contributor

it seems that people would either wait for FIG to figure out what they want or for a Monolog standardization.

As far as I can see, FIG is pretty dormant, in the sense that they don't actively maintain the open issues on GH nor anything else bigger is being talked about in the past 6 months or so ...

Imho, having end of the year as a dead-line is also a bit close as most of the people would get away with family / friends and so on.

So the only thing that comes natural is using Monolog and provide some sort of bridge when FIG will decide something about this.

@Seldaek
Copy link
Member

Seldaek commented Nov 29, 2012

It seems that people prefer to talk than help get shit done.

PR is at php-fig/fig-standards#60 - you can find the previous discussion at https://groups.google.com/forum/?fromgroups=#!topic/php-fig/yL_wDyiT6GY%5B1-25%5D - if no big opposition suddenly appears I will move it to vote soon, and hopefully we have something standardized by the end of the year. As soon as things seem to go in the right direction we can begin implementing the changes in symfony to make sure stuff is ready on time.

@Crell
Copy link
Contributor

Crell commented Nov 29, 2012

I would also favor option 1. This is exactly the problem space that FIG proposal is intended to solve, and it is the most promising discussion the FIG group has had to date. :-)

@webmozart
Copy link
Contributor

One alternative to option 2 is to implement #6129 and move the LoggerInterface to the symfony/api package.

@dlsniper
Copy link
Contributor

dlsniper commented Jan 2, 2013

@Seldaek I think that the Logger interface just got approved by FIG it seems it wasn't moved into the repository yet.

@fabpot How should we proceed from here? Help @Seldaek make Monolog PSR3 compatible then make the changes here to rely on PSR?

Best regards.

@stof
Copy link
Member

stof commented Jan 2, 2013

@dlsniper there is already a branch in Monolog making it compatible.

Note that it is incompatible with the current MonologBridge as some method names are the same in the PSR interface and the HttpKernel one.

@stof
Copy link
Member

stof commented Jan 4, 2013

FYI, the branch has been merged into Monolog master today

@olaurendeau
Copy link

About #6560, due to yesterday Monolog merge :
As symfony/monolog-bundle have a "monolog/monolog": "1.*" dependency on all branches/tags, anyone updating his vendors will have this issue if monolog is not locked on previous version.
So while symfony 2.1 (and may will not ?) have not been refactored for monolog master, wouldn't it be safer to lock symfony/monolog-bundle 2.1.x and earlier on "monolog/monolog": "<=1.2.1" ?

@fabpot
Copy link
Member Author

fabpot commented Jan 9, 2013

The first option was chosen as PSR-3 is now out. See #6628 for the PR that will this gappen.

@fabpot fabpot closed this as completed Jan 9, 2013
@Seldaek
Copy link
Member

Seldaek commented Jan 10, 2013

@frosas it wasn't merged yet, so no. And it's nothing to do with twig really, so I doubt it. Maybe the latest twig release broke something though. Anyway I would open a new issue somewhere else :)

@frosas
Copy link
Contributor

frosas commented Jan 10, 2013

@Seldaek sorry, the comment was for another issue.

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

No branches or pull requests