Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In #3192487: Warn about upcoming JSON database support requirement in Drupal 9 we added a requirements check. Here, we are making Drupal installation fail on Drupal 10 is JSON is not supported by the database.
Proposed resolution
Update the list of checked SQL commands.
Remaining tasks
TBD
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#22 | diff_3203193_18-22.txt | 1.88 KB | ankithashetty |
#22 | 3203193-22.patch | 2.07 KB | ankithashetty |
|
Comments
Comment #2
Gábor HojtsyI think the simplest is to add it to the install checklist. We already have infrastructure to check these. Should also remove the code added in #3192487: Warn about upcoming JSON database support requirement in Drupal 9 then, since we don't need the custom solution there anymore. Should only be committed to the 10.x branch but will send for a test out of curiosity :D
Comment #3
Gábor HojtsyComment #4
daffie CreditAttribution: daffie commented#3192487: Warn about upcoming JSON database support requirement in Drupal 9 has landed.
Comment #5
murilohp CreditAttribution: murilohp at CI&T commentedHey, 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?
Comment #6
murilohp CreditAttribution: murilohp at CI&T commentedComment #7
murilohp CreditAttribution: murilohp at CI&T commentedFixing phpcs.
Comment #9
murilohp CreditAttribution: murilohp at CI&T commentedForgot to remove the tests from #3192487, this new patch should fix the fail tests.
Comment #10
murilohp CreditAttribution: murilohp at CI&T commentedComment #11
daffie CreditAttribution: daffie commentedWe 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.
Comment #12
daffie CreditAttribution: daffie commentedComment #13
murilohp CreditAttribution: murilohp at CI&T commentedThanks for the review @daffie! I made this new patch addressing #11.
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.Comment #14
daffie CreditAttribution: daffie commentedThe 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.
Comment #16
murilohp CreditAttribution: murilohp at CI&T commentedThe failed tests were fixed, moving back RTBC
Comment #17
catchI 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
s/json/JSON/
Comment #18
ankithashettyUpdated patch as suggested in #17 , thanks!
Comment #20
daffie CreditAttribution: daffie commentedBack to RTBC
Comment #21
catchSorry 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.
Comment #22
ankithashettyHere is an updated patch, thanks!
Comment #23
daffie CreditAttribution: daffie commentedThe point of @catch has been addressed. Back to RTBC.
Comment #24
mondrakeComment #26
daffie CreditAttribution: daffie commentedBack to RTBC.
Comment #28
catchCommitted d214deb and pushed to 10.0.x. Thanks!
Comment #29
xjm