drush-ops / drush

Drush is a command-line shell and scripting interface for Drupal, a veritable Swiss Army knife designed to make life easier for those who spend their working hours hacking away at the command prompt.
https://www.drush.org
2.34k stars 1.08k forks source link

Allow using a relative path for SQLite database path #6142

Closed mglaman closed 4 weeks ago

mglaman commented 4 weeks ago

Is your feature request related to a problem? Please describe. I don't think it's a problem, but it's a change in behavior from Drush 12 (I think.)

The database for a SQLite database connection string is now an absolute path. I thought before it supported relative paths, which is especially helpful if the SQLite database is outside of the docroot. Now it assumes it's in $PATH/web/core.

Describe the solution you'd like A db-url like sqlite://localhost/../db.sqlite puts the db in the project root with the path ../db.sqlite which has the real path of /var/www/html/db.sqlite and not writing /var/www/html/web/db.sqlite

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

weitzman commented 4 weeks ago

Its possible that this change as a byproduct of https://www.drupal.org/node/3129492. Drush had no interest in a behavior change.

mglaman commented 4 weeks ago

I'll have to check, but I'm 80% sure it was working fine on 10.3. I just noticed it on Drupal CMS so maybe it's something in 10.4 even around the filepath generation?

Because using the default Drush prompts I just got this invalid path: /Users/matt.glaman/Projects/drupal_cms/trial/build/web/core/sites/default/files/.ht.sqlite

🙈 note web/core/sites

mglaman commented 4 weeks ago

I think it might be due to \Drupal\sqlite\Driver\Database\sqlite\Connection::createConnectionOptionsFromUrlwhich \Drush\Sql\SqlBase::dbSpecFromDbUrl calls

mglaman commented 4 weeks ago

One thing I noticed:

        // This command is 'bootstrap root', so we should always have a
        // Drupal root. If we do not, $aliasRecord->root will throw.
        $aliasRecord = $this->siteAliasManager->getSelf();
        $root = $aliasRecord->root();

This now returns /web/core and that root is passed to the DB string SQLite creates

mglaman commented 4 weeks ago

Found some proof:

drush 12: https://git.drupalcode.org/project/drupal_cms/-/jobs/3114006

$ vendor/bin/drush site:install --yes
 You are about to:
 * Create a sites/default/settings.php file
 * CREATE the 'db.sqlite' database.
 // Do you want to continue?: yes.     

after drush 13: https://git.drupalcode.org/project/drupal_cms/-/jobs/3114570

$ vendor/bin/drush site:install --yes
 You are about to:
 * Create a sites/default/settings.php file
 * CREATE the '/builds/project/drupal_cms/trial/build/web/core/db.sqlite' database.
 // Do you want to continue?: yes.   

I don't get it

phenaproxima commented 4 weeks ago

Yup - the x-factor here was definitely the change from Drush 12 to Drush 13, at least from Drupal CMS's perspective. That's why I think this has to be a Drush bug.

mglaman commented 4 weeks ago

I see this:

    public static function dbSpecFromDbUrl($db_url): array
    {
        $db_url_default = is_array($db_url) ? $db_url['default'] : $db_url;
        return Database::convertDbUrlToConnectionInfo($db_url_default, DRUSH_DRUPAL_CORE);
    }

Note the DRUSH_DRUPAL_CORE passed, which specifies the $root is core. This was added in 13: https://github.com/drush-ops/drush/commit/39e55920e99a248f3fa3f9fea8e239f3d418b7da#diff-e849f63682b37f81e1291d327014a18d9e5367b2d0fc60b67374515ad1f1e659

I think we need the root from the alias record here

This

        // This command is 'bootstrap root', so we should always have a
        // Drupal root. If we do not, $aliasRecord->root will throw.
        $aliasRecord = $this->siteAliasManager->getSelf();
        $root = $aliasRecord->root();
mglaman commented 4 weeks ago

Specifically, $root is documented as:

   * @param string $root
   *   The root directory of the Drupal installation. Some database drivers,
   *   like for example SQLite, need this information.

And the $root is only used if an absolute path is not passed:

    if ($url_components['path'][0] === '/' || $url_components['path'] === ':memory:') {
      $database['database'] = $url_components['path'];
    }
    else {
      $database['database'] = $root . '/' . $url_components['path'];
    }

So, if \Drush\Sql\SqlBase::dbSpecFromDbUrl pased the alias record root, we wouldn't have the broken web/core/sites/default/ path but still an absolute path

mglaman commented 4 weeks ago

@weitzman is right. It probably broke in https://www.drupal.org/node/3129492 but < Drush 13 never used the new method. Now that Drush 13 does, we find the bug. Drupal core is enforcing the absolute path.

mglaman commented 4 weeks ago

I'll close since the problem is Drupal: https://www.drupal.org/project/drupal/issues/3056698