Warn about upcoming JSON database support requirement in Drupal 9

I'm going to move this back to 9.2.x - I think we can implement the requirements test in Drupal 9, and show a warning in hook_requirements() to tell people if they don't have the extension configured and that it will be required in Drupal 10.

Then in Drupal 10 we'd just change the warning to an error and possibly update some wording.

Make title clearer.

Also opened #3203193: Fail install if JSON is not supported by the database in Drupal 10 for Drupal 10.

Here is a patch that works fine for me. Not sure how to do automated test for this at all.

We could have a test that checks the pass case - just add it to system_requirements() testing.

No idea with the fail case, seems like it could be prohibitively complex.

Hm, which test would that be. I could not find it. https://api.drupal.org/api/drupal/9.1.x/search/requirementstest

hmm I thought we had a generic status report test, but it looks like tests/src/Functional/UpdateSystem/UpdateScriptTest.php might be the closest thing, which isn't really appropriate here.

We shouldn't have the translated string split up as it is, this won't work across all languages (for like, RTL languages but maybe others too).

There's no harm in duplicating the $requirements lines.

@pobster: How does two individual full sentences cause that problem?

Right-to-left languages.

'value' => $json_message . ' ' . t('Currently not required, however it will be required from Drupal 10.'),

You can't guarantee the order of these sentences. Like this, they can only go in one order.

Drupal is full of composition of larger text from full translated sentences. See system_help(). Why is this case different?

That's different as each part is separate? Nearly every line is either a dynamic list item or a heading. I mean ... it's not SO different, this is true - but using the argument "we do it badly elsewhere, so we can do it badly here" isn't the best of reasons to make translating content difficult. Someone will end up having to patch this later as part of an i18n initiative - let's just give whoever that is one less thing to do?

I did not say we do it badly elsewhere, we did the breakup of those strings as part of an i18n initiative. Either way, I will nor be the one to decide whether this is good for commit or not, and it still requires a test to be considered for inclusion (which I don't have capacity currently to add).

Note that #3190641: [policy] Require SQLite 3.26 for Drupal 10 (same as Drupal 9) with the json1 extension required (new requirement for Drupal 10) does not yet specify the json1 extension as a requirement, so that would be important to document / propose there, so we don't put the cart before the horse writing code for it :D

cleaned a bit, gonna extend \Drupal\Tests\system\Functional\System\StatusTest

+      Database::getConnection()->query('SELECT JSON_TYPE(\'1\')');
+      $json_message = t('Supported. Currently not required, however it will be required from Drupal 10.');
+      $json_severity = REQUIREMENT_OK;
+    }
+    catch (\Exception $e) {
+      $json_message = t('Not supported. Currently not required, however it will be required from Drupal 10.');
+      $json_severity = REQUIREMENT_WARNING;
+    }
+    $requirements['database_support_json'] = [
+      'title' => t('Database support for JSON'),
+      'severity' => $json_severity,
+      'value' => $json_message,
+    ];
+  }

Instead of using t('Not supported. Currently not required, however it will be required from Drupal 10.') and t('Supported. Currently not required, however it will be required from Drupal 10.') for the value key, it should use t('Not supported') and t('Supported') for value and t('It is currently not required, but it will be required from Drupal 10.') for description.

Hm, good suggestion!

Addressed #20 - good idea

Added basic test and refactored code a bit

+++ b/core/modules/system/system.install
@@ -487,6 +487,24 @@ function system_requirements($phase) {
+      'value' => t('Supported'),
+      'description' => t('Currently not required, however it will be required from Drupal 10.'),
+    ];

I think we could shorten this to something like:

'Database JSON support will be required from Drupal 10.'

The 'not required, however it will be required' is a bit tricky to scan.

Just a nitpick: Currently not required, however it will be required from Drupal 10. is an example of comma-splice sentence.

It should be either It's currently not required. However, it will be required from Drupal 10. or It's currently not required, but it will be required from Drupal 10.

I find it kinda tautology - database support... supported

So description could also prevent duplicating words

Also I wonder if we should display description if supported, probably it useful only when not supported

Available is probably better. I suggested Supported to avoid using a phrase for which there wasn't a translation.

As for showing a description only when the support isn't available, Drupal core shows the description even when an extension is available. I would do the same here.

As for shortening the description, that could simple be It will be required from Drupal 10. The title is Database support for JSON, which means it's clear to what it will be required is referring to.

Here's updated patch - using Available and shorter description as 10.0 (like other deprecations using to, check with git grep 'is required in')

I think it's a good idea @daffie, I created a new function at core/lib/Drupal/Core/Database/Connection.php::hasJson and override it at core/modules/pgsql/src/Driver/Database/pgsql/Connection.php. Let's see if the tests can pass now.

Looks like it's passing @daffie, searching at the documentation, since postgres 9.5, we have some new functions to process json values.
Do you think we should use something else to validate json datatype on postgres?

FYI: https://www.postgresql.org/docs/9.5/functions-json.html#FUNCTIONS-JSON-P...

Thanks for the review @daffie! Here's a patch addressing #34.

Do you guys have any idea of what happened with the tests from #35? At the first moment I thought it could be an infrastructure error, but I think that's not the case.

PHP Parse error: syntax error, unexpected ')', expecting '|' or variable (T_VARIABLE) in /var/www/html/core/modules/pgsql/src/Driver/Database/pgsql/Connection.php on line 378

Thanks @cilefen! Let's see the tests now.

Committed 206d6ee and pushed to 10.0.x. Thanks!

Realised there wasn't a change record for this while committing it, so added one.

Is that correct that this change is states for Drupal 9.4.0 here and in change records, but only pushed in 10.0.x branch.

It will be cherry-picked to 9.4.x, or it's Drupal 10 feature?

@Niklan - not correct, I failed to push the cherry-pick successfully. Back to RTBC until that happens.

It's a clean cherry-pick to 10.x, I just didn't do it properly (but have now).

We'll need a follow-up to make this a requirement error in 10.x though.

Opened #3259311: Fix database JSON detection for PostgreSQL to fix this in Upgrade Status, it would break the same way for PostgreSQL.

If a contrib driver has not implemented the ::hasJson() method, the status report will be WSOD. I think we should also check that the method exists before calling it, at least in D9.4.

Also, we are missing a test here, maybe in ConnectionTest.

@daffie you're right, the method will be there but failing if the syntax is not appropriate. Sorry for the noise, thanks. (A KernelTest would still be helpful, but can be followup).

Re. #53 I filed #3259532: Add a kernel test for Connection::hasJson.