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

Add PreformattedEchoHandler #8930

Merged
merged 3 commits into from
Apr 18, 2019
Merged

Conversation

bergice
Copy link
Contributor

@bergice bergice commented Apr 16, 2019

cherry-picked from 4c3f3e6

Copy link
Contributor

@robbieaverill robbieaverill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind describing why this needs to exist in framework? It doesn't seem particularly useful to me. Wouldn't it better that the controller that renders dev output add its own preformatted tags around log output instead?

Also make logger work for stdout
@bergice
Copy link
Contributor Author

bergice commented Apr 17, 2019

Would you mind describing why this needs to exist in framework? It doesn't seem particularly useful to me. Wouldn't it better that the controller that renders dev output add its own preformatted tags around log output instead?

Time constraints.

This will be rewritten, there's an issue for this here: #5542

@bergice bergice closed this Apr 17, 2019
@bergice bergice reopened this Apr 17, 2019
Co-Authored-By: bergice <bergice@users.noreply.github.com>
@chillu
Copy link
Member

chillu commented Apr 18, 2019

Yep, I've authored that error handler - it's a hack, and marked as @internal. Really looking forward to refactoring this mess into Symfony Console, as mentioned in the code comments heh.

@chillu chillu merged commit 17f4d5f into silverstripe:4 Apr 18, 2019
@chillu chillu deleted the pulls/4/logging branch April 18, 2019 03:10
{
if ($logger = Injector::inst()->get(LoggerInterface::class)) {
if (Director::is_cli()) {
$logger->pushHandler(new StreamHandler('php://stdout'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoggerInterface doesn't have pushHandler methods

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is one of those cases where we squint a bit, and just assume that people will not apply loggers other than Monolog for now. We're effectively using LoggerInterface as a global singleton here, since @bergice other PR on queuedjobs is configuring that singleton with additional handlers, so both of these are assuming to work off the same instance. It's pretty messy, but the answer is not to use log handlers here at all, but rather Symfony Console.

@@ -23,6 +28,8 @@ class MigrateFileTask extends BuildTask

public function run($request)
{
$this->addLogHandlers();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we do that in the constructor rather than the run method?

*/
protected function addLogHandlers()
{
if ($logger = Injector::inst()->get(LoggerInterface::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we have a prototype specialisation, like

  Psr\Log\LoggerInterface.monolog:
    type: prototype
    class: Monolog\Logger

rather than taking the service and mutate its global state in here?

protected function addLogHandlers()
{
if ($logger = Injector::inst()->get(LoggerInterface::class)) {
if (Director::is_cli()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might worth having a separate logger class implementation rather than encoding this logic in here

@dnsl48
Copy link
Contributor

dnsl48 commented Apr 18, 2019

lol, too late :)

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.

5 participants