Problem/Motivation

Database drivers need to be able to override services in the container, but we already have container_needs_rebuild = true for that.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#8 3615455-nr-bot_ku46ujmr.txt1.52 KBneeds-review-queue-bot

Issue fork drupal-3615455

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:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !16608

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

That seems to be enough.

catch’s picture

catch
he/him
Primary language English
commented
Title: Install database driver modules alongside other modules » [PP-1] Install database driver modules alongside other modules
Status: Needs review » Postponed

Although it's postponed on #3614153: Install system module alongside other modules in the installer.

catch’s picture

catch
he/him
Primary language English
commented
Title: [PP-1] Install database driver modules alongside other modules » Install database driver modules alongside other modules
Status: Postponed » Needs review
catch’s picture

catch
he/him
Primary language English
commented

Rebased and added a change record.

nicxvan’s picture

nicxvan commented

So container_needs_rebuild will rebuild the container anyway. Do we need to worry about order of install? I don't think so since the connection will get the right driver.

I made a minor tweak to the CR.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3615455-nr-bot_ku46ujmr.txt1.52 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

daffie’s picture

daffie commented
Status: Needs work » Reviewed & tested by the community
Related issues: +#3618797: Replace _install_get_database_module_name() by Database::getConnection()->getProvider()

The code changes look good to me.
Created #3618797: Replace _install_get_database_module_name() by Database::getConnection()->getProvider() as a followup to remove the helper method.
For me it is RTBC.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

NW for merge conflict.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Rebased.

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs work

Comments on the MR.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Applied one suggestion, went a slightly different way with the second.

  • godotislate committed 02579164 on main 
    task: #3615455 Install database driver modules alongside other modules...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Needs review » Patch (to be ported)

Applied a fix to the docblock return type and Committed 0257916 and pushed to main. Thanks!

If this is to go to 11.x, it'll need a new MR because of merge conflicts.

catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » main
Status: Patch (to be ported) » Fixed

This depends on #3614153: Install system module alongside other modules in the installer which we left as main-only, so we'll need to do the same here too.

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.

quietone’s picture

quietone commented

Update change record and publish, correcting date.

godotislate’s picture

godotislate
he/him
commented
Status: Fixed » Needs work

This may have broken mysqli tests: https://git.drupalcode.org/project/drupal/-/pipelines/936533

godotislate opened merge request !17004

godotislate’s picture

godotislate
he/him
commented

Created a revert MR https://git.drupalcode.org/project/drupal/-/merge_requests/17004
This fixed all of the mysqli tests except for one (https://git.drupalcode.org/project/drupal/-/jobs/11985707), so not sure reverting on main will help that much until the source of the other failure is found.

    Generic (Drupal\Tests\mysqli\Functional\Generic)
     ⚠ Module generic issues
     ✔ Mysqli module
    
    There was 1 risky test:
    
    1) Drupal\Tests\mysqli\Functional\GenericTest::testModuleGenericIssues
    This test did not perform any assertions
    
    /builds/project/drupal/core/modules/system/tests/src/Functional/Module/GenericModuleTestBase.php:51
    
    OK, but there were issues!
    Tests: 2, Assertions: 2, Risky: 1.
catch’s picture

catch
he/him
Primary language English
commented

Switched to mysqli locally and ran various installer tests and can't reproduce a failure against HEAD yet.

catch opened merge request !17006

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Took a closer look. The existing config installer tests manually add the database driver to the config, but they only do the driver, not the driver's dependencies.

There were two possible places to fix this (see reverted commit in the tests):

1. In the existig config installer base class - we could add the dependent driver there were the driver itself gets added to the config.

2. We can explicitly add the driver's dependencies in the installer itself just before it gets installed.

Manual install with mysqli works completely fine so I'm not sure how much this is a test-specific problem or could somehow break actual installs, but it seems more robust to handle dependencies explicitly in the installer, prior to the main change here they were handle implicitly.

catch’s picture

catch
he/him
Primary language English
commented

Actually #24.2 doesn't fix those tests, so it looks like this is indeed test specific and we should do #24.1

There's already a follow-up for the installer code here because what we have is still somewhat pre-database-drivers-as-modules so we could probably look into being more explicit about module dependencies there #3618797: Replace _install_get_database_module_name() by Database::getConnection()->getProvider().

catch’s picture

catch
he/him
Primary language English
commented

Also looked at generic test. The failure is due to #3615570: Optimize GenericModuleTestBase. Before that issue, we would assert something just when installing the module, and that would be the only assertion to happen for mysqli because it's a database driver module and also hidden. Now that first assertion doesn't happen so the test got marked as risky. We need to mark it skipped for hidden database driver modules. Since that's a one-liner, added it in here too so we can get to a fully green mysqli test run.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

Both changes look good to me.
For me those are RTBC.
I ran into the same problems when working on #3609986: Improve database exception messages.

  • godotislate committed 35e3299a on main 
    fix: #3615455 Address test mysqli test failures

By: catch
By:...
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

Committed 35e3299 and pushed 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.

daffie’s picture

daffie commented
Status: Fixed » Needs work
Related issues: +#3615690: Removal of unnecessary savepoints from PostgreSQL

I am working on #3615690: Removal of unnecessary savepoints from PostgreSQL and I am getting test failures, because the module that is providing the database driver does not get installed as the first module. Therefore it not able to override any call to hook_schema() or any 'backend_overridable' service. The code $kernel->getContainer()->get('module_installer')->install([$provider], TRUE); installs the database driver module and that line has been removed.

Edit: The following commit fixes the problem: https://git.drupalcode.org/project/drupal/-/merge_requests/16629/diffs?c....

catch opened merge request !17101

catch’s picture

catch
he/him
Primary language English
commented
Category: Task » Bug report
Priority: Normal » Critical
Status: Needs work » Needs review

I've pulled @daffie's commit into an MR here.

I wondered if it would be simpler to add the database driver after sorting instead of before, so we can just put it at the beginning once instead of twice, but then we wouldn't get the weight for the file info. Probably needs #2968232: Deprecate module weights then we can delete that whole section instead.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

All code changes look good to me.
For me it is RTBC.

  • godotislate committed 0f9a918d on main 
    fix: #3615455 Ensure database drivers really are installed first
godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

Committed 0f9a918 and pushed 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.