New TTY detection in 9.7.0 breaks sql-cli input via STDIN (fixed, needs test)

[danepowell]

Describe the bug
In versions of Drush prior to 9.7.0, you could provide arguments to sql-cli via STDIN. This is no longer possible in 9.7.0. Most notably, this breaks the ability to import db dumps via Drush (I'm not sure of any easier way than using sql-cli).

To Reproduce
Create a db dump, attempt to import it like so:
drush sql-cli < mysqlDump.sql

Expected behavior
SQL tables to be imported

Actual behavior
Silent failure, nothing imported.

Workaround
Retrieve the SQL connection information via Drush, then feed it to mysql directly in order to import data.

System Configuration

Q	A
Drush version?	9.7.0
Drupal version?	8.x
PHP version	7.1
OS?	Mac/Linux/Windows

Additional information
Regression due to #4033

[greg-1-anderson]

This keeps breaking. 😢

[danepowell]

Yeah. I'm certainly not oblivious to the fact that I'm the one who introduced this regression in #4033 😢

Getting this right is hard. I'm not sure where to go from here. For one thing, we should probably add a test to ensure sql-cli < db.sql doesn't break in the future, assuming you want to officially support that usage.

[greg-1-anderson]

Yeah I've been testing with echo 'SELECT 1;' | drush sql-cli. However I'm having problem because I can't use git bisect due to some commits in the tree where composer update does not resolve. Thanks for the link to the PR where the behavior changed.

[greg-1-anderson]

This appears to be yet another instance where Drush stopped using its own code and started using a method from Symfony on the theory that the upstream was better tested and more maintainable, but ended up breaking some use-cases.

Maybe we should bring back our TerminalUtils method, and fall back on Symfony when posix_isatty is not available.

[greg-1-anderson]

Seems that consolidation/site-process#43 fixes this.

[greg-1-anderson]

Tagged stable release consolidation/site-process 2.0.3. If you are using a site-local Drush, just run composer update consolidation/site-process to fix.

[greg-1-anderson]

A PR with a new test for sql-cli would be most welcome.

[danepowell]

Thanks a lot for the quick response. It is indeed fixed by consolidation/site-process#43

I'll try to prioritize a test for this in the next few days.

[danepowell]

@greg-1-anderson I was just starting to write a test for this, and noticed an architectural discrepancy you should decide on

I've always done drush sql-cli < dump.sql (this is what we broke and fixed in this issue). But if you look at drush help sql-cli, doing this kind of import isn't recommended usage (maybe it was in the past, or I just picked up a bad habit?)

If you look at drush help sql-connect instead, it recommends `drush sql-connect` < dump.sql. This seems to work just as well, and was never broken as a result of this issue. In general it seems more reliable, although coupled more tightly to the shell (Bash).

It seems like we should either:

1. Officially support drush sql-cli < dump.sql by writing the test as we've proposed, and add that example to sql-cli help.
2. Officially not support drush sql-cli < dump.sql, don't write this test, and maybe add help text to sql-cli help advising to use sql-connect instead.

I don't have a strong opinion, let me know what you think.

[greg-1-anderson]

I tend to think that drush sql:cli with input redirection should work. It does not make sense to me that input redirection should not work or should not be supported here.

The drush sql:connect variant has the advantage of running the import operation entirely outside of Drush. As you said, this way of doing an import is potentially more stable. However, this variant won't necessarily work for remote sites, so I think that drush sql:cli is still useful.

[danepowell]

Thanks for the guidance, fixed in #4100