Set connection debug mode on StandardServiceContainer

[mringler]

In order to get log messages about executed queries, the connection has to be set to debug mode. Currently, this is done by accessing the connection through the service container:

serviceContainer = Propel::getServiceContainer();
$connection = $serviceContainer->getWriteConnection('default');
$connection->useDebug(true);

This is a bit cumbersome for two reasons (see this discussion):

1. Users have to interact with Propel internals
2. The method useDebug() is only available on ConnectionWrapper, but not on ConnectionInterface, which is what ServiceContainerInterface::getWriteConnection() returns, causing type errors when using a checker (or more boilerplate code).

A simple solution is to allow managing of the connection debug mode through the service container.

Currently, the new method to set connection debug mode allows for three values: true, false and null, where null means to leave the connection unchanged. If you prefer it, I can add consts for those values.

[codecov-commenter]

Codecov Report

Merging #1873 (22d0e6e) into master (ba9abb9) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #1873   +/-   ##
=========================================
  Coverage     87.61%   87.62%           
- Complexity     7807     7812    +5     
=========================================
  Files           227      227           
  Lines         21144    21157   +13     
=========================================
+ Hits          18525    18538   +13     
  Misses         2619     2619           

Flag	Coverage Δ
5-max	87.62% <100.00%> (+<0.01%)	:arrow_up:
7.4	87.62% <100.00%> (+<0.01%)	:arrow_up:
agnostic	66.98% <100.00%> (+0.02%)	:arrow_up:
mysql	68.91% <80.00%> (+<0.01%)	:arrow_up:
pgsql	68.93% <80.00%> (+<0.01%)	:arrow_up:
sqlite	66.75% <80.00%> (+<0.01%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...time/ServiceContainer/StandardServiceContainer.php	87.74% <100.00%> (+1.12%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ba9abb9...22d0e6e. Read the comment docs.

[yaroslav-spryker]

Hi @mringler from my perspective the service container does not really need a knowledge about connection it wraps and even more does not need to have a state ($this->setWrappedConnetionToDebug property) for debugging connection. Propel has it`s default way to inject a logger via service container and I saw you already mentioned this in related dicsussion https://github.com/propelorm/Propel2/discussions/1871

$loggerConfiguration = [
    'type' => 'stream',
    'path' => 'path/to/file'
    'level' => Logger::INFO, // Defaults to Logger::CRITICAL,
    'bubble' => true, // Defaults to true
];
Propel::getServiceContainer()->setLoggerConfiguration('defaultLogger', $loggerConfiguration);

[mringler]

Hey @yaroslav-spryker, not sure I understand your comment correctly. The code you posted is about setting a logger, but this is about setting the connection into debug mode - which uses the logger, but is a completely different matter.

In general, the idea is to allow fast and type-safe access to important Propel functionality, i.e. allow for this:

Propel::getServiceContainer()->useDebugMode(true);

instead of this:

$connection = Propel::getServiceContainer()->getWriteConnection('default');
if ($connection instanceof ConnectionWrapper) {
    $connection->useDebug(true);
}

Note that this (type-safe) snipped is not available in the documentation, the functionality is not apparent when browsing through the ServiceContainer or ConnectionInterface, and it requires users to know about the ConnectionWrapper class. So basically, you cannot use this functionality unless someone tells you about it, even though it is crucial to get connection output during development (without it, you are left with guessing why a query is not working).

I think that leads to the impression of bad usability. And hence my proposal to ease things up, similar to how setLoggerConfiguration() is a convenience over setting the logger manually.

You don't see this as an improvement?

[mringler]

Changed the a bit awkwardly named $setWrappedConnetionToDebug and setWrappedConnectionDebugMode() to the more general $useDebugModeFlag and useDebugMode(). Users probably don't care that debug mode is realized through the connection, and if there is ever another component with a debug mode, it can be integrated seamlessly.

[yaroslav-spryker]

@mringler Sorry seems I formulated the idea is not clear enough :( What I wanted to point out that to my opinion setting debug mode for connection is a bit out of scope of ServiceContainer. If we look at what ServiceContainer has inside - we see only setting and getting previously provided functionality. Moreover it should not have a logic inside of it like checking an instance of ConnectionWrapper. I would better implement it other ways, maybe via configuration setting? Propel has already something similar with setting connection wrapper class so maybe it's better to add one more setting from there?

Please note that I don't discuss implementation details but the approach itself :)

[mringler]

@yaroslav-spryker So your proposal is to bury the debug flag deeper into Propel, i.e. into the ConnectionManagers, the ConnectionFactory, or the ConnectionWrapper? Where exactly would you do that?

Note that debug mode can be enabled before and after all connection mangers are set and before and after all connections are initialized.

[mringler]

@yaroslav-spryker Only solution I can think of is to change ConnectionWrapper by making $useDebug (which is public) to a getter, and add a static variable:

class ConnectionWrapper implements ConnectionInterface, LoggerAwareInterface
{
    public static useDebug = false;
    protected useDebugOnInstance = null;

    public isInDebugMode(): bool
    {
        return $this->useDebugOnInstance ?? static::useDebug;
    }

Then debug mode can be enabled by changing the static variable:

ConnectionWrapper::useDebug = true;

But considering how static variables are not popular in the project, I am not sure if this will find much love. What do you think?

[dereuromark]

A static one in this particular case could be fair enough. Maybe make that separate PR and we can then as a group discuss what solution is better to merge.

[mringler]

A static one in this particular case could be fair enough. Maybe make that separate PR and we can then as a group discuss what solution is better to merge.

Sure, what else to do on vacation: https://github.com/propelorm/Propel2/pull/1874

[yaroslav-spryker]

A static one in this particular case could be fair enough. Maybe make that separate PR and we can then as a group discuss what solution is better to merge.

Sure, what else to do on vacation: #1874

@mringler Sorry for the late response but yeah, using ConnectionWrapper seems to be a way more cleaner solution in our case. Thank you very much for additional PR, I've already approved it.