enhancement Success

User tests: Successful: Unsuccessful:

avatar dongilbert
dongilbert
22 Oct 2013

This PR simplifies the Service Providers in the CliApp.

avatar dongilbert dongilbert - open - 22 Oct 2013
avatar dongilbert dongilbert - open - 22 Oct 2013
avatar dongilbert
dongilbert - comment - 22 Oct 2013

BTW, I made these changes and then did the full application install, so they're tested. :+1:

avatar elkuku
elkuku - comment - 22 Oct 2013

This looks very interesting ;)
I would assume that the "site" service providers can also be improved this way ?

avatar dongilbert
dongilbert - comment - 22 Oct 2013

@elkuku, they can be, sure. I just wanted to put this out there and get some feedback first. I'll work on the site service providers as well.

avatar dongilbert
dongilbert - comment - 22 Oct 2013

So, with the ConfigurationProvider, I was going to standardize on a way to implement it for the Framework. Essentially, the idea would be to pass in the path to the config, or an array of paths, to load in. The constructor would just save the paths into a variable, and then it would be used within the anonymous function to recursively load in the different config files that were specified.

$env = getenv('JTRACKER_ENVIRONMENT');
$configFiles = array(
    __DIR__ . '/etc/config.json',
    __DIR__ . '/etc/config.' . $env . '.json'
);
$container->registerServiceProvider(new ConfigurationProvider($configFiles));
avatar dongilbert
dongilbert - comment - 22 Oct 2013

This would allow for a more flexible config provider that we could actually have as a package for the Framework (although, I don't think it should be part of the core necessarily.)

avatar elkuku
elkuku - comment - 22 Oct 2013

I think this makes more sense then passing a configuration from the application object, as we are doing now. But we would then extend this base configuration provider with a "JTracker configuration provider" which is used in both CLI and site application and that does the job of looking for an env var ?
Or do I misunderstand the concept here ¿

avatar dongilbert
dongilbert - comment - 22 Oct 2013

I'm thinking something like this, just built around the Joomla\DI\Container instead: https://github.com/igorw/ConfigServiceProvider

avatar mbabker
mbabker - comment - 22 Oct 2013

The way the code reads to me, in whatever is calling the provider, we would supply it an array of paths to search (in order I'm assuming?) for the config. So in the app or wherever we make the call, we could do what's being done here and pass that path in or extend the generic provider to do that for us. For 3 real lines of code, we wouldn't need an extended provider.

avatar dongilbert
dongilbert - comment - 22 Oct 2013

Exactly, you wouldn't need to extend the config service provider, just put some code outside of it that looks at environment vars.

avatar elkuku
elkuku - comment - 23 Oct 2013

Well I think a configuration service provider that accepts multiple config files in different formats would be IT

I would love to have one common config provider for both of our tracker application, even if it is only looking for an env var. I think every line of code (aka responsibility) we can move out of our application(s) is good ;)

I am getting notices about JDEBUG not defined (on site). Seems like it is defined "too late". Moving it to the constructor works but has other side effects. - we'll sort that out later..

Actually this also violates the current coding standards (as already discussed and one of the most unimportant and annoying points...)
What if instead of

$container->set('JTracker\\Application', function () use ($app) {
    return $app;
}, true, true);

we write

$container->set('JTracker\\Application',
    function () use ($app)
    {
        return $app;
    }, true, true
);

The objective point here would be, that it "aligns" more with the overall style following a rule of "every curly on its own line"
A subjective view would be that it is "easier" to read...

At the end - all of the above mentioned concerns can be sorted out later. Now I would like to drive our toy with the improved wheels :)

avatar elkuku elkuku - reference | c5b5587 - 23 Oct 13
avatar elkuku elkuku - merge - 23 Oct 2013
avatar elkuku elkuku - close - 23 Oct 2013
avatar - close - 23 Oct 2013
avatar elkuku
elkuku - comment - 23 Oct 2013

Oh, and thanks a bunch @dongilbert :wink:

avatar dongilbert dongilbert - head_ref_deleted - 23 Oct 2013
avatar piotr-cz
piotr-cz - comment - 5 Nov 2013

Why having ApplicationProvider at all, not just

$container->set('app', $this, true, true);
define('JDEBUG', $this->get('debug.system'));

in the application constructor?

Add a Comment

Login with GitHub to post a comment