Added database adapter abstract service factory. #3933

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
@stephanoff
Contributor

stephanoff commented Mar 1, 2013

Added database adapter abstract service factory to configure multiple database adapters for application. This abstract factory reserved db.adapters key in configuration array.

array(
            'db' => array(
                'adapters' => array(
                    'Zend\Db\Adapter\Master' => array(
                        'driver' => 'mysqli',
                    ),
                    'Zend\Db\Adapter\Slave' => array(
                        'driver' => 'mysqli',
                    ),
                ),
            ),
)

weierophinney and others added some commits Mar 1, 2013

Added database adapter abstract service factory.
Change-Id: I35ed79abc898088c50d40467d6e64726e433e9f7
@blanchonvincent

View changes

library/Zend/Db/Adapter/AdapterAbstractServiceFactory.php
+ if (isset($config['db'][$name])) {
+ return new Adapter($config['db'][$name]);
+
+ } else if (isset($config['db'][$requestedName])) {

This comment has been minimized.

Show comment Hide comment
@blanchonvincent

blanchonvincent Mar 2, 2013

Contributor

you should :

  • no use "elseif" because you have check with "canCreateServiceWithName"
  • throw exception after "elseif"

But should not return null ?

@blanchonvincent

blanchonvincent Mar 2, 2013

Contributor

you should :

  • no use "elseif" because you have check with "canCreateServiceWithName"
  • throw exception after "elseif"

But should not return null ?

@blanchonvincent

View changes

library/Zend/Db/Adapter/AdapterAbstractServiceFactory.php
+ return true;
+
+ } else {
+ return false;

This comment has been minimized.

Show comment Hide comment
@blanchonvincent

blanchonvincent Mar 2, 2013

Contributor

delete the "else", finish with "return false" without "else"

@blanchonvincent

blanchonvincent Mar 2, 2013

Contributor

delete the "else", finish with "return false" without "else"

Removed redundant else statement and redundat condition in factory
method.

Change-Id: Ia6882123128c1752151f4c4560cb8d1fcb57efa5
@stephanoff

This comment has been minimized.

Show comment Hide comment
@stephanoff

stephanoff Mar 2, 2013

Contributor

@blanchonvincent, redundant code removed.

Contributor

stephanoff commented Mar 2, 2013

@blanchonvincent, redundant code removed.

@stephanoff stephanoff closed this Mar 2, 2013

@stephanoff stephanoff reopened this Mar 2, 2013

@blanchonvincent

This comment has been minimized.

Show comment Hide comment
@blanchonvincent

blanchonvincent Mar 2, 2013

Contributor

@stephanoff ok, now you must add unit tests with the PR

Contributor

blanchonvincent commented Mar 2, 2013

@stephanoff ok, now you must add unit tests with the PR

Added test case for database adapter abstract service factory.
Change-Id: Id77938e0298d3b214c40db7f2325f5b1efd14d81
@stephanoff

This comment has been minimized.

Show comment Hide comment
@stephanoff

stephanoff Mar 2, 2013

Contributor

@blanchonvincent Test case added.

Contributor

stephanoff commented Mar 2, 2013

@blanchonvincent Test case added.

@ralphschindler

This comment has been minimized.

Show comment Hide comment
@ralphschindler

ralphschindler Mar 8, 2013

Member

This doesn't belong in the Zend\Db namespace, it really belongs in the Zend\Mvc namespace. Please see this discussion: zendframework#2903

Member

ralphschindler commented Mar 8, 2013

This doesn't belong in the Zend\Db namespace, it really belongs in the Zend\Mvc namespace. Please see this discussion: zendframework#2903

@stephanoff

This comment has been minimized.

Show comment Hide comment
@stephanoff

stephanoff Mar 8, 2013

Contributor

@ralphschindler Should I move AdapterAbstractServiceFactory to MVC namespace or my PR rejected?

Contributor

stephanoff commented Mar 8, 2013

@ralphschindler Should I move AdapterAbstractServiceFactory to MVC namespace or my PR rejected?

@ralphschindler

This comment has been minimized.

Show comment Hide comment
@ralphschindler

ralphschindler Mar 8, 2013

Member

I would think we should just move the files, and keep this PR open. I'd like to also address the previous work that was done one creating a service factory for Db in the other PR.

Member

ralphschindler commented Mar 8, 2013

I would think we should just move the files, and keep this PR open. I'd like to also address the previous work that was done one creating a service factory for Db in the other PR.

@stephanoff

This comment has been minimized.

Show comment Hide comment
@stephanoff

stephanoff Mar 8, 2013

Contributor

@ralphschindler Could you please provide FQCN for the AbstractClass since I'm new with ZF2 conventions. Thanks.

Contributor

stephanoff commented Mar 8, 2013

@ralphschindler Could you please provide FQCN for the AbstractClass since I'm new with ZF2 conventions. Thanks.

@blanchonvincent

This comment has been minimized.

Show comment Hide comment
@blanchonvincent

blanchonvincent Mar 9, 2013

Contributor

@ralphschindler I think it's good because :
• there is a "Zend\Db\Adapter\AdapterServiceFactory" in Zend\Db, i think the AdapterAbstractServiceFactory must to be in the same place that AdapterServiceFactory.
• there are lot of factories in her own namespaces : Zend\Navigation\Service\DefaultNavigationFactory, Zend\Cache\Service\StorageCacheFactory, etc.

Contributor

blanchonvincent commented Mar 9, 2013

@ralphschindler I think it's good because :
• there is a "Zend\Db\Adapter\AdapterServiceFactory" in Zend\Db, i think the AdapterAbstractServiceFactory must to be in the same place that AdapterServiceFactory.
• there are lot of factories in her own namespaces : Zend\Navigation\Service\DefaultNavigationFactory, Zend\Cache\Service\StorageCacheFactory, etc.

Added db.adapters section for multiple adapters configuration.
Improved validation for existing adapter configuration.
Updated unit test.

Change-Id: I2ae710320d34a2fb888fcce49969cb776b27b5a7

@ghost ghost assigned weierophinney Apr 3, 2013

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney Apr 3, 2013

Member

@ralphschindler I've been doing a lot of thinking about whether or not these factories should be in the individual components, or in Zend\Mvc or Zend\ServiceManager.

It's not unlikely that a developer will want to use the factories outside of the MVC system. If we have the factories in Zend\Mvc, the developer then either had to add that component as a dependency, or write their own factories. This makes a great argument for having the factories in either the components or in Zend\ServiceManager. My inclination is to make them part of the components:

  • The factories serve as some documentation as to how to instantiate a component and/or classes in the component, particularly in how to wire up dependencies.
  • While the factories are defined by the ServiceManager component, they could potentially be consumed by another service locator and/or IoC system easily.
  • Because you do not need to use the factories, the dependency on Zend\ServiceManager is an optional one. Developers would only need to add it if they choose to use the factories.

As such, I plan to merge this to the develop branch for 2.2.0.

Member

weierophinney commented Apr 3, 2013

@ralphschindler I've been doing a lot of thinking about whether or not these factories should be in the individual components, or in Zend\Mvc or Zend\ServiceManager.

It's not unlikely that a developer will want to use the factories outside of the MVC system. If we have the factories in Zend\Mvc, the developer then either had to add that component as a dependency, or write their own factories. This makes a great argument for having the factories in either the components or in Zend\ServiceManager. My inclination is to make them part of the components:

  • The factories serve as some documentation as to how to instantiate a component and/or classes in the component, particularly in how to wire up dependencies.
  • While the factories are defined by the ServiceManager component, they could potentially be consumed by another service locator and/or IoC system easily.
  • Because you do not need to use the factories, the dependency on Zend\ServiceManager is an optional one. Developers would only need to add it if they choose to use the factories.

As such, I plan to merge this to the develop branch for 2.2.0.

weierophinney added a commit that referenced this pull request Apr 3, 2013

weierophinney added a commit that referenced this pull request Apr 3, 2013

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney Apr 3, 2013

Member

Merged.

Member

weierophinney commented Apr 3, 2013

Merged.

@chellaifajardo

This comment has been minimized.

Show comment Hide comment
@chellaifajardo

chellaifajardo May 16, 2013

Is there a sample code snippet on how to implement this update? Thanks.

Is there a sample code snippet on how to implement this update? Thanks.

@stephanoff

This comment has been minimized.

Show comment Hide comment
@stephanoff

stephanoff May 16, 2013

Contributor

@chellaifajardo


array(
            'db' => array(
                'adapters' => array(
                    'Zend\Db\Adapter\Master' => array(
                        'driver' => 'mysqli',
                    ),
                    'Zend\Db\Adapter\Slave' => array(
                        'driver' => 'mysqli',
                    ),
                ),
            ),

    'service_manager' => array(
        'abstract_factories' => array(
            'Zend\Db\Adapter\AdapterAbstractServiceFactory',
        ),
    ),
)
Contributor

stephanoff commented May 16, 2013

@chellaifajardo


array(
            'db' => array(
                'adapters' => array(
                    'Zend\Db\Adapter\Master' => array(
                        'driver' => 'mysqli',
                    ),
                    'Zend\Db\Adapter\Slave' => array(
                        'driver' => 'mysqli',
                    ),
                ),
            ),

    'service_manager' => array(
        'abstract_factories' => array(
            'Zend\Db\Adapter\AdapterAbstractServiceFactory',
        ),
    ),
)
@stephanoff

This comment has been minimized.

Show comment Hide comment
@stephanoff

stephanoff May 16, 2013

Contributor

Hi Chellai,

You are right, 'ims_db' should be adapter name, this allow access to
adapter through serviceLocator->get('ims_db'') call.

Best regards,
Andrew Stephanoff

On Thu, May 16, 2013 at 11:24 AM, Chellai Fajardo
notifications@github.comwrote:

Hi Andrew,

I'm not sure if I implemented this correctly cause I'm getting an error
'could not find driver'.

I have this code on global.php

array( 'adapters' => array( 'ims_db' => array( 'driver' => 'Pdo', 'driver_options' => array( PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES \'UTF8\'' ), ), 'products_db' => array( 'driver' => 'Pdo', 'driver_options' => array( PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES \'UTF8\'' ), ), ), ), 'service_manager' => array( 'abstract_factories' => array( 'Zend\Db\Adapter\AdapterAbstractServiceFactory', ), ), ); Is it right that ims_db will be the adapter name? — Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/3933#issuecomment-17987902 .
Contributor

stephanoff commented May 16, 2013

Hi Chellai,

You are right, 'ims_db' should be adapter name, this allow access to
adapter through serviceLocator->get('ims_db'') call.

Best regards,
Andrew Stephanoff

On Thu, May 16, 2013 at 11:24 AM, Chellai Fajardo
notifications@github.comwrote:

Hi Andrew,

I'm not sure if I implemented this correctly cause I'm getting an error
'could not find driver'.

I have this code on global.php

array( 'adapters' => array( 'ims_db' => array( 'driver' => 'Pdo', 'driver_options' => array( PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES \'UTF8\'' ), ), 'products_db' => array( 'driver' => 'Pdo', 'driver_options' => array( PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES \'UTF8\'' ), ), ), ), 'service_manager' => array( 'abstract_factories' => array( 'Zend\Db\Adapter\AdapterAbstractServiceFactory', ), ), ); Is it right that ims_db will be the adapter name? — Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/3933#issuecomment-17987902 .
@chellaifajardo

This comment has been minimized.

Show comment Hide comment
@chellaifajardo

chellaifajardo May 16, 2013

Hi Andrew,

I finally made it worked. Thanks to you. 👍

Hi Andrew,

I finally made it worked. Thanks to you. 👍

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