Zend\Log\Writer\Db via config throws exception #5254

Closed
wants to merge 1 commit into from

4 participants

@Martin-P

I use this config to create a logger:

'log' => array(
    'Application\Log' => array(
        'writers' => array(
            array(
                'name'     => 'db',
                'priority' => 1,
                'options'  => array(
                    'separator' => '_',
                    'column'    => array(),
                    'table'     => 'applicationlog',
                    'db'        => 'Zend\Db\Adapter\Adapter',
                ),
            ),
        ),
        #'exceptionhandler' => true,
        #'errorhandler'     => true,
    ),
),

With this config I can get the logger via the service manager:

$serviceManager->get('Application\Log');

This results in the following exception:

Zend\Log\Exception\InvalidArgumentException: You must pass a valid Zend\Db\Adapter\Adapter in [...]\vendor\zendframework\zendframework\library\Zend\Log\Writer\Db.php on line 74

Zend\Log\Writer\Db expects an instance of Zend\Db\Adapter\Adapter for the option key db.

Suggested fixes:

  • If db is a string, use the service manager to get the class by the name provided in the db option key.
  • If db is a closure, get the result of the closure.

When the string possibility and the closure possibility is checked and processed there can be checked if this results in a valid instance of Zend\Db\Adapter\Adapter.

@ThaDafinser

@Martin-P i think string should be already fine, see here: https://github.com/zendframework/zf2/blob/master/library/Zend/Log/Logger.php#L236-L250

But i think you have to use the FQN: Zend\Log\Writer\Db

Does that work?

@Martin-P

But i think you have to use the FQN: Zend\Log\Writer\Db

Thanks for your reply, but the location of the writer is not the problem. The writer is found and the exception (see opening post) is thrown by the correct writer. If the writer itself could not be found, it would not be able to throw an exception.

The problem is the database adapter. Zend\Log\Writer\Db says the adapter is not valid, because it only accepts an instance of Zend\Db\Adapter\Adapter.

Because you want to be able to cache your config, it should be possible to get a database adapter by providing a string. The constructor of Zend\Log\Writer\Db should detect that the db parameter is a string and try to get an instance of the adapter via the service manager.

@ThaDafinser

Ah sorry 😄 ....the same problem, but one "array level" deeper =)

@weierophinney
Zend Framework member

The problem is that the LoggerAbstractServiceFactory simply passes the configuration on to the Logger's constructor -- and the Logger class does not have access to the service manager.

The solution would be to pre-analyze the configuration to see if it contains any DB writers, and, if so, pull the associated adapter and replace the configuration value with that adapter. I'll see if I can work something up.

