[#3330] Fix logic leading to creation of PGPASSFILE when connecting t…

[nbertram]

As discussed in #3330 Drush 9 regressed the use of a PGPASSFILE environment variable to connect to PostgreSQL databases that require a password. This failure is quite crucial in Drush 9 because of the use of the 'psql' tool to perform more operations directly, so even things like drush status will fall into a password prompt with the current code.

The issue here is reversed logic in the SqlPgsql::createPasswordFile() method:

if (null !== ($this->getPasswordFile()) && isset($dbSpec['password'])) {

This will only create the password file if it is not null, in other words if it's already existing. See below for why getPasswordFile() always returns null regardless, making this code block unreachable in any scenario.

If you contrast this line to Drush 8's (working) version:

if (!isset($password_file) && isset($this->db_spec['password'])) {

you can see that it's meant to be the other way around, creating the password file if it's not already set.

This Drush 8 line entered Drush 9 in b23f454 with a slight refactor:

if (!isset($this->getPasswordFile()) && isset($dbSpec['password'])) {

which was corrected to avoid isset() on a method return by 2d98048:

if (null !== ($this->getPasswordFile()) && isset($dbSpec['password'])) {

which accidentally reversed the logic of the first part of the statement, leading to the current code. Looking at the Travis config, it looks like pgsql tests with passwords are not performed, so that regression slipped through.

In addition to this logic issue, it appears getPasswordFile() will never return anything except null, because nothing sets the password_file member variable after writing the file. This was also the case in Drush 8, where the private variable $this->password_file actually has no references whatsoever. All code to get the name of the password file in both Drush 8 and Drush 9 actually calls createPasswordFile(), which returns the name. This means a new password file is created for every command, rather than reusing the existing one, because it can never detect that it has actually already created one. I suspect this came about because further back in Drush's history the local variable $password_file was possibly static.

I've fixed these two issues, and a third one where Drush reports that psql is not available because drush_program_exists() can't handle the prepended environment variable that command() adds:

[warning] The command 'PGPASSFILE=/tmp/drush_42DoXS psql -q' is required for preflight but cannot be found. Please install it and retry.

My approach there was to use a regex to remove prepended environment variables before running 'command' on $program. Given it appears the pgsql class is the only one in Drush core actually prepending environment variables in this manner, a less generic approach might be warranted to fix this. It might even be better to extend the way the database commands are executed to support passing environment variables through in the environment rather than the shell line, but I'll leave that call to more seasoned Drush contributors.

This PR might also supersede #2073, as I inadvertently fixed the same issue for pg_dump in my commit, coming to the same solution as that PR.

[darrenoh]

I have reviewed and tested the changes. This patch fixes the issue.

[weitzman]

Thanks everyone. Hopefully @tstoeckler gets a chance to review this soon.

[weitzman]

I went ahead and merged this as its causing some pain and can be changed later. Thanks all for the help. We are short on postgres maintainers at this time.

[dhruveonmars]

I just installed drush 9.3.0 and when I run drush status from the root of my Drupal installation, I get the same error: [warning] The command 'PGPASSFILE=/tmp/drush_42DoXS psql -q' is required for preflight but cannot be found. Please install it and retry..
After that, it still gives me the relevant information.

I'm on a Mac, using pgsql.

What can I do to fix this? Or should I use a different version of drush?

[nbertram]

Hi @dhruveonmars,

I've checked and Drush 9.2.2+ should have my patch included. I've tested Drush 9.3.0 locally and can't reproduce this issue.

Could you double check you're running Drush 9.3.0 by running drush version please? It can be a bit complicated with vendor installed Drush and a global Drush conflicting sometimes, though with Drush 9 this isn't likely to be the issue...

[dhruveonmars]

Hey @nbertram yeah I ran that and it says 9.3.0

I'll create a new Drupal installation and add drush to see if it's a problem I'm having globally or specific to this project.

I guess it shouldn't make too much of a difference what version of Drupal I'm using, but in case it does, I'm using 8.5.3

[dhruveonmars]

Hey @nbertram I created a new Drupal installation and installed drush. Before running Drupal, I ran drush status and it was fine, but after installing Drupal with a pgsql database, I ran drush status and I still got the same warning as before

[nbertram]

@dhruveonmars Could you verify that you do in fact have the psql command available in the shell you're running Drush from?

Reading the code in DrupalBoot.php, it looks like it's still going to report the command with the environment variable on it when it can't find psql. My patch only made it find psql correctly by removing the variable, but the error when it's not available is likely to still be the one with the variable on it...

Something like psql -V should output something along the lines of psql (PostgreSQL) 9.5.12 if the command is there. If not, you might need to install a postgresql-client package or similar.

[dhruveonmars]

@nbertram thank you! It looks like I didn't have it available. I re-installed postgresql using brew, and now I don't get that warning when I run drush status

[nbertram]

Glad that helped!

Yeah Drush 9 started shelling out to psql instead of bootstrapping Drupal for some operations, so that's a new requirement compared to Drush 8.