Add YAML Config Files #27
Conversation
Codecov Report
@@ Coverage Diff @@
## master #27 +/- ##
=========================================
Coverage 98.41% 98.41%
Complexity 35 35
=========================================
Files 3 3
Lines 126 126
=========================================
Hits 124 124
Misses 2 2Continue to review full report at Codecov.
|
jonathangreen
force-pushed
the
issue-603
branch
from
64237a0 to
c1d362e
Compare
|
@jonathangreen could you move |
|
@whikloj moved. |
Gemini/src/app.php
Outdated
|
|
||
| $app['gemini.controller'] = function () use ($app) { | ||
| $app->register(new IslandoraServiceProvider()); | ||
| $app->register(new YamlConfigServiceProvider('../cfg/config.yaml')); |
There was a problem hiding this comment.
I'm having trouble with this line, not sure if its because I have my code shared into the vagrant box, but I needed to make it
$app->register(new YamlConfigServiceProvider(__DIR__ . '/../cfg/config.yaml'));
There was a problem hiding this comment.
This is minor and I think I mentioned it to @dannylamb on a previous PR. But should we do a sanity check for the existence of this file and if it is not there, throw a simple clean message. Currently you just get an ugly stack trace.
There was a problem hiding this comment.
It is fairly hard to give a simple clean error with no logging bootstrapped at this point. Do you have any suggestions for how to accomplish what you would like?
Houdini/src/app.php
Outdated
|
|
||
| $app['houdini.controller'] = function ($app) use ($config) { | ||
| $app->register(new IslandoraServiceProvider()); | ||
| $app->register(new YamlConfigServiceProvider('../cfg/config.yaml')); |
There was a problem hiding this comment.
See above comment regarding needing __DIR__
Hypercube/src/app.php
Outdated
| $app = new Application(); | ||
| $app->register(new IslandoraServiceProvider($config)); | ||
| $app->register(new IslandoraServiceProvider()); | ||
| $app->register(new YamlConfigServiceProvider('../cfg/config.yaml')); |
There was a problem hiding this comment.
last time, see above comment regarding need for __DIR__
|
Updated the directory where we load the configuration from as suggested. |
|
I don't have any issues with the code here per se, but I am concerned about the design. My intention with the original issue was twofold: first, make the configuration YAML-based (rather than based on a PHP array) -- that is clearly solved here; and second, to use the existing Symfony facilities for handling configuration -- that is (from what I can tell) not the case. There are many advantages of using the Symfony infrastructure for this -- first, you can define a schema for the configuration, so structural errors are clearly caught, and secondly, you can define default values. This means that a user needs only to define the required elements, and the rest can default to sensible values. Why is that important? Imagine a user has Crayfish version 1.2.3 installed with a given configuration, but then updates to version 1.2.4. If that version introduces an optional configuration value, there would need to be no change to the user's config; but as I am reading the code here, it seems that, given this structure, the new version would throw an error. That is exactly what I was trying to avoid when I opened this issue. |
|
@acoburn there is more discussion on the other pull request. See my response there. |
|
@jonathangreen needs a rebase. |
GitHub Issue
Part of:
Islandora/documentation#603
Related to:
Islandora/Crayfish-Commons#8
What does this Pull Request do?
Updates all the micro services to use YAML configuration files, instead of the PHP ones we were using before.
How should this be tested?
Interested parties
@Islandora-CLAW/committers