slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.94k stars 1.95k forks source link

Slim\Container isn't passing settings array to Pimple #1407

Closed guice closed 9 years ago

guice commented 9 years ago

We're unable to leverage full Pimple due to a key pass-thru missing in Slim\Container's constructor:

    /**
     * Create new container
     *
     * @param array $userSettings Associative array of application settings
     */
    public function __construct(array $userSettings = [])
    {
        parent::__construct();
        [...]
    } 

Slim\Container does not pass $userSettings to the parent (Pimple) class, resulting in a failed Pimple initialization. This doesn't work:

// config.php
return [
    // Slim configuration.
    'env' => 'production',
    // DI classes
    'logger' => PhpConsole\Handler::getInstance(),
];

// index.php
$app = new \Slim\App('config.php');
$app->logger->debug('test'); // Call fails, but is successful if I add $userSettings to parent::__construct().

Or should this be handled some other way within Slim? The point here is to use DI to pass in environment dependent classes, not hard code them into my index.php file.

silentworks commented 9 years ago

These settings are passed into the settings key of the container.

https://github.com/slimphp/Slim/blob/3.x/Slim/Container.php#L100

We are currently discussing this on irc #slimphp on freenode if you want to hop on.

akrabat commented 9 years ago

This code would never work as App doesn't load settings files.

The correct way to do it is:

$settings = require 'config.php';
$container = new \Slim\Container();
foreach ($settings as $key => $value) {
    $container[$key] = $value;
}
$app = new \Slim\App($container);
JoeBengalen commented 9 years ago

If its just settings (and not pimple services) you want from the config file you can do

# config.php
return [
  'key' => 'val'
];

#index.php

$app = new Slim\App(require 'config.php'); // note the require here that you forgot

But like akrabat said; services will have to be added to the container, not the app.

guice commented 9 years ago

services will have to be added to the container, not the app.

I noticed the app itself puts everything into a container: https://github.com/slimphp/Slim/blob/3.x/Slim/App.php#L79

There doesn't actually seem to be a separation of app and container (at least on the surface).

(oops, did forget the require -- I edited the actual code to remove paths)

akrabat commented 9 years ago

If we did want to change the behaviour, the PR looks like this: https://github.com/slimphp/Slim/compare/slimphp:3.x...akrabat:di-settings?expand=1

geggleto commented 9 years ago

+1 for passing array to pimple.

I want to have all the config in one file, which includes the TwigView

Example: https://gist.github.com/geggleto/81cd05c5239ed56d70e2

$this->settings['view'] is where the closure is...

silentworks commented 9 years ago

@akrabat can you open a PR with this so I can leave my comment on the code please. I am happy to make this change but have 1 reservation.

silentworks commented 9 years ago

@guice this is now working the way you expect it to, we have merged in a PR to allow what you are after.

guice commented 9 years ago

Awesome. Great news! Thanks @silentworks. :+1: