Fail install if JSON is not supported by the database in Drupal 10

#3192487: Warn about upcoming JSON database support requirement in Drupal 9 has landed.

Hey, I made a new patch based on #3192487, I created a new function to test the json support and used the \Drupal\Core\Database\Connection::hasJson to handle different databases.

We could find a way to refactor it to show the query and the error in case if something goes wrong during the installation. What do you think?

Fixing phpcs.

Forgot to remove the tests from #3192487, this new patch should fix the fail tests.

+++ b/core/modules/system/system.install
@@ -487,22 +487,6 @@ function system_requirements($phase) {
-  if ($phase == 'runtime') {
-    // Test JSON support. To be required from Drupal 10.0.
-    // @todo remove in https://www.drupal.org/project/drupal/issues/3203193
-    $requirements['database_support_json'] = [
-      'title' => t('Database support for JSON'),
-      'severity' => REQUIREMENT_OK,
-      'value' => t('Available'),
-      'description' => t('Is required in Drupal 10.0.'),
-    ];
-
-    if (!Database::getConnection()->hasJson()) {
-      $requirements['database_support_json']['value'] = t('Not available');
-      $requirements['database_support_json']['severity'] = REQUIREMENT_WARNING;
-    }
-  }
-

We cannot remove this. It needs to change to a 'REQUIREMENT_ERROR' when there is no JSON support. The stuff in Install/Tasks is only for new sites. Existing sites also need to have JSON support.

The if-statement with $phase == 'runtime' can be removed. Existing sites need to have JSON support when they are in "install", "update" or in the "runtime" phase.

Thanks for the review @daffie! I made this new patch addressing #11.

The if-statement with $phase == 'runtime' can be removed. Existing sites need to have JSON support when they are in "install", "update" or in the "runtime" phase.

I kept the if-statement on this new patch, validating the "runtime" and "update", the "install" phase, does not have a database connection(which would fail the tests and probably the installation), for this scenario, I tested installing a new site and the core/lib/Drupal/Core/Database/Install/Tasks.php::checkJsonSupport were able to validate the JSON support.

The code changes look good to me.
The problem is that we should add testing, only I do not see how.
The new method in Drupal\Core\Database\Install\Tasks must pass the functional installertest and therefor has testing.
The change in the file system.install has already have testing.
For me it is RTBC.

@murilohp: Thank you for working on this.

The failed tests were fixed, moving back RTBC

1. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -386,4 +390,16 @@ protected function getConnection() {
   +   */
   +  protected function checkJsonSupport() {
   +    if ($this->getConnection()->hasJson()) {
   +      $this->pass(t('Drupal can use JSON database commands.'));
   +    }
   +    else {
   +      $this->fail(t('Failed to use <strong>JSON_TYPE</strong>. We tried testing database for JSON support and the test is failing.'));
   +    }
   +  }
   +
   

   I think this should be something like:

   'Database connection supports the JSON type'.

   For the fail message, not sure entirely, but it should probably link to https://www.drupal.org/docs/system-requirements

2. +++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
   @@ -386,4 +390,16 @@ protected function getConnection() {
   +   */
   

   s/json/JSON/

Updated patch as suggested in #17 , thanks!

Back to RTBC

+++ b/core/lib/Drupal/Core/Database/Install/Tasks.php
@@ -386,4 +390,16 @@ protected function getConnection() {
+    else {
+      $this->fail(t('<a href="https://www.drupal.org/docs/system-requirements">Failed to use <strong>JSON_TYPE</strong>. We tried testing database for JSON support and the test is failing.</a>'));
+    }

Sorry this still needs to be more neutral I think. The 'We tried to use' language is a bit personal for a database connection test. Also JSON_TYPE is too specific here, we should just say JSON.

"Database connection does not support JSON." might be enough.

Here is an updated patch, thanks!

The point of @catch has been addressed. Back to RTBC.

Back to RTBC.

Committed d214deb and pushed to 10.0.x. Thanks!