ppi / framework

The PPI Framework Engine
http://www.ppi.io
MIT License
154 stars 30 forks source link

New Config System #22

Closed dragoonis closed 12 years ago

dragoonis commented 12 years ago

Looking to re-use an existing component library if its simple and fast enough then we can cherry-pick from it.

Usually PPI doesn't need all the fancy features from configuration that zf/symfony ship with. In the past this has lead PPI to create a similar but very trimmed down and faster version.

This is no longer a project goal for PPI to make its own components completely so re-using existing code is important.

The wiki for this part of the system is here: https://github.com/ppi/framework/wiki/PPI-v2-Configuration

\ Please read the wiki document before commenting on this discussion

evert commented 12 years ago

I'm a big fan of configuration being an optional thing. So rather than having a yml or a php configuration array, it's equally easy to bootstrap everything through real code. Injecting everything myself.

Configuration may make this simpler, but if it's optional I feel it will be very flexible.

Just a thought :)

dragoonis commented 12 years ago

@evert and what about making parts of the application maintainable, here is an example configuration file for the current version of PPI App. I partially agree about config files being optional so there can easily be a bootstrap property $app->useConfig = false and the frameworks's boot process will not try to load any config stuff. By default however it is enabled as most applications require or benefit from it.

https://github.com/ppi/cfpManager/blob/master/App/Config/general.ini-dist

dragoonis commented 12 years ago

I just benchmarked:

Symfony\Component\Yaml\Parser vs Zend\Config\Yaml

On average: Symfony loaded in 0.01 seconds and Zend loaded in 0.20 seconds.

This is mainly due to the fact that zend converts the yaml into a zend\config object to access like $config->some->value whereas symfony returns the raw array.

dragoonis commented 12 years ago

In Symfony2,

It appears that you don't have access to your config file directly from your controller, you have to create a 'parameter' mapping for every single variable in what's called a "DI Extension".

Ocramius commented 12 years ago

@dragoonis: a very simple adapter that returns arrays is enough imo... So your config could just be an object implementing ArrayAccess and having a swappable, with a default to, for example, Symfony\Component\Yaml\Yaml (constructor with optional parameter implementing some interface?)

dragoonis commented 12 years ago

@Ocramius after talking lots with you on #ppi irc channel and such.

The current state of this is that each module has a ->getConfig() method.

In this method you can return whatever you want as long as it's an array. Currently i'm using symfony\yaml to parse the /config/config.yml file.

At this time there is no "global configuration" file.

As things evolve in PPI2 we'll see how modules interact with global configuration.

Ocramius commented 12 years ago

@dragoonis the ->getConfig() method is used (from the module point of view) to build a merged configuration. You can use the Yaml parser in it as long as an iterable object is returned.

What you shouldn't do there is changing how that method is used, but instead provide some additional method if you want to use that config. Just warning you :)

dragoonis commented 12 years ago

@Ocramius I haven't broke backwards compatability here.

The ->getConfig() method still returns an iterable array.

It just so happens that I don't merge the configs when I boot up Zend\Module from PPI\App. Nothing broken here, or changed.

Clarify?

Ocramius commented 12 years ago

@dragoonis the module system and basic concept is based on this config merging to build a DiC container. Not using this functionality causes modules to be something completely different. You should keep the merged config idea, strip the Zend\Mvc\Bootstrap and Zend\Mvc\Application concepts and plug in your logic instead of them in my opinion :) As said yesterday, probably every attempt to remove pieces of the logic removes very basic functionality from the concept of modules as the module manager is really simple and basic (any attempt to add "magic" has been stripped over these months of zf2 development). Trying to eagerly use ->getConfig() is in my opinion not a good idea, as you're going against what the module developer would expect his module to do.

@EvanDotPro eventually some advice about how to proceed could be needed here :)

dragoonis commented 12 years ago

@Ocramius I think I'm missing one of your points because having PPI modules use a ->getConfig() method to return that modules configuration, is a good thing since it's also "The ZF Way" ?

There's nothing stopping this module's config from being merged, I just don't do anything with it. Since i don't use Zend\Mvc.

Ocramius commented 12 years ago

@dragoonis actually, that config is used to build a DiC, which you should probably reuse :)

dragoonis commented 12 years ago

@evert @Ocramius We now merge the global app config with the config from all modules. It's available as a service named "config" and you can get it from the controller using getService("config") or getConfig().

Thanks for the input.

Ocramius commented 12 years ago

@dragoonis fyi you should ping @EvanDotPro these days: he's been playing around with benchmarks and you know quite a bit of that stuff...

dragoonis commented 12 years ago

@EvanDotPro ping? :-)

EvanDotPro commented 12 years ago

I can tell you that within ZF, we've pretty much quietly deprecated the use of Zend\Config -- it's a lot of useless overhead just to be able to recursively access properties with object -> notation. Though, the framework does not care what modules do within their getConfig() method, so they're free to still use the Zend\Config readers if they want -- any performance hit is then the module's responsibility. Internally in ZF2 we are 100% using plain PHP arrays, which keeps everything very fast -- there should be no performance concerns with config in ZF2, and even so, we allow you to turn on config caching which caches the entire merged config array, bypassing any merging or collecting of configs from modules.

I'm with @ocramius that you should stick with the config merging strategy -- not because it's the "ZF way", and it doesn't matter that you're not using Zend\Mvc. I suggest it because merging configs lends itself very well to the entire concept of modular applications. Keep in mind that config can be anything, not necessarily just MVC configuration. This approach allows for modules to make configuration and setting options trivially easy: for example, see the ZfcUser wiki.

