Inheritance of local dirs for configuration files

[ThoWagen]

For each local dir a localSystem.ini can be included. In the INI a parent dir can be configured. This dir will be added to the localConfigDirStack in the PathResolver. The defaultConfigSubdir can also be changed.

This allows inheritance of configurations that can be very useful when working with multiple instances of VuFind that have common configurations.

In the future the localSystem.ini could be extended so that e.g. all configs automatically add the parent dirs configs without adding the [Parent_Config] section to the specific INI file. This would provide a simple way to have development configs overwriting the production configs.

[demiankatz]

Thanks, @ThoWagen! This seems like a pretty straightforward and flexible solution to the problem. I still think an argument could be made for using module.config.php, but I don't think that's obviously a better solution than this one. I'd be interested in hearing some more opinions, though!

In the meantime, however, I've just left a bit of general feedback.

[ThoWagen]

@demiankatz I'm not sure about using module.config.php for this. As far as I can tell this would require to create own modules or overwrite VuFinds module.config.php. And one would have to create new modules for each new local directory or it would be way less flexible.

But maybe I don't understand what you want to do. Could you elaborate on how you would do this with the module.config.php?

[demiankatz]

@ThoWagen, this keeps getting better! Just a few more questions and suggestions.

Regarding module.config.php, I am increasingly convinced that your approach here is better than anything we could achieve there, but just for the sake of completing the conversation: my original thinking was that most custom VuFind instances have at least one custom module, and because of the way Laminas module configuration merging works, if we turned the directory stack into an array in module.config.php, then it would be possible for any module to add one or more paths to the search stack. You definitely wouldn't need one Laminas module per path -- a single module could specify the whole stack if desired. This has the advantage of allowing very flexible configuration without requiring the parsing of additional configuration files. However, it has what I increasingly believe is a much bigger disadvantage: it's difficult to control the order effectively, since the module merging is always going to merge arrays together in the same way, so it's hard to add new paths to the "middle" of the stack between the home directory and the local directory. We would probably have to use some kind of nested array format with priority values and then sort the end result, and that's less elegant, less understandable, and less efficient than what you're doing here with .ini files.

I'm still open to other input and ideas, but I think if we can reach some consensus on this approach, it could reasonably be included in release 9.1, since it doesn't break backward compatibility in any way. I've put the 9.1 milestone on this, and I'll make sure we discuss it at the next Community Call to solicit some additional reviews.

I'd be interested to hear from @EreMaijala about this as well, but he's on holiday until August, so if you're eager to get this done sooner rather than later, we may have to proceed without him. :-)

[ThoWagen]

@demiankatz Thank you for the explanation. I understand your idea better now. But I'd prefer the current solution when I think about our setup.

To avoid parsing multiple INI files each request, maybe one could cache the localConfigDirStack?

I can't participate in the next Community Call but I think you can discuss this without me.

And when it comes to the question when the PRs should be included, there is no time pressure from our side. We will update our version at release 10 at the earliest.

[demiankatz]

Thanks, @ThoWagen! I've replied to a couple of your comments above.

I'll let you know the outcome of the Community Call conversation, but I strongly suspect we'll all reach similar conclusions about this.

I agree that it may be possible to cache the localConfigDirStack, though it's probably best to wait and see if there's a performance need before bothering to spend time on increasing code complexity. I think odds are good that in most situations, it won't make a significant difference.

[demiankatz]

Thanks again, @ThoWagen, this is looking good to me. I'll try to find some time for hands-on testing next week, and then I'll let you know the out come of Community Call discussion early next month.

[demiankatz]

@ThoWagen, I did some hands-on testing today and noticed a couple of problems. I fixed one (a simple documentation omission) but would be interested in your thoughts on the others.

[demiankatz]

One other thing to contemplate, @ThoWagen: we're currently set up so that .gitignore prevents local configurations from being committed, but this setting does not apply to the newly-added config file. Should we perhaps consider renaming local/DirLocations.ini to local/DirLocations.ini.dist and adding local/DirLocations.ini to the .gitignore, so that an example config is provided, but if it's locally applied, it doesn't get committed? That might be more consistent with the rest of our approach, but I'm open to other opinions.

[ThoWagen]

One other thing to contemplate, @ThoWagen: we're currently set up so that .gitignore prevents local configurations from being committed, but this setting does not apply to the newly-added config file. Should we perhaps consider renaming local/DirLocations.ini to local/DirLocations.ini.dist and adding local/DirLocations.ini to the .gitignore, so that an example config is provided, but if it's locally applied, it doesn't get committed? That might be more consistent with the rest of our approach, but I'm open to other opinions.

That makes sense. I have implemented the suggestion.

[demiankatz]

Thanks again, @ThoWagen! I tested this again and it seems to work as expected now. I'll leave this PR open until after next week's Community Call in case any further changes are suggested in discussion there.

[demiankatz]

It has been a week since our Community Call discussion, and no one has objected to this addition or made alternate suggestions, so I'm going ahead and merging it. Thanks again, @ThoWagen!