@weierophinney weierophinney [#5254] Pre-process configuration to inject DB adapter instance
- Looks for a named DB adapter in the writer configuration, and, if
  found, retrieves it from the service locator passed to the factory,
  re-setting the db configuration to use the discovered adapter.
6643729
@ralphschindler ralphschindler added a commit that closed this pull request Oct 31, 2013
@ralphschindler ralphschindler Fixes #5254
Merge branch 'weierophinney-hotfix/5254'

* weierophinney-hotfix/5254:
  [#5254] Pre-process configuration to inject DB adapter instance
213cbca
@ralphschindler ralphschindler added a commit that referenced this pull request Oct 31, 2013
@ralphschindler ralphschindler Forward port #5254
Merge branch 'weierophinney-hotfix/5254' into develop

* weierophinney-hotfix/5254:
  [#5254] Pre-process configuration to inject DB adapter instance
0aab17b
@Martin-P

This fix introduces a new problem. It is now impossible to extend the Zend\Log\Writer\Db and use my own writer in the config like this:

'log' => array(
    'Application\Log' => array(
        'writers' => array(
            array(
                'name'     => 'MyLog\Writer\DbWriter',
                'priority' => 1,
                'options'  => array(
                    'separator' => '_',
                    'column'    => array(),
                    'table'     => 'applicationlog',
                    'db'        => 'Zend\Db\Adapter\Adapter',
                ),
            ),
        ),
    ),
),

I need my own writer to compensate for a bug #2589 that still exist in Zend\Log\Writer\Db. The check on line 99 of Zend/Log/LoggerAbstractServiceFactory is the cause of this problem:

|| strtolower($writerConfig['name']) != 'db'

That check is not needed at all and makes it impossible to use my own writer. The checks on the following lines already check if $writerConfig['options']['db'] exist and that is enough. If for some reason there is an invalid writer provided in the config, Zend\Log\WriterPluginManager throws an exception and therefore all possible scenarios are already covered by the existing code.

Edit:
Nevermind this post, I thought fixing this bug would make Zend\Logger usable via config, but it still has too many issues.

@weierophinney
Zend Framework member

@Martin-P I was testing on the writer name, but really I only need to test for a "db" option. I'll create a PR for that in the next day or two.

@weierophinney weierophinney added a commit to weierophinney/zendframework that referenced this pull request Dec 5, 2013
@weierophinney weierophinney Improvement to #5254
- Do not check on the writer name, only the presence, type, and
  availability of the "db" option value.

  See zendframework#5254 (comment)
d0d6a79
@ralphschindler ralphschindler added a commit that referenced this pull request Dec 5, 2013
@ralphschindler ralphschindler Merging PR #5588
Merge branch 'weierophinney-hotfix/5254-improvement'

* weierophinney-hotfix/5254-improvement:
  Improvement to #5254
55d43c9
@ralphschindler ralphschindler added a commit that referenced this pull request Dec 5, 2013
@ralphschindler ralphschindler Foward PR #5588
Merge branch 'weierophinney-hotfix/5254-improvement' into develop

* weierophinney-hotfix/5254-improvement:
  Improvement to #5254
88480e7
@Martin-P

A bit late, but thanks, #5588 fixed the issue with the custom writer.

@ThaDafinser ThaDafinser added a commit that referenced this pull request Mar 25, 2014
@weierophinney weierophinney Improvement to #5254
- Do not check on the writer name, only the presence, type, and
  availability of the "db" option value.

  See #5254 (comment)
e5859ed
@weierophinney weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#5254] Pre-process configuration to injec…
…t DB adapter instance

- Looks for a named DB adapter in the writer configuration, and, if
  found, retrieves it from the service locator passed to the factory,
  re-setting the db configuration to use the discovered adapter.
4f2a7d8
@gianarb gianarb pushed a commit to zendframework/zend-log that referenced this pull request May 15, 2015
@ralphschindler ralphschindler Fixes zendframework/zendframework#5254
Merge branch 'weierophinney-hotfix/5254'

* weierophinney-hotfix/5254:
  [zendframework/zendframework#5254] Pre-process configuration to inject DB adapter instance
ce75f80
@gianarb gianarb pushed a commit to zendframework/zend-log that referenced this pull request May 15, 2015
@ralphschindler ralphschindler Forward port zendframework/zendframework#5254
Merge branch 'weierophinney-hotfix/5254' into develop

* weierophinney-hotfix/5254:
  [zendframework/zendframework#5254] Pre-process configuration to inject DB adapter instance
81c3d1e
@weierophinney weierophinney added a commit to zendframework/zend-log that referenced this pull request May 15, 2015
@weierophinney weierophinney Improvement to zendframework/zendframework#5254
- Do not check on the writer name, only the presence, type, and
  availability of the "db" option value.

  See zendframework/zendframework#5254 (comment)
0307992
@gianarb gianarb pushed a commit to zendframework/zend-log that referenced this pull request May 15, 2015
@ralphschindler ralphschindler Merging PR zendframework/zendframework#5588
Merge branch 'weierophinney-hotfix/5254-improvement'

* weierophinney-hotfix/5254-improvement:
  Improvement to zendframework/zendframework#5254
df3a849
@gianarb gianarb pushed a commit to zendframework/zend-log that referenced this pull request May 15, 2015
@ralphschindler ralphschindler Foward PR zendframework/zendframework#5588
Merge branch 'weierophinney-hotfix/5254-improvement' into develop

* weierophinney-hotfix/5254-improvement:
  Improvement to zendframework/zendframework#5254
d91945a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment