wa0x6e / Cake-Resque

Resque plugin for CakePHP : for creating background jobs that can be processed offline later
MIT License
159 stars 56 forks source link

Ease personalized plugin configuration. #28

Closed bar closed 11 years ago

bar commented 11 years ago

The concept is:

There are many ways to configure the plugin:

  1. This file is automagically loaded by the bootstrapping process, when no 'CakeResque' configuration key exists.
CakePlugin::load('CakeResque', array('bootstrap' => true));
  1. If a 'CakeResque' configuration key already exists, then no default configuration is done and the configuration file loaded must have all the necessary keys to work.
Configure::load('my_cakeresque_config');
CakePlugin::load('CakeResque', array('bootstrap' => true));
  1. The default configuration file, can also be loaded from the App's bootstrap file, and later modified (on demand) if needed. For this to work, the bootstrapping must be disabled when loading the plugin (before loading the configuration file), and later be done explicitly (after loading, and possibly modifying, the configuration file).
CakePlugin::load('CakeResque');
Configure::load('CakeResque.config');
// Edit configuration here
CakePlugin::load('CakeResque', array('bootstrap' => true));
  1. Another way to configure the plugin is to load it using a custom bootstrap file.
CakePlugin::load('CakeResque', array('bootstrap' => 'my_bootstrap'));
// APP/Plugin/CakeResque/Config/my_bootstrap.php
require_once dirname(__DIR__) . DS . 'Lib' . DS . 'CakeResque.php';
$config = array(); // Custom configuration
CakeResque::init($config);
coveralls commented 11 years ago

Coverage Status

Coverage decreased (-3.54%) when pulling c7795cc1401ecd7b2c49f7fd6f10811993111921 on bar:configuration into 08fd09e4219a0e190aad65e3ca2c118588445586 on kamisama:master.

bar commented 11 years ago

@kamisama Currently, I'm using the 3rd way to load my configs, because I want all the default ones, except for the hostname. And I don' t want to update the custom config every time I update (which could happen with the 2nd way).

This way you don' t need to exclude anything from git, and you don't alter the repo.

Another approach would be to just bootstrap requiring the CakeResque.php and place the CakeResque::init() call inside the App's bootstrap as an initialization process.

Any thoughts?¿

wa0x6e commented 11 years ago

Very interesting, I'll look into it as soon as I have some time

bar commented 11 years ago

@kamisama Thanks, I need another point of view on that :)

wa0x6e commented 11 years ago

From what I see, the difference between #.3 and #.4 is the way the path of the config file is loaded. Can you not achieve the same thing using #.4, but using a a relative path to your config file in bootstrap ? That way, the file can be located outside the plugin folder, and the submodule doesn't have to be modified in any way.

.2 is for when you already have an array holding all your config ?

bar commented 11 years ago

Right now, every time you Configure::load() in the main bootstrap.php, the configuration is later incorrectly merged (by default) when the plugin calls again Configure::load() so #2, #3 or #4 can't be done.

As a bonus, #4 allows a $config var to be passed and be used as default configuration, avoiding the Configure::load() call.

1 is just there so that nothing changes for those users which need no special config (maintaining BC).

I think I don't quite understand the question, but I'll try to elaborate more: #3 and #4 (without config file) are different:

4

4 needs a "clone" of the plugin's bootstrap file, and that file needs to be inside the plugin's domain, take a look at CakePlugin::bootstrap() which is called by CakePlugin::load(). It uses no config file (but a $config var) because it is pointless to have 2 files (one with a config file, and another with the bootstrap file) when a custom setup is needed, and it can be achieved with one file. But you should be able to keep it decoupled if needed:

CakePlugin::load('CakeResque', array('bootstrap' => 'my_bootstrap'));
// APP/Plugin/CakeResque/Config/my_bootstrap.php
require_once dirname(__DIR__) . DS . 'Lib' . DS . 'CakeResque.php';
Configure::load('my_cakeresque_config'); // For instance
CakeResque::init();

For that, #4 is no good when you want to keep the plugin clean and untouched (which was my goal in the first place), but the restriction of the custom bootstrap file living inside the plugin's domain is a deal breaker.

3

3 is better explained in my later comment. In a nutshell, it avoids touching the plugin, while giving the power to the user to maintain a custom config file, and makes updating the plugin a breeze :)

// Must be called to be able to make the next call, it needs to know the plugin.
CakePlugin::load('CakeResque');

// Loads the *default* config file
Configure::load('CakeResque.config');

// Edit configuration here as needed. Notice that the default configuration file is loaded first.

// Reload the plugin bootstrapping it so that when it detects that a configuration is already loaded, it won't reloadit.
CakePlugin::load('CakeResque', array('bootstrap' => true));
bar commented 11 years ago

2

2 Is the same as #3 but the difference is that it does not load the default config, but a custom one, which needs to have the minimum options set at least.

It is simple, easy and does not mess with the plugin (like #3), but... the user has to be more aware of the updates in the plugin's default configuration file so that it is up to date after updating.

bar commented 11 years ago

Please elaborate more in your original concern if I did not answer the question, maybe an example will help :)

wa0x6e commented 11 years ago

This is from CakePlugin::bootstrap() :

$bootstrap = (array)$config['bootstrap'];
foreach ($bootstrap as $file) {
    self::_includeFile(
        $path . 'Config' . DS . $file . '.php',
        $config['ignoreMissing']
    );
}

I'm not 100% sure, as I didn't test it yet, but can't $file be ../../Config/cake_resque.php ? That way, the config file lived outside the plugin, and case 3 === case 4

wa0x6e commented 11 years ago

For #2, you're calling Configure::load('my_cakeresque_config');

Again, I have not tested it yet, but bootstrap in CakePlugin::load() can takes a callable function. Maybe you can pass it directly ?

CakePlugin::load('CakeResque', array('bootstrap' => Configure::load('my_cakeresque_config')));

bar commented 11 years ago

Yes, You are 100% correct. it can be a relative path, the thing is $path is obtained from CakePlugin::path() which returns the FS path to the plugin :/

https://github.com/cakephp/cakephp/blob/master/lib/Cake/Core/CakePlugin.php#L145

wa0x6e commented 11 years ago

And using something like ../ in the relative path should take you everywhere you want

bar commented 11 years ago

I believe Cakeplugin::bootstrap() needs a callable not an array like the one Configure::load() returns.

Take a look at the examples in Cakeplugin::load() docs

wa0x6e commented 11 years ago

I confirm that using

CakePlugin::load('CakeResque', array('bootstrap' => '../../../Config/cake-resque'));

is loading the config file app/Config/cake-resque.php.

So I guess that there's not much difference between #3 and #4

bar commented 11 years ago

You are right! a bit weird because how it is built $path . 'Config' . DS . $file . '.php'.

But there is still difference, in that #4 needs to replicate the plugin's bootstrap, and load the config. #3 doesn't care of the bootstrapping process, it just loads the config ;)

wa0x6e commented 11 years ago

Yeah, to use #3 with all the default options, you still have to require the original bootstrap file in the head of the new bootstrap file. Or is there other difference ?

wa0x6e commented 11 years ago

That works for the #2 method :

CakePlugin::load('CakeResque', array('bootstrap' => function() { Configure::load('my_cakeresque_config'); }));

bar commented 11 years ago

Exactly, it would be the same. but the user should take into the account the require an some other magic which may be included in a future bootstrapping.

bar commented 11 years ago

Nicely pulled, I didn't wanted to end up calling call_user_func_array() to speed up every request, but it is very doable :)

You, would still need to require CakeResque and load it, though.

wa0x6e commented 11 years ago

So:

1: default

2: CakePlugin::load('CakeResque', array('bootstrap' => function() { Configure::load('my_cakeresque_config'); }));

3: CakePlugin::load('CakeResque', array('bootstrap' => '../../../Config/cake-resque')); with require

4: Same as #3

I guess that PR can be closed ? Or is there additional features I skipped ?

bar commented 11 years ago

1 is correct

2 I think it is missing the require and the CakeResque:::init() if you put it that way, cause you have to move the plugin's bootstrap process to the abstract function too, if you load the config using the bootstrapping callable.

3 is correct if you want the whole bootstrap inside the app domain, you still need the require and the CakeResque:::init() of course.

4 is correct too

If you think about it, the way you put it, the whole bootstrap process:

// Load configuration
Configure::load()

// Bootstrap
require
CakeResque:::init()

can be

2 inside an abstract function

3 inside the app realm

4 inside the plugin realm

The original #2 and #3 were meant to only "play" with the configuration part, without considering the bootstrap part, in a nutshell:

// Load configuration
Configure::load()

// Make the plugin bootstrap with that config
CakePlugin::load('CakeResque', array('bootstrap' => true));
bar commented 11 years ago

There is also a bit of documentation/hints here https://github.com/kamisama/Cake-Resque/pull/28/files#L1R43 that may need a review.

wa0x6e commented 11 years ago

Config documentation is pertinent. The rest of the PR, like the use of Configure to load the configuration, doesn't seems too useful.

bar commented 11 years ago

So you say it is better to keep the whole configuration inside the bootstrap file?

wa0x6e commented 11 years ago

Yes. It'll be much more easier to override when calling a custom bootstrap file in CakePlugin::load().

bar commented 11 years ago

You still can do it, but why bother a user that just need/want to configure the plugin, with all the bootstrapping process?¿

I think keeping the configuration inside a configuration file is cleaner/easier.

That's why CakePHP uses different files like app/Config/core.php and app/Config/bootstrap.php, separation of concerns. If would be sad if someone needs to copy lib/Cake/bootstrap.php entirely to change a configuration :/

wa0x6e commented 11 years ago

What I don't like with the separation approach is the need to call Configure::load('my_cakeresque_config'); before the CakePlugin::load().

Maybe you can do that:

CakePlugin::load('CakeResque', array('bootstrap' => ['../../../Config/cake-resque', 'bootstrap']));

That way, the user configuration is loaded first, then the bootstrap. Bootstrap will check if a config is already loaded, else will load the default config file.

wa0x6e commented 11 years ago

FYI, Configure::check doesn't seems to exists in Cake 2.2.

bar commented 11 years ago

Yes, with the PR implementation, it doesn't matter, you can do it both ways, because the change tests for a loaded configuration first. In my case, I feel it is more natural to configure something using a Configure call without the disadvantage of knowing bootstrapping internals.

You can pass that array to CakePlugin::load(), it will load both bootstrap files, one inside the App, and the other inside the plugin.

But... if I understand correctly, you still need to load the configuration inside the first bootstrap file.

bar commented 11 years ago

You are right, it can be easily ported from 2.3 and be placed inside CakeResque::loadConfig()

wa0x6e commented 11 years ago

I'm beginning to be confused. With that PR, can we load a custom config file by using only the bootstrap key of CakePlugin::load() ?

bar commented 11 years ago

Yes, if you don't want to call Configure::load(), before the CakePlugin::load(), you need to have a $config array with the config options, inside the bootstrap file you load, like the original #4.

CakePlugin::load('CakeResque', array('bootstrap' => '../../../my_bootstrap'));
// APP/Config/my_bootstrap.php
require_once dirname(__DIR__) . DS . 'Lib' . DS . 'CakeResque.php';
$config = array(); // Custom configuration
CakeResque::init($config);
wa0x6e commented 11 years ago

But with this PR, it can be done with

CakePlugin::load('CakeResque', array('bootstrap' => ['../../../Config/cake-resque', 'bootstrap'])); too, right ?

bar commented 11 years ago

Yes, but there is no point in calling CakePlugin::load() with an array because the second file becomes unnecessary.

If you just do CakePlugin::load('CakeResque', array('bootstrap' => '../../../Config/cake-resque')): it should be enough.

wa0x6e commented 11 years ago

But the cake-resque file contains only the config, and not the require, that's located in the bootstrap file, right ?

bar commented 11 years ago

Nope, you will always need to load a config at some point. The 'bootstrap' key just requires the file, it does not load configs. That's why you used Configure to do it before.

bar commented 11 years ago

It would be a yes, if cake-resque had a Configure call inside, then the other bootstrap can have the require and init. But I understand you don't want a Configure call.

wa0x6e commented 11 years ago

I'm fine with having Configure inside the config file. I just don't like something like that :

