Drupal\Tests\file\Kernel\Views\RelationshipNodeFileDataTest fails on HEAD with PostgreSQL

🤦‍♂️

Tests all green.

Question if this needs a test or not?

Renamed the new join plugin to casted_int_field_join which should describe the plugin a bit better. I also added kernel tests for the plugin. The file tests already cover this from integration standpoint.

@lauriii explained this to me at DrupalCon. It makes sense to me, and although it's awkward, its awkwardness comes from the fact that a lot of what Views does is awkward.

So, the code things that jumped out at me:

1. +++ b/core/modules/views/src/Plugin/views/join/CastedIntFieldJoin.php
   @@ -0,0 +1,74 @@
   +    switch (\Drupal::database()->databaseType()) {
   +      case 'mysql':
   +        $cast_data_type = 'UNSIGNED';
   +        break;
   +
   +      default:
   +        $cast_data_type = 'INTEGER';
   +        break;
   +    }
   

   Can this be a match expression?

2. +++ b/core/modules/views/src/Plugin/views/join/CastedIntFieldJoin.php
   @@ -0,0 +1,74 @@
   +    switch ($this->configuration['cast'] ?? 'right') {
   +      case 'left':
   +        $left_field = "CAST($left_field AS $cast_data_type)";
   +        break;
   +
   +      case 'right':
   +        $right_field = "CAST($right_field AS $cast_data_type)";
   +        break;
   +    }
   

   This should be a match expression so that it's not possible to pass in an invalid value for cast.

3. +++ b/core/modules/views/tests/src/Kernel/Plugin/CastedIntFieldJoinTest.php
   @@ -0,0 +1,178 @@
   +   * A plugin manager which handlers the instances of joins.
   

   s/handlers/handles

4. +++ b/core/modules/views/tests/src/Kernel/Plugin/CastedIntFieldJoinTest.php
   @@ -0,0 +1,178 @@
   +    if ($connection->databaseType() === 'mysql') {
   +      $this->assertStringContainsString('views_test_data.uid = CAST(users4.uid AS UNSIGNED)', $join_info['condition']);
   +    }
   +    else {
   +      $this->assertStringContainsString('views_test_data.uid = CAST(users4.uid AS INTEGER)', $join_info['condition']);
   +    }
   

   IMHO we could do this:

   $this->assertStringStartsWith('views_test_data.uid = CAST(users4.uid AS ', $join_info['condition']);
   if ($connection->databaseType() === 'mysql') {
     $this->assertStringEndsWith('UNSIGNED)', $join_info['condition']);
   }
   else {
     $this->assertStringEndsWith('INTEGER)', $join_info['condition']);
   }
   

Addressed #6.

For #6.2 I had to opt for an assert since the switch statement doesn't currently result in a single variable.

Please do not hardcode SQL depending on the database type. It fails database abstraction and makes it hard for contrib database drivers to try and keep up. Backend-overrideable services were introduced for this purpose. As a reference, see the data management functions.

Haven't manually tested this one but the code looks good.

Laurii's sitting right here though and wants to get @mondrake's change in.

@mondrake I wasn't sure if it was required for the Views plugins since there were already couple Views plugins depending on database type directly. Happy to implement that though 👍

All green so assuming it's good as it's fixing a test case.

Migrating tags to do tag cleanup

1. +++ b/core/modules/views/src/Plugin/views/join/CastedIntFieldJoin.php
   @@ -0,0 +1,65 @@
   +    switch ($this->configuration['cast'] ?? 'right') {
   +      case 'left':
   +        $left_field = \Drupal::service('views.cast_sql')->getFieldAsInt($left_field);
   +        break;
   +
   +      case 'right':
   +        $right_field = \Drupal::service('views.cast_sql')->getFieldAsInt($right_field);
   +        break;
   +    }
   

   Are there more possibilities then 'left' or 'right'? if not can we do a simple if-statement?

2. +++ b/core/modules/views/src/Plugin/views/query/CastSqlInterface.php
   --- /dev/null
   +++ b/core/modules/views/src/Plugin/views/query/MysqlCastSql.php
   
   +++ b/core/modules/views/src/Plugin/views/query/MysqlCastSql.php
   --- /dev/null
   +++ b/core/modules/views/src/Plugin/views/query/PostgresqlCastSql.php
   
   +++ b/core/modules/views/src/Plugin/views/query/PostgresqlCastSql.php
   --- /dev/null
   +++ b/core/modules/views/src/Plugin/views/query/SqliteCastSql.php
   
   +++ b/core/modules/views/views.services.yml
   @@ -95,3 +95,13 @@ services:
   +  pgsql.views.cast_sql:
   +    class: Drupal\views\Plugin\views\query\PostgresqlCastSql
   +    public: false
   +  sqlite.views.cast_sql:
   +    class: Drupal\views\Plugin\views\query\SqliteCastSql
   +    public: false
   

   Can we move these plugins to the database modules. This make it easier for contrib database driver to do the same.

3. +++ b/core/modules/views/tests/src/Kernel/Plugin/CastedIntFieldJoinTest.php
   @@ -0,0 +1,167 @@
   +class CastedIntFieldJoinTest extends RelationshipJoinTestBase {
   

   This test has database specific testing. Can we move the database specific parts to the database modules. This allows contrib database modules to do the same. See: core/tests/Drupal/KernelTests/Core/Database/DriverSpecificKernelTestBase.php

> Can we move these plugins to the database modules. This make it easier for contrib database driver to do the same.

Even if most use mysql, mysql is the one which require a different behavior. So discussed with @lendude, @lauriii and @longwave and we agree that it makes the most sense to have PostgreSQL/SQLite as the default behavior, and override for MySQL, so we skip the overrides and hopefully other implementations like MSSql won't need to override it. Patch coming shortly.

Patch attached. Still pending #17.1, #17.3

Changed switch to a simpler if-else. Added test per db engine as requested in #17.

The last submitted patch, 21: 3364621--21.patch, failed testing. View results →

Fixed namespace. Not sure if I need to move the abstract test class?

The last submitted patch, 23: 3364621--23.patch, failed testing. View results →

Renamed to *TestBase as apparently I can't have an abstract Test.

Need to catch up on sleep when I'm home definitely.

fixed phpstan complaints.

+++ b/core/modules/views/tests/src/Kernel/Plugin/CastedIntFieldJoinTestBase.php
@@ -0,0 +1,203 @@
+    $cast_type = match ($connection->databaseType()) {
+      'mysql' => 'UNSIGNED',
+      default => 'INTEGER',
+    };

The idea with database specific testing is that we put the database specific stuff in the database override of the test. My suggestion would be to a class variable called something like "casting_type". The default value would be "INTEGER". The MySQL test will override it and set the value to "UNSIGNED".

@penyaskito: The patch look great! Just 1 minor point.

My train of thought was that this might not be enough for some engines and they will end up needing to override the complete test.
But let's do that anyway and if that's needed we will figure out later.

All my points have been addressed.
Back to RTBC.

OK this is very tricky but I see why it's necessary...

Can we open a follow-up to potentially remove this handling once 🌱 Add a reliable entity-usage system to core Active is complete (or more specifically, once the file_usage table is gone)? I'm slightly concerned that if file_usage is the only use-case, we might end up maintaining it forever once that's eventually gone. Having said that there might be similar schemas in contrib that also break now that this would fix.

Agreed that we should be able to get rid of this once 🌱 Add a reliable entity-usage system to core Active is done. Opened follow-up: 📌 [PP-1] Deprecate and remove Drupal\views\Plugin\views\join\CastedIntFieldJoin Postponed .

Committed 6e8cab8 and pushed to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.