-
-
Notifications
You must be signed in to change notification settings - Fork 203
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
Pass a PSR logger to LoadDataFixturesDoctrineCommand #454
base: 3.6.x
Are you sure you want to change the base?
Conversation
af42133
to
5d0c3eb
Compare
config/services.xml
Outdated
@@ -8,6 +8,8 @@ | |||
<service id="doctrine.fixtures_load_command" class="Doctrine\Bundle\FixturesBundle\Command\LoadDataFixturesDoctrineCommand"> | |||
<argument type="service" id="doctrine.fixtures.loader" /> | |||
<argument type="service" id="doctrine" /> | |||
<argument type="collection"></argument> | |||
<argument type="service" id="logger" on-invalid="null"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if this is the right logger instance to use here, @derrabus maybe you will know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, I guess. In case, the app uses Monolog, we should subscribe to the Doctrine channel.
<tag name="monolog.logger" channel="doctrine" />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks, that was the missing piece 👍
$executor->setLogger($this->logger ?? static function ($message) use ($ui): void { | ||
$ui->text(sprintf(' <comment>></comment> <info>%s</info>', $message)); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing a closure as logger is deprecated. We could use an anonymous class instead that extends PSR-3's AbstractLogger
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your comment, but I see I made a big mistake here: I didn't check for support of the PSR-3 logger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a straightforward way to detect it. I think I'll just bump the dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I understood, Copilot gave me an example implementation. So… I guess I should remove the new $logger
property entirely then.
fb8fefc
to
5b9e0c0
Compare
composer.json
Outdated
@@ -21,7 +21,7 @@ | |||
], | |||
"require": { | |||
"php": "^7.4 || ^8.0", | |||
"doctrine/data-fixtures": "^1.3", | |||
"doctrine/data-fixtures": "^1.8", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I use __invoke
, this could be postponed to 3.7 🤔
5b9e0c0
to
0f4f1ac
Compare
Not doing so is deprecated. See doctrine/data-fixtures#462
0f4f1ac
to
ba7e567
Compare
Not doing so is deprecated.
See doctrine/data-fixtures#462