Configure::load('my_cakeresque_config');
CakePlugin::load('CakeResque', array('bootstrap' => true));
bar commented 11 years ago

I think something like this...

CakePlugin::load('CakeResque', array('bootstrap' => ['../../../Config/cake-resque', 'bootstrap']));

// APP/Config/cake-resque.php
Configure::load(´whatever_name_you_want_for_cakeresque');
// And... whatever change you want to apply here

// This should be the content of the plugin bootstrap
// APP/Plugin/CakeResque/Config/bootstrap.php
require_once dirname(__DIR__) . DS . 'Lib' . DS . 'CakeResque.php';
CakeResque::init();
wa0x6e commented 11 years ago

That's exactly what I'm thinking :+1:

bar commented 11 years ago

Nice! I think it will work, the only thing I don't like is managing two files :) In that case you should do this:

CakePlugin::load('CakeResque', array('bootstrap' => ['../../../Config/cake-resque', 'bootstrap']));

// APP/Config/cake-resque.php
Configure::write('CakeResque´, array(
  // config here
));

But if you need the default config before manipulating it... like in #3, you will need something like

CakePlugin::load('CakeResque', array('bootstrap' => ['../../../Config/cake-resque', 'bootstrap']));

// APP/Config/cake-resque.php
Configure::load('CakeResque.config');
Configure::write('CakeResque.foo´, ´bar');

What I don't know is if the Configure::load('CakeResque.config') will let you reach the plugin's config...

bar commented 11 years ago

I think it should, because CakePlugin::bootstrap() is done at the end of CakePlugin::load(), so self::$_plugins[´CakeResque´] will already be set, hopefully.

wa0x6e commented 11 years ago

Why would you need the default config before manipulating it ?

bar commented 11 years ago

So that I don't need to copy it whole every time I update the plugin. I just load the template (default conf) with the minimum confs, and then just change... 'Redis.localhost' for example.

wa0x6e commented 11 years ago

No, you don't need to, Configure can handle that, and merge your setting into the default one.

You can just do Configure::write('CakeResque.Redis.host', 'localhost'); in the cake-resque file, and Configure::load() can merge that into the default settings.

bar commented 11 years ago

If you write something to the configuration file, the default config won't be autoloaded later because is will be "checked", remember?

And if you call Configure::load() manually after, it will automerge using Hash::merge() which you don't want because the settings inside the config are not 1 level deep, they have arrays of arrays some times, `'CakeResque.Queues'`` for example.

wa0x6e commented 11 years ago

For the merge issue, a possible solution would be to remvoe the Configure in the default config, and leave it as a simple multi dimensional array.

If a custom config exists, load it in init() via Configure::read("CakeResque"), then merge the returned array with the array in the default config.

bar commented 11 years ago

Can you elaborate more on that idea? I don't quite understand what you meant by loading it in init() via Configure::read() and then merging the returned array with the default one.

You need to make a call to Configure at some point inside the default config/bootstrap, an array will not suffice. When there is no custom config, you still need to load a configuration because all the Configure::read() calls needed in CakeResque, which I think they are the correct way to go.

bar commented 11 years ago

I don't think it is a problem loading a template and then modifying it when you need to, it can all be done with 1 extra line, in the custom bootstrap for example.

You can pass an array to init() and it will be loaded but you have a problem every time you "merge", it doesn't matter if its in the App or in the Plugin, the problem is the merge, which is not controlled, neither an array_merge(), an array_merge_recursive() or even a Hash::merge(), because you don't have control.

As I said, if you merge a key with an array inside, you will not modify the content of the first one, but append a new array, or you will end up replacing the whole key, when you only need to replace one internal value from the key. It all depends on the "merge" function you use :(

wa0x6e commented 11 years ago

=> array_merge_recursive()

// CakeResque/Config/custom.php Configure::write('CakeResque.Redis.host', 'localhost');

Then you can merge that with the default config with

// Reading the custom config
$r = Configure::read('CakeResque');

require './default-config.php';
// $config variables holding all the default config is visible
// Merge it with the custom config
Configure::write(array_merge_recursive($config, $r);

I don't pass anything to init(), merging is done by bootstrap before init()

Boostraping role will be to collect the various config and merge them.