- Issue created by @daffie
- Status changed to Needs review
over 1 year ago 2:02pm 3 April 2023 - 🇳🇱Netherlands Arantxio Dordrecht
I've created the patch so instead of relying on a extra function we now set the value depending on the driver class.
The last submitted patch, 2: 3351886-2.patch, failed testing. View results →
- 🇳🇱Netherlands Arantxio Dordrecht
So the naming had to be TestBase instead of Test, so the testbot does not run it. This way it should work.
- 🇳🇱Netherlands daffie
Giving @mondrake issue credits for his remark in #1148856-83: Postgres schema doesn't support keylength on a unique index → .
- Status changed to RTBC
over 1 year ago 9:05am 11 April 2023 - Status changed to Needs work
over 1 year ago 9:25am 11 April 2023 - 🇮🇹Italy mondrake 🇮🇹
+++ b/core/modules/mysql/tests/src/Kernel/mysql/SchemaUniquePrefixedKeysIndexTest.php @@ -0,0 +1,19 @@ +use Drupal\KernelTests\Core\Database\SchemaUniquePrefixedKeysIndexTestBase as CoreSchemaUniquePrefixedKeysIndexTest; +++ b/core/modules/pgsql/tests/src/Kernel/pgsql/SchemaUniquePrefixedKeysIndexTest.php @@ -0,0 +1,19 @@ +use Drupal\KernelTests\Core\Database\SchemaUniquePrefixedKeysIndexTestBase as CoreSchemaUniquePrefixedKeysIndexTest; +++ b/core/modules/sqlite/tests/src/Kernel/sqlite/SchemaUniquePrefixedKeysIndexTest.php @@ -0,0 +1,19 @@ +use Drupal\KernelTests\Core\Database\SchemaUniquePrefixedKeysIndexTestBase as CoreSchemaUniquePrefixedKeysIndexTest;
I don't think an alias is needed, here just extend
SchemaUniquePrefixedKeysIndexTestBase
? - 🇳🇱Netherlands Arantxio Dordrecht
@mondrake that's true, changed the naming so we don't have the alias anymore.
- Status changed to Needs review
over 1 year ago 9:34am 11 April 2023 - 🇮🇹Italy mondrake 🇮🇹
+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaUniquePrefixedKeysIndexTestBase.php @@ -7,16 +7,19 @@ + protected $columnValue;
you can typehint the property directly in PHP 8.1, and skip the annotation
protected string $columnValue;
- 🇮🇹Italy mondrake 🇮🇹
I am afraid you need to do the same on the extending classes...
- 🇳🇱Netherlands Arantxio Dordrecht
@mondrake That's true my bad. added the string typehint to the other classes.
- 🇳🇱Netherlands daffie
All points of @mondrake have been addressed.
Back to RTBC. - last update
over 1 year ago 29,291 pass - last update
over 1 year ago 29,308 pass - last update
over 1 year ago 29,310 pass - last update
over 1 year ago 29,308 pass - last update
over 1 year ago 29,369 pass - last update
over 1 year ago 29,374 pass - Status changed to Needs work
over 1 year ago 9:41am 1 May 2023 - 🇬🇧United Kingdom longwave UK
-
+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaUniquePrefixedKeysIndexTestBase.php @@ -7,16 +7,17 @@ + * Set the value used to test the Schema unique prefixed keys index.
Schema -> schema
-
+++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaUniquePrefixedKeysIndexTestBase.php @@ -109,27 +104,11 @@ protected function checkUniqueConstraintException(string $table, string $column) - * The basic syntax of passing an array (field, prefix length) as a key column - * specifier must always be accepted by the driver. However, due to technical - * limitations, some drivers may choose to ignore them. - * - * @return bool - * TRUE if the current database (driver) will conform to the prefix length - * specified as part of a key column specifier, FALSE if it will be ignored.
We are losing some of this comment, and without it it is not at all obvious why Postgres/SQLite are different to MySQL. I think it would be good to move some of this explanation to the property declaration in the base class?
-
- Status changed to Needs review
over 1 year ago 11:52am 1 May 2023 - last update
over 1 year ago 29,375 pass - 🇳🇱Netherlands Arantxio Dordrecht
I've adjusted the comments based on the feedback from @longwave.
I adjusted the "lost" comment to what I think is a better description of the current situation.
- last update
over 1 year ago 29,375 pass - last update
over 1 year ago 29,375 pass - Status changed to RTBC
over 1 year ago 12:53pm 1 May 2023 - 🇳🇱Netherlands daffie
All remarks of @longwave have been addressed.
Back to RTBC. - last update
over 1 year ago 29,382 pass - last update
over 1 year ago 29,386 pass - last update
over 1 year ago 29,387 pass - last update
over 1 year ago 29,365 pass, 2 fail - last update
over 1 year ago 29,391 pass - last update
over 1 year ago 29,396 pass - last update
over 1 year ago 29,396 pass - last update
over 1 year ago 29,396 pass - last update
over 1 year ago 29,396 pass - last update
over 1 year ago 29,396 pass - last update
over 1 year ago 29,396 pass - last update
over 1 year ago 29,403 pass - last update
over 1 year ago 29,407 pass - last update
over 1 year ago 29,407 pass 43:30 40:01 Running43:28 36:57 Running- last update
over 1 year ago 29,421 pass - last update
over 1 year ago 29,423 pass - last update
about 1 year ago 29,428 pass - last update
about 1 year ago 29,428 pass - last update
about 1 year ago 29,433 pass - last update
about 1 year ago 29,437 pass - 🇬🇧United Kingdom longwave UK
Committed and pushed 224543f183 to 11.x (10.2.x). Thanks!
- Status changed to Fixed
about 1 year ago 1:48pm 16 June 2023 -
longwave →
committed 224543f1 on 11.x
Issue #3351886 by Arantxio, daffie, mondrake, longwave: Change...
-
longwave →
committed 224543f1 on 11.x
Automatically closed - issue fixed for 2 weeks with no activity.