SQL Server Transactions / Package Database Config

[jcwatson11]

Override Connection::transaction(...) to support MSSQL Server transactions, since pdo_dblib doesn't support the native beginTransaction() set of calls. Their implementation is incomplete.

#534

[bencorlett]

You can't use facades in core classes I'm afraid. This will need some refactoring or to use the config injected by the service provider.

[bencorlett]

Also Laravel isn't using PSR2 fully so make sure you flow the coding standard very closely. Tabs, new lines before brackets, docblocks etc

[jcwatson11]

Hi Ben, okay. I'll change my editor settings when working on framework classes. Thanks for working with me on this.

So how could a core class get access to the expanded config set that is available to the Facade?

[bencorlett]

It's a bit more complex than what I can explain over a github comment from my phone. Look at "SOLID" and dependency injection pattern. You need to inject all app-specific config through the service provider. Look at how Taylor has done this.

Sent from my iPhone
Please excuse my brevity

On 12/03/2013, at 6:48 AM, Jon Watson [email protected] wrote:

In src/Illuminate/Database/DatabaseManager.php:

    {

•       return call_user_func($this->extensions[$name], $config);
  

•   }
  

•       $resolver = call_user_func($this->extensions[$packageName], $this->app, $this->factory);
  

•        return $resolver->connection($name);
  

•   } else {
  

•        // If there is no connection resolver, try to use the global Config object to see
  

•        // if a database config file exists in the app config.
  

•        if($config = Config::get($name)) {
  

•        } else {
  

•            // Otherwise use the default connection
  

•            $config = $this->getConfig($name);
  

  Hi Ben, okay. I'll change my editor settings when working on framework classes. Thanks for working with me on this.

So how could a core class get access to the expanded config set that is available to the Facade?

—
Reply to this email directly or view it on GitHub.

[jcwatson11]

Okay, I'll re-work my dependency injection. However, the first commit related to SQL Server Ticket #534 should probably be implemented in any case. That's unrelated to most of this discussion and would add transaction support to Linux implementations using FreeTDS and dblib.

Thanks for all your help, Ben!

[jcwatson11]

Okay, I think my code is good to go now. I replaced spaces and put tabs. Also made sure bracket placement is PSR2. Also removed reference to Config facade.

BTW: I also figured out a less than ideal way to inject my database connection into the global config. I think the framework should be modified so the following code in an Eloquent Model would work.

    protected $connection = "package::database.connection.name";

I would imagine that the DatabaseManager class should resolve names like the Config object does. But I'm not quite sure how to do that.

For now, I have modified my service provider like so:

        public function boot()
        {
                $this->package('fh/reporting');
                include __DIR__."/routes.php";
                // Add my database configurations to the default set of configurations
                $this->app['config']['database.connections'] = array_merge(
                    $this->app['config']['database.connections']
                   ,Config::get('reporting::database.connections')
                );
        }

Any thoughts on this? Good, bad, ugly?

Is this code merge-able into laravel 4? I'm happy to make any further changes.

[bencorlett]

Just add a new, empty line above here so there is a space between the code above it and the comment.

[bencorlett]

Looking good mate, now to wait and see what the powers-that-be say.

[taylorotwell]

Done.

[jcwatson11]

Thanks! Much appreciated!

Is there any chance the DatabaseManager might eventually support resolving connections using the same config syntax that the Config Facade supports? For example:

package::database.connection.name

I tried coming up with a solution to this, but eventually just ended up just using my app to inject the db config by modifying the global config array stored in $app. But ideally, wouldn't the DatabaseManager just know how to resolve the connection to the package config using a string like the one above?

Since the config facade uses this syntax, it seems non-intuitive that the DatabaseManager doesn't support it.

Thanks again!

[taylorotwell]

What was your case for wanting to do resolve a connection from a
package? I'm curious why you would ever want to do that?

[jcwatson11]

Good question. As you know, one of the beautiful things about the Eloquent framework is that the entire framework does not assume a single database connection for all models in the business logic. One model can define a connection to database A while another model can define a connection to database Z, and so-on.

In the documentation, it is suggested that each model can name the connection using $this->connection = "name".

But now if I'm developing a package, I want to define my connection and various options name inside my package config, as default connection options. Of course these could be overridden by publishing the config and changing them in the application configuration directory, but if the default configuration works, then there's no need to publish if the DatabaseManager class supports resolving a named connection to a package.

Does that make sense?

... Not to mention it's far more convenient to store database configuration information in some kind of config namespace, which seems to intuitively fit in the package config naming convention that the Config facade uses.

As it is now, the Database Manager only loads database configurations from the global app/config/database.php file.

If new connections are required, it requires special knowledge of the structure of the $app container in order to inject it, or special knowledge of how to construct the call back and objects returned in the package extend() method.

This could be made far easier by simply adding support for the same kind of namespace that the Config facade uses inside Database Manager.

So then, for example, my package (called "mypackage") would have models in them that define connections like so:

class SomeModel extends Model {

    protected $connection = 'mypackage::database.connection.mydbname';

}

And my composer package would have a file called /src/config/database.php which defines an array just like the one in app/config/database.php for the connections in this package.

It should also be noted that usernames and passwords in our environment are stored as apache environment variables. So defining a default configuration for a database connection inside the composer package is not problematic for security, since it doesn't contain any usernames and passwords. But regardless, the developer can always publish the config and modify it.