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

Already on GitHub? Sign in to your account

[RFC] Move logger and stopwatch to separate component #5911

Closed
dlsniper opened this Issue Nov 5, 2012 · 25 comments

Comments

Projects
None yet
8 participants
Contributor

dlsniper commented Nov 5, 2012

Hello,

In order to keep the dependencies better, shouldn't the LoggerInterface and Stopwatch be moved to a separate component, say Debug (or something better named) and so eliminate the need to pulling the HttpKernel in order to use them?

Also since 2.2 is the last chance to break BC I think we should really think of this as in the future some other debugging things might appear and HttpKernel is not really the place for them imho.

What do you think?

Best regards.

Owner

fabpot commented Nov 5, 2012

I'm -1 for the LoggerInterface as it defines this interface in the context of HttpKernel. It's not useful if you are not using HttpKernel.

For the StopWatch, I'm -0 for the move.

Member

stof commented Nov 5, 2012

@fabpot Some other components have an optional dependency to HttpKernel for its logger: Routing for instance. Should we provide a different LoggerInterface in the Routing component with the same methods just to avoid the dependencies ?

Contributor

dlsniper commented Nov 5, 2012

Hi @fabpot. The problem is that there is no common interface for a logger and if you look at #5902 for example, you will see that the component now depends on HttpKernel to be present if you want to use the component alone. I think that this is not in the spirit of having components decoupled or to provide a foundation for other people to use.

I don't want to be harsh, it's far by me, but thinking about a user who would just want to use a certain component it's not very pleasant either.

A smaller component would allow for a better separation, even if it will still not solve the dependency problem, just shift it to another point.

Since 2.2 is the last chance to do changes like this, I'd consider it a very good time to think about it.

Best regards.

Owner

fabpot commented Nov 7, 2012

I understand this, but having a Log component with just an interface does not sounds like a good idea. Another possibility would be to just rely on Monolog. Does it even make sense to be compatible with something else than Monolog?

Member

stof commented Nov 7, 2012

@fabpot I don't think depending on Monolog explicitly is a good idea. People may want to use the Symfony2 components in their ZF2 projects and implement the LoggerInterface by wrapping the ZF logger instead of having to setup a second logger

Owner

fabpot commented Nov 7, 2012

@stof: So, you would also recommend that we create a Log/Logging/Logger component with just the interface? And perhaps two concrete classes for the wrappers needed for Zend Logger and Monolog?

Contributor

dlsniper commented Nov 7, 2012

Depending on Monolog isn't a very good idea, like @stof mentioned.

Also having a component named Log with just one interface is no good either.

So why not have a Debug component with LoggerInterface, StopWatch and other debug helpers?

Contributor

schmittjoh commented Nov 7, 2012

Can't the Monolog logger proxy to a possible Zend logger?

Member

stof commented Nov 7, 2012

@schmittjoh this would still require these guys to setup Monolog in their project

Contributor

schmittjoh commented Nov 7, 2012

By "setup", you mean add it to their composer.json file?

This doesn't sound too bad to me, and btw it would also be necessary to
set-up (read add to composer.json) a to-be-created Logging component.

On Wed, Nov 7, 2012 at 12:48 PM, Christophe Coevoet <
notifications@github.com> wrote:

@schmittjoh https://github.com/schmittjoh this would still require
these guys to setup Monolog in their project


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/issues/5911#issuecomment-10145777.

Member

stof commented Nov 7, 2012

@schmittjoh And then instantiate a Monolog logger and register a (yet-to-be-implemented) ZendHandler wrapping their ZF logger.
Thus, it is totally counter-intuitive as the Symfony LoggerInterface maps easily to the Zf Logger interface whereas the Monolog HandlerInterface is asking for different methods.
I don't think Symfony should introduce a hard dependency to a specific logger implementation, especially as it does not have one right now.

Contributor

schmittjoh commented Nov 7, 2012

If it is decided that we use Monolog, @Seldaek might be open to add a
LoggerInterface, and a ZendLoggerAdapter implementation to Monolog.

