Allow altering of cache class even if site alias is not used.

[laurentiu1981]

Is there any way to force a specific drushrc.php config file for a drush command in versions of drush greater than 6.3?

Once you removed these lines: https://github.com/drush-ops/drush/blob/6.2.0/includes/sitealias.inc#L279-L282,
combined with this line in bootstrap.inc:
https://github.com/drush-ops/drush/blob/6.x/includes/bootstrap.inc#L505
right before the line that loads the config... you avoided any way in setting a default cache class for drush.

That's because at some point you are looking for cached aliases, so drush_cache_get is called and cache object is initialized, that's a static variable:
https://github.com/drush-ops/drush/blob/6.x/includes/cache.inc#L40-L51
Having no config loaded... default DrushJSONCache class is used to create cache object.

Previously a default config file was set in sitealias.inc (sites/default/drushrc.php) when you looked up for aliases, see again: https://github.com/drush-ops/drush/blob/6.2.0/includes/sitealias.inc#L279-L282

Now... even if I alter the default cache class in drushrc.php, by using

$options['cache-default-class'] = 'CustomCacheClass';

or

$options['cache-class-default'] = 'CustomCacheClass';

the default class will be DrushJSONCache initially.

Do you see any way to alter the cache class even if I don't use site aliases?

E.g. By running: drush sitefolder command everything is fine, but if I run only drush command from root folder of drupal, the cache class is not altered early enough.

[weitzman]

What is drush sitefolder command. Do you mean running a command from inside a /sites subdir?

Regardless, have you tried to put a folder called 'drush' under your Drupal docroot? drushrc files there should get read when you start the drush rewuest from Drupal root. The drushrc files should also get read when you start your command from anywhere and pass --root.

[laurentiu1981]

Sitefolder is a folder under sites, yes.
I've tried to create a drush folder in drupal root to put drushrc.php there... it's working as before.

If I'm running drush "command-example" from drupal root, the config will be loaded at some point, but my cache class will not be altered.
No matter where my config is situated, if no site alias is used, cache class is initialized when drush is trying to create an alias by itself.

If I'm running drush default "command-example" from drupal root, an alias will be generated without calling the cache object and my config will be correctly loaded before the cache object is needed.

Flow without alias -> drush "command-example":

1. Detect alias or create one on the fly, see: https://github.com/drush-ops/drush/blob/6.x/includes/bootstrap.inc#L505
2. In the process of creating the alias, process will load the cache object from drush options if available, if not... set default class in that static variable. see: https://github.com/drush-ops/drush/blob/6.x/includes/cache.inc#L40-L51
3. After the site alias is generated, it will try to load config: https://github.com/drush-ops/drush/blob/6.x/includes/bootstrap.inc#L507-L509. At this point the cache object is already initialized and code:
   $options['cache-default-class'] = 'CustomCacheClass';
   has no effect.

When you're trying to load cache again, it will pass directly the initialized object from static variable and will ignore my option from config.

Flow with sub-folder is a bit different -> drush default "command-example":

1. It will try to detect the alias, it will detect the folder and create the alias record using this code: https://github.com/drush-ops/drush/blob/6.2.0/includes/sitealias.inc#L258-L263. In this scenario cache object is not needed, so cache object will not be loaded before loading the custom config.
2. Load config.
3. When cache object will be requested for the first time, the drush option will be already set from my custom config and right class will be used.

[weitzman]

Thanks a lot for clarifying. Either myself or @greg-1-anderson will have a look and see how we can refactor.

[greg-1-anderson]

Where exactly is the cache initialized? Is it here:

https://github.com/drush-ops/drush/blob/6.x/includes/sitealias.inc#L54

Note at the beginning of drush_bootstrap_drush, we have this:

https://github.com/drush-ops/drush/blob/6.x/includes/bootstrap.inc#L431

If this line isn't a problem, then we could fix this by using a less expensive way to determine if '@self' exists, similar to how _drush_sitealias_cache_alias() works.

I did not take time to trace through the code and determine if this fixes the problem, and I am not sure of all of the ways we might get to https://github.com/drush-ops/drush/blob/6.x/includes/cache.inc#L40-L51. Even if we fixed this now, there would always be the possibility that future code changes might once again set up the cache too early. If this happens, we don't have too many good options.

[laurentiu1981]

Yep, the cache initialization starts there and the function that actually calls the cache is here:
https://github.com/drush-ops/drush/blob/6.x/includes/sitealias.inc#L2144
And we got there through this code:
https://github.com/drush-ops/drush/blob/6.x/includes/sitealias.inc#L184-194 see line 191.

Do we really need this line: https://github.com/drush-ops/drush/blob/6.x/includes/sitealias.inc#L191 ?
because $cached_alias_record comes empty anyway and it doesn't seem to be called when a site alias or folder is used. I don't think it has any use. But by using that line we'll get to the function that loads the cache, see above.

Basically... instead of:

    if (array_key_exists('#name', $alias_record)) {
      if ($alias_record['#name'] == 'self') {
        $path = drush_sitealias_local_site_path($alias_record);
        if ($path) {
          $cached_alias_record = drush_sitealias_lookup_alias_by_path($path);
          $alias_record = array_merge($alias_record, $cached_alias_record);
        }
      }
    }
    else {
      $alias_record['#name'] = drush_sitealias_uri_to_site_dir($alias);
    }

why not using directly:

if (!array_key_exists('#name', $alias_record)) {
   $alias_record['#name'] = drush_sitealias_uri_to_site_dir($alias);
}

?