The only small catch to the merging strategy is that neither array_merge_recursive() nor array_replace_recursive() have the desired merging behavior for the context of modules, which is why we had to create Zend\Stdlib\ArrayUtils::merge(). This function is a line-by-line port of the C code behind array_merge_recursive, with a slight behavior change with how numeric keys are handled. We've found that performance is also not a concern here at all. Even with enormously large configurations, and tons of modules, config merging barely even shows up when profiling.

Also, I agree with @evert that the concept of config should be optional... This is the approach we've taken in ZF2, as all config is ultimately ran through factories or similar which simply translate it to method calls on objects. However, in practice, you'll run into issues with flexibility in a modular environment when trying to do everything in code, without config. My advice is to just design your class API's with stand-alone use in mind. There are only a few exceptions to this in ZF2, and most of them are in Zend\Mvc\Service* which is really just about a default bootstrapping strategy (it can be completely swapped out for your own).

dragoonis commented 12 years ago

Thanks @EvanDotPro.

As per my previous message, we're only returning raw arrays from $module->getConfig().

Although, I do only do an array_merge($appConfig, $modulesConfig).

Do you feel array_merge_recursive() or your special ::merge() call is recommended?

Thanks.

Ocramius commented 12 years ago

@dragoonis the problem occurs with numeric keys. Int-keyed arrays should stack the values through different modules instead of overwriting them.

dragoonis commented 12 years ago

@Ocramius I think as a rule, module config should only be key => val based.

Although, does this fall within the numerical ruleset?

return array('categoryIDs' => aray(12,34,56,74,24,67));
Ocramius commented 12 years ago

@dragoonis this isn't always true. For example, when I work with Zend\Di, I often want an array of injections like

<?php 
return array(
    'config' => array(
        'di' => array(
            'instance' => array(
                'My\\EventManager' => array(
                    'injections' => array(
                        'My\First\Listener',
                        'My\Second\Listener',
                    ),
                ),
            ),
        ),
    ),
);

Edge case indeed, but your example is also valid :)

dragoonis commented 12 years ago

Okay.

I will switch to array_merge_recursive() for now, until someone wants to make a PR to use the zend ::merge() instead. The code is in PPI\App.

Thanks.

On Thu, Jul 12, 2012 at 11:40 AM, Marco Pivetta reply@reply.github.com wrote:

@dragoonis this isn't always true. For example, when I work with Zend\Di, I often want an array of injections like

<php
return array(
    'config' => array(
        'di' => array(
            'instance' => array(
                'My\\EventManager' => array(
                    'injections' => array(
                        'My\First\Listener',
                        'My\Second\Listener',
                    ),
                ),
            ),
        ),
    ),
);

Edge case indeed, but your example is also valid :)


Reply to this email directly or view it on GitHub: https://github.com/ppi/framework/issues/22#issuecomment-6931981

EvanDotPro commented 12 years ago

@dragoonis, Actually array_replace_recursive() is closer to what you want -- the only catch with it is that, as @Ocramius said, numeric keys are overidden, so if under the 'injections' key you have array('foo', 'bar') in one module and array('baz') in another, the 'baz' will override 'foo', as they're both integer key 0. What we've found in practice is that 99.99% of the time in scenarios like this, what the user wants is to append the array as if they were using $array[] = 'value';

A great example of this is adding paths to a path stack for a view renderer. For a while, we were having to tell users to put arbitrary array keys in their 'script_paths' array, simply to prevent the integer key replacements. This caused support issues as users did not understand what these keys were for or what they should put for their value, not understanding that they were there simply to prevent some behavior of array_replace_recursive.

You don't want to use array_merge_recursive because if you have 'foo' => 'bar' in one module and 'foo' => 'baz' in another module, the result will be 'foo' => aray('bar', 'baz'), which is not what users are expecting or desiring.

dragoonis commented 12 years ago

@EvanDotPro @Ocramius I can't directly use the utils class it's an abstract class, what's the recommended implementation approach, do I need to needlessly create a class with no methods, to extend this to get access to merge() ?

https://github.com/zendframework/zf2/blob/master/library/Zend/Stdlib/ArrayUtils.php#L23

Ocramius commented 12 years ago

@dragoonis it is abstract because you don't want to instantiate it :) The methods are static since it is an utility.

dragoonis commented 12 years ago

@Ocramius the class was abtract and the method was static.

I'm getting a silly error right now Fatal error: Undefined constant 'PPI\Zend\Stdlib\ArrayUtils'

my use statement is

use Zend\Stdlib\ArrayUtils;
Ocramius commented 12 years ago

@dragoonis that is perfectly fine... Can you point me to where you get that error?

dragoonis commented 12 years ago

@Ocramius i found the problem, i closed my 'Use' statement on the previous line to the 'ArrayUtils' load.

I guess PHP silently ignores random text "Zend\Stdlib\ArrayUtils" on a line with no use statement.

The merged config is now this

<?php
$mergedConfig         = ArrayUtils::merge(
    $this->_options['config'], 
    $defaultListener->getConfigListener()->getMergedConfig(false)
);

Now config is available as the service 'config'

<?php
$defaultServices = array(
    'request'       => $this->_request,
    'response'      => $this->_response,
    'session'       => $this->getSession(),
    'templating'    => $this->getTemplatingEngine(),
    'router'        => $router,
    'config'        => $mergedConfig
);

Thanks.

Ocramius commented 12 years ago

Nice :)