add AdapterManager in to Zend\Db\Adapter namespace #2903

Closed
wants to merge 19 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

ClemensSahs commented Nov 6, 2012

this AdapterManger allows handle multible database connections and aliase

unit tests will follow

#autoload/global.php
return array(
    'db_adapter_manager'=>array(
        'user'=>'global',

        'global'=>array(
            'driver'=>array(
                'driver'=>'Pdo_Mysql',
                'database'=>'my_database_name',
                'username'=>'root',
                'password'=>null,
                'characterset'=>'utf8'
            )
        ),
    ),
);
# module.php
    public function getServiceConfig()
    {
        return array(
            'factories' => array(
                'db_adapter_manager' => 'Zend\Db\Adapter\AdapterServiceFactory',
                'my_user_db_adapter' => function ($sm) {
                    $dbAdapterContainer = $sm->get('db_adapter_manager');
                    return $dbAdapterContainer->getAdapter('user');
                },
            )
        );
    }
# controller
    public function fooAction()
    {
         /* @var $dam\Zend\Mvc\Service\DbAdapterManager */
         $dam = $this->getServiceLocator()->get('db_adapter_manager');

         if ( $dam->hasAdapterConfig('global') ) {
             /* @var $adapter \Zend\Db\Adapter\Adapter */
             $adapter = $dam->getAdapter('global');
             $dam->addAdapter('news',$adapter);
         } else {
             // do something else
         }
    }
    public function userFooAction()
    {
         /* @var $userAdapter \Zend\Db\Adapter\Adapter */
         $userAdapter= $this->getServiceLocator()->get('my_user_db_adapter');
         $result = $userAdapter->query('SQL Querys',$userAdapter::QUERY_MODE_EXECUTE);
         var_dump ( $result->current );
    }
Member

ralphschindler commented Nov 9, 2012

I think this all belongs in the Zend\Mvc\Service location.

Contributor

ClemensSahs commented Nov 12, 2012

@ralphschindler
yes this is a better place for "AdapterManagerFactory" ... you think move both?

Member

ralphschindler commented Nov 13, 2012

Yes, I think both should be moved, they are only useful in an MVC application that consumes the ServiceManager (as described since both of these have dependencies on the ServiceManager).

@weierophinney weierophinney commented on an outdated diff Nov 16, 2012

library/Zend/Db/Adapter/AdapterManager.php
+ ) {
+ throw new \RuntimeException("adapter config on key (%s) is not an valid key or array");
+ } else {
+ $this->_dbAdapter[ $key ] = $this->factoryAdapter( $config, $this->getServiceLocator() );
+ }
+
+ return $this->_dbAdapter[ $key ];
+ }
+
+ /**
+ * @param array $config
+ * @param ServiceLocatorInterface $serviceLocator
+ * @throws InvalidArgumentException
+ * @return Adapter
+ */
+ public function factoryAdapter($config, ServiceLocatorInterface $serviceLocator=null)
@weierophinney

weierophinney Nov 16, 2012

Owner

rename to "adapterFactory"

@weierophinney weierophinney commented on an outdated diff Nov 16, 2012

