Skip to content

Improve dbal config#228

Open
rustamwin wants to merge 19 commits into
masterfrom
improve-config
Open

Improve dbal config#228
rustamwin wants to merge 19 commits into
masterfrom
improve-config

Conversation

@rustamwin
Copy link
Copy Markdown
Member

@rustamwin rustamwin commented Jul 13, 2025

Q A
Is bugfix?
New feature? ✔️
Breaks BC? ✔️

Before:

definition

[
    //...
    SimplePsrLogger::class => getPsrLogger()
];

params

'dbal' => [
        // SQL query logger. Definition of Psr\Log\LoggerInterface
        'query-logger' => SimplePsrLogger::class,
        // Default database
        'default' => null,
        'aliases' => [],
        'databases' => [],
        'connections' => [],
],

After:

definition

[
//...
DatabaseManager::class => static function (DbalFactory $dbalFactory) use (&$params) {
        $config = $params['yiisoft/yii-cycle']['dbal'];

        return $dbalFactory->create($config);
    },
//...
];

params

'dbal' => [
        // Whether to enable logging of all SQL queries to PSR logger.
        'query-logging' => false,
        // Default database
        'default' => null,
        'aliases' => [],
        'databases' => [],
        'connections' => [],
],

Using any PSR logger:

[
//...
DatabaseManager::class => static function () use (&$params) {
        $config = $params['yiisoft/yii-cycle']['dbal'];
        $logger = new MyPsrLogger();
        $dbalFactory = new DbalFactory($logger)
        return $dbalFactory->create($config);
    },
//...
];

@rustamwin rustamwin requested a review from a team July 13, 2025 11:53
@rustamwin rustamwin added the status:code review The pull request needs review. label Jul 13, 2025
Comment thread composer.json
Comment thread src/Factory/DbalFactory.php Outdated
Copy link
Copy Markdown
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

Looks good. Is it possible to avoid BC break?

Comment thread src/Factory/DbalFactory.php Outdated
Comment thread src/Factory/DbalFactory.php
Comment thread config/di.php Outdated
Comment thread config/di.php Outdated
@rustamwin rustamwin requested a review from vjik July 16, 2025 06:21
@rustamwin
Copy link
Copy Markdown
Member Author

Is it possible to avoid BC break?

It's OK, since there are already changes in the changelog that breaks BC.

Comment thread config/di.php Outdated
Comment thread config/di.php Outdated
Comment thread src/Factory/DbalFactory.php Outdated
Comment thread config/params.php Outdated
Comment thread src/Factory/DbalFactory.php Outdated
if ($this->logger !== null && $loggingEnabled === true) {
$logger = $this->logger;
$dbal->setLogger($logger);
/** Remove when issue is resolved {@link https://github.com/cycle/orm/issues/60} */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Issue is opened yet.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

But it works, see tests.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But it works, see tests.

Which test checks driver logger?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@roxblnfk
Copy link
Copy Markdown
Member

Could you provide an example of the configuration before and after in the PR description? It seems that previously you could specify any PSR logger for database queries, but now it's a boolean value that turns the default logger on or off.

@rustamwin
Copy link
Copy Markdown
Member Author

Could you provide an example of the configuration before and after in the PR description? It seems that previously you could specify any PSR logger for database queries, but now it's a boolean value that turns the default logger on or off.

Done.

@rustamwin rustamwin requested a review from a team July 27, 2025 06:09
@rustamwin rustamwin changed the title Add support to configure logger in definition format Improve dbal config Jul 27, 2025
@rustamwin rustamwin requested a review from roxblnfk July 29, 2025 04:57
@roxblnfk
Copy link
Copy Markdown
Member

The logs configuration behavior looks breaking changed there.
I can't say that I like the new behavior but I think it's 2.0

@roxblnfk roxblnfk added this to the 2.0 milestone Jul 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status:code review The pull request needs review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants