Fixes #3277: Site-specific drush.yml file not loaded. #3278

grasmash · 2017-12-29T15:52:31Z

No description provided.

greg-1-anderson · 2017-12-29T16:05:11Z

uri is often https://example.com rather than a directory name. Not sure how many existing places in Drush already make the assumption that uri is the directory name, but some modules need uri to be set to an fqdn in order to work correctly. Drush allows Drupal to do the conversion from fqdn to directory name during bootstrapping, but we may need to do some level of transformation ourselves prior to that point to support features such as this.

I don't fully remember what Drush 8 does, but I think that sitealias.inc had some provision for this.

grasmash · 2017-12-29T16:28:44Z

https://github.com/drush-ops/drush/blob/8.x/includes/sitealias.inc#L1593

greg-1-anderson · 2017-12-29T16:55:45Z

Yeah, that's pretty minimal.

greg-1-anderson · 2017-12-29T17:20:52Z

src/Config/ConfigLocator.php

+        }
+
+        // We might have already processed this site.
+        if (in_array($uri, $this->siteUris)) {


Maybe we should cache the directory rather than the uri?

greg-1-anderson · 2017-12-29T17:22:29Z

src/Config/ConfigLocator.php

+        }
+
+        // There might not be a site directory.
+        $site_dir = "$drupalRoot/sites/$uri";


Maybe site dir lookup should be a method of the alias record class (bring along convertUriToHostname, lookupSiteDirFromHostname).

n.b. I'm not sure about this - considering it. Maybe factoring out into a utility class would be better. I don't think that I want the alias record code to know about Drupal site directory conventions. Ideally the site alias classes will be factored out into a separate consolidation project.

greg-1-anderson · 2017-12-29T17:26:00Z

src/Preflight/Preflight.php

-        $this->configLocator->addSitewideConfig($root);
+        $this->configLocator->addDrupalConfig($root);
+        // Set multisite config using uri from alias.
+        $this->configLocator->addSiteConfig($root, $selfAliasRecord);


Is it necessary to pass in $root here? i.e. will $root ever be different than the value held by the self alias record? If it is, wouldn't the value in the self alias be more correct?

grasmash · 2017-12-29T18:06:40Z

src/Config/ConfigLocator.php

+     * @return $drupalRoot
+     *   The directory associated with that hostname.
+     */
+    public function lookupSiteDirFromHostname($hostname, $drupalRoot) {


Not sure where this belongs. I can move to StringUtils, though it's not strictly doing string manipulation.

grasmash · 2017-12-29T20:54:08Z

src/Commands/core/SiteInstallCommands.php

@@ -276,7 +276,7 @@ public function pre(CommandData $commandData)
            // a fallback name when '--sites-subdir' is not specified, but
            // only if the uri and the folder name match, and only if
            // the sites directory has already been created.
-            $dir = $this->getSitesSubdirFromUri($root, $aliasRecord->get('uri'));
+            $dir = StringUtils::lookupSiteDirFromUri($aliasRecord->get('uri'), $root);


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 have drush.yml file that sets options.uri: http://example.com, I expect drush si to install the site even if I don't have a sites.php file.

weitzman · 2017-12-29T21:51:37Z

Drupal 8 requires fqdn for lookup. We have moved drush toward assuming that also. I think it is unwise to accept anything else.

grasmash · 2017-12-30T00:10:48Z

Yes, I agree. My previous comment referred to setting the site directory value to default, not the URI.

Here is what I am proposing. If someone passes a fully qualified domain name as the URI value and that value does not have a corresponding directory mapping in sites.php, drush will assume that the site directory that it refers to is “default”.

grasmash · 2017-12-30T00:52:11Z

Tbh, this detail is incidental. I’m fine with leaving that as is.

Any issues with the PR as it is?

weitzman · 2018-01-02T17:15:52Z

src/Preflight/Preflight.php

@@ -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);


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.

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 --uri=foo and set URI to https://foo.example.com in their drush.yml file, to give one example.

That is exactly my use case.

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.

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.

By the current design, the config locator is not available during bootstrap.

Does it not currently work to set $options['uri'] from a site-specific drushrc.php file in Drush 8 with Drupal 8?

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, ...).

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).

I have found one more workaround. If you set the environment variable DRUSH_OPTIONS_URI=http://example.com then your custom uri will be used. No need to fuss with Drush config or aliases. For Docker/Vagrant sites, this is an easy solution.

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.

Anyone used direnv?

I'm using PHPEnv successfully on Drupal projects but at this time there is no way to load custom code early enough in a Drush request for 'uri' to take effect. That happens at end of Runtime https://github.com/drush-ops/drush/blob/master/src/Runtime/Runtime.php#L88-L93

