#227 | 2605284-227.patch | 26.62 KB | mondrake |
|
#227 | interdiff_223-227.txt | 3.99 KB | mondrake |
#223 | interdiff_222-223.txt | 752 bytes | mondrake |
#223 | 2605284-223.patch | 26.5 KB | mondrake |
|
#222 | 2605284-222.patch | 26.5 KB | mondrake |
|
#222 | interdiff_220-222.txt | 3.01 KB | mondrake |
#220 | interdiff_218-220.txt | 729 bytes | mondrake |
#220 | 2605284-220.patch | 24.08 KB | mondrake |
|
#218 | 2605284-218.patch | 24.08 KB | mondrake |
|
#218 | interdiff_216-218.txt | 1.59 KB | mondrake |
#215 | interdiff_214-216.txt | 587 bytes | mondrake |
#215 | 2605284-216.patch | 22.49 KB | mondrake |
|
#214 | interdiff_210-214.txt | 3.19 KB | mondrake |
#214 | 2605284-214.patch | 22.49 KB | mondrake |
|
#210 | 2605284-210.patch | 23.82 KB | mondrake |
|
#210 | interdiff_207-210.txt | 7.07 KB | mondrake |
#207 | 2605284-207.patch | 21.44 KB | mondrake |
|
#207 | interdiff_205-207.txt | 4.93 KB | mondrake |
#205 | interdiff_202-205.txt | 5.77 KB | mondrake |
#205 | 2605284-205.patch | 19.49 KB | mondrake |
|
#202 | 2605284-202.patch | 18.57 KB | mondrake |
|
#197 | interdiff_194-197.txt | 2.75 KB | mondrake |
#197 | 2605284-197.patch | 18.55 KB | mondrake |
|
#194 | 2605284-194.patch | 20.3 KB | mondrake |
|
#194 | interdiff_193-194.txt | 729 bytes | mondrake |
#193 | 2605284-193.patch | 20.3 KB | mondrake |
|
#193 | interdiff_191-193.txt | 1.36 KB | mondrake |
#191 | interdiff_188-191.txt | 8.76 KB | mondrake |
#191 | 2605284-191.patch | 19.92 KB | mondrake |
|
#188 | 2605284-188.patch | 16.2 KB | mondrake |
|
#182 | 2605284-182.patch | 42.41 KB | mondrake |
|
#182 | interdiff_167-182.txt | 2.57 KB | mondrake |
#169 | interdiff_146-167.txt | 16.6 KB | mondrake |
#167 | interdiff_164-167.txt | 722 bytes | mondrake |
#167 | 2605284-167.patch | 42.61 KB | mondrake |
|
#164 | interdiff_163-164.txt | 776 bytes | mondrake |
#164 | 2605284-164.patch | 42.56 KB | mondrake |
|
#163 | 2605284-163.patch | 41.8 KB | mondrake |
|
#163 | interdiff_161-163.txt | 4.59 KB | mondrake |
#161 | interdiff_159-161.txt | 4.55 KB | mondrake |
#161 | 2605284-161.patch | 44.25 KB | mondrake |
|
#159 | interdiff_146-159.txt | 15.92 KB | mondrake |
#159 | 2605284-159.patch | 43.34 KB | mondrake |
|
#154 | 2605284-154.patch | 42.91 KB | mondrake |
|
#154 | interdiff_146-154.txt | 1.18 KB | mondrake |
#146 | interdiff_143-146.txt | 4.62 KB | mondrake |
#146 | 2605284-146.patch | 40.8 KB | mondrake |
- 8.5.x:
- PHP 7 & MySQL 5.5 22,981 pass
- PHP 5.5 & MySQL 5.5 21,058 pass
- PHP 5.5 & PostgreSQL 9.1 21,056 pass
- PHP 5.5 & SQLite 3.8 21,055 pass
- PHP 5.6 & MySQL 5.5 21,060 pass
- PHP 5.6 & PostgreSQL 9.1 21,058 pass
- PHP 7.1 & MySQL 5.5 22,936 pass
- PHP 7.2 & MySQL 5.5 23,274 pass
- PHP 7 & PostgreSQL 9.1 22,965 pass
- PHP 7 & SQLite 3.14 22,961 pass
|
#143 | 2605284-143.patch | 42.37 KB | mondrake |
|
#143 | interdiff_137-143.txt | 3.21 KB | mondrake |
#138 | 2605284-137.patch | 41.44 KB | mondrake |
|
#138 | interdiff_136-137.txt | 1.02 KB | mondrake |
#137 | 2605284-136.patch | 41.44 KB | mondrake |
|
#137 | interdiff_134-136.txt | 9.77 KB | mondrake |
#134 | 2605284-134.patch | 40.51 KB | mondrake |
|
#132 | 2605284-132.patch | 40.56 KB | mondrake |
|
#132 | interdiff_129-132.txt | 8.63 KB | mondrake |
#130 | interdiff_128-129.txt | 17.46 KB | mondrake |
#130 | 2605284-129.patch | 35.57 KB | mondrake |
|
#128 | 2605284-128.patch | 32.92 KB | mondrake |
|
#128 | interdiff_121-128.txt | 2.86 KB | mondrake |
#121 | 2605284-121.patch | 29.76 KB | jofitz |
|
#121 | interdiff-119-121.txt | 4.27 KB | jofitz |
#119 | 2605284-119.patch | 29.45 KB | jofitz |
|
#119 | interdiff-116-119.txt | 1.38 KB | jofitz |
#116 | simpletest_is_broken_on-2605284-115.patch | 29.92 KB | david_garcia |
|
#107 | simpletest_is_broken_on-2605284-107.patch | 23.18 KB | david_garcia |
|
#88 | interdiff-81-89.txt | 621 bytes | david_garcia |
#88 | simpletest_is_broken_on-2605284-89.patch | 22.53 KB | david_garcia |
|
#81 | simpletest_is_broken_on-2605284-81.patch | 22.47 KB | jofitz |
|
#81 | interdiff-78-81.txt | 1.1 KB | jofitz |
#78 | simpletest_is_broken_on-2605284-78.patch | 23.04 KB | jofitz |
|
#78 | interdiff-75-78.txt | 1.58 KB | jofitz |
#75 | simpletest_is_broken_on-2605284-75.patch | 24.14 KB | GoZ |
|
#75 | interdiff-2605284-73-75.txt | 299 bytes | GoZ |
#73 | simpletest_is_broken_on-2605284-73.patch | 23.9 KB | GoZ |
|
#69 | interdiff-61-69.txt | 573 bytes | david_garcia |
#69 | simpletest_is_broken_on-2605284-69.patch | 23.28 KB | david_garcia |
|
#62 | interdiff-56-61.txt | 1.35 KB | david_garcia |
#61 | simpletest_is_broken_on-2605284-61.patch | 23.28 KB | david_garcia |
|
#59 | interdiff-55-56.txt | 1.3 KB | david_garcia |
#57 | simpletest_is_broken_on-2605284-56.patch | 23.98 KB | david_garcia |
|
#55 | interdiff-49-56.txt | 3.79 KB | david_garcia |
#55 | simpletest_is_broken_on-2605284-55.patch | 23.59 KB | david_garcia |
|
#50 | interdiff-46-49.txt | 1.15 KB | david_garcia |
#49 | interdiff-46-49.patch | 20.75 KB | david_garcia |
|
#49 | simpletest_is_broken_on-2605284-49.patch | 20.67 KB | david_garcia |
|
#46 | simpletest_is_broken_on-2605284-46.patch | 20.75 KB | david_garcia |
|
#45 | simpletest_is_broken_on-2605284-45.patch | 21.33 KB | david_garcia |
|
#42 | simpletest_is_broken_on-2605284-42.patch | 20.51 KB | david_garcia |
|
#39 | simpletest_is_broken_on-2605284-39.patch | 18.33 KB | david_garcia |
|
#39 | simpletest_is_broken_on-2605284-38.patch | 18.33 KB | david_garcia |
|
#38 | simpletest_is_broken_on-2605284-38.patch | 18.24 KB | david_garcia |
|
#27 | interdiff-25-27.diff | 684 bytes | GoZ |
|
#27 | interdiff-15-27.txt | 10.05 KB | GoZ |
#27 | simpletest_is_broken_on-2605284-27.patch | 26.25 KB | GoZ |
|
#25 | interdiff-15-25.txt | 10.05 KB | GoZ |
#25 | simpletest_is_broken_on-2605284-25.patch | 26.25 KB | GoZ |
|
#22 | 2605284-22.patch | 53.1 KB | Pradnya Pingat |
|
#15 | 2605284-15.patch | 22.8 KB | alexpott |
|
#15 | 13-15-interdiff.txt | 3.94 KB | alexpott |
#13 | 2605284-13.patch | 10.45 KB | alexpott |
|
#9 | 2605284-broken-simpletest.patch | 2.14 KB | david_garcia |
|
#7 | 2605284-broken-simpletest.patch | 2.82 KB | david_garcia |
|
#4 | 2605284-broken-simpletest.patch | 2.14 KB | david_garcia |
|
#3 | 2605284-broken-simpletest.patch | 1.16 KB | david_garcia |
|
Comments
Comment #2
david_garcia CreditAttribution: david_garcia commentedSo the issue is here:
There seems to be custom logic for Windows regarding the phpunit path. But the generated path points to a non existent file.
Comment #3
david_garcia CreditAttribution: david_garcia commentedThis doesn't really fix the issue, just to make sure it passes tests.
Comment #4
david_garcia CreditAttribution: david_garcia commentedAs if the path was the only broken thing... Database::openConnection() is doing quite a crap job trying to find out the connection class for a driver, combined with the mess inside install's .inc (core/include/install.inc):
drupal_get_database_types()
drupal_detect_database_types()
And of course, int SiteSettingsForm.php this is the elegant and straightforward way of determining the namespace for a driver:
Probably for a follow-up issue, driver detection and namespacing should be reviewed and moved to the Database class.
Embeding the namespace in the connection URL as a query string is just a quick workaround, Drupal should be able to - given a driver name - be able to tell what Connection class it is going to use. I tried to implement it but too many things needed to be changed.
Comment #5
david_garcia CreditAttribution: david_garcia commentedComment #6
david_garcia CreditAttribution: david_garcia commentedComment #7
david_garcia CreditAttribution: david_garcia commentedAnd when exceptions are thrown that does not necessarily mean that there is nothing meaningful that we can show in the test results...
Comment #9
david_garcia CreditAttribution: david_garcia commentedOk then, turn this 3 bug combo into just a 2 bug combo.
Comment #10
david_garcia CreditAttribution: david_garcia commentedComment #11
alexpottDiscussed with @catch and @xjm - we agreed that the that part of this issue about running tests on contributed database drivers is a major bug.
This is problematic... since namespace is SqlServer thing - other databases could have other requirements. Actually I think the bug here is that we should call the method on the selected database driver and not on the abstract base class. Then each db can do it's own thing.
I guess this is an unrelated change to help debugging? If so can this be filed as a separate issue.
This should be in a separate issue. Given that phpunit can be run from the command line I think this is a just a normal bug.
Comment #12
alexpottSo part of #11.1 is wrong - namespace is not an SqlServer concept - it is the namespace of the driver class - which is a Drupal concept. But the point still stands. We need to be able to defer to the real database driver class in these functions.
Comment #13
alexpottHere's a patch that allows each database driver to do custom stuff to the URI <-> connection options conversions. And because SQLite is different in core it uses this.
Comment #15
alexpottFixed up the test fails.
Comment #16
david_garcia CreditAttribution: david_garcia commentedYep I tried to stuff a 3 bug combo into a single patch. Just being practial.
I'll give this a decent review when I get some time. Your patch is touching more stuff than mine did, but for good :)
Comment #19
daffie CreditAttribution: daffie commentedA quick review:
Nitpick: No black line between the two.
Is this necessary? Basic object oriented coding stuff.
I see the problem. But I do think that internal function should be protected.
The class GuzzleHttp\Psr7\Uri implements the interface Psr\Http\Message\UriInterface. Should we not use the interface for parameter typehinting.
Can we rewrite this to:
There are two getHost()'s. One should be getSchema().
There is no docblock and no @covers data.
Nitpick: We do not do this any more.
Nitpick: No new line.
Comment #20
daffie CreditAttribution: daffie commentedComment #21
Pradnya Pingat CreditAttribution: Pradnya Pingat at Blisstering Solutions commentedI am working on rerolling patch
Comment #22
Pradnya Pingat CreditAttribution: Pradnya Pingat at Blisstering Solutions commentedI fixed the issues according to comment #19 except point no 2 and 3 , need more explanation about that.
Thanks
Comment #24
mradcliffeThank you for contributing the patch, @Pradnya Pingat.
The patch in #22 makes many coding standard changes. You may want to confirm your IDE or text editor is configured for the Drupal coding standards. The child pages linked at the bottom of the Development tools page may help as well.
Comment #25
GoZ CreditAttribution: GoZ at Centarro commentedRewrite #22 fixing coding standard issues. I only fix those added by #22.
I think a big clean should be done on all this files but it's not the purpose of this issue.
I agree with #19 2., so i remove this part of comment.
For #19 3. I understand why alexpott put this @internal comment and i think we should keep it, so contributor know this method should not be called directly.
Comment #27
GoZ CreditAttribution: GoZ at Centarro commentedComment #29
GoZ CreditAttribution: GoZ at Centarro commentedI cancel test for interdiff. Sorry for that
Comment #31
daffie CreditAttribution: daffie commentedIt looks good. Got 2 points left:
I only can do a code review because I have no windows PC.
Comment #32
GoZ CreditAttribution: GoZ at Centarro commentedError typing, $connection_opptions should be $connection_options.
Good question, i rename it, but it seems git diff make them separated as delete and create.
Interdiff make some strange things to. It the first time i use interdiff, i usually use git diff between to patched branchs, but i cannot apply alexpott patch. May be i miss something.
I don't understand what you want to do with Interface.
Comment #33
daffie CreditAttribution: daffie commented@GoZ: I think you get the git diff rename with the commend
git diff -C -M
orgit diff -C -M HEAD
. And a good find with connection_opption.In Drupal we like to use an interface for typehinting, so that we can change the implementing class with an other one without having to do an API change. We now have only to make sure that the replacing class implements the interface (Psr\Http\Message\UriInterface).
We would like to use the class GuzzleHttp\Psr7\Uri. And if we look at that class we see that that class is implementing the interface Psr\Http\Message\UriInterface. So the code change that I am suggesting is:
Comment #34
GoZ CreditAttribution: GoZ at Centarro commentedTrying to figure out why tests fail, here is what i found :
Test fail in KernelTestBaseTest::testGetDatabaseConnectionInfoWithOutManualSetDbUrl() comparing prefix database connection.
In this test, default prefix from connectionOptions is composed of two concatenated prefixes. Example : simpletest123456simpletest987654. This concatenation is made by KernelTestBase::getDatabaseConnectionInfo().
Before this patch, in KernelTestBase::getDatabaseConnectionInfo(), default prefix values was empty. It was empty because Database::convertDbUrlToConnectionInfo() never set prefix (get it in $info variable but never put it in $database).
Before patch:
With the patch, prefix is setted.
After patch:
We should see this issue with someone who implemented KernelTestBase and Database class (see #2232861: Create BrowserTestBase for web-testing on top of Mink and #2304461: KernelTestBaseTNG™) or with someone who can tell if:
Comment #36
david_garcia CreditAttribution: david_garcia commentedComment #37
david_garcia CreditAttribution: david_garcia commentedComment #38
david_garcia CreditAttribution: david_garcia commentedRerolled as it did not apply anymore.
Comment #39
david_garcia CreditAttribution: david_garcia commentedFixes issues from #33.
Comment #42
david_garcia CreditAttribution: david_garcia commentedFixes some tests.
Comment #43
david_garcia CreditAttribution: david_garcia commentedI am wondering if we should refactor the installer to move driver discovery into a method:
And instead of storing the "namespace" in the URL representing the connection, have the convertDbUrlToConnectionInfo() method look for the driver name inside the filesystem in the same way it does during the install process.
And methods like this one look horribly bad to me:
But looking into this might be out of scope...
Comment #45
david_garcia CreditAttribution: david_garcia commentedThis fixes the issue described in #34.
I'm not sure what the test is really testing, but given that this is a simpletest nested call (simpletest UI being tested from a simpletest itself) I believe we whould be getting nested database prefixes. So updated the test acoordingly.
Comment #46
david_garcia CreditAttribution: david_garcia commentedAnd this should yield a green light.
Comment #48
GoZ CreditAttribution: GoZ at Centarro commentedJust a little thing in comments, replace 'a absolute path' by 'an absolute path'.
Sorry for this insignificant review :/
Everything else looks OK. Good job david_garcia
Comment #49
david_garcia CreditAttribution: david_garcia commentedComment #50
david_garcia CreditAttribution: david_garcia commentedFirst attempts at an interdiff ever :(
Comment #51
GoZ CreditAttribution: GoZ at Centarro commented@david_garcia, can you cancel test on interdiff file please ?
I miss the document comment. You are right
Comment #53
GoZ CreditAttribution: GoZ at Centarro commentedRe-launch tests for #49 fail. Test fails due to random javascript fail see #2837676: Provide a better way to validate all javascript activity is completed
Comment #54
GoZ CreditAttribution: GoZ at Centarro commentedTests fail with SQLite
Comment #55
david_garcia CreditAttribution: david_garcia commentedLet's try with this.
Comment #56
jofitz CreditAttribution: jofitz at ComputerMinds commentedNo longer appears to require reroll - tag removed.
Comment #57
david_garcia CreditAttribution: david_garcia commentedLet's try again...
Comment #58
GoZ CreditAttribution: GoZ at Centarro commented@david_garcia Can you add interdiff please so we can see difference between both patchs ? thanks
Comment #59
david_garcia CreditAttribution: david_garcia commentedThis was it.
So doing a little summary:
UrlToConnectionInfo() and ConnectionInfoToUrl() should deal with the prefix. Prefix is an important part to identify an installation/connection string.
In the case of tests, connection info is passed between installs using it's URL form and though the prefix is "not needed" (because each test environment will generate it's own prefix and override whatever prefix comes from the upstream environment), the essence of these methods require the prefix to be part of the information sent back and forth.
My assumptions in #45 were wrong. Nested tests should not have nested database prefixes, as each test environment generates it's own and unique database prefix, hence this change:
The failing tests in #55 were due to KernelTestBaseTest.php assuming $this->databasePrefix was the real prefix being used (which SHOULD be) but the lines I just copied above this text broke that completely now that the prefixes are properly embeded into the URL generated by getConnectionInfoAsUrl:
Because in this case, $this->databasePrefix() is generated in such a way that it is unique and will ensure no leakage happens between tests, or main site and tests.
Test coverage has also been expanded in the final patch to ensure that the convertConnectionInfoToUrl() and convertDbUrlToConnectionInfo() properly work back and forth, including namespace support.
Comment #60
daffie CreditAttribution: daffie commentedThe patch looks good. I do have some remarks:
Is this really necessary! With your comment in #59 can we try the patch without this added.
Can we use the methods withScheme(), withHost() and withPath().
@david_garcia: You are the maintainer for the Microsoft SQLServer. Can you confirm that this patch works for the Microsoft SQLServer driver? And what namespace do you use?
Comment #61
david_garcia CreditAttribution: david_garcia commentedDepending on how this performs on the tests I'll expand my comments.
Comment #62
david_garcia CreditAttribution: david_garcia commentedTrue, this was not needed anymore.
Done.
https://ci.appveyor.com/project/david-garcia-garcia/sqlsrv/build/1.0.53
Other things are broken, but the test suite itself is working.
It's not that I pick a namespace... the way Drupal Core is designed contributed drivers are forced to be place into "Drupal\\Driver\\Database\\contribdrivername"
In the SQL Server case it's "Drupal\\Driver\\Database\\sqlsrv".
The current approach breaks a composer based workflow (you can workaround it with a few xcopy commands in post-install scripts), that is another matter.
Comment #63
daffie CreditAttribution: daffie commentedFor me the patch is almost RTBC. I do have one issue left:
Why do we not remove
$uri =
part? So we will get:@david_garcia: Can you elaborate on what "Other things are broken". I have written a comment #2209307-47: Convert database drivers into a regular extension type and I was wondering what you think about it. And thank you for your interdiff file!
Comment #64
david_garcia CreditAttribution: david_garcia commentedBecause these methods do not modify the original URI object, but return a new URI object with the requested changes.
I linked to an appveyour CI for the sqlsrv driver (which I'm just setting up) and there are lots of tests failures, mostly due to the build script still having some flaws. It's better now than before, but still too many failures.
Will post my comments there.
Comment #65
daffie CreditAttribution: daffie commentedWith the information from the previous comment, the patch is for me RTBC.
I do not have a contributed driver to test with and @david_garcia claims/proves that the patch works for MS SQL server driver.
The code looks good to me.
Comment #67
catchThis looks good to me (just not sure on the Helper suffix to the new methods on Drupal\Core\Database\Connection), but I think it'd be good to get review from a database subsystem maintainer, so tagging for that. Moving to database since the more serious changes are there. Getting driver-specific logic into drivers is always good.
Comment #68
xjmNR #67. Thanks @catch!
Comment #69
david_garcia CreditAttribution: david_garcia commentedUsing this patch on a project that has an updated version of Guzzle/Psr7, it crashes when calling ->withPath():
Here's the fix.
Comment #70
daffie CreditAttribution: daffie commentedNow that @Crell is gone and @David_Strauss has not been seen in the database issue queue for months, am I removing the tag "Needs subsystem maintainer review" and putting it back to RTBC.
Comment #72
daffie CreditAttribution: daffie commentedComment #73
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedReroll patch
Comment #75
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedMiss to rename stubConnection.php to Connection.php
Comment #76
david_garcia CreditAttribution: david_garcia commentedBack to RBTC
Comment #77
alexpottThese changes, whilst correct, are out-of-scope. The @file change is especially problematic as this is supposed to be checked by phpcs but apparently the sniff is broken in some way. We need to find out why (it's because there is one than one class in this file). So we need to file an issue against the coder module and also against core to fix it. Fixing it in core might involve just move the test class to its own file like it should be in strict psr-0/4.
If we're chagning this (and as there are other things to do) Let's do
Connection::class
instead of the hard-coded fully-qualified-classname.Comment #78
jofitz CreditAttribution: jofitz at ComputerMinds commentedReplaced
'Drupal\Tests\Core\Database\Stub\Connection'
withConnection::class
as per 77.2.Comment #79
jofitz CreditAttribution: jofitz at ComputerMinds commentedSetting back to Needs Work for 77.1, I tried to open a new ticket against core, but couldn't quite put the problems into words, sorry.
Comment #80
daffie CreditAttribution: daffie commentedCan somebody remove the changes from the patch as @alexpott wants in comment #77.1.
Comment #81
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-instated @file. Reverted out-of-scope code-style correction.
Comment #82
daffie CreditAttribution: daffie commented@Jo Fitzgerald: Thanks.
Comment #83
daffie CreditAttribution: daffie commentedAll points of @alexpott are addressed.
Back to RTBC.
Comment #84
alexpottThis change is odd - how come? Reading back... the answer is in #34. Looks good and makes sense.
Sorry I've realised that there is more work we need - the issue summary doesn't really contain the arrived at solution and we need a CR to tell alternate DB drivers how to be updated for this change.
Comment #85
david_garcia CreditAttribution: david_garcia commented#84 The change was discussed #59.
What happens here is that nested tests generate their own unique prefixes for subsites (at some point in the tests there's a 2nd level nesting), so there is no need to propagate prefixes. Can't recall exactly why, but the additional $value['prefix']['default'] simply made no sense, and did not match the behaviour of KernelTestBase expecting the test ID to be uniquely generated without reference to the parent test.
This is not always easy...
Comment #86
dawehnerAm I the only one who is a bit confused about adding a HTTP library as dependency for a database subsystem?
I'm wondering where we could document this general capability.
I like checking for misconfigured database connections here. This allows people to debug failures much easier than when the system breaks somewhere later.
We could typehint with array.
... It'd be nice to document why
getConnectionInfo
could fail here.Well, right, it requires you to read the patch and the issue :) I did that now, so here is my approach to it.
Is it just me that "helper" as a method name is kind of not descriptive?
Comment #87
mondrake1. Tested the patch in #81 in conjuction with an experimental driver for Doctrine DBAL https://github.com/mondrake/drudbal. Works fine, thank you.
2. In the driver's code, added overrides for
Connection::getConnectionInfoAsUrlHelper
and forConnection::convertDbUrlToConnectionInfoHelper
, in order to add an additional part to the URL querystring. It also work fine. Added this below as an example for a CR if you believe relevant.Note: if you use the URL in the
phpunit.xml
file to set the db environment for testing, and the driver adds parts to the querystring, you have to manually convert the querystring separators&
to HTML entities counterparts&
. So, if you getfrom call to
\Drupal\Core\Database\Database::getConnectionInfoAsUrl
, yourphpunit.xml
entry should be something like<env name="SIMPLETEST_DB" value="dbal://username:password@host:port/database?namespace=Drupal%5CDriver%5CDatabase%5Cdbal&dbal_driver=pdo_mysql#prefix"/>
3. While trying to get a clean run of
phpunit --group Database
with the driver, I incurred in a couple of issues that IMO need change in core:a) Contrib/custom database drivers are not in the
Drupal\Core\Database
namespace, and in this case Log::findCaller will not report the query caller correctly. Filed #2867788: Log::findCaller fails to report the correct caller function with non-core drivers. for that.b) If a database driver (contrib) is not implementing a PDO connection as Connection::$connection, the ConnectionUnitTest::testConnectionOpen test fails because it expects that the connction is PDO and PDO methods are available. Filed #2867700: ConnectionUnitTest::testConnectionOpen fails if the driver is not implementing a PDO connection for that.
Comment #88
david_garcia CreditAttribution: david_garcia commentedAbout comments in #86
We use URI's to move around connection info between installs (convertConnectionInfoToUrl and convertDbUrlToConnectionInfoHelper).
Having tools to reliably manipulate this URI's requires an HTTP library...
Well... passing around connection url's is something very specific to the testing framework (don't see where else it would/could be used). Not sure if documenting this somewhere would really have any impact.
Done.
Comment #89
dawehnerCall me weird, but if you don't know whether it has an impact than it certainly has one :) You know that kind of stuff already, but what if someone stumbles upon on. They wonder why its there etc. The amount of hidden costs in maintaining software is a LOT.
Comment #90
david_garcia CreditAttribution: david_garcia commentedBut this is nothing to be dealt with in the patch itself right? I mean, this is for the docs in drupal.org or something similar.
Comment #91
mondrakeAdding a related issue.
Does anybody know is there any Drush issue to allow
Drush si
with a --db-url option that will be according to the changes in this issue? ATM Drush can only cope with core db drivers AFAICSComment #92
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI fully agree with @dawehner in #86.1, it is very weird to see a HTTP library in the DB layer. Why can't we continue to use
parse_url()
and send around the array that results from it?Comment #93
david_garcia CreditAttribution: david_garcia commentedBecause it is way more robust to use the URI builder than dealing with magic arrays. I.E. the bug in #69 only showed up thanks to using such a library, that ensures that URI's are well formed. This makes code more maintainable and error-proof.
Drush will not work with contributed database drivers. Try with the Drupal Console project, it is way more robust, properly designed and coded, etc. I can confirm it does support contributed database drivers for stuff such as installing a site through console.
Comment #94
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@david_garcia, the interdiff from #69 shows, to me at least, that the patch simply ported some code incorrectly:
One of the goals of the database layer is to be made available as a generic PHP component outside of Drupal, and, IMO, adding a dependency on a HTTP library hinders that effort.
Comment #95
mondrakeNice, thank you. I tried it to establish a test webserver on Travis via
drupal server
, works fine. Still, it would be nice to supportdrupal site:install
with a kind of--db-url
option after this patch is in, it would simplify installation for testing purposes (it would be the same info passed on to PHPUnit in theSIMPLETEST_DB
variable for Kernel tests).Comment #96
david_garcia CreditAttribution: david_garcia commentedYes, that was the original intention many years ago (https://github.com/Crell/DBTNG).
But given the state of PHP, the state of Drupal, and how things have evolved, IMHO it makes little to no sense to still hold such a statement.
Comment #97
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedI'm not sure what are the states you are referring to, but #1826054: [Meta] Expose Drupal Components outside of Drupal is still open and actively (more or less) discussed.
Comment #98
david_garcia CreditAttribution: david_garcia commented@amateescu I believe this discussion is moving out of scope (my fault). Let's focus on getting this done.
The fact that it depends on another library does not mean that it cannot be isolated and distributed outside of Drupal. Ultimately, we are moving everything to composer based packages, and packages can perfectly depend on other packages.
Comment #99
mondrakeAdding a related issue.
Comment #100
david_garcia CreditAttribution: david_garcia commentedComment #101
david_garcia CreditAttribution: david_garcia commentedChanging back to RTBC, last comments are mostly nitpicks and this has had extensive review and testing, including test coverage.
This issue sort of blocks any contributed database backend work as it prevents Drupal's testing system to work with them.
#2877583: [Meta] Remove database specific logic from core
And most important, the inability of Drupal to scale (without 100% relying on caches) due to the current limited ability to deal with database transactions:
#2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate
Comment #102
dawehnerI think a database system maintainer should be asked in this case of conflict.
Comment #103
dawehnerI doubt this can be RTBC with "needs change record" and "needs issue summary update"
Comment #104
david_garcia CreditAttribution: david_garcia commentedThe issue summary was updated a while ago, but no one removed the "needs issue summary update" :(
I did my best attempt to add a through change record to the issue summary (this also helps understand what changes have been made in the patch).
There seems to be little (or none) engagement/dedication towards the database abstraction layer. And that is worrying because this is a key component to Drupal :(
See #70:
Comment #105
dawehnerComment #106
mondrakeAdded a draft Change record https://www.drupal.org/node/2896416, editing from @david_garcia proposal in the IS, and adding some of my points from #87.
This patch is a must have if we want to allow any proper testing of contributed/custom database drivers. An experimental Drupal database driver for Doctrine DBAL has been using the patch in #88 for Travis testing for a couple of months with no problems. Just setting a different environment variable for SIMPLETEST_URL allows to identify the combination of database platform/database driver to execute the tests with (see https://travis-ci.org/mondrake/drudbal).
With regards to discussion about adding dependency from guzzlehttp/psr7 to manipulate URI's, I can only say I found very handy to have its methods available to be able to add an additional URL querystring parameter without having to write additional code (see the example in the draft CR).
Launched a retest on #88 yesterday, it came back green. RTBC from my point of view.
Comment #107
david_garcia CreditAttribution: david_garcia commentedLast 8.3.x path needed a reroll.
Comment #108
mondrakeNot sure this can make into 8.3.x, but the 8.4.x failures of the patch in #88 seem related to a commit of #2891911: Random fail in Drupal\Tests\locale\Functional\LocaleTranslationUiTest::testStringTranslation that was reversed. Triggered new tests.
Comment #109
david_garcia CreditAttribution: david_garcia commentedI know it won't make it, but I need the patch for the CI of several projects.
Hope this gets in before 8.4.0 release.
Comment #110
dawehnerApplying patches is surprisingly easy these days with composer :) I think we should really not worry anymore, when a specific patch goes in, as long it does at some point.
Comment #111
larowlanShould we be using ltrim here instead? Presence of
isAbsolutePathReference
on Uri class suggests it doesn't always start with a leading / ?#77.1 references out of scope changes, these still exist in the current patch.
#67 asks for subsystem maintainer review, which hasn't happened yet - pinged david-strauss via his contact form.
Let's give him a week or two to review. If someone is willing to put their hand up to become database subsystem co-maintainer, that'd be great - please open an issue.
Comment #112
david_garcia CreditAttribution: david_garcia commentedNo please don't.
Complex patches that have to be rerolled over and over again exhaust contributors.
Comment #113
David StraussI'm reviewing this issue at the request of Lee (larowlan).
I'm not a fan of calling it just "namespace." As we can see from Alex's comment (#11), doing so can create confusion. I would at least change it to "php-namespace" or "driver-namespace" if we take the approach of putting it into the query string.
What I'd prefer, though, is moving the driver specification to the scheme. Specifying the Drupal-level driver should imply the underlying (PHP extension-level) driver. Valid characters in a scheme include ".", "-", and "+". Using one of those instead of backslashes (probably periods) would allow specifying the PHP namespace within the scheme. We could abbreviate common namespaces to avoid becoming too verbose in the scheme (if there's objection to a lengthy scheme). Seeing the PHP namespace written in the query string with lots of URI escaping (for the backslashes) is ugly, anyway.
I don't have a strong opinion on adding a dependency on Guzzle for URI parsing, especially because I can't find (via Ctrl-F) any explanation on this issue why it's preferable over parse_url(). I imagine it's because the "namespace" query parameter needs to get parsed out, and parse_url() only returns the whole query string. If so, then that's even more reason to put the driver namespace into the scheme.
Comment #114
david_garcia CreditAttribution: david_garcia commentedThis was already there. Changing the naming would either break BC, or create an inconsistent usage where in some places we use "namespace" (for BC) and others "driver-namespaces".
What I'd prefer, though, is moving the driver specification to the scheme. Specifying the Drupal-level driver should imply the underlying (PHP extension-level) driver. Valid characters in a scheme include ".", "-", and "+".
Another possibility is not to pass around namespaces at all in the URI. There is a function somewhere in the installer that does database driver discovery. We could abstract that function and use it during the ConnectionURL to ConnectionArray conversion to obtain the actual driver's namespace:
Maybe moving that away from the Installer and into something more "core".
Comment #116
david_garcia CreditAttribution: david_garcia commentedLet's see how the test runner behaves with this new approach. Namespaces are not passed around anymore.
All driver discovery logic has been centralized in a component for re-usability.
Comment #117
dawehner@david_garcia Please post interdiffs, yes, using patch files is really antique, but at least let's make it not harder than it could be.
Comment #119
jofitz CreditAttribution: jofitz at ComputerMinds commentedMany of the test failures were due to a call to file_scan_directory() so I have replaced that with a (over-?)simplified version of the function. Let's see what this gives us...
Comment #121
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #122
mondrakeThanks a lot! This runs fine with a contrib db driver too: https://travis-ci.org/mondrake/drudbal/builds/318605474?utm_source=githu...
We are deprecating 3 functions in
install.inc
now, and no longer passing the 'namespace' in the querystring. I think we need to update both the IS and the CR with this evolution.AFAIK, we need a
@see
referencing the CR, a@trigger_error
at the beginning of each method repeating the deprecation information, addition of silenced deprecation messages to the list inDrupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations
, and a follow-up to remove the deprecated usages (yes, a lot of stuff:()dirname(dirname(dirname(dirname(dirname(__DIR__)))))
is ugly... can we just change the discoveryLocations to something like'Drupal\\Core\\Database\\Driver' => '../../../../..core/lib/Drupal/Core/Database/Driver'
, and dropdrupalRoot
altogether?I think we should keep this since there are 2 classes in this file, and according to CS only single-class php files can miss the annotation
Comment #123
mondrakeWorking on update of IS and CR.
Comment #124
mondrakeComment #125
mondrakeUpdated IS and CR.
Just realized... do we need separate tests for the new
DatabaseDriverDiscovery
class?Comment #126
daffie CreditAttribution: daffie commentedI reviewed the patch. I am sorry for being a nitpick.
Why do we need the second parameter? Its not used in the method. Remove?
Update: I found that it being used by SQLite. Should we document that here?
Not sure if we need to do something if the value for
$user
is empty. Do we have testing for this?This method will never return
$this
.Nit. Can we change the variable name
$user
to$username
, because that is the name we use in the Drupal DB layer.The parameter value "default" can be removed. It is the default parameter value.
The parameter name should be
$connection_options
and not$database
.Nit. Can be put on one line.
Nit. "THE" custom namespace to use.
There is no documentation in the docblock for this exception.
Nit. This empty line can be removed.
Again. There is no documentation in the docblock for this exception.
I am sorry, but I do not understand this line. What is a valid PHP identifier?
Should this line not be written as
while (($driverName = readdir($handle)) !== FALSE) {
Not sure about this. If somebody wants to make a better/faster/improved version of say for instance the Postgres driver, then that is not possible.
Can we add some documentation about what the value can and/or should be. It is a bit vague to me.
Missing documentation about the possible exception that can be thrown. Also the return value is not documented. This is a complicated array. Lots of documentation for this please.
Can we move this line to just before the line with:
foreach ($files as $driver => $namespace) {
.Comment #127
mondrakeI'll be working on #122, #125 and #126
Comment #128
mondrakeThis is adding only the deprecation triggers per #122.1. Running a test to see if they pop up as expected, will silence in next patch.
Comment #129
mondrakeThis patch is
DatabaseDriverDiscovery
class - include renaming the methods to something more understandable and specific to what they are doing (the function names in install.inc are misleading)Database::getDatabaseConnectionClass
in favour of a more genericDatabase::getDatabaseDriverNamespace
that can be used also for other purposes (for example, in #2867788: Log::findCaller fails to report the correct caller function with non-core drivers.).With regards to #126.14 - yes, you can write an alternative db driver for a db already covered by a core driver - but you have to specify its name differently otherwise you'll get namespace clashes.
This addresses #122 and #126.7/12/13/14/15/16/17/18.
Left out: #125 (tests) and #126.1-6/8-11
Comment #130
mondrakethe patch...
Comment #132
mondrakeThis adds tests for the
DatabaseDriverDiscovery
class. The 'Stub' database driver in the Database unit test directory is expanded wit a stub installer too, so we can test its discovery. Additional change to runtime code to match the testing.Left out: #126.1-6/8-11
Comment #134
mondrake#133 was a conflict in the list of silenced deprecations.
Comment #135
mondrakeComment #136
daffie CreditAttribution: daffie commented@mondrake: The changes that you made in #127 - #135 look good to me. There are some points left to fix, so back to "Needs work".
Comment #137
mondrakeAddressed the remaining points from #126: 1-6 and 8-11.
Comment #138
mondrakeTwo small CS issues fixed.
Comment #140
mondrake#139 is an apcu failure
Comment #141
mondrakeComment #142
mondrakeVery hard to get tests to pass these days...
Thinking about
DatabaseDriverDiscoveryTest
, this assumes that during a test run all of the core drivers (mysql, sqlite, pgsql) are installable. This is true for DrupalCI (and TravisCI), but on local runs this may lead to failure if the dbs are not installed. Let's see if we can sort out something that can work in these cases.Comment #143
mondrakeThis should be safer.
Comment #145
daffie CreditAttribution: daffie commentedIt is almost RTBC for me. Two minor points left for me. Great work @mondrake and @Jo_Fitzgerald.
It is better to change this to a rename operation.
Can we add an assertion that the "Stub" driver is not installable.
Comment #146
mondrake@daffie thanks for review. #145 done, plus moved the deprecation silencers a little bit in the mid of the array to avoid re-roll juggling with this patch, external CI tools applying this patch, and other silencers being added to the end of the list by other issues. We can move it at the end of the list later, but while it waits...
Comment #147
mondrakeAlways, always forget to change status...
Comment #148
daffie CreditAttribution: daffie commentedThe patch looks now great to me.
There is a change record added for this issue.
We have no subsystem maintainer that can do a review. Setting the status to RTBC.
Comment #150
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #151
Mile23Are these usages in core? If they are, then they need a follow-up to remove them.
Comment #152
mondrakeFiled followup #2933407: [PP-1] Remove usages of drupal_detect_database_types, drupal_get_database_types and db_installer_object.
Comment #153
alexpottThis is a breaking change. If the core directory is a symlink as some people do this will not work now. Whereas before with DRUPAL_ROOT it would. I guess DatabaseDriverDiscovery should be a service with drupal root injected.
Isn't this an incorrect change?
Comment #154
mondrakeThanks @alexpott for your review. This addresses #153.
I did not know that we can use services during installation. I will experiment a bit.
Comment #156
daffie CreditAttribution: daffie commented@mondrake: Your solution is not what @alexpott is proposing:
Comment #157
mondrake@daffie yes, you are right, I am working on that and will post a patch when I will have sth ready. So far I found out that you need to register the service in the basic container in install.core.inc to have it available at installation time - it’s not sufficient to put it in the services YAML.
Comment #158
daffie CreditAttribution: daffie commentedAs an alternative we could also just do:
And not create a service. Maybe easier to do.
Comment #159
mondrake@daffie AFAICS the usage of DRUPAL_ROOT is discouraged. This patch now introduces the service. It is required very early for instance in KernelTest, so I had to add it in a basic container in KernelTestBase::bootEnvironment. I do not know if that's the right way.
Interdiff against #146.
Comment #161
mondrakeNeed to enable a basic container also in UpdatePathTestBase, then deprecation silencing does not work for Simpletest tests?
Comment #163
mondrakeMaybe the entire setting up of the basic container in UpdatePathTestBase could be moved from ::runDbTask to ::setUp, but I'd love feedback before doing so.
Comment #164
mondrakeNot all UpdatePath test have been converted to PHPUnit, so we need to include the new service also in the legacy UpdatePathTestBase.
Comment #167
mondrakeUh, yes.
Comment #169
mondrakePosting an interdiff 146-167 to facilitate review.
Comment #170
mondrake#167 is green and ready for review. #168 is an apcu failure.
Comment #171
mondrakeComment #172
mondrakeComment #173
daffie CreditAttribution: daffie commentedIt all looks good to me.
Only the second point of @alaxpott in comment #153 is not been addressed.
For the rest is this patch RTBC for me.
Great work @mondrake!
Comment #174
mondrake@daffie it is addressed, the comma is added back. or i did not understand 🤨 thanks
Comment #175
daffie CreditAttribution: daffie commentedok, it has been addressed.
For me it is back to RTBC.
Comment #176
alexpottDo we actually need this service ever apart from during the very early installer?
The original code had no statics for this. The new code before the service introduced a static and the new service is maintaining that by keeping it as a property on the class. Is this worth it? Does it actually save anything during installation? I'd be surprised.
Comment #177
mondrake#176:
1.
drupal_get_database_types
is used inMigrateUpgradeForm
of themigrate_drupal_ui
module. Besides, not having it as a service during runtime will mean a BC break - today custom/contrib can includeinstall.inc
and call one of the deprecated functions to inspect the available database drivers.2. Dunno... in
install.core.inc
we currently havea call to
drupal_get_database_types()
followed by one todb_installer_object($driver)
a few lines below. These will both (currently) discover the drivers with I/O then, if we do not cache. Not a big deal, but who knows how these functions/methods will be called - caching during the request lifetime seemed appropriate to me. But I have no strong argument, we can remove @alexpott if you believe so.Comment #179
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #180
alexpott@mondrake not having it as a service won't be a BC break as long as those functions still work. We could just do something like:
in there. Which is fine afaics. Let's take the opportunity not to pollute the container will services that will never ever be needed during the normal runtime of the site.
Comment #181
alexpottAlso let's not add cache layers here. Yes it might save I/O but adding it here seems unnecessary. install_database_errors() is only ever called once.
Comment #182
mondrake@alexpott this is removing the caching.
I disagree on doing
this will only work if the 'app.root' service is available from the container. But the deprecated functions will be called during early install when the 'normal' container is not yet available. So in these functions we'll have to conditionally call the service (if the normal container is NOT available) or instantiate a new class (if the container IS available) which is ... DX confusing. Also,
Database::convertDbUrlToConnectionInfo
is invoking the service, and this method can be called at runtime - here again this would then have to be conditional, and in case of \Drupal::root not available, we'll need to pass in the app root in the way we're trying to avoid.Comment #183
daffie CreditAttribution: daffie commented@alexpott wanted 2 things changed. The caching layers has been removed. For the not adding a new service: @mondrake has made a good argument, why it should be added. Changing the status of the issue back to RTBC, so that @alexpott can respond.
Comment #184
mondrakePlease also see #2933407-2: [PP-1] Remove usages of drupal_detect_database_types, drupal_get_database_types and db_installer_object where usage of the deprecated functions is removed from core.
Comment #186
alexpottRe #182 we can add the existing app.root service to the early installer really simply. It works I've tested this. We need to avoid polluting the run-time container with installer services. I think this issue sets the wrong precedent.
I'm still not really understanding why we are rewriting database discovery in this change. The issue is doing two things at the same time which is why progress is slow. It's (a) rewriting db discovery and (b) it is fixing db url conversion to support non core drivers. It we need to do (a) to do (b) then we should create a blocking issue where we can concentrate on the refactor of db discovery.
As I discussed the Uri pollution of the Connection class with @dawehner and I wondered if we could move all of the connection url stuff to a separate class for databases eg. ConnectionUrl where core provides an implementation and non-core drivers can implement there own versions.
Comment #187
mondrakeOn it, trying to simplify.
Comment #188
mondrakeActually, was just following the flow and never thought about that. It turns out we can do without touching db driver discovery at all.
Removed usage of
GuzzleHttp\Psr7\Uri
from this patch.The patch here does the minimum AFAICS - only
Database
,Connection
andsqlite/Connection
classes are involved + test changes. Just needed to add a couple methodsgetConnectionInfoFromUrl
andgetConnectionUrl
to abstractConnection
class and an override of those for 'sqlite' to cope with its specifics.Did not try to use this in contrib yet, wanted to get feedback if this is going in the right direction first.
No interdiff, will not make sense as the patch is a totally different approach.
Comment #189
alexpottThis doesn't look right. Haven't we worked that out before. I think we might able to work this out from the concrete class.
Let's do this in the base connection class. And just pass the url along here.
It's fine to refactor the code into a method but I don't think we need to make it public here.
Comment #191
mondrakeThanks for quick review @alexpott
Made changes according to #189. We could also refactor
Connection::getDriverClass
to use the newly introducedgetNamespace
method and avoid reflection.Comment #193
mondrakeReadded
KernelTestBase
fix from #57 and removed a stray @throws.Comment #194
mondrakeCS fix.
Comment #195
alexpottAll of the implementations are going to be same. Why not use reflection here and just have a protected static or even just inline in getConnectionInfoFromUrl()
Comment #196
mondrakea) Sure we can, but reflection is heavy. Are we sure we want it?
b) I would like to have a public API to retrieve the actual namespace of the db driver. We need it in #2867788: Log::findCaller fails to report the correct caller function with non-core drivers..
Comment #197
mondrakeHow about this: only implements on the base Connection class, avoids reflection and should avoid the PHP 5.5 failures.
Comment #198
mondrakeComment #199
Kuldeep K CreditAttribution: Kuldeep K commentedI have applied the above patch, well it's still showing me "InvalidArgumentException: Missing scheme in URL" when I am running functional test.
Comment #200
mondrake#199 @Kuldeep K can you please share your steps to reproduce? Core tests pass and for me it also works with an experimental driver for Doctrine DBAL, once implemented
Connection::getConnectionInfoFromUrl
andConnection::getConnectionUrl
methods.Comment #202
mondrakeRerolled, was conflicting with #2572613: Fix 'Drupal.Array.Array.CommaLastItem' coding standard.
Comment #203
daffie CreditAttribution: daffie commentedThe patch looks good. I do have some remarks:
Do we need to use the isset() function here?
The method getConnectionInfoFromUrl() will throw an exception when there is something wrong with the parameter $url.
Can we do the same for the method getConnectionUrl()?
Can we give both methods the same kind of name. Change getConnectionUrl() to getConnectionUrlFromConnectionOptions(). Not sure about this. Just an idea.
Can we add some documentation about why we use the preg_match() function here.
Is it not better to test $db_info['default'] instead of just $db_info.
We do not have a subsystem maintainer any more after @crell left. So changing it to the framework maintainer.
Comment #204
mondrakeComment #205
mondrakeAddressed @daffie's points in #203.
Comment #207
mondrakeFix test failure in #205 + add tests for #203.2
Comment #208
daffie CreditAttribution: daffie commented@mondrake: Thank you for your great work on this patch!
All my points from comment #203 are fixed.
For me it is RTBC.
Comment #209
alexpottout of scope.
Let's assert on the message.
It would be great to have test coverage of this fallback and exception. Also the as the exception message is per url we should assert on that too.
The check for !== NULL doesn't make sense. See https://3v4l.org/fsL3c
Given that we're dealing with statics I've prefer to run the test in a separate process rather than clean up. Let's stick
at the top of the test and note that we don't want the database static to affect other tests.
Comment #210
mondrakeThanks for review @alexpott.
This should address #209.
Comment #211
mondrakeUpdated IS to remove some parts that no longer stand. The CR will also need updates, but let's do that when we are agreed on final status.
Comment #212
daffie CreditAttribution: daffie commented@mondrake: The changes that you made look good. Only I am missing some things that @alexpott wants in comment #209:
There is no testing on the expected exception message. The parameter is added to the testing method, but it is not used!
@alexpott reviewed this patch. His words were: "This issue feels close - thanks for continuing to work on it". My conclusion is that we can remove the "needs framework manager review" tag.
@mondrake: What updates do you think we need on the change record?
Comment #213
alexpottLet's not add this. Do we need to? It's only called once. Also I'm not a huge fan of using regular expressions here. We can do this in the getOptionsFromUrl(). Here's a way without regular expressions:
substr(get_called_class(), 0, strrpos(get_called_class(), "\\"))
But I think given performance is not an issue here - we're not on the critical path and this is not called millions of times why not use reflection? Ie.
As far as I can see - and I might be missing something - but we are missing test coverage for this custom driver actually working. Can we not add a fake one to the autoloader during a test (or something like that)?
Why is this change necessary?
Comment #214
mondrakeThanks for reviews @daffie and @alexpott.
#212
1. done
2. which other new exception?
It still mentions the changes to the installer/database driver discovery that we dropped in latest versions of the patch here + we need to update the names of the methods added.
#213
1. done
2. not done, above me ATM, will look into it
3. I am sure there was a test failure if not, just removing that change here to see what happens
Comment #215
mondrakeComment #218
mondrake#213.3
SimpletestPhpunitRunCommandTest::testSimpletestPhpUnitRunCommand
fails because being a unit test,Database::getConnectionInfoAsUrl()
throws an exception now since no default db connection is defined. Here, changing the test instead of changing runtime code, by adding a default database connection entry.#213.2 still outstanding.
Comment #220
mondrakeComment #221
daffie CreditAttribution: daffie commented@mondrake: On comment #212.2. I was wrong the other exception is only being moved. The changes you are making look good to me.
Only comment #213.2 still outstanding.
Comment #222
mondrakeRe. #213.2
Here's what I came up with - trying to register an additional class loader, adding an additional namespace for a 'fake' db driver, and prepending the loader to others to avoid name collisions.
I does not work though, test will fail.
@alexpott maybe you can lend a hand?
Comment #223
mondrakeSuper stupid typo took me two hours to get around :(
#213.2 should be addressed now.
Comment #225
daffie CreditAttribution: daffie commentedAll the changes look good to me and as far as I can see all the points of @alexpott have been addressed. Back to RTBC. Great work @mondrake!
Comment #226
alexpottI feel like columbo. Sorry. One more thing. I just was wondering about naming. Looking at the existing Connection class
options
has multiple meanings. See\Drupal\Core\Database\Connection::defaultOptions()
and\Drupal\Core\Database\Connection::getConnectionOptions
.So let's use the existing nomenclature. And also lets use the word create instead of get. Imo get implies it already exists whereas this methods are creating something from something. So
createConnectionOptionsFromUrl
andcreateUrlFromConnectionOptions
.We should also update the doc blocks respectively.
Comment #227
mondrake#226:
No worries. Let's do things right here :)
Comment #228
mondrakeComment #229
mondrakeUpdated draft CR.
Comment #230
daffie CreditAttribution: daffie commentedWe must keep Colombo happy or you will go to jail. ;-)
Back to RTBC.
Comment #231
alexpottAdding review credit.
Comment #232
alexpottThe postgres fail is a known issue - #2982755: Random failure in SchemaTest::testSchemaChangePrimaryKey with order of composite primary key
Comment #233
alexpottCommitted 61cbaba and pushed to 8.6.x. Thanks!
Comment #235
mondrakeFor anyone interested, filed [site:install] Add a --db-url option in Drupal Console issue queue on GitHub.