Changed/unexpected location behavior

[Berdir]

Steps to reproduce

Put xy.site.yml files into a non-default folder, e.g. .drush/site-aliases, which is what https://github.com/platformsh/platformsh-cli defaults to.

Configure drush.yml like this:

drush:
    paths:
        alias-path:
            - '${env.home}/.drush/site-aliases'

Expected behavior

Site aliases are available as @xy.env

Actual behavior

Site aliases are available as @site-aliases.xy.env.

The directory name of the file is always considered the location ( site group) *unless it is either .?drush or .?sites.

The question to answer first, is whether not this is expected behaviour. IMHO not, if you configure a folder like /foo then you expect that to behave like default locations and only have a location/group if you put folders inside that.

There are two ways to fix this, the quickfix that adds more hardcoded assumptions would be to special case site-aliases as this is commonly used and was the default location in drush8. But it would only work for that folder name:

diff --git a/src/SiteAliasName.php b/src/SiteAliasName.php
index d8aa32e..407457e 100644
--- a/src/SiteAliasName.php
+++ b/src/SiteAliasName.php
@@ -85,7 +85,7 @@ class SiteAliasName
     public static function locationFromPath($path)
     {
         $location = ltrim(basename(dirname($path)), '.');
-        if (($location === 'sites') || ($location === 'drush')) {
+        if (($location === 'sites') || ($location === 'drush') || ($location === 'site-aliases')) {
             return '';
         }
         return $location;

Alternatively, we need to cut off the search path from the path before passing it to that method, as it is static and doesn't have access to that information. That's a bit complicated, needs a loop over the search paths and needs to be done in every place that calls SiteAliasName::locationFromPath() (or we change it to locationFromPath($path, $searchPaths)). Would look like this in one example:

diff --git a/src/SiteAliasFileLoader.php b/src/SiteAliasFileLoader.php
index 58325ad..3f26ef3 100644
--- a/src/SiteAliasFileLoader.php
+++ b/src/SiteAliasFileLoader.php
@@ -306,7 +306,17 @@ class SiteAliasFileLoader
     protected function loadSingleSiteAliasFileAtPath($path)
     {
         $sitename = $this->siteNameFromPath($path);
-        $location = SiteAliasName::locationFromPath($path);
+
+        // The location should only be relative to the configured search paths,
+        // pick the shortest of them as the most specific relative path.
+        $relativePath = $path;
+        foreach ($this->discovery()->searchLocations() as $searchLocation) {
+            if (strlen(str_replace($searchLocation, '', $path)) < strlen($relativePath)) {
+                $relativePath = str_replace($searchLocation, '', $path);
+            }
+        }
+
+        $location = SiteAliasName::locationFromPath($relativePath);
         if ($siteData = $this->loadSiteDataFromPath($path)) {
             return $this->createSiteAliassFromSiteData($sitename, $siteData, $location);
         }

Or, the last "solution" is to just document that your custom alias paths must end in drush or sites if you don't want them to become site locations. And tools like .plaform.sh will have to put drush9 aliases in a different place. Which is not a stupid idea FWIW as you wouldn't have a long list of duplicated drush8/drush9 alias files in the same folder (I have 200+ files in that folder, ~100 each)

[greg-1-anderson]

The unit tests load fixtures from arbitrary locations that are not named "drush" or "sites":

https://github.com/consolidation/site-alias/blob/master/tests/SiteAliasFileLoaderTest.php#L55

Given this, it is unclear to me what relationship the conditional in locationFromPath() has to discovery. Maybe this is actually a bug in Drush? Maybe this conditional is working around that bug?

[Berdir]

The default location for site aliases is .drush/sites, if you wouldn't exclude sites then that all aliases would be @sites.site.env

[greg-1-anderson]

Oh, right, for the group code. I thought this was just about discoverability. If the Site Alias library thinks the alias name is @sites.site.env, won't it also find it as @site.env?

[Berdir]

Ah, maybe that is the actual regression here, that this doesn't work anymore? Might be filtered out too eary.

[greg-1-anderson]

Yeah, maybe; in theory, group stripping should not be critical. If we did want it to happen for folders other than drush or sites, perhaps it could be configured rather than hardcoded.

Let's first see if we can figure out why the group-free alias is not found. The first question is whether or not there is one or more site with the same alias name. Ambiguity could be one reason why the group name is required.

[Berdir]

Hm, I think I just figured out the real regression here compare to Drupal 8 and maybe earlier versions. It's that it doesn't like . in the "site" part.

So with head, drush sa shows (confusingly) the aliases with @site-aliases.foo.master, but both drush @site-aliases.foo.master status and drush @foo.master status works.

That is, unless your site name contains a ".", e.g. a domain, which I have in a lot of my aliases. Then neither drush @site-aliases.foo.com.master status nor drush @foo.com.master status works.

They do fail in different ways, without the site-aliases group, it fails in preflight, when it doesn't find the alias:

[preflight] The alias @foo.foo.master could not be found.

With site-aliase prefix, it's even more confusing, because it doesn't even see this as an alias and fails with the error that this is not a command:

Command "@site-aliases.foo.com.master" is not defined.

The second one fails like that because apparently site alias validation is now more strict and it doesn't allow extra . in \Consolidation\SiteAlias\SiteAliasName::isAliasName? That said, doing a return TRUE there then afterwards fails with the same preflight error.

So I guess the question is whether or not . are allowed in the site part, and if not, then it should probably fail when aliases define these, or at least show a warning or something?

If that is supported, then I think the problem is then in \Consolidation\SiteAlias\SiteAliasName::parse(), which hardcodes that 3 . means the first is the location and then it doesn't match.

cc @pjcdawkins

[Berdir]

That said, I still think that the current handling of location in configured folders is pretty confusing and should ignore the configured root folder and not treat it as a location, but it could be argued that this would be a behavior change at this point.

[greg-1-anderson]

See Alias naming conventions:

Site alias names always begin with a @, and typically are divided in three parts: the alias file location (optional), the site name, and the environment name, each separated by a dot. None of these names may contain a dot.

This is a limitation of the site alias library, which simplified many things from Drush 8 aliases.

PRs to add better error handling would be appreciated. e.g. warnings or errors could be raised when alias files containing an extra dot in their name are found. If sa actually shows invalid names (with dots), that definitely should be fixed.