On Wed, Nov 7, 2012 at 12:57 PM, Christophe Coevoet <
notifications@github.com> wrote:

@schmittjoh https://github.com/schmittjoh And then instantiate a
Monolog logger and register a (yet-to-be-implemented) ZendHandler wrapping
their ZF logger.
Thus, it is totally counter-intuitive as the Symfony LoggerInterface maps
easily to the Zf Logger interface whereas the Monolog HandlerInterface is
asking for different methods.
I don't think Symfony should introduce a hard dependency to a specific
logger implementation, especially as it does not have one right now.


Reply to this email directly or view it on GitHubhttps://github.com/symfony/symfony/issues/5911#issuecomment-10145970.

Contributor

lsmith77 commented Nov 7, 2012

there is also the option of just pushing for a LoggerInterface in the FIG group ..

Member

Seldaek commented Nov 8, 2012

I think a generic LoggerInterface would make more sense than every framework having the same interface. Duck typing would be best, but this is PHP.. another approach would be screw the type hints and just assume ->err and whatnot are implemented by what's passed, which is true of most (all?) logging libs.

Contributor

dlsniper commented Nov 8, 2012

@Seldaek based on your suggestion I think that we should have this submitted as a RFC to the PHP devs to have it included in PHP 5.5 and backported to the 5.3 and 5.4 releases as well.

What do you think about this?

Member

Seldaek commented Nov 8, 2012

That'd be ideal, but most likely not gonna happen. A PSR package shipping the interface would be good enough IMO.

Contributor

aerialls commented Nov 8, 2012

What about the idea of moving Monolog to a new Logging component?

Contributor

willdurand commented Nov 8, 2012

A LoggerInterface made by the FIG group would be nice. But the ML has been overflooded by CS discussions..

Owner

fabpot commented Nov 8, 2012

AFAIK, there aren't that many logging libraries in PHP (good ones). Monolog and ZF are probably the only/main ones. And honestly, I don't see the need to have yet another one in the future. As such, I don't think we need to overdo it. Let be honest, depending on Monolog or on a package that defines just an interface is almost the same (except that in the latter case, you also need to make a choice about which lib you want to install that implements the interface).

IMO, the best would be to depend on Monolog (and trying to convince @Seldaek to provide an adapter for ZF).

Member

Seldaek commented Nov 8, 2012

No need to convince me much :) It should be fairly easy AFAIK.

Contributor

dlsniper commented Nov 8, 2012

So what should we do next and how should we do it?

Thanks everyone for the time taken to talk about/fix this!

Owner

fabpot commented Nov 9, 2012

Here is the PR for the stopwatch classes: #5952

Contributor

dlsniper commented Nov 9, 2012

@fabpot thanks for the Stopwatch component.
Should I close this ticket then and try to the attention of FIG about this?

Owner

fabpot commented Nov 10, 2012

@dlsniper I'm closing this issue and I've opened a new one for the Logger interface discussion.

@fabpot fabpot closed this Nov 10, 2012

Owner

fabpot commented Nov 10, 2012

see #5968 for the logger discussion.

@ondrejmirtes ondrejmirtes pushed a commit to ondrejmirtes/symfony that referenced this issue Nov 25, 2013

@fabpot fabpot merged branch fabpot/stopwatch-component (PR #5952)
This PR was merged into the master branch.

Commits
-------

6b54a51 moved the Stopwatch classes to their own component

Discussion
----------

moved the Stopwatch classes to their own component

see symfony#5911

---------------------------------------------------------------------------

by stof at 2012-11-09T08:06:06Z

you should also add the ``.gitattributes`` in the new component

---------------------------------------------------------------------------

by fabpot at 2012-11-09T09:09:35Z

@stof: I've added the .gitattributes file, but also the phpunit and LICENSE ones.

---------------------------------------------------------------------------

by dlsniper at 2012-11-09T13:51:23Z

Also you should add this to the changelog/update files. I'll do it myself but only in about 8 hours or so as I'm not at home.
7ed728a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment