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

Introduced a PSR-3 logger adapter #3156

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions UPGRADE.md
@@ -1,3 +1,11 @@
# Upgrade to 2.8

## Deprecated logger implementations

With the introduction of the [PSR-3](https://www.php-fig.org/psr/psr-3/) logger interface standard, we can focus on implementing DBAL-specific features and delegate to [other libraries](https://packagist.org/providers/psr/log-implementation).

`Doctrine\DBAL\Logging\EchoSQLLogger` has been deprecated. Please use `Doctrine\DBAL\Logging\PsrAdapter` and a PSR-3 compatible implementation instead.

# Upgrade to 2.7

## Doctrine\DBAL\Platforms\AbstractPlatform::DATE_INTERVAL_UNIT_* constants deprecated
Expand Down
2 changes: 2 additions & 0 deletions lib/Doctrine/DBAL/Logging/EchoSQLLogger.php
Expand Up @@ -31,6 +31,8 @@
* @author Guilherme Blanco <guilhermeblanco@hotmail.com>
* @author Jonathan Wage <jonwage@gmail.com>
* @author Roman Borschel <roman@code-factory.org>
*
* @deprecated
*/
class EchoSQLLogger implements SQLLogger
{
Expand Down
37 changes: 37 additions & 0 deletions lib/Doctrine/DBAL/Logging/PsrAdapter.php
@@ -0,0 +1,37 @@
<?php

namespace Doctrine\DBAL\Logging;

use Psr\Log\LoggerInterface;

/**
* Logs every query as a debug message
Copy link
Member

Choose a reason for hiding this comment

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

Probably no longer true

Copy link
Member Author

Choose a reason for hiding this comment

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

Why?

*/
final class PsrAdapter implements SQLLogger
Copy link
Member

Choose a reason for hiding this comment

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

Rather than PsrAdapter, PsrLoggerSqlLogger? PsrSqlLogger?

Copy link
Member Author

@morozov morozov May 25, 2018

Choose a reason for hiding this comment

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

I think the "adapter" here is essential since unlike other implementations, this class just converts calls from one API to another. While the SQL part is implied since the DBAL doesn't log anything else than SQL.

{
/** @var LoggerInterface */
private $logger;

public function __construct(LoggerInterface $logger)
Copy link
Member

Choose a reason for hiding this comment

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

This requires an addition to composer.json

Copy link
Member Author

Choose a reason for hiding this comment

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

It’s not a hard requirement. Do you want to add a suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

Would need to be a hard requirement if we rely on its signature in any way.

Copy link
Member Author

Choose a reason for hiding this comment

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

We rely on this signature in the same way as on signatures of PDO, mysqli and other extensions, all of which are optional.

Looks like we have different expectations from PSR-3 in DBAL. As I see it, it will just enable dumping debug information into an abstract channel as requested in #2911. We're not deprecating SQLLogger which has a more specific interface oriented on query execution, not just messages. Therefore, no need to introduce a hard dependency on PSR-3.

{
$this->logger = $logger;
}

/**
* {@inheritDoc}
*/
public function startQuery($sql, array $params = null, array $types = null)
{
$this->logger->debug($sql, [
'params' => $params,
'types' => $types,
]);
}

/**
* {@inheritDoc}
*/
public function stopQuery()
{
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to log stopQuery as debug too.

Copy link
Member Author

Choose a reason for hiding this comment

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

This way we’ll have two almost identical consecutive messages per query. Without precise timestamps it will be just a noise. Want to elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but the start/end are vital to profile queries. Filtering them out would be trivial here, while adding them wouldn't be as easy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think, using a logging interface for profiling is an abuse. SQLLogger suits this need better.

}
}
1 change: 1 addition & 0 deletions tests/Doctrine/Tests/DBAL/ConnectionTest.php
Expand Up @@ -196,6 +196,7 @@ public function getQueryMethods()
* Pretty dumb test, however we want to check that the EchoSQLLogger correctly implements the interface.
*
* @group DBAL-11
* @group legacy
*/
public function testEchoSQLLogger()
{
Expand Down
29 changes: 29 additions & 0 deletions tests/Doctrine/Tests/DBAL/Logging/PsrAdapterTest.php
@@ -0,0 +1,29 @@
<?php

namespace Doctrine\DBAL\Logging;

use Doctrine\DBAL\ParameterType;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;
use function interface_exists;

class PsrAdapterTest extends TestCase
{
public function testLogging()
{
if (! interface_exists(LoggerInterface::class)) {
$this->markTestSkipped('PSR-3 LoggerInterface is unavailable');
}

$logger = $this->createMock(LoggerInterface::class);
$logger->expects($this->once())
->method('debug')
->with('SELECT name FROM users WHERE id = ?', [
'params' => [1],
'types' => [ParameterType::INTEGER],
]);

$adapter = new PsrAdapter($logger);
$adapter->startQuery('SELECT name FROM users WHERE id = ?', [1], [ParameterType::INTEGER]);
}
}