I might not see some feature in that code, but strictly for my scenarios... the replaced code is doing nothing, except the early initialization of cache object, while trying to get info related to site alias from cache.
Also, even if that code would return some info... then I should be able to alter the cache class that is used.

I might suggest that we could use a default folder in drupal filesystem, e.g. sites/all/drush/global where we could put a drushrc.php that should be checked after these lines:
https://github.com/drush-ops/drush/blob/6.x/includes/bootstrap.inc#L483-L486. That's early enough I think.
That should work as a global config for drush at drupal project level. That should be able to override the settings from drushrc.php found at ~/.drushrc.php or ~/.drush directory.

Basically we would load the global drushrc.php only in this place. After that, we'll use only the locations we had before(/sites/default, sites/all/drush) to look for config files.

PS:
In earlier versions we had this line: https://github.com/drush-ops/drush/blob/6.2.0/includes/sitealias.inc#L279-L282 which kind of forced the load of a default config if something is not provided.

[greg-1-anderson]

No time to evaluate what you said above, but it looks sound to me. I think I agree that the existing code is too heavyweight, and can probably be successfully improved as you suggest. If I don't come back here in a couple of days, hit this issue again with another comment. Better yet, make a change and submit a PR if it works -- looks like you are on the right track.

[laurentiu1981]

It took some time...
I've decided to modify a bit the code to load 'drupal' and 'site' configs, if they exist, before creating default alias (the step that initialize the cache object with default one).

This looks a bit simpler compared to the other option of adding another config. Also, I think that each sub-site should have the option to alter cache class.

[greg-1-anderson]

The problem with altering the order as you have done is that the alias that the user selected on the command line might (probably will) alter which 'drupal' and 'site' config file is loaded.

Also, any bugfix or enhancement must happen on the master branch, prior to being backported to an older branch. Drush 6.x is not eligible for this sort of change, because it could change the behavior of existing scripts, breaking backwards compatibility. Any change made on a stable branch must be demonstrably backwards compatible.

[laurentiu1981]

I might miss something, but on master branch, 'drupal' and 'site' config files are loaded before the moment when 'drush_sitealias_create_self_alias()' is called, which is responsible of early initialization of cache object using default class, for 6.x branch.
We have the problem on 6.x branch, which might have code backported in a wrong way.

While looking on master branch, it seems that it is harder to fix, because cache object is initialized when _drush_find_commandfiles_drush() is called, before loading 'drupal' and 'site' config files.
https://github.com/drush-ops/drush/blob/master/includes/preflight.inc#L193

Considering that we are using cache object before loading site specific configs, I don't see a way to set a custom Class for caches per site or per project, unless I return to initial approach of adding a global config for all sites, somewhere in /sites/all/drush/global.

Are you fine with that?

[greg-1-anderson]

Sorry, I misread / misinterpreted your change; my comment above was therefore not correct.

Regarding the fix on the 6.x branch, delaying the creation of @self would only cause problems if this alias was referenced in the 'drupal' or 'site' configuration file. Calling drush_invoke_process('@self', ...); in such a situation would often result in an infinite loop, so it might be rare for anyone to need this reference. I am reluctant to make @self unavailable in this context, though, because it does still break our backwards-compatibility contract, and you are trying to do something new here. So, I think this change must be for the master branch only.

Speaking more generally about the strategy of the change, it is dangerous to "fix" this problem by simply changing the order that things are initialized during bootstrap. Certainly that will be part of the solution, but what we are missing here is a definitive way to initialize the cache object before it is lazy-initialized by some other action -- behavior that might change, depending on the other needs Drush has while bootstrapping. The relationship between the creation of the cache object and the configuration it depends on needs to be documented, and protected with unit tests that confirm the behavior, to guard against future changes breaking the necessary behavior.

I think that your idea of having an "early" site-local configuration object separate from the general 'drupal' and 'site' configuration files is on the right track. However, we also have to consider what the expected behavior is when this configuration file alters Drush's state in a way that is specific for this site prior to the point when Drush decides which site is being used. One example of this would be if Drush implicitly selected a site by searching from the cwd, but the user then specified a different site via an alias file that was defined in an alias file inside this site. If state is changed in a way that might break a different Drupal site, that would be bad. This situation is intricate enough that I wonder if we shouldn't just remove site-local aliases as a supported feature. Perhaps there is a better solution, e.g. converting alias files to yml, and appropriately ordering configuration and alias-file parsing to avoid this sort of scenario. This might make it possible to document the specific set of initialization functions that are not allowed to initialize the cache--something that would be hard to do in the current code, I think.

This issue is related to #1340. Whatever we do here, we should be consistent with how Drush selects the 'boot' class. This is another example of an object that needs to be initialized "early", but is tied strongly to the Drupal site that it is associated with.

[laurentiu1981]

Sadly, the only thing that we can do is to include the cache class option before any usage of cache object and I also need the DRUSH_SELECTED_DRUPAL_ROOT context initialized.

First function that use the cache is _drush_find_commandfiles_drush(), which is called before drush_preflight_root(), where we initialize the DRUSH_SELECTED_DRUPAL_ROOT and load drupal config.

What about calling drush_preflight_root() before _drush_find_commandfiles_drush()?
Then we would be able to alter cache class in the drupal config, without any other custom config.

[greg-1-anderson]

Maybe it would work to tie this functionality to the site-local Drush feature -- that is, if Drush is included in the composer.json of the Drupal site it is associated with (or just exists inside the Drupal root), then we identify the Drupal site early and lock it in. See #1340.