Configuration in Crayfish

[acoburn]

The three services in Crayfish are configured with config/cfg.php files. While this certainly works, I wonder if it wouldn't be better to use a Symfony interface with a defined set of configuration values (with sensible defaults), and then allow users to set their own configuration via a simple YAML file.

If that's of interest, I can implement that; otherwise, feel free to close this issue.

[DiegoPino]

Déjà vu @acoburn, its like time traveling. Crayfish had YAML file configuration loading a year and a few months ago. Same piece Jared added to your static ldp.

Islandora/Crayfish@6ef1b23

That/or similar is more microservice friendly than the whole Symfony implementation (which is what drupal 8 uses), one of the reasons Silex is based on and compatible with Symfony but simpler (and faster).

Ups! update, this piece god cropped. If you can re-implement it for sure claw committers will be happy. YAML configs make more sense since people will dealing with them on the drupal 8 side a lot.

[jonathangreen]

+1 for YAML config. I was thinking this should be a next step in Crayfish as well.

[dannylamb]

👍 sounds like a good improvement

[jonathangreen]

@acoburn do you mind if I try this one? I was thinking about making the config we do in IslandoraServiceProvider.php a bit more idiomatic by loading the config from the container itself. As part of that work I think it would be easy to add a YAML config.

[acoburn]

@jonathangreen that's fine with me

[whikloj]

@jonathangreen has completed some nice work around a Yaml config for Crayfish microservices, but the question I and @acoburn had raised was whether the Symfony ConfigurationInterface would not be a better choice for handling our configuration.

@jonathangreen described his position here on his PR.

I think my concerns/questions with regard to the choice difference between the YamlConfigServiceProvider and ConfigurationInterface has a couple parts to it. Some of them are personal preference and so not a statement about what is right. They are:

1. The ConfigurationInterface does some checking of the configuration, which allows us to catch simple errors off the hop.
2. It also allows us to set defaults, which means that without a config.yaml the system can still run in a default state.
3. I'm not sure that I like the configuration parameters all stored in the container. I know understand the naming strategy, but wonder if leaving them under a "config" entry, might not keep them cleanly separated from all the various services?
4. I realize the naming strategy is to allow for configuration re-use (or that is my take on it) but as each microservice is (currently) instantiated on its own, they will all require their own copies of the config.yaml. This makes the default (in 2 above) seem like a happy alternative.

[whikloj]

@acoburn could you look at allowing for any and all possible Doctrine DBAL configuration options. At the very least I am thinking about setting up some functional tests using a sqlite in-memory (non-persistent) database so I would need it to respect the memory: (boolean) parameter.

[acoburn]

@whikloj this is an excellent idea.

This does raise the question of default values -- should there be a default database configuration? If we want to support all of these configurations, I am leaning toward not having a complete set of default values. Thoughts?

[acoburn]

As per http://symfony.com/doc/current/best_practices/configuration.html, I plan to split out the db configuration into separate files (./config/parameters.yml) and put the microservice configuration in ./config/config.yml files.

[acoburn]

Also, (sorry @jonathangreen) but I will be removing a lot of the code that was merged recently related to configuration.

[jonathangreen]

No worries. Remove away.

I will say that I am of the opinion that IslandoraServiceProvider should load its configuration from the container, as other service providers do, and whatever you do for configuration should load the configuration into the container, where it can access it. So that 'IslandoraServiceProvider` is decoupled from configuration, but I may be in the minority in this opinion.

[acoburn]

@jonathangreen what I have in mind is that the IslandoraServiceProvider will be configured like this:

$app->register(new IslandoraServiceProvider(), ["crayfish" => $config]);

Where the $config is what gets loaded from the various configuration files + any defaults. That way, most of what currently exists in IslandoraServiceProvider will remain, but we can remove the whole Yaml configuration provider.

[whikloj]

@acoburn I'm not sure, perhaps we should consider using something like the environment configuration instead? Then we can set the environment different? Maybe that absolves us of having to account for any and all DBAL values?

[jonathangreen]

@acoburn that seems reasonable to me.

[whikloj]

As this is still open and with the deprecation of Silex, we will have to move to Symfony/Flex.

@jonathangreen and I had a conversation on IRC and think that when we do the Silex -> Flex migration we should have another look at the SymfonyConfigurationInterface for the following issue I ran into.

To allow for Yaml configuration like:

fedora_resource:
  base_url: http://localhost:8080/fcrepo/rest

being parsed into the $app as $app['crayfish.fedora_resource.base_url'] the YamlConfigServiceProvider does not allow first level associative arrays.

When trying to add a user-configurable list I ran into this.

For example

namespaces:
  acl: "http://www.w3.org/ns/auth/acl#"
  fedora: "http://fedora.info/definitions/v4/repository#"

are parsed as

crayfish.namespaces.acl = http://www.w3.org/ns/auth/acl#

and

crayfish.namespaces.fedora = http://fedora.info/definitions/v4/repository#

But I won't know the keys in the code, the work around is to embed the associative array in an array.

namespaces:
  -
    acl: "http://www.w3.org/ns/auth/acl#"
    fedora: "http://fedora.info/definitions/v4/repository#"

But we should review how much we are using this nested associative array functionality. It could also be removed to make configuration for Homarus more flexible.

[dannylamb]

@whikloj 👍 to that. I bumped into it a while ago and was confused.

[whikloj]

I think this is the same issue @Natkeeran ran into when doing configuration for Homarus.

[dannylamb]

Yeah, and I stuck my foot in my mouth when reviewing it ^_^