I will now writeup docs for #3009 as I now understand that system.

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.

greg-1-anderson · 2018-01-02T19:20:50Z

src/Config/ConfigLocator.php

@@ -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)


n.b. "not implemented yet (mulitsite)" comment.

This was removed intentionally, as this PR adds site support.

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.

Oh, it did get saved; see above.

weitzman · 2018-03-12T16:34:53Z

So, the recommended way to get site specific config files to load is set an environment variable See #3278 (comment) and follow its links. This is now documented at https://github.com/drush-ops/drush/blob/master/examples/example.drush.yml#L26-L35

Fixes drush-ops#3277: Site-specific drush.yml file not loaded.

b8e94bf

grasmash force-pushed the issue-3277-site-config branch from 25fad2c to b8e94bf Compare December 29, 2017 15:53

grasmash added 2 commits December 29, 2017 10:56

Check site dir exists.

1ffc5df

Removing todo comment.

ecfe555

grasmash added 2 commits December 29, 2017 12:11

Converting fqdn uri to site dir.

062e4da

Fixing test.

26c0ce8

greg-1-anderson reviewed Dec 29, 2017

View reviewed changes

Making changes as per review.

7567410

grasmash commented Dec 29, 2017

View reviewed changes

grasmash force-pushed the issue-3277-site-config branch 3 times, most recently from d8c7f65 to 6292e13 Compare December 29, 2017 19:56

grasmash added 2 commits December 29, 2017 14:56

Use URI parsing logic from DrupalKernel to support more complex URIs.

6292e13

Updating site install command to use URI method.

697296e

grasmash commented Dec 29, 2017

View reviewed changes

weitzman reviewed Jan 2, 2018

View reviewed changes

greg-1-anderson reviewed Jan 2, 2018

View reviewed changes

grasmash added 2 commits January 2, 2018 16:18

Merge branch 'master' into issue-3277-site-config

1aca731

Merge branch 'master' into issue-3277-site-config

837755f

grasmash mentioned this pull request Jan 25, 2018

Drush 9: The sql-sync doesn't use the sites.php to resolve the site's folder #3322

Closed

grasmash mentioned this pull request Feb 12, 2018

Site-specific drush.yml file not loaded #3277

Closed

weitzman closed this Mar 12, 2018

greg-1-anderson mentioned this pull request Mar 4, 2019

Multisite installs do not load drush.yml #3968

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #3277: Site-specific drush.yml file not loaded. #3278

Fixes #3277: Site-specific drush.yml file not loaded. #3278

grasmash commented Dec 29, 2017

greg-1-anderson commented Dec 29, 2017

grasmash commented Dec 29, 2017

greg-1-anderson commented Dec 29, 2017

greg-1-anderson Dec 29, 2017

greg-1-anderson Dec 29, 2017

greg-1-anderson Dec 29, 2017

greg-1-anderson Dec 29, 2017

grasmash Dec 29, 2017

grasmash Dec 29, 2017 •

edited

Loading

weitzman commented Dec 29, 2017 via email

grasmash commented Dec 30, 2017

grasmash commented Dec 30, 2017

weitzman Jan 2, 2018

greg-1-anderson Jan 2, 2018

grasmash Jan 2, 2018

weitzman Jan 2, 2018

greg-1-anderson Jan 2, 2018

greg-1-anderson Jan 4, 2018

weitzman Jan 4, 2018

greg-1-anderson Jan 4, 2018

weitzman Feb 8, 2018 •

edited

Loading

weitzman Mar 12, 2018 •

edited

Loading

greg-1-anderson Jan 2, 2018

grasmash Jan 2, 2018

greg-1-anderson Jan 2, 2018

greg-1-anderson Jan 2, 2018

weitzman commented Mar 12, 2018

Fixes #3277: Site-specific drush.yml file not loaded. #3278

Fixes #3277: Site-specific drush.yml file not loaded. #3278

Conversation

grasmash commented Dec 29, 2017

greg-1-anderson commented Dec 29, 2017

grasmash commented Dec 29, 2017

greg-1-anderson commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grasmash Dec 29, 2017 • edited Loading

Choose a reason for hiding this comment

weitzman commented Dec 29, 2017 via email

grasmash commented Dec 30, 2017

grasmash commented Dec 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzman Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

weitzman Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weitzman commented Mar 12, 2018

grasmash Dec 29, 2017 •

edited

Loading

weitzman Feb 8, 2018 •

edited

Loading

weitzman Mar 12, 2018 •

edited

Loading