library/Zend/Db/Adapter/AdapterManager.php
+
+class AdapterManager
+ implements ServiceLocatorAwareInterface
+{
+ /**
+ *
+ * @var Adapter[]
+ */
+ protected $_dbAdapter = array();
+
+ protected $_dbAdapterConfig = array();
+
+ /**
+ * @var ServiceLocatorInterface
+ */
+ protected $_serviceLocator;
@weierophinney

weierophinney Nov 16, 2012

Owner

Do not prefix non-public members with an underscore. We only allow it in the CS for legacy reasons of the old codebase; new code should not have them.

@weierophinney weierophinney commented on an outdated diff Nov 16, 2012

library/Zend/Db/Adapter/AdapterManager.php
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2012 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ * @package Zend_Db
+ */
+
+namespace Zend\Db\Adapter;
+
+use Zend\ServiceManager\ServiceLocatorInterface;
+use Zend\ServiceManager\ServiceLocatorAwareInterface;
+use Zend\Db\Adapter\Adapter;
+
+class AdapterManager
+ implements ServiceLocatorAwareInterface
@weierophinney

weierophinney Nov 16, 2012

Owner

Move the "implements" keyword to the previous line.

@weierophinney weierophinney commented on an outdated diff Nov 16, 2012

library/Zend/Db/Adapter/AdapterManager.php
+ * @var Adapter[]
+ */
+ protected $_dbAdapter = array();
+
+ protected $_dbAdapterConfig = array();
+
+ /**
+ * @var ServiceLocatorInterface
+ */
+ protected $_serviceLocator;
+
+ /**
+ * @param array $config
+ * @throws \RuntimeException
+ */
+ public function addDbAdapterConfig (array $configArray)
@weierophinney

weierophinney Nov 16, 2012

Owner

Throughout the file:

  • Remove any spaces between the function name and the opening paren
  • Remove spaces after the opening paren of an argument list
  • Remove spaces before the closing paren of an argument list

@weierophinney weierophinney commented on an outdated diff Nov 16, 2012

library/Zend/Db/Adapter/AdapterManager.php
+ protected $_dbAdapter = array();
+
+ protected $_dbAdapterConfig = array();
+
+ /**
+ * @var ServiceLocatorInterface
+ */
+ protected $_serviceLocator;
+
+ /**
+ * @param array $config
+ * @throws \RuntimeException
+ */
+ public function addDbAdapterConfig (array $configArray)
+ {
+ foreach ( $configArray as $key=>$config) {
@weierophinney

weierophinney Nov 16, 2012

Owner

Throughout the file, on conditional statements or loop operators:

  • Remove spaces after the opening paren of a conditional
  • Remove spaces before the closing paren of a conditional
  • Add spaces around operators (including =>)

@weierophinney weierophinney commented on an outdated diff Nov 16, 2012

library/Zend/Db/Adapter/AdapterManager.php
+ */
+ public function hasAdapter ($key)
+ {
+ return ( isset($this->_dbAdapter[$key]) );
+ }
+
+ /**
+ * @param string $key
+ * @throws \RuntimeException
+ * @return Adapter
+ */
+ public function getAdapter ($key)
+ {
+ if ( !$this->hasAdapter($key) ) {
+ if ( !$this->hasAdapterConfig($key) ) {
+ throw new \RuntimeException(sprintf("adapter key (%s) not exist",$key));
@weierophinney

weierophinney Nov 16, 2012

Owner

Use the component-level exceptions, please -- not global exceptions.

@weierophinney weierophinney commented on an outdated diff Nov 16, 2012

library/Zend/Db/Adapter/AdapterManager.php
+ $driver = null;
+ $platform = null;
+ $queryResultPrototype = null;
+
+ if ( isset($config['driver']) ) {
+ if ( is_array($config['driver']) ) {
+ $driver = $config['driver'];
+ } elseif ( is_string($config['driver']) ) {
+ if( class_exists($config['driver']) ) {
+ $driver = new $config['driver']();
+ } else {
+ $driver = $serviceLocator->get($config['driver']);
+ }
+
+ if ( !is_object($platform) ) {
+ throw new InvalidArgumentException("database config['driver'] string is not a confirmed class/service name");
@weierophinney

weierophinney Nov 16, 2012

Owner

This exception class has not been imported.

@weierophinney weierophinney commented on an outdated diff Nov 16, 2012

library/Zend/Db/Adapter/AdapterManagerFactory.php
+ * @package Zend_Db
+ */
+
+namespace Zend\Db\Adapter;
+
+use Zend\ServiceManager\FactoryInterface;
+use Zend\ServiceManager\ServiceLocatorInterface;
+
+/**
+ * @category Zend
+ * @package Zend_Db
+ * @subpackage Adapter
+ */
+class AdapterManagerFactory
+ implements FactoryInterface
+{
@weierophinney

weierophinney Nov 16, 2012

Owner

Move "implements" keyword up a line.

Owner

weierophinney commented Nov 16, 2012

Two additional notes:

  • Needs unit tests! :)
  • Also, Zend\Db\Adapter\AdapterServiceFactory already uses the "db" key, which means that we could end up with a configuration conflict. I think the top-level configuration key for your AdapterManager needs to be renamed -- perhaps something like "db_adapter_manager" or "db_multi".
Contributor

ClemensSahs commented Nov 17, 2012

after the proposal from ralph i tried to solve the dependencies to the ServiceManager package...
The tow last commits of this branch are one Version.
Currently i only have this case in for databases. Theoretically this comes for every class you want init smartly...
So I'm not sure what way is the right:

  • do it only for DB ( with the current proposal )
  • do it with a abstract way
  • or do nothing and everyone do it self ( but ugly )

@weierophinney
thanks i will patch this ;)

tihe commented Dec 22, 2012

Is this available for end users (configuring/using multiple databases)? If not could you please point me in the right direction for an intermediate solution?

Owner

weierophinney commented Jan 2, 2013

@clemenssahs do you have time to do this in the next 1-2 weeks? If not, I need to change the milestone.

@zensys This is a pull request adding the feature; it's not available currently in the master repository.

Member

ralphschindler commented Jan 4, 2013

Do you have any additional comments here? If not, do you mind if I take this for inclusion in 2.1, my only change is that it resides in the Zend\Mvc\Service namespace as it has dependencies on Zend\Mvc conventions and also promotes a configuration convention.

Contributor

ClemensSahs commented Jan 15, 2013

@weierophinney
currently i have time agin, sry for be so far away

@ralphschindler yes i will change some think and will implement unit tests

for all
i will try this changes to this Weekend

@ClemensSahs ClemensSahs - move in to Zend/Mvc/Service
- patch some coding standards
- rename config key from 'db' to 'db_adapter_manager'
4a6a63b
Owner

weierophinney commented Jan 21, 2013

@clemenssahs Feature freeze is in a couple days. I'm going to remove the milestone. If you get it done by then, I'll re-assign it to 2.1.

Contributor

ClemensSahs commented Jan 23, 2013

@weierophinney
I think I'm finished. if no one has if objections or improvements.

@zensys
no it is finished, I have update the description. So its easy to write a Doc

Contributor

radnan commented Jan 23, 2013

I'm wondering if DbAdapterManager can just extend off of Zend\ServiceManager\AbstractPluginManager. That way you don't have to recreate so much of the service locator functions. The DbAdapterManagerFactory can then just be an extension of Zend\Mvc\Service\AbstractPluginManagerFactory.

The DbAdapterManager should be added to the list of configurable service managers in Zend\Mvc\Service\ModuleManagerFactory. This way users can manage the db adapters just as they would any other service/plugin provider.

Contributor

ClemensSahs commented Jan 23, 2013

To implement the ServiceLocator has only one benefits. Add the ability to use ServiceKeys like aliase.
So if you have a "driver factory" you can use it, the same with a platform object.

I think a extention of AbstractPluginManager to much for this case.
Or I misunderstand your point?

Contributor

radnan commented Jan 24, 2013

Right, but extending the AbstractPluginManager gives you all that for free essentially. This way you can:

  • add factories to define custom adapters
    • look up config and other stuff from within the factory
  • use the built-in aliasing
  • use invokables for custom drivers and platforms

and not have to reinvent a lot of the stuff you're doing in your manager.

Contributor

ClemensSahs commented Jan 24, 2013

Sounds nice but overstocked. The main benefit of my PR is a multi db alternative for Zend\Db\Adapter\AdapterServiceFactory if your improvements make it easy for End-User, then I'm vote for it too.

But I see currently only more complexity. Can you give me a example config for your version like the one in the description?

Contributor

radnan commented Jan 24, 2013

Sure:

class Module
{
    public function getConfig()
    {
        return array(
            'db_adapters' => array(
                'adapters' => array(
                    'module1' => array(
                        'driver' => 'pdo_mysql',
                        'username' => 'user',
                        'password' => 'pass',
                    ),
                ),
                'aliases' => array(
                    'default' => 'module1',
                ),
                'factories' => array(
                    'module2' => function ()
                    {
                        return new Adapter($driver, $platform);
                    },
                ),
            ),
        );
    }
}

class IndexController
{
    /** @var \Zend\Db\AdapterManager */
    protected $adapters;

    public function indexAction()
    {
        $adapter = $this->adapters->get('default');
    }
}
Contributor

ClemensSahs commented Jan 25, 2013

@radnan
here is a version with implement your improvements. Its a preview, currently I want move the building methods in to a separate factoring Class.
ClemensSahs/zf2@bcd7815

refactoring in progress for Unit-Test and Exceptions

PS: the methodes I mean

  • getQueryResultPrototype
  • getDriverObjectFromConfig
  • getPlatformObjectFromConfig
  • createFromAdapterConfig

any feedback?

Member

ralphschindler commented Jan 25, 2013

@clemenssahs I find your patch pretty interesting.
Since @EvanDotPro wrote the originaly factory in Zend\Db, I'd like to get his opinion on this too, also since he had a hand in ZfcUser, which consumes database adapters.

@weierophinney weierophinney commented on the diff Jan 25, 2013

library/Zend/Mvc/Service/DbAdapterManager.php
+use Zend\Db\Adapter\Adapter;
+use Zend\Db\Sql\Platform\Platform;
+use Zend\Mvc\Service\Exception\DbAdapterManagerAdapterAlreadyRegistered;
+use Zend\Mvc\Service\Exception\DbAdapterManagerAdapterCoundInit;
+use Zend\Mvc\Service\Exception\DbAdapterManagerAdapterNotExist;
+use Zend\Mvc\Service\Exception\DbAdapterManagerAdapterConfigNotValid;
+use Exception;
+
+class DbAdapterManager implements ServiceLocatorAwareInterface
+{
+ /**
+ *
+ * @var Adapter[]
+ */
+ protected $_dbAdapter = array();
+
@weierophinney

weierophinney Jan 25, 2013

Owner

Don't prefix non-public members with underscores; no longer needed with ZF2 CS.

Contributor

radnan commented Jan 26, 2013

I'd like to humbly submit my take on the issue:

radnan/zf2@42245d4

Contributor

ClemensSahs commented Jan 26, 2013

@radnan
its looks nice ;)

but on this way we don't can customizes "plattform objects" and "result prototype"
Currently I miss this is a feature on your commit...

Member

ralphschindler commented Feb 15, 2013

I am going to close this PR. The idea is good, but this particular implementation is not what needs to happen. Probably @radnan's implementation is closer, if he were to move it into the Zend\Mvc namespace. From there I think we can open an new PR when that one is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment