dflydev / dflydev-doctrine-orm-service-provider

Doctrine ORM Service Provider
MIT License
209 stars 59 forks source link

Use an array of configuration for "orm.default_cache" value #51

Closed damiankloip closed 9 years ago

damiankloip commented 9 years ago

Having the default value as a string makes it trickier to merge other default values.

E.g. if using something like https://github.com/igorw/ConfigServiceProvider you maybe want to override the orm.default_cache option on production to use memcache by default. You need an array for this like:

[
  'driver' =>  'memcached',
  'host' => '127.0.0.1'
  'port' => '11211',
  'cache_namespace' => 'some_namespace',
]

You can't merge that by default as it's currently just a string value.

If we used an array of configuration instead, we could always merge:

[
  'driver' => 'array',
]
damiankloip commented 9 years ago

Rebased against master since the tests pass again.

simensen commented 9 years ago

@damiankloip Hm, I wonder if this would be considered a BC break. I don't imagine so, but I'm not really sure what this might do. Do you have any thoughts on this? Would you be able to see what happens if someone would try to use a string as a default in their existing configuration?

damiankloip commented 9 years ago

@simensen I can't see that this would break anything, as to overwrite the orm.default cache currently (if anyone is doing that) they would have to be completely overwriting that parameter, either string or array. Then there is this code in DoctrineOrmServiceProvider :

if (isset($options[$cacheNameKey]) && !is_array($options[$cacheNameKey])) {
    $options[$cacheNameKey] = array(
        'driver' => $options[$cacheNameKey],
    );
}

So it's normalized to the array version anyway.

I also tested this change with one of my own Silex apps, overwriting orm.default_cache back to a string, and this is also fine. For the reasons mentioned first off.

So I'm pretty sure we're all good on that front :)

damiankloip commented 9 years ago

This also works OK if you want to overwrite that option with the ConfigServiceProvider back to a string.

simensen commented 9 years ago

ok, thanks @damiankloip!