liip / LiipMonitorBundle

Integrates the LiipMonitor library into Symfony
http://liip.ch
467 stars 103 forks source link

Use current command's output for Console reporters #234

Closed kbond closed 4 years ago

kbond commented 4 years ago

I had an issue when trying to capture the command's output programatically. This is because the Console reporters use ConsoleOutput by default.

This PR has the command create the proper console reporter using it's current output. This enabled me to remove the dependencies. I kept the services for BC.

Does anyone think this would cause a BC break?

lsmith77 commented 4 years ago

I don’t quite understand what this change offers you. How does this avoid the dependency?

I definitely think it was rather ugly that we injected both instances when we only ever use one or the other but I guess in that case we should have made it two commands and not an option.

That being said, you change imho is not ideal since now its no longer dependency injection. So at the very least it should be a method to make it possible to create a different instance via extending the class.

This also brings me to the question of BC, it would obviously be a BC break in case someone extended the class (less likely) or if someone injected other classes (maybe slightly less likely but still not very likely). So overall I am not overlay concerned about this BC break.

That being said, maybe the best approach is instead to do what we should have done from the get go, two separate commands.

So maybe what should be done is split off a new command that expects ReporterInterface and we have two separate commands for the raw and non-raw case and we deprecate injection of the raw instance.

kbond commented 4 years ago

Thanks @lsmith77.

The aim of this PR is to fix a bug where this doesn't work as expected. The command will execute correctly but it is currently impossible to capture the output as the console reporters create their own output.

I need to somehow have the command inject it's output into the console reporters. There are two options I can see:

  1. Create the console reporters in the command and inject the command's output using the constructor (this PR) - I see your point about losing dependency injection on the command
  2. Add a setOutput method to the reporters and inject with this. Maybe this is the way to go?

I agree the command needs to be refactored as it's pretty ugly but at this point, I'd like to keep the scope of this PR to just fixing the bug.

WDYT?

lsmith77 commented 4 years ago

adding a setOutput sounds much less invasive

kbond commented 4 years ago

Ok, I switched to method injection. You're right, much less invasive.

The remaining potential BC break is if a user has overridden either of the console reporter services to inject their own output. When running the command, it overrides this output. I could add some logic to account for this but I'm inclined to wait until someone has a problem. WDYT?

lsmith77 commented 4 years ago

agreed