Skip to content
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

Selective distinct/combine merge strategy. #22

Merged
merged 7 commits into from
Aug 7, 2018

Conversation

greg-1-anderson
Copy link
Member

In Drush configuration files, it is possible to define additional paths where command files and alias file paths may be found. For example, the following config will make the Drush 9 configuration behavior similar to what Drush 8 does:

drush:
  paths:
    include:
      - '${env.home}/.drush/commands'
      - /usr/share/drush/commands
    alias-path:
      - '${env.home}/.drush/sites'
      - /etc/drush/sites

A problem arises, though, when there are multiple drush.yml files, each with their own include &/or alias-path configurations, then the current implementation will return the paths from only one of the configuration files. The paths in all of the other configuration files will be overwritten and ignored.

What is needed to fix this limitation is a merge strategy that unions together all of the paths from all of the drush.yml files. However, it would not necessarily be appropriate to union all of the arrays from all configuration files.

The preliminary idea in this PR is to provide an API to specify the exact path for config items where array values should be unioned rather than replaced.

Here is how it would be used in Drush \Drush\Config\ConfigLocator:

    public function addConfigPaths($contextName, $paths)
    {
        $loader = new YamlConfigLoader();
        // Make all of the config values parsed so far available in evaluations.
        $reference = $this->config()->export();
        $processor = new ConfigProcessor();
        $processor->selectMergeStrategy(['drush.paths.include', 'drush.paths.alias-path']);

I'm not sure what the right solution is here, though. If we go with a solution similar to this one, I think I'll switch from an array of strings to a class with an API.

@greg-1-anderson
Copy link
Member Author

I did consider whether it might be simpler, yet still acceptable to simply always merge rather than replace simple (non-associative) arrays. I'm not sure whether this might have unintended consequences, so it might not be something that could be considered to be a backwards compatible change if we did it that way.

@weitzman
Copy link
Member

I havent heard much demand for this. Is this another ISP led feature?

Regardless, it looks OK to me, as far as it goes.

@greg-1-anderson
Copy link
Member Author

I discovered it when setting up isp configuration for Drush. I don't need it for the isp configuration per-se, because isps should only need one drush.yml file. Most folks will have zero drush.yml files in an isp context, and usually only one locally. If anyone ever added a commandfile or alias file location in a site-local drush.yml file, though, this would be a huge wtf, so I thought I should fix it while I understood the code path rather than waiting to do it all again later.

@greg-1-anderson greg-1-anderson changed the title Preliminary implementation of a selective distinct/combine merge strategy. Selective distinct/combine merge strategy. Aug 7, 2018
@greg-1-anderson greg-1-anderson merged commit c9fc25e into master Aug 7, 2018
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.

2 participants