[4.0] [Installation] Fix minimum required version check when creating a new database#28346
Merged
HLeithner merged 5 commits intojoomla:4.0-devfrom May 2, 2020
Merged
Conversation
- Move CMS server version requirement constants from setup model to database helper. - Add function to get the version requirement to database helper. - Let setup model use database helper for server version check.
Use minimum required version from database helper for version test in database model and add connection encryption tests.
Quy
reviewed
Mar 15, 2020
Co-Authored-By: Quy <quy@fluxbb.org>
Member
Author
|
Wait a bit with testing ... I have to adjust description so it is clear that the problem fixed by this PR doesn’t appear with MySQLi, only with MySQL (PDO) and PostgreSQL (PDO). |
Member
Author
|
Done. Description adapted. Ready for test. |
Member
|
I have tested this item ✅ successfully on ef7f047 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28346. |
Contributor
|
I have tested this item ✅ successfully on ef7f047 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28346. |
Contributor
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28346. |
Member
Author
|
Anything wrong with my PR? It is RTC, has no conflicts and is still valid. |
Member
|
Thanks |
Member
Author
|
Thanks. |
sakiss
pushed a commit
to sakiss/joomla-cms
that referenced
this pull request
Oct 16, 2020
… a new database (joomla#28346) * Move CMS requirement for minimum server version - Move CMS server version requirement constants from setup model to database helper. - Add function to get the version requirement to database helper. - Let setup model use database helper for server version check. * Correct version test and add connection encryption tests Use minimum required version from database helper for version test in database model and add connection encryption tests. * Use $type as like before * Fix silly typo in comment Co-Authored-By: Quy <quy@fluxbb.org> Co-authored-by: Quy <quy@fluxbb.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request for Issue # .
Summary of Changes
With Pull Request (PR) #28135 I have added the check for the CMS' minimum database server version requirement to the installation, because the CMS requirement is at least for PostgreSQL more strict than the database driver's requirement, which was the only checked one before that PR.
But that PR has fixed that only for the case when the database already exists in case of MySQL (PDO) and PostgreSQL (PDO). When creating a new database during installation and using these drivers, still only the database driver's requirement is checked. For MySQLi that is not a problem because there the check in the setup model (which respects the CMS' requirement) works also when the database doesn't exist (yet).
This PR here corrects this so it works for all 3 drivers.
In addition it adds the same checks for connection encryption after successful connection to the database model as I have added them with my PR #27348 when I had added the connection encryption feature to the installation. The reason for this change is the same: Without this PR here these checks take only place when the database already exists, but not when a new database is created. This change is harder to test, because it is only relevant for the special case that the server doesn't support encryption, or negotiating an encrypted connection fails, and the server is configured to allow to fall back to an unencrypted connection. This part can only be tested by code review, I think.
Testing Instructions
Requirements
The minimum database server versions currently required by the database drivers are:
The minimum database server versions currently required by the CMS are:
Because for MySQL and MariaDB the requirements of database driver and CMS are equal, you have to patch either the driver's or the CMS' requirement so that the above condition is fulfilled. For PostgreSQL you either use a server version 10 or have to patch some requirements, too.
The minimum version as required by the particular database driver for the particular database type can be patched at the following places:
For MySQL databases
MySQLi driver - file
libraries/vendor/joomla/database/src/Mysqli/MysqliDriver.php, line 86MySQL (PDO) driver - file
libraries/vendor/joomla/database/src/Mysql/MysqlDriver.php, line 72For MariaDB databases
MySQLi driver - file
libraries/vendor/joomla/database/src/Mysqli/MysqliDriver.php, line 94MySQL (PDO) driver - file
libraries/vendor/joomla/database/src/Mysql/MysqlDriver.php, line 80For PostgreSQL databases
PostgreSQL (PDO) driver - file
libraries/vendor/joomla/database/src/Pgsql/PgsqlDriver.php, line 54The minimum version as required by the CMS for the particular database type can be patched here:
joomla-cms/installation/src/Helper/DatabaseHelper.php
Lines 23 to 48 in 8925612
You need an up-to-date 4.0-dev branch so it contains the recently merged PR [4.0] [Installation] Fix creation of new database for MySQL (PDO) and PostgreSQL (PDO) #28338 , or when using a nightly build, wait until tomorrow so that PR is included.
This PR has to be tested with the PDO drivers, i.e. database type MySQL (PDO) or PosgreSQL (PDO). Of course if you have a MySQL or MariaDB database, you can test in addition if this PR doesn't break anything when using the MySQLi driver.
Instructions
Start a new Joomla 4 installation without the patch of this PR applied, using a database server and user combination which allows to create a new database, and a database name for which no database exists on that server, and make sure that for the server version the 1st point in section "Recuirements" above is fulfilled.
Result: See section "Actual result" below.
Do the same with the patch of this PR applied. When getting an error message about version requirement not being fulfilled, change database server or patch the requirement so that it will be fulfilled, and try again, using the "Install Jooma >" button again.
Result: See section "Expected result" below.
Finally check by code review that the checks for connection encryption added by this PR to here
joomla-cms/installation/src/Model/DatabaseModel.php
Lines 486 to 497 in 8925612
are the same as already present here
joomla-cms/installation/src/Model/SetupModel.php
Lines 569 to 584 in 8925612
except that in the database model they raise exceptions instead of enqueueing messages and returning, but the exception texts are equal to the message texts used in the setup model.
Expected result
When creating a new database at installation, the CMS' requirement on the minimum database server version is checked, and if it is not fulfilled, the installation stops with an appropriate error message.
The user can correct database information in the installation form and then finish the installation by using the "Install Joomla >" button again.
Actual result
When creating a new database at installation, the CMS' requirement on the minimum database server version is not checked, i.e. Joomla is installed in that database even if the server doesn't fulfill the version requirement by the CMS.
Documentation Changes Required
None.
Additional information
This PR doesn't fix the fact that a database will also be created with MySQL (PDO) or PosgreSQL (PDO) when the server doesn't fulfill the minimum version requirement because that version check is done after database creation. This issue I plan to fix with another, future PR.