@self alias is missing its path-aliases definitions [#2042565]

~/htd/alice$ dr sa @self
$aliases["self"] = array (
  'root' => '/Users/moshe.weitzman/htd/alice',
  'uri' => 'http://default',
  '#name' => 'self',
  '#id' => '@self',
);
~/htd/alice$ dr sa @alice
$aliases["demo.alice"] = array (
  '#name' => 'demo.alice',
  '#id' => '@alice',
  'path-aliases' => 
  array (
    '%dump-dir' => '/Users/moshe.weitzman/sqldump',
  ),
  'root' => '/Users/moshe.weitzman/htd/alice',
  '#file' => '/Users/moshe.weitzman/.drush/demo.aliases.drushrc.php',
);

Comment	File	Size	Author
#9	issue-2042565-find-attributes-for-self-alias-9.patch	13.04 KB	greg.1.anderson
#6	issue-2042565-find-attributes-for-self-alias-6.patch	11.29 KB	greg.1.anderson
#5	issue-2042565-find-attributes-for-self-alias.patch	5.55 KB	greg.1.anderson

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

greg.1.anderson CreditAttribution: greg.1.anderson commented 16 July 2013 at 01:35

The thing is, that in the first example, Drush is bootstrapping the Drupal site based on the cwd (and perhaps the --uri option), whereas in the second example, Drush is given the alias, and finds the site from there.

In order for Drush to "recover" information from the alias that defines the site, it would have to exhaustively search all defined aliases to find one whose 'root' and 'uri' parameters happen to match the root and uri of the bootstrapped site. What if multiple aliases match? Should we cache results? Searching could be slow if there are many aliases.

At one time, I had the intention of parsing drushrc.php files from the 'sites' directory, or perhaps even merging in a 'self' alias record from the 'sites' directory, or some other common location; however, this is not implemented now, and I don't remember if it was an unfinished or rejected patch, or something that was implemented for a while, and then removed.

How should we proceed here?

Comment #2

moshe weitzman CreditAttribution: moshe weitzman commented 16 July 2013 at 14:31

This came up for me when I noticed I got a 'Using temporary files' warning during sql-sync @self @bob but not when using sql-sync @alice @bob despite me being in the @alice site when I issued the command. So, using @self during sql-sync can be a bad idea.

It seems to me that we should either make @self more robust or get rid of @self. As for searching through alias files for a matching uri+root, thats somewhat like a rejected feature request from msonnabaum when he was building Drush Deploy. He wanted a way to deploy to a dynamic group of sites like 'dev' sites or other grouping that can't be represented with current alias lists. I'm still not sure we need a query API for aliases, but now we have another use case.

It certainly sounds like the easiest thing is to get rid of @self. What do folks think of that approach, at least for the short term.

Comment #3

greg.1.anderson CreditAttribution: greg.1.anderson commented 16 July 2013 at 15:58

This issue isn't really limited to sql-sync. Imagine that you have a command-specific option in @alice that adds structure-tables for sql dump. Then:

1. cd drupalroot/site/alice; drush sql-dump
2. cd drupalroot/site/alice; drush @self sql-dump
3. drush @alice sql-dump

In examples (1) and (2), the sql-dump options from the alias are ignored, so example (3) behaves differently. Perhaps this example is substantially different than the rsync / sql-sync examples, though, as the user has no need to use @self in this position, whereas it must be explicitly used when supplying a source or target parameter for rsync or sql-sync.

I don't want to entirely remove the concept of @self, because it is used in code (drush_invoke_process), and it doesn't seem like a good idea to change the way that invoke-to-self works. I suppose it would be possible to explicitly reject @self for commandline use, but maintain it for use in code, but this seems undesirable as well.

Perhaps it is best to fix it, then. I would recommend fixing (2) (but not (1)) in addition to the sql-sync / rsync instances. Here are the steps that would be required:

1. drush @alice foo will cache the name and file for @alice in a Drush cache whose key is the path to the site folder for the Drupal site.
2. Alias lookups for @self will find and use @alice if the cache has an entry for the site folder
3. If there is no entry in the cache found when @self is being looked up, then Drush will do an exhaustive search for an alias that identifies the appropriate site path. Remote aliases are ignored in the search.

This leaves just one more issue unsolved. It is possible that some folks might have @alice and @alice2 defined to point at the same site, where @alice2 has a different set of options than @alice. If the cache is set up, then @self will use whichever of @alice or @alice2 was used last. If the cache is empty, then we would pick an aribtrary @alice or @alice2 to satisfy the @self reference. I have never done anything like this, so perhaps this does not need to be addressed; however, it might be a good idea to put in at least some sort of selection criteria (e.g. alphabetical by alias name?) so that the same alias is picked every time.

This is probably not too hard, and probably not too slow with the cache in place. Worth doing?

Comment #4

moshe weitzman CreditAttribution: moshe weitzman commented 16 July 2013 at 16:22

That sounds like a solid solution. We should make sure that `drush cc drush` clears this cache. If we don't get to this for Drush6, I think thats fine. We can probably add it for 6.1 as a bug fix.

Comment #5

greg.1.anderson CreditAttribution: greg.1.anderson commented 17 July 2013 at 03:23

Status:

Active

» Needs review

File	Size
issue-2042565-find-attributes-for-self-alias.patch	5.55 KB

Here is a patch that implements #3. If an exhaustive search is done, the first match is returned; no attempts at consistency are made. Hopefully, there will only be one match in most instances.

Comment #6

greg.1.anderson CreditAttribution: greg.1.anderson commented 17 July 2013 at 15:52

File	Size
issue-2042565-find-attributes-for-self-alias-6.patch	11.29 KB

Here is an updated patch with tests and improved comments.

Comment #7

greg.1.anderson CreditAttribution: greg.1.anderson commented 17 July 2013 at 16:57

Status:

Needs review

» Needs work

This causes a failure in saCase::testSAList.

Comment #8

moshe weitzman CreditAttribution: moshe weitzman commented 17 July 2013 at 18:01

With #3 applied, I was seeing cache misses every time because we refuse to cache aliases that omit a uri. The code checks for isset($alias_record['uri']). I felt no need to put that in my alias because my uri is 'default'. Could we remove that check?

After adding a uri to my alias, I got sidelined by #2044243: --parent sent on backend calls and is then flagged as an unrecognized option. After working around that, I am seeing a problem where my dump-dir is not in the alias record and thus I still get the 'rsync with temporary files' warning. I have a feeling this is happenning because dump-dir not properly copied from its parent. I was executing dr sql-sync @self @bob -d -n. Here are my aliases from a file named demo.aliases.drushrc.php. @self maps to @alice in the list below.

$aliases['base'] = array(
  'path-aliases' => array(
     '%dump-dir' => '/Users/moshe.weitzman/sqldump',
 ),
);
$aliases['alice'] = array(
  'root' => '/Users/moshe.weitzman/htd/alice',
  'uri' => 'default',
  'parent' => '@demo.base',
);

$aliases['bob'] = array(
  'root' => '/Users/moshe.weitzman/htd/bob',
  'parent' => '@demo.base',
  'remote-host' => 'bob',
  'remote-user' => 'moshe.weitzman',
);

Comment #9

greg.1.anderson CreditAttribution: greg.1.anderson commented 17 July 2013 at 21:57

Status:

Needs work

» Needs review

File	Size
issue-2042565-find-attributes-for-self-alias-9.patch	13.04 KB

Not all of my aliases have 'parent' records -- the one I was using for testing did not. Introducing one made the errors described in #8 apparent. This patch fixes them. I also had to adjust completeTest.php; the introduction of alias path caching broke some tests, as the test cases were assuming that the only cache events that were happening were "-complete-" cache entries.