Problem/Motivation
Currently, drupal_get_database_types()
only looks for database drivers in /core/lib
and /drivers/lib
. This means that contrib modules that implement database drivers must instruct users to copy the driver files from where modules are normally installed (e.g., /modules
) into the required place within /drivers/lib
. This is a tedious and error-prone step that needs to be re-performed every time the module is updated. While this can in some cases be automated with Composer, doing so requires edits to the website's root composer.json, and it doesn't help Drupal users who don't use Composer.
Proposed resolution
- Define a location within the module's namespace and src directory where database drivers can go. This issue proposes
Drupal\MODULE_NAME\Driver\Database\DRIVER_NAME
for the namespace, which using the already existing PSR-4 layout of the module's "src" directory corresponds to thesrc/Driver/Database/DRIVER_NAME
directory. - Expand
drupal_get_database_types()
to scan that location for all modules in the codebase. Note that this is done for all modules in the codebase, not only for enabled modules, because this needs to happen at the beginning of the installation process, before any modules have been enabled. - Add an optional
'autoload'
key to the database connection info array that's defined in settings.php for specifying the directory that corresponds to the driver's namespace. This is needed because the database driver needs to be loaded every request prior to when module namespaces are added to the autoloader, since that information lives in the service container which lives in the database. Having the value specified in settings.php avoids having to rescan the filesystem to find it for every request. - Expand
SiteSettingsForm::validateForm()
to fill in the'autoload'
key, so that it is automatically written to settings.php by the installer. - Expand
db_installer_object()
to take an optional$namespace
parameter. This allows it to avoid having to rescan for all modules in the filesystem afterdrupal_get_database_types()
already did it. - Expand database URLs (used by console commands and test runners in lieu of a settings.php connection info array) to include a
module
query parameter for when the driver is provided by a module, and changeDatabase::convertDbUrlToConnectionInfo()
andConnection::createUrlFromConnectionOptions()
to use and set that accordingly. - Retain backwards compatibility for the
Drupal\Driver
namespace indrivers/lib
. Existing Drupal 8 drivers using those namespaces will continue to work exactly as before.
Follow-ups:
- #3123461: Deprecate support for database drivers placed in DRUPAL_ROOT/drivers
- #3123240: Provide a better way to get driver name and database type
- #3125257: Deprecate support for database connections as URLs and replace with JSON strings
- #3125476: Add an API for determining whether a given PHP namespace or class name is within the namespace of an extension
Remaining tasks
- A release manager review to evaluate whether this can be committed to 9.0.x during beta. The motivation to do so is to enable people to more smoothly be able to use the contrib drivers created to help people on older versions of databases than Drupal 9 core's minimums, such as the PostgreSQL fallback driver.
User interface changes
None
API changes
Additions:
- An optional $namespace parameter added to
db_installer_object()
. - A new static method
Drupal\Core\Database\Database::findDriverAutoloadDirectory()
added. - For database drivers in a module's namespace (as opposed to in Drupal\Driver or Drupal\Core), a
module
query parameter is added to the database connection URL. For example:pgsql://test_user:test_pass@test_host:5432/test_database?module=driver_test
. URLs for database drivers in the Drupal\Driver and Drupal\Core namespaces are unaffected. - For database drivers in a module's namespace (as opposed to in Drupal\Driver or Drupal\Core), an
'autoload'
key is added to the database connection array in settings.php. Setting it is required if using a database driver from a module's namespace unless the Drupal site is using Composer and the driver's namespace is in Composer's autoloader. Setting it is not required for drivers in the Drupal\Driver and Drupal\Core namespaces.
Data model changes
None
Release notes snippet
Database drivers provided by modules can be placed in src/Driver/Database. Such drivers will be listed in the installer. Existing custom or contributed drivers do not need to make a changes and continue to work as before.
Comment | File | Size | Author |
---|---|---|---|
#138 | 3120096-138.patch | 723 bytes | Beakerboy |
#133 | 3120096-8.9.x-133.patch | 66.57 KB | alexpott |
#133 | 131-133-interdiff.txt | 1.13 KB | alexpott |
#118 | 3120096-3-9.1.x-117.patch | 66.7 KB | alexpott |
#118 | 3120096-3-117.patch | 66.57 KB | alexpott |
Comments
Comment #2
daffie CreditAttribution: daffie commentedFirst patch still needs a lot of work.
Comment #4
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedComment #5
kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 for Drupal India Association commentedComment #6
Gábor HojtsyThis would be great! Thanks for working on this.
Comment #7
daffie CreditAttribution: daffie commentedStraight reroll after #3118832: Allow custom database drivers to extend and have the same name as the core ones has landed.
Comment #8
daffie CreditAttribution: daffie commentedSome minor improvements. Mostly documentation.
Comment #9
daffie CreditAttribution: daffie commentedThis patch should fix some of those test failures.
Comment #12
daffie CreditAttribution: daffie commentedThis issue needs a framework managers review.
Comment #13
alexpottAs I pointed out in slack I think this should be ?module and not ?driver. ne thing we can’t avoid in Drupal (yet) is that extension name is synonomous with namespace i.e . node has to refer to \Drupal\node and what driver in your version means is what \Drupal\{driver} it is in. I think it is better to call an apple an apple and use ?module instead of driver. In this case ?module means the module that provides the driver. Modules are an existing concept with how to load it’s namespace the way we want it to ie. \Drupal\MODULE. And one is an existing concept with super awkward code loading (driver). So using ?module points out
We've already determined this. We've determined if a module is providing a database driver above ... if
"Drupal\\{$driver}\\Driver\\Connection";
exists then $driver is the module. Also I think we should support this naming scheme first before the old contrib on ofDrupal\\Driver\\Database\\{$driver}\\Connection
because we need to deprecate that in Drupal 9 for removal in Drupal 10 because this change makes it obsolete.If we pass around the module then we don't need this awkwardness. The whole concept of database type vs driver name should be deprecated. We should have a driver name and the module that provides it (or core it is it core).
Comment #14
effulgentsia CreditAttribution: effulgentsia at Acquia commented+1 to #13.1. I would also suggest being explicit in the namespace that it's a database driver, and add the driver name (which might be different than the module name). In other words, scan
MODULE_PATH/src/Driver/Database
for potentially multiple drivers (since we can't infer the driver name from the module name). I don't know if there's any good examples of a module that would want to provide multiple database drivers, but an example of a module with a different name than the driver is https://www.drupal.org/project/mysql56, where I'd like to havemysql56
as the module name andmysql
as the driver name.This prevents the above goal of separating module name and driver name.
If we're only adding this one class to the autoloader, then when we get to the "Setup database" step of the web installer, then it errors out when trying to instantiate the connection class, because that's not in the autoloader (at this point in the installation, the module isn't enabled). Do we want to add the entire driver namespace to the autoloader here, or should we make it the module's responsibility to add its
autoload
entry to itscomposer.json
?We should flip these, so that we check the module first. But also, if we separate module name and driver name, how do we know which module to check?
I'm kind of wondering if we even want to keep going down the road of searching within each module. What if we just keep Drupal's existing namespace of
Drupal\\Driver\\Database\\DRIVER_NAME
, and if modules want to add to this namespace, they can put the desiredautoload
entry into their composer.json. And then for scanning withindrupal_get_database_types()
, can we scan for what in$class_loader->getPrefixesPsr4()
matchesDrupal\\Driver\\Database\\...
? That would also allow projects that want to provide a driver to not be modules if they don't have to be. E.g., their code can remain in/vendor
, out of the webroot, if there's nothing module-like that they want to offer.Comment #15
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIf we don't complete this issue before beta, then can we get #3119853: Add database driver installer path to Composer project templates in before beta instead, and then keep refining this issue post-beta? Because it looks like this issue isn't removing the scanning for what's in the "drivers" directory, just making it obsolete. But for as long as we continue to have the "drivers" directory, #3119853: Add database driver installer path to Composer project templates would be a helpful way of managing it.
Comment #16
daffie CreditAttribution: daffie commentedThank you @alexpott and @effulgentsia for your reviews!
For #13.1: Changed "?driver=" to "?module=". Now we are mixing the driver key in settings.php with "?module=". To me this will cause confusion with site owners and site builder. They all know what the "driver" is from using settings.php for a very long time. The "?module=" is something new to them. One could misinterpreted the setting "?module=" far more easily then "?driver=", it is just more specific.
For #13.2 Fixed. Only the new
"Drupal\\{$driver}\\Driver\\Connection"
has the constant.For #13.3 Removed the code. Core uses the function
parse_url()
. If you have schema name with an underscore in it the function will fail. If you now create a contrib driver with a module/driver name with an underscore in it and set the constantDATABASE_TYPE
to the module/driver name, it will not work!For #14.1, #14.2 and #14.3 Adding multiple database drivers in a single module makes thing a lot more complicated. If we do #13.1 and we have the setting "?module=", then which driver in the module should be used? I see the advantage of allowing multiple drivers per module. I am curious what the viewpoint of @alexpott is about this and the comments #14 and #15.
For #14.4 Fixed.
For #15 I agree that we should do this issue or your alternative. I am not sure if we can remove the scanning of
DRUPAL_ROOT/drivers
in this issue. If it is allowed, then I am all for it.I have also a question: In the patch we are adding the constant
DATABASE_TYPE
. I named it, because of the method$connection->databaseType()
. The docblock of the method is: "Returns the name of the PDO driver for this connection.". With the PHP functionparse_url
we use schema name for this. What name should we use for the added constant?Comment #17
daffie CreditAttribution: daffie commentedAll the previous patches in this issue contain the patch from #3118679: Allow driver name to be different then the PDO driver name during site install proces. Removed that patch.
Comment #18
alexpott@daffie we don't need to be so concerned with the URL structure. The representation of a db connection as a URL is wrong and we need to deprecate that in Drupal 9 and turn it into a json string that is the same as the databases array in PHP.
Yes driver can't contain an underscore but that's already the case. But it shouldn't affect your module. Taking the example of the
pgsql_fallback
module. Imo this should have a driver called pgsql. So the urls will bepgsql://usename:password@localhost/database?module=pgsql_fallback
. This should map to a namespace of\Drupal\pgsql_fallback\Driver\Datbase\pgsql
. This will allow modules to have more that one driver.We need to change
drupal_get_database_types()
to key on "module:driver" and also we need to make everything work as expected when module = core.Comment #19
effulgentsia CreditAttribution: effulgentsia at Acquia commentedJust want to leave a note here that I won't have time to review this until next week at the soonest. Please feel free to carry on and commit without me. For drivers of the same name as core ones, I want to point out #3119853-8: Add database driver installer path to Composer project templates, but there's still plenty of other improvements here that would be needed for drivers of different names, so I hope this lands in beta or soon after.
Comment #20
xjmThe "beta target" tag means the change is allowed and prioritized for commit during the beta phase (i.e., between the release of 9.0.0-beta1 and 9.0.0-rc1.)
This issue does not meet the criteria for allowed changes during the beta phase. (It includes API additions, etc.) Therefore, it would need release management signoff to go into 9.0.x as a beta target after 9.0.0-beta1 ships. This issue might be a candidate for a release management exception as a contrib blocker. Let's tag it as such if that's appropriate. Let's also update the issue summary to fill out a summary of the API additions, internal API changes, potential disruptive impacts, etc.
Thanks everyone!
Comment #21
xjmOh also, if this meets the criteria for a major task or bug, we should also promote it accordingly. Thanks!
Comment #22
daffie CreditAttribution: daffie commentedThis issue blocks #3118455: [META] Create contrib fallback database driver for PostgreSQL 9.6.
Comment #23
daffie CreditAttribution: daffie commentedI did not added a interdiff, because the patch has changed too much.
Comment #25
daffie CreditAttribution: daffie commentedComment #26
daffie CreditAttribution: daffie commentedAnd the interdiff file for the patch from the previous comment.
Comment #27
daffie CreditAttribution: daffie commentedThe key changes for the patch are:
Drupal\{$module}\Driver\{$driver}
.MODULE_ROOT/src/Driver/{$driver}
. For testing when a module has a database driver core looks for the existence of the fileMODULE_ROOT/src/Driver/{$driver}/Install/Tasks.php
. The chance that a non-database driver has that file in the given directory is almost zero.mysql://test_user:test_pass@test_host/test_database?module=driver_test#bar
, where bar is the database prefix to be used and the providing module is driver_test.drupal_get_database_types()
returns the database types keyed by a concatenated string containing the module name, a colon character and the driver name. If there is no providing module, the module name is the driver name.db_installer_object()
has now two parameters instead of just one. The added parameter is the module name for the module that is providing the driver to be used. Both parameters will be required.Drupal\Core\Database\Install\Tasks
has a new class variable called$driver
. This variable is a concatenated string of the module name, a colon character and the driver name. If there is no providing module, the module name is the driver name.fakedriver
anddriver_test
. The first module has a driver named "fakedriver". This is for testing contrib database drivers like SQL-Server and OracleDB, where the module name is the same as the driver name. The second module has two drivers, one for MySQL and the other one for PostgreSQL. This is for testing that modules can have multiple drivers.Comment #28
alexpottI discussed the return value changes to
drupal_get_database_types()
with @daffie. I think we can get away without having to make them if we change the patch to make the same assumption about database drivers as core makes about modules. If you place a node module in modules/contrib it will take precedence of core/modules/node. In fact the system will behave as if the core node module does not exist.Therefore we could return use just the driver in the key to preserve BC but use a defined precedence to say what happens if contrib provides the a driver with the same name as core. The order of precedence - increasing in priority would be core, old contrib namespace, the new contrib namespace added here.
With respect to the fallback drivers being added to sort mysql 5.6 and postgres 9 I don't think these drivers should use the same name as the core driver since it's not really necessary.
Comment #29
daffie CreditAttribution: daffie commentedChanged the return array for
drupal_get_database_types()
back the way it was as requested by @alexpott.and
If I wanted to create an alternative implementation for the node module, I should create a contrib module called node. How do I install such a module with composer? Am I allowed to have two namespaces called
Drupal\node
. Is this the way we want to extend database drivers? If themes would work the same way, then only one theme could make an override for an by core supported theme.For my fallback driver for PostgreSQL the module name should then be "pgsql". That makes me then the only person who can create an alternative driver for PostgreSQL. There can just only be one module called "pgsql". If I look at the issue #2846366: Improve Drupal's Database Abstraction Layer Extensibility and Capabilities then there are more people who want to try all different kind of things with the PostgreSQL driver or with the one for MySQL. For instance having different contrib drivers possible gives contrib developers the option to try and show that new functionality works without implementing it in core. Maybe someone want to try implementing the menu storage in a JSON field in a way that the menu does not have to be compiled every time.
Comment #30
alexpottWe shouldn't construct our own class loader here. We can use the one we already have.
Let's swap the args, default to NULL and trigger a deprecation if $module is not provided. There are some contrib usages - http://grep.xnddx.ru/search?text=db_installer_object&filename= - and we can support the old namespaces for now. We should do the deprecation after we've deprecated support for the old custom driver namespace and potentially moved core drivers into their own modules.
We need to check
$task_class = "Drupal\\{$module}\\Driver\\{$driver}\\Install\\Tasks";
before$task_class = "Drupal\\Driver\\Database\\{$driver}\\Install\\Tasks";
so that drivers in the module namespace have the top priority.This should use
parse_str();
... ie.Also I think we should set module if we have it.
I think we should set $module if $connection_options['module'] is set. We don't need the additional complexity here.
As above - this should use parse_str. Also the comment needs updating.
The new module namespace should have priority
The patch attached addresses these points.
Comment #31
alexpottThe big issue we now need to deal with is that we need to add a real test of a installing with a module provided driver. I'm pretty sure we have a big problem at the moment since the container is cached in the database and that has the module list and that is used to populate the autoloader. I think we need to add autoloading instructions to the database array. In fact I think we need to add a driver_autoload property that contains the path to the module's driver directory. That way we'll be able to set up the autoloader in \Drupal\Core\Site\Settings::initialize() to be able to load the cached container using a module provided database driver.
@daffie to answer
. You can do this by putting your node-overriding module inside your project. I.e. create a project on d.o called
funkynode
and inside that have a directorymodule/node
that contains your override of the node module. This whole problem space is driven by how we place modules anywhere and everywhere. And do not use composer to build this but of the autolooader. We do it ourselves in \Drupal\Core\DrupalKernel::classLoaderAddMultiplePsr4()Comment #32
daffie CreditAttribution: daffie commentedI think we only should set the "?module=" when the providing module name is not the same as the driver name. For me it will be a better experience for site owners if we do that. Most site owners will not be using a contrib driver where the driver name is not the same as the providing module and therefor they do not need to be confronted with an extra "module" setting in the database settings array/URL.
drupal_get_database_types()
. Thank you for that. As for you comment #31: I understand that that needs to be done and I have no idea how to do that and where to do it. I need some more help with it.Also I was wondering whether that we are with this patch adding for every KernelTest the driver_test module that contains the contrib drivers for MySQL and PostgreSQL. The contrib drivers have presedence over the by core supported ones. Are we now going to use those contrib drivers for all KernelTests when we run the testbot with MySQL or PostgreSQL?
Comment #33
xjmThe IS still needs to be updated with the proposed API changes here. How will the change affect existing custom drivers? Is there a BC version we can backport to D8?
Comment #34
xjmComment #35
alexpottRe #33 we're trying to ensure that we do not break support at all for Drupal 8 custom drivers and that this change only results in new improved functionality and ease-of-use. Once we'll settled a few questions we'll update the issue summary and add a change record.
Comment #36
alexpottHere are my thoughts on how to address #31.
\Drupal\Core\Installer\Form\SiteSettingsForm::submitForm()
\Drupal\Core\Site\Settings::initialize()
namespace_autoload_dir
or something… not sure… we’ve got namespace already and this telling the system where to load that namespace from.\Drupal\Core\Database\Database::convertDbUrlToConnectionInfo()
will need to scan the module directories to populate the same info.And then, fingers-crossed, we’ll have no copy custom/contrib db driver support!
Comment #37
alexpottHere's a patch the adds proper autoloading (ie. the test no longer has to autoload the test drivers) and you can install Drupal using the test drivers!
$settings['extension_discovery_scan_tests'] = TRUE;
put that in your settings.php and you'll be able to select them in the installer.I think this patch might fail because the test drivers are using the same driver name as core's and therefore replace them - so the interactive installer tests are going to use them :) let's find out.
I also think we'll be able to refactor the patch to have less duplicate functionality.
Comment #38
alexpottSo looking at http://grep.xnddx.ru/search?text=createConnectionOptionsFromUrl&filename= we can do better for BC (and performance and simplicity) by setting module and namespace_dir in \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() and not each driverzs implementation of \Drupal\Core\Database\Connection::createConnectionOptionsFromUrl().
Comment #40
alexpottComment #41
daffie CreditAttribution: daffie commented@alexpott: Let me start by thanking you very much. I could not have done the part you did.
This is my first short/quick review. I will do more reviewing and I will test the patch in combination with my fallback driver for PostgreSQL.
This comment is no longer correct.
Why do we not use the service
\Drupal::service('class_loader')
here?These 2 methods need testing.
Can we also test that
$info['namespace']
is set.The method starts with setting the value for
$databases
to an empty array. Why does this work?@alexpott: Can you explain a bit how this works. With contrib databases driver in tests and production. Why did you add Settings::get() part?
With the removal of this line the method can no longer throw an exception. The docblock needs to be updated. And the exception from the method
getModuleNamespaceDir()
needs to be added.This method does not return a boolean. It returns the namespace directory or FALSE. The method can also throw an exception and the method description is also not correct. The docblock needs to be be updated for all points.
We need to make sure that the variable
$namespace_dir
is not FALSE.Comment #42
daffie CreditAttribution: daffie commentedManual check the patch in combination with my updated version of https://www.drupal.org/project/pgsql_fallback.
During install on the select your database screen the name was "PostgreSQL Fallback" instead of the usual "PostgreSQL".
The installation process did what it normally does. After installation the database settings array in settings.php listed the following:
All settings are as they should be. The patch works great!
Comment #43
alexpott$databases
variable.extension_discovery_scan_tests
to true in settings.php. For our test system we also need to always scan tests whendrupal_valid_test_ua()
this is how we can have 100s of test modules and ensure they don't affect anything outside of tests.$namespace = "Drupal\\$module\\Driver\\$driver";
- in this situation it's impossible for that method to return FALSE. It's either a string or an exception.Looking at #42 I'm surprise you don't have an entry for 'module'
Comment #44
alexpottAh I see - I realised that now we're writing namespace_dir to the connection options array we didn't need to write the module to the array as well. So the code in \Drupal\Core\Database\Connection::createUrlFromConnectionOptions() is wrong. Because we don't have 'module' in the connection options. Need to fix that.
Comment #45
alexpottFixing #44.
One other consideration is that maybe we need to cope with contrib drivers like http://grep.xnddx.ru/search?text=createUrlFromConnectionOptions&filename= - so we need to do the same as we do for \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() and move the module stuff to (\Drupal\Core\Database\Database::getConnectionInfoAsUrl(). But this is tricky because then we have to parse the URL we're creating an add to the query string. But the URLs we're creating are fake. The scheme is not valid for example. So *maybe* it's acceptable to say that if a current contrib driver wants to move to the new module provided namespace they have to update the implementation to cope with this. The implementation here adds module information in \Drupal\Core\Database\Database::getConnectionInfoAsUrl() to make things as simple as possible for contrib.
Comment #47
alexpottI keep on forgetting that #3112476: Always set $info['namespace'] on database connection info has not landed yet :( we really need that patch prior to do this one so we can make correct assumptions about this property being set and to make core and contrib db driver info the same.
Comment #48
alexpottAddressing #41.9
So for me there are two things left to do here:
Drupal\mymodule\Driver\mydriver
toDrupal\mymodule\Database\Driver\mydriver
as there other drivers - maybe not in Drupal-land yet but this also nicely mirrors the core namespaces likeDrupal\Core\Database\Driver\mysql
- consistency ftw!Comment #49
daffie CreditAttribution: daffie commentedHi @alexpott, I was also working on this. I did not have time to review your work. I will do so later. Thank you for working on this issue.
I have added testing for the installer stuff and made a small fix.
Comment #50
alexpottThis change is not a fix - it's a break. It is supposed to be an OR. Otherwise we can't set
extension_discovery_scan_tests
in settings.php and see the same as a BrowserTestBase.This is not necessary as far as I know. drupal_valid_test_ua() will return something.
Ah I see why you were doing that because you want tests to use the regular core drivers. Well imo we should change the module provided drivers to not override core drivers. The use-case for overriding a core driver is very small and I don't believe we should be showing it in the main demo of this functionality - ie the driver_test module. We should encourage people developing the fallback drivers to name their drivers things like mysql_fallback.
Comment #51
daffie CreditAttribution: daffie commentedI have added the test module driver_test to test that the we can override a by core supported database driver. Having a contrib database driver for MS SQL-server or OracleDB is something we cannot test on Drupal Core. If have updated the Drupal\FunctionalTests\Installer\InstallerTest to show that we by defaut use the by core supported database driver and not the one from the driver_test module. The new Drupal\FunctionalTests\Installer\InstallerNonDefaultDatabaseDriverTest is to show the overriding a by core supported database driver works.
Comment #52
alexpott@daffie just because the driver is for Postgres or Mysql the driver does not have to be called psql or mysql. It can be whatever we like. That's why we have \Drupal\Core\Database\Connection::databaseType(). I think overriding core drivers should not be the norm. I don't think that alternative MySQL implementations should replace the core one - I think they should be available alongside it. So we you look at the list in the installer you see the full range of options. The whole magically replace code stuff Drupal does is very confusing and I feel it's against https://en.wikipedia.org/wiki/Principle_of_least_astonishment. It's very hard to explain to some why placing code in one place makes functionality that was available go away.
Also the interdiff on #51 seems to be incorrect.
Comment #53
andypost++ #52 looks like another drupalism based on hook_world_alter
EDIT mysql-legacy and pgsql-unsupported
Comment #54
daffie CreditAttribution: daffie commentedYes, @alexpott you are right I missed that one. I was so happy that the patch worked with my contrib pgsqL_fallback database driver that I missed it.
That sounds great, the only problem is that the driver name has to be the same in a couple of places. Lets name our driver for example "driver_test_mysql". This will result in that:
1. On the file system the driver location (namespace_dir) will be: "core/modules/system/tests/modules/driver_test/src/Driver/driver_test_mysql/"
2. The connection URL will become something like: "driver-test-mysql://test_user:test_pass@test_host:3306/test_database?module=driver_test";
3. The namespace will be: Drupal\driver_test\Driver\driver_test_mysql;
4. The class variable $pdoDriver in the class Drupal\driver_test\Driver\mysql\Install\Tasks will become: "driver_test_mysql";
5. The method Drupal\driver_test\Driver\mysql\Connection::driver() will have to return: "driver_test_mysql";
The connection URL does not use underscore characters because the PHP function parse_url() does not allow that. I did a underscore-to-dash character in my patch from comment #9 and in comment #13.3 @alexpott said that should not do this "awkwardness". I have no idea how to fix this. The driver name is used in all these places and they must be the same. That is how Drupal core is using it.
The interdiff is between the latest patch from @alexpott (comment #48) and the patch from comment #51. I wanted to point out that I have only added the two tests.
My review of the patch from comment #51, I have just one remark: In the connection URL to connection array conversion and the reverse, we have in the URL the key
?module=
and in the connection array there is no such key. To me this is strange and confusing. The module name is used to get the "namespace" and "namespace_dir" values. Can we just add the module key to the connection array or change the?module=
from connection URL into "namespace" or "namespace_dir"?For #48.1: I get why you want to change that. On ly I do not think it is a good idea. For me the by core supported driver should move to a module. Just with other modules, the only difference between the ones that live in
DRUPAL_ROOT/core/modules
andDRUPAL_ROOT/modules
is that the first ones are supported by the core project and the secod ones are not. Lets do the same for modules with database drivers. Let them al have them the namespaceDrupal\{$module_name}\Driver\{$driver_name}
.Comment #55
alexpottSo re the module in the connection url string and not in the connection options. The module key is only a shorthand for namespace / namespace_dir because from knowing the module we can determine the two things. But we cannot do that at runtime because scanning module directories on every request would be extremely expensive. As I've said before the whole URL to represent a database connection is wrong headed and needs to be deprecated in favour of a json string in an environment variable. There's already tonnes of things that go in connection options that cannot be represented on the URL - namespace, collation, init_commands, pdo. So I disagree that it is confusing or wrong. Also the only time these URLs are needed is to setup SIMPLETEST_DB or for a CLI install (eg drush si). In both cases the more usual thing to do is to pre-populate settings.php with this info anyway. The whole underscore vs dash thing highlights the complete awkwardness of representing db connections as URLs. It feels really wrong that we're driving design decision based on a URL representation of something that is not a URL - so it looks like we're going to need the - to _ conversion thing :( ho hum.
@daffie thanks for explaining about the interdiff... makes sense now.
Re the
Drupal\{$module_name}\Driver\{$driver_name}
vsDrupal\{$module_name}\Database\Driver\{$driver_name}
as pointed out before a driver is not necessarily a database concept. I guess your concern is that the database folder is only going to have a single folder called Driver in it. I guess that's possible so maybeDrupal\{$module_name}\DatabaseDriver\{$driver_name}
is a compromise but it is possible the a module might put other things inDrupal\{$module_name}\Database
. Not sure but I am sure that we should improveDrupal\{$module_name}\Driver\{$driver_name}
. Whatever we decide will be the same for core drivers provided as modules and contrib / custom.Comment #56
daffie CreditAttribution: daffie commentedI shall rename the driver names in the driver_test module to something without underscores. Then it will pass the Drupal\Tests\Core\Database\UrlConversionTest. Will be something like "drivertestmysql" and "drivertestpgsql".
I still think that not adding the module key to the connection array is a bit awkward, but ok. I shall have to live without that being in the connection array.
For the namespace thing: I like your suggestion for
Drupal\{$module_name}\DatabaseDriver\{$driver_name}
and I will update patch for it. Happy to hear that you also want the same naming for the by core supported drivers and the contrib/custom ones.Comment #57
daffie CreditAttribution: daffie commentedComment #58
daffie CreditAttribution: daffie commentedThis patch has the following changes with the one from comment #48:
Drupal\{$module_name}\Driver\{$driver_name}
toDrupal\{$module_name}\DatabaseDriver\{$driver_name}
$driver
. This was necessary because for the drivers in the module driver_test the PDO connection name is not the same as Drupal driver name. If this change is accepted a followup needs to be created.install_database_errors()
needs the module name to run the codedb_installer_object($driver, $module)->runTasks()
.Comment #59
alexpottRe #58.3
The fixes to Drupal\Core\Database\Install\Tasks are necessary.
This is actually a bug fix. This should never have been using the
$this->pdoDriver
- fortunately the fix is BC compat (nice one) and also easy to add a deprecation to in 9.1.x and also not that important - a field that would normally be required won't be. Fun.The above docs from Drupal 9 HEAD show that the way that pdoDriver is being uses to decide whether a field is required is wrong. Doing this properly is revealing all the skeletons. So we definitely need to create the follow-up to deprecate thew BC layer.
Re #58.4 nice spot. But this also shows us that we shouldn't be using 'module' here at all. We can pass in the namespace and use that. The nice thing here is the once the namespace is required we can simplify this method and remove all the ifs :)
Comment #60
alexpottAdd now we no longer need this code... should have removed in #59.
Comment #63
alexpottFixing the tests. The database_statement_monitoring_test module is a bit dodgy and the way \Drupal\KernelTests\Core\Cache\EndOfTransactionQueriesTest::getDatabaseConnectionInfo() works to override the namespace without providing a complete driver is meh. But the fix is pretty simple - and makes the drivers provided by this module a little bit more complete. In the future we could change this module to provide complete drivers and use ability of modules to provide db drivers and make the implementation a whole less hacky.
I think we still need to deal the _ in driver name not being allowed in URL schema issue. Because I think pqsqlfallback is not as nice a namespace / driver name as pgsql_fallback. I guess we could bump that to a follow-up as it is not essential for delivering the functionality. But it is not that bad to do...
Comment #64
alexpottPatch
Comment #65
daffie CreditAttribution: daffie commentedI am assigning the issue to me, only for working on the IS. and CR.
@alexpott: What do we need to do before the framework manager review can take place?
Comment #66
daffie CreditAttribution: daffie commentedAdding the API changes to to IS.
Comment #67
daffie CreditAttribution: daffie commentedComment #68
daffie CreditAttribution: daffie commentedComment #69
daffie CreditAttribution: daffie commentedAdded a change record.
Comment #70
daffie CreditAttribution: daffie commentedIn this patch:
- Removed the now unused method Drupal\Core\Database\Database::getModuleFromNamespace(). Including in testing.
- Updated some documentation.
Comment #71
mondrakeThanks @daffie @alexpott for doing this. +1 on the approach in general. I will try to get my experimental DruDbal driver fit with this, and report back.
Comment #72
alexpottDid some more thinking about the underscore in driver name issue. It's funny because we suggest it in the docs for \Drupal\Core\Database\Connection::driver() in HEAD but actually it doesn't work at all with out "let's represent a DB connection as a URL even though its not" approach. We could go down the path of converting - to _ and vice versa when handling URLs but then what happens when someone wants a - in there driver name? (Hint; it'll break without a lot of work). As I've said before we need to ditch the URL approach anyway - it introduces hidden complexity as is only needed to pass as a parameter into site-install commands and for SIMPLETEST_DB. So I no longer think we should even try to handle it. However that doesn't mean we can't do better in the current patch in terms of setting a standard for naming database drivers.
Database drivers map to PSR-4 namespaces. PSR-4 namespaces have clear rules about how classes and namespaces are supposed to be. CamelCase. So let's use that. It makes the test driver namespace much more readable and works perfectly with our db connection as URL out-of-the-box.
I think this patch is now complete for a code perspective and now I'm going to review the issue summary and change record - thanks for working on this already @daffie
Comment #73
mondrakeI've adjusted DruDbal per #71, it used to be a driver-only codebase and now it's a module. See:
PR with changes in GitHub: https://github.com/mondrake/drudbal/pull/172/files
Test run of the driver on TravisCI with the Database group of tests: https://travis-ci.org/github/mondrake/drudbal/builds/668624989?utm_sourc...
The test also installs the site in a interactive-like mode, and that passes too.
Notes:
1. It's important to make clear you need to have a
{module}.info.yml
file in the code base, otherwise the driver is not discovered2. You need to specifiy
?module
in the DB URL for tests; also, if your querystring already has other parts, these are preserved (good!)3. Calling
db_installer_object
without the $namespace parameter on sites installed with a contrib driver leads toi.e. tries to load a class with a namespace built on D8 pattern. Not a big deal though - I encountered it in an external script, not Drupal. Also, calls to
db_installer_object
in contribland are very limited and would have to be adjusted anyway http://grep.xnddx.ru/search?text=db_installer_object&filename=RTBC for me, thanks
Comment #74
alexpottDone some formatting of the issue summary and change record - the content is great. Nice work @daffie.
Comment #75
daffie CreditAttribution: daffie commentedUpdated the IS with the requirement of the driver names from a module to be PSR-4 compatible.
Comment #76
daffie CreditAttribution: daffie commentedComment #77
alexpottWas discussing with @daffie whether we should change this to
Drupal\fakedriver\DatabaseDriver\FakeDriver
when I realised now we have the drivertest module this thing is not needed at all. It's only duplicating test coverage we already have in the patch.Less code to review ftw.
Comment #78
daffie CreditAttribution: daffie commentedNitpick in th latest patch:
We do not need to set the variable $pdoDriver here. It is inherited from its parent.
Comment #79
alexpottNice spot @daffie even less test code ftw :)
Comment #80
daffie CreditAttribution: daffie commentedUpdated the CR with the hint for the maintainers of contrib/custom database driver that they need to add a
{module}.info.yml
in order for the database driver to be discovered. Thanks @mondrake. Also for the RTBC.Comment #82
catchNot really a full review, but a couple of questions.
The test failure is a random fail so moving back to RTBC.
It's the configuration that needs to set it rather than the driver? Maybe 'need to have the ... properties set'?
nit: s/exists/exist/
Would it be worth a follow-up to add an optional interface for Tasks.php, making it required in Drupal 10?
So is this necessary for all contrib drivers now?
Comment #83
daffie CreditAttribution: daffie commentedFor #82.4: At the moment, yes. In #3113403: Make Drupal\Core\Database\Query\Condition driver overridable we made it for the added Condition class no longer necessary. We used there
return new static($conjunction);
to create a new Condition instead ofreturn new Condition($conjunction);
. See: https://git.drupalcode.org/project/drupal/-/blob/9.0.x/core/lib/Drupal/C....Comment #84
xjmThanks for the careful updates for the technical changes.
I've read the IS, CR, and release note carefully (not the patch), and it's still not clear to me whether existing drivers that don't use the new fixed location will continue to work. That's the big BC break that we would need to worry about here. Do we support both the old and new ways, or are existing drivers that are not affected by or have worked around this issue going to break? Is the old way deprecated, etc.? We should specifically address that in the release note and the beginning of the CR.
The release note should briefly state:
<a>
tag that are meaningful out of context, not just the URL)Thanks!
Comment #85
alexpottRe #82.4 - that's not something changed by this patch. It's the current state of affairs. There are some old issues where some dubious arguments are made that this situation is a good thing but it's not changed by this patch.
Comment #86
alexpottFixing #82.1 and .2
Re #83.3 well we have the abstract class \Drupal\Core\Database\Install\Tasks that all dbs currently need to extend in order to be seen in the installer. I think any discussion about interfaces in the database system needs to take in an overview of the entire API because we've got the similar classes contained in core/lib/Drupal/Core/Database/Query to deal with too... eg \Drupal\Core\Database\Query\Delete & \Drupal\Core\Database\Query\Select etc...
Re #84 Existing driver contrib / custom drivers don't have to do anything. They will continue to work as they did. If they want to take advantage of this new functionality then they can move code around and no longer require their users to copy code to
DRUPAL_ROOT/drivers
. That said we should open a follow-up to deprecate support forDRUPAL_ROOT/drivers
and remove it in Drupal 10.Comment #87
catchOK so #82.4 it is already necessary for contrib drivers, but the test drivers have been getting away without doing it?
Comment #88
alexpottI've opened a follow-up to #3123461: Deprecate support for database drivers placed in DRUPAL_ROOT/drivers
I've addressed #84 - improving the CR and release note.
As the changes in #86 are to comments and the issue management task have been addressed - back to rtbc.
Comment #89
alexpott@catch the database monitoring thing is an almighty hack - it's using the core drivers and then switching the namespace on them. It's completely wrong. But fixing that here whilst we have to copy so much of the driver feels very very painful. What it does in \Drupal\KernelTests\Core\Cache\EndOfTransactionQueriesTest::getDatabaseConnectionInfo() is completely unsupported and is probably only working because the driver names match core driver names. There's no way for the database_statement_monitoring_test module to be autoloadable at the time when the container is being built.
Comment #90
mondrakeI'm afraid this needs a reroll now after commit of #2278971: Deprecate Connection::supportsTransactions().
Comment #91
alexpottWe can move the text in default.settings.php so this applies to both 9.1.x and 9.0.x - feels like the best thing to do.
Comment #93
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #94
mradcliffeThis is amazing work!
I finally had some time to read over the latest patch. I agree with limiting the scope with the follow-up issues.
Comment #95
daffie CreditAttribution: daffie commentedComment #96
Neslee Canil Pinto@alexpott , In #91 there was a small typo error don;t, which is replaced to don't
Comment #97
mondrakeComment #98
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOverall, I like the approach of #96, so removing the "Needs framework manager review" tag.
I'd like to propose some extensive (hence the large increment.txt) but relatively superficial changes, included in this patch:
DatabaseDriver
toDriver\Database
. Because I'd like to err on the side of fewer top-level namespaces within modules than more of them. While we might not ever come up with another kind of driver other than a database driver, if we do, then I'd like us to not have to create another top-level namespace within modules for them.'namespace_dir'
connection info key to'autoload'
. Partly to avoid using an abbreviation (which we try to avoid within Drupal), and partly for nominal parity with the 'autoload' key in composer.json (though in Composer, it's a structure in order to support more features, whereas here we only need a string).getModuleNamespaceDir()
tofindDriverAutoloadDirectory()
. In particular there, "find" rather than "get" to surface the distinction between just reading the 'autoload' key vs. finding what should populate it.$including_test_drivers
parameter. I don't see why we need it. ExtensionDiscovery::scan() already checks a setting and a test runner agent, which I think should be sufficient. If this patch fails some tests, I'd like to try to fix them via those already existing mechanisms.@alexpott, @daffie: I hope you like these changes, but if not, we can adjust whatever we need to.
I also have some questions, but I'll post those in a separate comment.
Comment #99
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI have a few questions about this method:
Comment #101
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #102
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCondensed the issue summary and updated it for #98.
Comment #103
daffie CreditAttribution: daffie commented@effulgentsia: Thank you for you framework manager review and you many suggestions to make this patch better, including documentation.
For #98.1: Not what I would like most, but I can understand why you want the change. Lets do that then.
For #98.2-6: Great!
For #99: I think keeping it in a seperate method improves readability. Also for I would like to deprecate the Drupal 8 way of including database drivers and moving the by core supported database drivers to their own module. That makes this method not very useful after those changes. There are now a lot code exceptions being made in core for the by core supported database drivers that belong in the module that has the by core supported database driver.
DRUPAL_ROOT/lib
is for generic code, not for specific code for modules, themes or database drivers. This will also improve the live of contrib database drivers. They how now have to do things that the by core supported drivers do not have to do.My review:
Can we change it to:
[$first, $second, $rest]
Why are we adding the 'module' key to the database connection array? The will the key be used anywhere? It is not documented in default.settings.php.
I think we should add testing for these methods.
This change is no longer necessary. The parameter $include_test_drivers was removed.
On a number of places you have put "{" on a new line. I can point you to the Drupal coding standards, but you know them properly better then me. :)
It should be the one or the other, not both.
Comment #104
alexpottautoload
- it does what it says. Nice.For me, this is almost a reason to pass in $including_test_drivers ... also now the code in setUp is not needed. Fortunately we can achieve what we need and only affect a single test case rather than the entire test.
I'm not a huge fan of this change. I really don't like relying on case to determine whether a namespace is a module. I'd rather relax this to saying everything is a module that's not 'Core' or 'Driver'. But if we do keep the above formulation then is the Database class the right place for this? This feels like it is more like a special fact of Drupal's autoloading than something that is database specific. This needs more discussion. Leaving alone for now.
I've inlined getModuleNameFromNamespace() and getModuleRelativeNamespace() - having these as separate functions is not necessary and pollutes the Database API for little reason.
Performance really is never a worry here as parsing from a URL is never really done - as I've pointed out quite a few times on this issue. Also we should completely remove parsing from a URL and replace with a JSON string - again I've pointed this out many times. So added protected API surface here is also unwarranted.
Comment #105
alexpottRe #103
Patch attached also removes some duplicate @internal documentation.
Comment #106
alexpottForgot to actually remove Database::getDatabaseClass(). Done that now... and a little bit more clean-up of the code.
I'm debating a lot whether ww should inline \Drupal\Core\Database\Database::isWithinModuleNamespace() because the only usage outside \Drupal\Core\Database\Database::findDriverAutoloadDirectory() is in \Drupal\Core\Database\Database::getConnectionInfoAsUrl() and in D9 we should deprecation DB connections as URLs and replace them with JSON strings. Opened #3125257: Deprecate support for database connections as URLs and replace with JSON strings to do this.
Also providing a 9.1.x patch as the recent changes reverted #91. Whilst it is possible to create a patch that applies to both branches I think the location of the documentation is better so let's take the pain of rolling against multiple branches.
Comment #107
daffie CreditAttribution: daffie commentedI think something went wrong here!
Comment #108
daffie CreditAttribution: daffie commentedAdded #3125257: Deprecate support for database connections as URLs and replace with JSON strings as a followup.
Comment #109
alexpottWhoops forgot to pull locally on 9.1.x. That's why the 9.1.x patch above contains unexpected changes.
I've refactored \Drupal\Core\Database\Database::convertDbUrlToConnectionInfo() further to account for removing Database::getDatabaseClass() - there's now less change from HEAD.
Re #107 nope - that's just fine... you can dereference an array like that an ignore the first element.
Comment #110
alexpottI've updated the issue summary and the change record for the latest changes.
Comment #111
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI missed from my earlier review that marking this (and the other method) @internal is not a change. Removing it from the list of changes in the IS.
Comment #112
effulgentsia CreditAttribution: effulgentsia at Acquia commented#109 looks great to me. I like all of the improvements that are in there since #101.
I think the only feedback that hasn't been addressed yet is:
I agree with #104.1 that I should not unilaterally decide on
Driver\Database
vs.DatabaseDriver
. I'm ok with being overruled on that and changing it back toDatabaseDriver
. If we decide on that, then I'm willing to update the patch to do it, and I think it's worth getting right the first time, because once we mint a new top-level namespace for Drupal modules, it's disruptive to change later. I'm unclear on what the benefit ofDatabaseDriver
is. Is it a concern that it's premature to pretend that Driver is a generic concept, when database drivers are our only type of driver?#104.5 suggests checking based on an explicit list of non-module names (like "Core" and "Driver") rather than reserving ahead of time all non-lowercase names. And also, maybe moving this out of the Database class. I agree with moving it out of the Database class, and maybe making it a public static method in Extension (in which case, isWithinExtensionNamespace() would be the more accurate name), but wasn't sure if I wanted to add another API addition to this issue. Regardless, we do need to decide on whether to do this by casing or by an explicit list of non-module names.
Comment #113
alexpottThe only benefit of DatabaseDriver as far as I know is less directory structure. The other thing is that
Drupal\MODULE\Driver\Database
is the opposite ofDrupal\Core\Database\Driver
- which is a shame. In many ways I'm not sure it matters too much what we decide tbh. There are other types of drivers in Drupal and even in core - foro example \Drupal\FunctionalJavascriptTests\DrupalSelenium2Driver - so let's go withDrupal\MODULE\Driver\Database
.Re #112.2 given #3125257: Deprecate support for database connections as URLs and replace with JSON strings I'm more in favour of inlining the logic - less API decisions.
Comment #114
alexpottI've inlined
Database::isWithinModuleNamespace()
- if we want to add this to Extension or some other class we can do that in a follow-up.Comment #115
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIt's called from two places, which makes it less appealing to inline. Can we bring it back as a helper method but make it private and add a @todo linking to #3125476: Add an API for determining whether a given PHP namespace or class name is within the namespace of an extension?
Comment #116
daffie CreditAttribution: daffie commentedThe patch looks good to me. I have only 1 nitpick:
When I look at the data provider method and the values it returns, the default value of
$include_tests
should be TRUE, not NULL. We are now mixing up NULL and FALSE in the test method and that is a bit confusing.I understand why @effulgentsia want to put the driver directory in
Driver\Database
instead ofDatabaseDriver
. It is not my preference and I do not see any other driver types being added, but if we want to keep that option open, then it is fine by me.Comment #117
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have tried to address comment #116.
Comment #118
alexpottRe #115 - okay let's split the difference at private. I'm still not 100% convinced given I'm not sure we should add the API #3125476: Add an API for determining whether a given PHP namespace or class name is within the namespace of an extension - in an ideal world we would not have to any of this and use composer 100% for class loading. Doing
composer require drupal/MODULE
and that not affecting the class loader is very surprising to PHP developers coming from Symfony and other frameworks. But in order to fix that we have to revisit what installing a module is... so not a lot of appetite for that. But at least if this is private and we deprecate and then remove db connections as URLs we can inline this again.@daffie yeah that was bothering me too. I've improved the test to make everything a bit more obvious and less duplication.
@ravi.shankar sorry but this is an x-post.
Comment #119
daffie CreditAttribution: daffie commentedThe patch looks good to me.
All remarks are addressed.
For me there is enough testing for the added functionality.
The method
isWithinModuleNamespace()
has already indirect testing withgetConnectionInfoAsUrl()
andfindDriverAutoloadDirectory()
.The documentation in the patch is great.
All my remarks are addressed.
Added #3125476: Add an API for determining whether a given PHP namespace or class name is within the namespace of an extension as a followup.
Back to RTBC for a release manager review.
Thank you: @effulgentsia, @alexpott and ravi.shankar for working on this issue!
Comment #120
catchThis is something that would normally be minor-only, and hence for 9.1.x. However this is primarily intended to support being able to install the compatibility database drivers for Drupal 9.0.x more easily (i.e. for older versions of postgres, mysql, sqlite). While it adds a new API feature, it is not adding a lot of new API surface that any code is going to call, it's just about enabling different driver layouts and locations.
So untagging for release manager review, I think it's OK for this to go into 9.0.x. Did not do a full review of the patch for everything else yet so leaving status unchanged.
Comment #121
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI condensed the CR: https://www.drupal.org/node/3123251/revisions/view/11812745/11815140
Comment #122
effulgentsia CreditAttribution: effulgentsia at Acquia commentedAdding issue credits.
Comment #126
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThank you, for everyone's great work on this, especially @daffie and @alexpott!
As mentioned in #98, the changes I posted there were all superficial, and have either been approved by @daffie and @alexpott or changed by them since then. Therefore, I still feel eligible to commit this.
Pushed to 9.1.x and cherry picked (with minor conflict resolution) to 9.0.x and 8.9.x. The reason for 8.9.x is that we're still trying to keep that and 9.0.x as close to identical as possible, other than deprecation removals and library updates. Also, this way a contrib database driver can take advantage of the new namespace and be simultaneously compatible with 8.9 and 9.0, to facilitate an upgrade from one to the other.
Tagging for including in release notes. The IS already includes the note to add.
Comment #127
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #128
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #130
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis broke HEAD on 8.9. My mistake for not queueing a test run on that first. Reverted on that branch only, and re-uploading patch for testing on it.
Comment #131
alexpottHere's a fix for 8.9.x
Comment #133
alexpottHo hum... I didn't notice that /InstallerNonDefaultDatabaseDriverTest is using $this->container... at some point we really ought to remove that from functional tests they have no business accessing the container of the site under test.
Also fwiw I'm not 100% sold on this having to go in 8.9.x but given that this is API addition only there's no harm.
Comment #134
daffie CreditAttribution: daffie commentedThe changes looks good to me. Back to RTBC for 8.9.
Comment #135
mondrakeThis may have caused contrib db driver fail Installer tests when run on PHPUnit CLI
The fix seems straightforward as it just seems the app root is not setup. Will file a separate issue.
Comment #136
mondrakeFiled #3125987: EndOfTransactionQueriesTest fails with contrib db driver.
Comment #137
BeakerboyShould there be a
instead of just
I was using the query parameter in the sqlsrv module to pass in the database schema setting.
Comment #138
BeakerboyHere's a patch for my minor suggested improvement.
Comment #140
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed #133 to 8.9.x. Let's open a separate issue for committing #133's interdiff to 9.1 and 9.0, and a separate one for #138.
Comment #141
daffie CreditAttribution: daffie commentedI would like to thank everybody that helped with this issue. Thank you all!
Especially @alexpott and @effulgentsia for without their input this issue would never have landed. Very BIG thank you!!!
Comment #142
mondrakeHEAD PHP 7.0 testing is broken https://www.drupal.org/pift-ci-job/1645682 because
uses a syntax that was introduced in later PHP
Comment #143
daffie CreditAttribution: daffie commented@mondrake: Your testbot result is saying it is
[, $module, $module_relative_namespace] = explode('\\', $namespace, 3);
. Line 571 of /var/www/html/core/lib/Drupal/Core/Database/Database.php.Comment #144
mondrakeYeah.. I meant general use of square brackets array assignment in place of list(). It’s been introduced in PHP 7.1.
The testbot log is for Drupal 8.9 under PHP 7. It’s Drupal Core head.
Comment #145
daffie CreditAttribution: daffie commentedCreated a fix for the broken testbot for Drupal 8.9 with PHP 7.0. See: #3126923: HEAD is broken for Drupal 8.9 with PHP 7.0.