Contrib.social

database driver crashes during install (dependency not autoloaded)

Open on Drupal.org →
Created on 4 September 2024, 3 months ago

Problem/Motivation

A custom database driver, that extends an existing driver can't be configured during installation with drush. There is already some logic to take care of this edge case, but in the wrong order. When `class_exists` is called, the autoloader for the dependencies isn't loaded and an Exception is thrown.

Context: I want to test Drupal 11, but I can't install it in my default docker container (debian-bookworm, has SQLite < 3.45). So I tried the new sqlite337 module → . For some reason, the installation works when using the web UI. But using `drush site:install` fails:

```bash
./vendor/bin/drush site:install --yes --db-url="sqlite://../../../db/test.sqlite?module=sqlite337" --site-mail noreply@example.com
Error: Class "Drupal\sqlite\Driver\Database\sqlite\Connection" not found in /var/www/web/modules/contrib/sqlite337/src/Driver/Database/sqlite/Connection.php on line 7 #0 /var/www/vendor/composer/ClassLoader.php(576): include()
#1 /var/www/vendor/composer/ClassLoader.php(427): Composer\Autoload\{closure}('modules/contrib...')
#2 [internal function]: Composer\Autoload\ClassLoader->loadClass('Drupal\\sqlite33...')
#3 /var/www/web/core/lib/Drupal/Core/Database/Database.php(547): class_exists('Drupal\\sqlite33...')
#4 /var/www/vendor/drush/drush/src/Sql/SqlBase.php(583): Drupal\Core\Database\Database::convertDbUrlToConnectionInfo('sqlite://../db/...', '/var/www/web/co...')
#5 /var/www/vendor/drush/drush/src/Sql/SqlBase.php(104): Drush\Sql\SqlBase::dbSpecFromDbUrl('sqlite://../db/...')
#6 /var/www/vendor/drush/drush/src/Commands/core/SiteInstallCommands.php(342): Drush\Sql\SqlBase::create(Array)
# ... some more output
```

Steps to reproduce

* use a docker image based on debian-bookworm, e. g. `php:8.3-apache`
* install Drupal with composer and drush:

```bash
mkdir drupal11_l10n_test && cd drupal11_l10n_test
composer create-project drupal/recommended-project .
# Installing drupal/recommended-project (11.0.1)
composer require --dev drupal/core-dev
composer require drush/drush
composer require 'drupal/sqlite337:^1.0@alpha'

mkdir -p db
# triple '../' works in Drupal 11 (single '../' worked in Drupal 10)
./vendor/bin/drush site:install --yes --db-url="sqlite://../../../db/test.sqlite?module=sqlite337" --site-mail noreply@example.com
```

Proposed resolution

Check for the existence of the driver class __after__ it's dependencies are added to the autoloader (see attached patch file).

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Active

Version

11.0 🔥

Component
Database  →

Last updated about 3 hours ago

  • Maintained by
  • 🇳🇱Netherlands @daffie
Created by

raffaelj

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @raffaelj
  • Status changed to Needs review 3 months ago
  • Comment 3 months ago →
    raffaelj
  • Status changed to Needs work 3 months ago
  • Comment 3 months ago →
    cilefen
  • Comment 3 months ago →
    raffaelj

    I'll have another look at the issue fork tomorrow.

  • Merge request !943111.x → (Open) created by raffaelj
  • Pipeline finished with Failed
    3 months ago
    Total: 143s
    #274875
  • Status changed to Needs review 3 months ago
  • Comment 3 months ago →
    raffaelj

    I found the root cause of this edge case bug. All existing modules (even if not installed) where already autoloaded in `core/tests/bootstrap.php`. So it was never possible to throw this Exception during unit tests. I reverted the autoloading of core drivers in the responding test. With that change, also all tests of the driver_test module failed during the class_exists call. With my provided fix all tests pass again.

  • Pipeline finished with Success
    3 months ago
    Total: 455s
    #274897
  • Comment 2 months ago →
    🇺🇸United States smustgrave

    Can we get test coverage added for the new exception being introduced.

  • Comment 2 months ago →
    raffaelj

    > Can we get test coverage added for the new exception being introduced.

    @smustgrave I don't think, this is needed. And if so, I can't wrap my head around how I could test it. I think, it's a misunderstanding because of my wording in #6 and because I should have commited my changes in a different order.

    What I did:

    1. moved a view lines of code in `Database.php` to throw an Exception at the right moment (was wrong before)
    2. fixed test, that never caught the bug, because the test has a different autoloading mechanism, than a production system

    What I should have done:

    1. Fix `UrlConversionTest` to mimic production system, which now throws Exceptions, that weren't caught before. Of course, this commit wouldn't have passed the CI pipeline.
    2. Fix `Database.php`, so no Exceptions are thrown anymore.

    No new Exception is introduced. An existing Exception is only moved a few lines below to throw at the right moment. But I should have commited (and written) the changes in a different order to complement my thought process.

    Because commits are squashed on merge, this can stay in this order.

  • Comment 2 months ago →
    manibharathi ezhimalai ravi

    Hi,
    I am able to reproduce the issue. Done manual testing for the Drupa Version 11.x.

    Steps followed for the Update:

    1. Install docker image with the php 8.3
    2. Go inside the docker container
    3. mkdir drupal11_l10n_test && cd drupal11_l10n_test
    4. composer create-project drupal/recommended-project .
    5. # Installing drupal/recommended-project (11.0.1)
    6. composer require --dev drupal/core-dev
    7. composer require drush/drush
    8. composer require 'drupal/sqlite337:^1.0@alpha'
    9. mkdir -p db
    10. # triple '../' works in Drupal 11 (single '../' worked in Drupal 10) ./vendor/bin/drush site:install --yes --db-url="sqlite://../../../db/test.sqlite?module=sqlite337" --site-mail noreply@example.com

    MR !9431 applied successfully. Changes looking good to me.

    Before Patch:

    After Patch:

  • Status changed to Needs work 12 days ago
  • Comment 12 days ago →
    🇳🇱Netherlands daffie

    In 📌 [PP-1] Create the database driver for MySQLi Postponed we shall get testing for this problem. The new MySQLi database driver has the old MySQL database driver as a dependency.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024