-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fixes #3277: Site-specific drush.yml file not loaded. #3278
Changes from 8 commits
b8e94bf
1ffc5df
ecfe555
062e4da
26c0ce8
7567410
6292e13
697296e
1aca731
837755f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
use Drush\Config\Loader\YamlConfigLoader; | ||
use Consolidation\Config\Loader\ConfigProcessor; | ||
use Consolidation\Config\Util\EnvConfig; | ||
use Drush\Utils\StringUtils; | ||
use Symfony\Component\Finder\Finder; | ||
|
||
/** | ||
|
@@ -37,7 +38,9 @@ class ConfigLocator | |
|
||
protected $sources = false; | ||
|
||
protected $siteRoots = []; | ||
protected $drupalRoots = []; | ||
|
||
protected $siteDirs = []; | ||
|
||
protected $composerRoot; | ||
|
||
|
@@ -101,7 +104,7 @@ public function __construct($envPrefix = '') | |
} | ||
$this->config->addPlaceholder(self::USER_CONTEXT); | ||
$this->config->addPlaceholder(self::DRUPAL_CONTEXT); | ||
$this->config->addPlaceholder(self::SITE_CONTEXT); // not implemented yet (multisite) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. n.b. "not implemented yet (mulitsite)" comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was removed intentionally, as this PR adds site support. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I made that note for @weitzman to point out that this PR is implementing a TODO. Then I forgot to save the comment that referenced it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, it did get saved; see above. |
||
$this->config->addPlaceholder(self::SITE_CONTEXT); | ||
$this->config->addPlaceholder(self::ALIAS_CONTEXT); | ||
$this->config->addPlaceholder(self::PREFLIGHT_CONTEXT); | ||
$this->config->addPlaceholder(self::ENVIRONMENT_CONTEXT); | ||
|
@@ -256,26 +259,64 @@ public function addDrushConfig($drushProjectDir) | |
* Add any configuration files found around the Drupal root of the | ||
* selected site. | ||
* | ||
* @param Path to the selected Drupal site | ||
* @param $drupalRoot | ||
* Path to the selected Drupal site. | ||
* @return $this | ||
*/ | ||
public function addSitewideConfig($siteRoot) | ||
public function addDrupalConfig($drupalRoot) | ||
{ | ||
// There might not be a site. | ||
if (!is_dir($siteRoot)) { | ||
if (!is_dir($drupalRoot)) { | ||
return; | ||
} | ||
|
||
// We might have already processed this root. | ||
$siteRoot = realpath($siteRoot); | ||
if (in_array($siteRoot, $this->siteRoots)) { | ||
$drupalRoot = realpath($drupalRoot); | ||
if (in_array($drupalRoot, $this->drupalRoots)) { | ||
return; | ||
} | ||
|
||
// Remember that we've seen this location. | ||
$this->siteRoots[] = $siteRoot; | ||
$this->drupalRoots[] = $drupalRoot; | ||
|
||
$this->addConfigPaths(self::DRUPAL_CONTEXT, [ dirname($drupalRoot) . '/drush', "$drupalRoot/drush", "$drupalRoot/sites/all/drush" ]); | ||
return $this; | ||
} | ||
|
||
/** | ||
* Add any configuration files found around the multisite directory. | ||
* | ||
* @param \Drush\SiteAlias\AliasRecord $alias | ||
* Site URI of the multisite. | ||
* | ||
* @return $this | ||
*/ | ||
public function addSiteConfig($alias) | ||
{ | ||
$uri = $alias->uri() ?: 'default'; | ||
|
||
// Parse a fqdn and look for matching entry in sites/sites.php. | ||
if (filter_var($uri, FILTER_VALIDATE_URL)) { | ||
if ($dir_name = StringUtils::lookupSiteDirFromUri($uri, $alias->root())) { | ||
$uri = $dir_name; | ||
} | ||
} | ||
|
||
// There might not be a site directory. | ||
$site_dir = $alias->root() ."/sites/$uri"; | ||
if (!is_dir($site_dir)) { | ||
return; | ||
} | ||
|
||
// We might have already processed this site. | ||
if (in_array($site_dir, $this->siteDirs)) { | ||
return; | ||
} | ||
|
||
// Remember that we've seen this site. | ||
$this->siteDirs[] = $site_dir; | ||
|
||
$this->addConfigPaths(self::DRUPAL_CONTEXT, [ dirname($siteRoot) . '/drush', "$siteRoot/drush", "$siteRoot/sites/all/drush" ]); | ||
$this->addConfigPaths(self::SITE_CONTEXT, [ "$site_dir", "$site_dir/drush" ]); | ||
return $this; | ||
} | ||
|
||
|
@@ -381,7 +422,7 @@ public function getSiteAliasPaths($paths, Environment $environment) | |
{ | ||
// In addition to the paths passed in to us (from --alias-paths | ||
// commandline options), add some site-local locations. | ||
$base_dirs = array_filter(array_merge($this->siteRoots, [$this->composerRoot])); | ||
$base_dirs = array_filter(array_merge($this->drupalRoots, [$this->composerRoot])); | ||
$site_local_paths = array_map( | ||
function ($item) { | ||
return "$item/drush/sites"; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -249,7 +249,7 @@ public function preflight($argv) | |
// Extend configuration and alias files to include files in | ||
// target site. | ||
$root = $this->findSelectedSite(); | ||
$this->configLocator->addSitewideConfig($root); | ||
$this->configLocator->addDrupalConfig($root); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Drush8, we don't load any site specific config until the Site phase of the bootstrap. I'd like to keep consistent with that in Drush9. Since this change is in Preflight (i.e. happens before bootstrap), I dont think its properly placed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that moving all config processing to preflight is an improvement. When config was php, delaying loading site-specific config until site bootstrap has the benefit of allowing folks to use Drupal APIs in their config code. With yaml config, I'm not sure that there is a benefit to postponing config loading. Loading config in preflight might allow folks to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is exactly my use case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we are working around the problem that config is static after its loaded. It would be preferable IMO if uri were discovered during SITE phase and then used for link building and such during command execution. We already do ROOT discovery during Preflight. This PR effectively moves SITE discovery there too. It begs the question of how much do we need the progressive Bootstrap that we do. Maybe NONE and FULL is all we need? We don't need to figure this out now but it would be good to get some clarity on future directions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that site selection is already happening in preflight. Note the comment removed in this PR: "// not implemented yet (multisite)" (see my comment #3278 (review)). I think this PR is essentially implementing a TODO. I still don't see any practical benefit to postponing uri discovery to the SITE phase, and as previously mentioned, there are some disadvantages to doing that. Regarding progressive bootstrap, for command discovery we only need NONE (built-in and global commands) and FULL (module commands). Built-in and global commands can also declare the bootstrap level they want, but it is not possible for them to declare their bootstrap level, because we already have to bootstrap to FULL in order to find them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the current design, the config locator is not available during bootstrap. Does it not currently work to set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dont know. If so, we just accept that as a regression with known workarounds (use an alias, use site-set, specify uri on the request, ...). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I agree. While I'd like to see this feature work, I think it's enough of an edge case that I'd like the solution to be clean (keep Drupal-specific code in the bootstrap phase). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have found one more workaround. If you set the environment variable So, how to set that env variable? If you have many sites on same host, or don't control the server, this can be challenging.
I will now writeup docs for #3009 as I now understand that system. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a follow-up, the drupal-project will soon support .env files and documents how to set DRUSH_OPTIONS_URI so that site specific works. See drupal-composer/drupal-project#351. You can implement that in your composer project even before this PR is merged. |
||
$this->configLocator->setComposerRoot($this->drupalFinder()->getComposerRoot()); | ||
|
||
// Look up the locations where alias files may be found. | ||
|
@@ -276,7 +276,9 @@ public function preflight($argv) | |
|
||
// If we did not redispatch, then add the site-wide config for the | ||
// new root (if the root did in fact change) and continue. | ||
$this->configLocator->addSitewideConfig($root); | ||
$this->configLocator->addDrupalConfig($root); | ||
// Set multisite config using uri from alias. | ||
$this->configLocator->addSiteConfig($selfAliasRecord); | ||
|
||
// Remember the paths to all the files we loaded, so that we can | ||
// report on it from Drush status or wherever else it may be needed. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we fall back to 'default' if the URI does not have a matching entry in sites.php?
If I have a single site in
sites/default
and I havedrush.yml
file that setsoptions.uri: http://example.com
, I expectdrush si
to install the site even if I don't have asites.php
file.