Problem/Motivation

In #3129043: Move core database drivers to modules of their own we are moving the core database drivers to their own module. That is great, only in D9 contrib database drivers can still live in the "drivers" directory. Those drivers are deprecated in D9 and will be removed before D10. Until D10 we shall have to support those drivers that are not provided by a module.

Proposed resolution

Remove all code/tests to database drivers living in the directory DRUPAL_ROOT/drivers.

Remaining tasks

TBD

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#3 3250999-3.patch17.33 KBarantxio

Issue fork drupal-3250999

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

daffie created an issue. See original summary.

mondrake’s picture

arantxio’s picture

Status: Active » Needs review
StatusFileSize
new17.33 KB

Here is a start for this change. I removed some function that was designed for D8 and then mapped and adjusted all the functions that referred to Drupal\Driver. Some might be removable, but we have to decided on that because its also kind off test coverage.

arantxio’s picture

Version: 10.0.x-dev » 11.x-dev

Setting the version to D11 because 10.2 doesn't exist yet, we'll need to adjust this when the it's added.

smustgrave’s picture

10.2 will just be a tag off 11.x so as long as it works there it should be good.

Requeuing tests to make sure mysql and sqllite passes.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Refactor didn't seem to cause a regression. Think it would be good to get this in early for 10.2

daffie’s picture

Title: Refactor Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() as now all database drivers are provided by a module. » Remove all code/tests to database drivers living in the directory DRUPAL_ROOT/drivers
Issue summary: View changes

Updated the title and the IS.

catch’s picture

Issue tags: +Major version only

This could have gone in prior to 10.0.0 but I think it will need to wait until (actual) 11.x now.

catch’s picture

Status: Reviewed & tested by the community » Needs review
smustgrave’s picture

Status: Needs review » Postponed

Seems like postponed makes sense then.

Is Major version only how you find D11 tickets or is there another tag?

mondrake’s picture

Status: Postponed » Needs work

This is actionable now. Needs rebase and change to MR workflow.

arantxio’s picture

I've added most of the patch to a issue fork, however some of the code has changed already and some classes are gone.

I don't see any more tests that reference Drupal\Driver or Drupal\\Driver. But a check on it would be great.

I couldn't run the tests locally so I hope someone else could go through it.

andypost’s picture

Issue tags: +Needs reroll
andypost’s picture

As I see only 4 usages left

$ git grep database_drivers
core/tests/Drupal/KernelTests/Core/Database/SelectExtenderTest.php:122:    $additional_class_loader->addPsr4("Drupal\\core_fake\\Driver\\Database\\coreFake\\", __DIR__ . "/../../../../../tests/fixtures/database_drivers/module/core_fake/src/Driver/Database/CoreFake");
core/tests/Drupal/KernelTests/Core/Database/SelectExtenderTest.php:123:    $additional_class_loader->addPsr4("Drupal\\core_fake\\Driver\\Database\\CoreFakeWithAllCustomClasses\\", __DIR__ . "/../../../../../tests/fixtures/database_drivers/module/core_fake/src/Driver/Database/CoreFakeWithAllCustomClasses");
core/tests/Drupal/Tests/Core/Database/ConnectionTest.php:324:    $additional_class_loader->addPsr4("Drupal\\core_fake\\Driver\\Database\\CoreFake\\", __DIR__ . "/../../../../../tests/fixtures/database_drivers/module/core_fake/src/Driver/Database/CoreFake");
core/tests/Drupal/Tests/Core/Database/ConnectionTest.php:325:    $additional_class_loader->addPsr4("Drupal\\core_fake\\Driver\\Database\\CoreFakeWithAllCustomClasses\\", __DIR__ . "/../../../../../tests/fixtures/database_drivers/module/core_fake/src/Driver/Database/CoreFakeWithAllCustomClasses");
quietone’s picture

I wasn't sure about this issue so I confirmed with @daffie that this is still relevant. In the same message they mentioned that there are other higher priority issues than removing these tests.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

zeeshan_khan made their first commit to this issue’s fork.

zeeshan_khan’s picture

Status: Needs work » Needs review

The 'custom' and 'core' fixture directories under core/tests/fixtures/database_drivers/ simulated database drivers
placed directly in DRUPAL_ROOT/drivers, a layout that is no longer supported now that all core and contrib database drivers are provided
by modules. No test file references these fixtures anymore; the only fixture still in use is 'module', which represents the current, supported driver location.

AI-assisted, reviewed and tested by Zeeshan Khan (Manually)

daffie’s picture

Status: Needs review » Needs work

The MR needs to be rebased and the Gitlab CI pipeline needs to return green.

zeeshan_khan’s picture

Status: Needs work » Needs review

@daffie Thanks for the review
Rebased onto current main and CI has been re-run.

The pipeline is green except for two jobs: PHPUnit Unit (Component): [8.6-ubuntu] and PHPUnit Unit (Core): [8.6-ubuntu]. These are not caused by this fix. Both come from the same root cause: the mikey179/vfsstream package (used across hundreds of core unit tests for virtual filesystem mocking) calls spl_object_hash(), which PHP 8.6 deprecates. That single deprecation notice is what fails these two jobs, not any actual test failure, all 6466 tests in the affected suite pass.

This is a pre-existing, project-wide issue, not something introduced by this MR. I checked an unrelated open MR (!15266) touching a completely different part of the codebase, and it fails on the exact same two jobs with the exact same numbers. Both jobs are also marked allow_failure: true in the pipeline config, so they don't block merge status.

I also manually ran the full additional test matrix (MariaDB, PostgreSQL-adjacent, Kernel, Functional, FunctionalJavaScript) against this change, all green.

Let me know if there's anything else needed here.

daffie’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Major version only, -Needs reroll

Looks good to me.
The 2 fake database drivers can be removed.
For me it is RTBC.

  • longwave committed 595e1cff on main
    test: #3250999 Remove all code/tests to database drivers living in the...
longwave’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 595e1cffb5a to main. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.