Skip to content

Allow for a local configuration file outside of the admin/tool/mergeusers directory so it can be tracked by git #252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: MOODLE_401_STABLE
Choose a base branch
from

Conversation

tgmayfield
Copy link

Simple change for a configuration file right next to the mergeusers/ directory.

My project is all tracked in git, and customizations that we make to the merging also need to be included. Those can't be inside a submodule, so this allows for another local configuration file outside the submodule.

@jpahullo
Copy link
Owner

jpahullo commented Nov 29, 2023

Hi @tgmayfield ,

Thanks a lot for your contribution. It seems reasonable to provide different ways of loading/overriding default configuration.

However, It seems to me that the patch provided is concrete for your use case, instead of providing a generic way of providing a configurable path.

I can imagine right now of a possible way of providing some settings into the config.php into the $CFG object, like custom setting like $CFG->tool_mergeusers_alternative_config_path or by overriding plugin config into config.php so that a get_config('tool_mergeusers', 'alternative_config_path') would provide a generic path for that. Even though that alternative_config_path setting does not exist for the plugin in settings.php, I think it should work. Actually, I would try this second option in the first place.

I think that a change in that way, for a generic way of searching for an alternative path for a config file would help to include this change.

In addition, the README should be updated to explain this additional way of loading settings for the plugin.

Thanks again,

Jordi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants