User tests: Successful: Unsuccessful:
This PR simplifies the Service Providers in the CliApp.
This looks very interesting ;)
I would assume that the "site" service providers can also be improved this way ?
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));
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.)
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 ¿
I'm thinking something like this, just built around the Joomla\DI\Container
instead: https://github.com/igorw/ConfigServiceProvider
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.
Exactly, you wouldn't need to extend the config service provider, just put some code outside of it that looks at environment vars.
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 :)
Oh, and thanks a bunch @dongilbert
Why having ApplicationProvider
at all, not just
$container->set('app', $this, true, true);
define('JDEBUG', $this->get('debug.system'));
in the application constructor?
BTW, I made these changes and then did the full application install, so they're tested.