? ? ? ? Pending

User tests: Successful: Unsuccessful:

avatar richard67
richard67
9 Apr 2021

Pull Request for Issue # .

Summary of Changes

This pull request (PR) adds a check if the database schema checker has found database problems to the "Required PHP & Database Settings" checks of the pre-update checker.

The reason should be clear: Nobody should update Joomla when there are unfixed database problems.

Currently we have nothing which helps to avoid that if the user doesn't check the database checker.

Testing Instructions

Start with a clean 3.10-dev or latest 3.10 nightly build using a database type and version which will be supported by Joomla 4.

Clean means there are no database problems found in backend on "Extensions - Manage - Database", and there has no patch of any PR been applied.

  1. Go to the Joomla Update component's options and set the update channel to the custom update URL of the 4.0 nightly builds so that an update to J4 is found.
    Result: After saving, the "Pre-Update Check" tab is shown.
  2. Expand the "Required PHP & Database Settings" by selecting "More Details", and check if you can see database checks.
    Result: See section "Actual result BEFORE applying this Pull Request" below.
  3. Apply the patch of this PR, and then go again to the Pre-Update Check or if still there reload the page.
    Result: The "Database Supported" check has been moved below the PHP related checks, and below that at the bottom there is a new check "Database Table Structure Up to Date", which has a successful check (green "yes").
    See first image in section "Expected result AFTER applying this Pull Request" below.
  4. Add a new, empty update SQL script with a version in it's name which is later than the latest one of the available SQL scripts, e.g. "3.10.0-2021-04-09.sql", to folder "administrator/components/com_admin/sql/updates/mysql" or "administrator/components/com_admin/sql/updates/postgresql", depending on your database type.
  5. Go to "Extensions - Manage - Database" or reload the page if still there, and check if database problems are found.
    Result: 1 database problem found. Database schema version (3.10.0-2020-08-10) does not match CMS version (3.10.0-2021-04-09).
  6. Go to the Pre-Update Check or reload the page if still there, and expand the "Required PHP & Database Settings".
    Result: The "Database Table Structure Up to Date" check has failed.
    See second image in section "Expected result AFTER applying this Pull Request" below.
  7. Go to "Extensions - Manage - Database" or reload the page if still there, and use the "Fix" button.
    Result: No database problems found.
  8. Go to the Pre-Update Check or reload the page if still there.
    Result: Required PHP and database settings are all ok.
  9. Edit the previously added SQL script and add a "CREATE TABLE" statement for some new table, e.g.:
CREATE TABLE IF NOT EXISTS `#__testtable` (
  `id` int unsigned NOT NULL AUTO_INCREMENT COMMENT 'Primary Key',
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 DEFAULT COLLATE=utf8mb4_unicode_ci;
  1. Save the file and go back to "Extensions - Manage - Database" or reload the page if still there, and check if database problems are found.
    Result: 1 database problem found. Table 'abc_testtable' does not exist. (From file 3.10.0-2021-04-09.sql.)
    (Replace 'abc' by your table prefix.)
  2. Go to the Pre-Update Check or reload the page if still there, and expand the "Required PHP & Database Settings".
    Result: The "Database Table Structure Up to Date" check has failed.
  3. Go to "Extensions - Manage - Database" or reload the page if still there, and use the "Fix" button.
    Result: No database problems found.
  4. Go to the Pre-Update Check or reload the page if still there.
    Result: Required PHP and database settings are all ok.
  5. Using a tool like e.g. phpMyAdmin, modify the version number in column "manifest_cache" of the extension with id=700, which is the "files_joomla" extension (see following screenshot about where to find it), and apply the change:
    phpmyadmin-modify-manifest-cache
    In my test I have changed "3.10.0-alpha6-dev" to "3.10.0-alpha5-dev".
  6. Go to "Extensions - Manage - Database" or reload the page if still there, and check if database problems are found.
    Result: 1 database problem found. Database update version (3.10.0-alpha5-dev) does not match CMS version (3.10.0-alpha6-dev).
  7. Go to the Pre-Update Check or reload the page if still there, and expand the "Required PHP & Database Settings".
    Result: The "Database Table Structure Up to Date" check has failed.
  8. Go to "Extensions - Manage - Database" or reload the page if still there, and use the "Fix" button.
    Result: No database problems found.
  9. Go to the Pre-Update Check or reload the page if still there.
    Result: Required PHP and database settings are all ok.
  10. Using a tool like e.g. phpMyAdmin, copy the value of column "params" of the extension with id=23, which is the "com_config" extension, into a text editor for later use, then clear the field in database so that it's empty, and apply the change.
  11. Go to "Extensions - Manage - Database" or reload the page if still there, and check if database problems are found.
    Result: 1 database problem found. No default text filters found.
  12. Go to the Pre-Update Check or reload the page if still there, and expand the "Required PHP & Database Settings".
    Result: The "Database Table Structure Up to Date" check has failed.
  13. Go to "Global Configuration" and use the "Save & Close" button.
    This will fix the previously shown database error. Using the "fix" button in "Extensions - Manage - Database" for this purpose will not work for reasons not related to this PR, see comment #33080 (comment) below.
  14. Go to "Extensions - Manage - Database" or reload the page if still there, and check if database problems are found.
    Result: No database problems found.
  15. Go to the Pre-Update Check or reload the page if still there.
    Result: Required PHP and database settings are all ok.

Actual result BEFORE applying this Pull Request

The check if the database type will be supported by J4 is somewhere in the middle of the PHP related checks.

There is no check if the database checker has found database problems.

without-pr

Expected result AFTER applying this Pull Request

There is a new check if database structure is up to date.

The database checks have been moved to the bottom.

No database problems found, database structure is up to date:

with-pr_db-ok

Database problems found, tool-tip when hovering:

with-pr_db-problems

Additional information

At least the following two things should to be done with other PRs for the pre-update checks:

  1. The tool tip shown when a problem has been found is the same as used for some of the other required PHP settings if the check has failed (but of course with different text). These tool tips should be changed to a normal text shown below the check name and result so if is accessible and can provide a link e.g. in case of the check added by this PR to the database checker.

  2. If there is some failed check for a required PHP or database setting (including the one added by this PR here), there should be at least a confirmation dialogue shown like it is when an incompatible 3rd party system plugin has been found, or the same dialogue has to be shown in such a case but with a different or additional text. Or the update should be blocked in such cases. Either the one or the other way should be done.

Documentation Changes Required

The pre-update check documentation needs to be updated with the change made by this PR.

avatar richard67 richard67 - open - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 9 Apr 2021
Category Administration com_joomlaupdate Language & Strings
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar richard67 richard67 - change - 9 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 9 Apr 2021
avatar zero-24
zero-24 - comment - 12 Apr 2021

Looks cool thanks @richard67 ?

We might should move the checks a bit up in the order given that using a non-supported database version or other database errors are more critical than not supporting json right? Yes I know they are all required but seeing the issue would be more easier when its more up in the order right?

avatar richard67
richard67 - comment - 12 Apr 2021

We might should move the checks a bit up in the order given that using a non-supported database version or other database errors are more critical than not supporting json right? Yes I know they are all required but seeing the issue would be more easier when its more up in the order right?

No, to me it seems the database stuff is not easier to find when more up but in the middle of the PHP stuff. It's easier to find when it's upmost, or when it's downmost like with my PR.

avatar richard67
richard67 - comment - 12 Apr 2021

Another reason why I've moved them down was because they are mentioned as last in the headline "... & Database Settings".

As said, to me it seems they are easier to locate now.

@zero-24 If you insist on it I move them back up, but maybe let's get other opinions first.

avatar zero-24
zero-24 - comment - 13 Apr 2021

Its fine for me it was just an first idea, but your reasoning makes sense to me too. Lets get this one here tested ?

avatar richard67 richard67 - change - 14 Apr 2021
Labels Added: ? ?
avatar sksuryan
sksuryan - comment - 17 Apr 2021

@richard67 I was testing the for the last test case i.e. the one with empty JSON and these were the results.

image
image

Although, if I remove the empty JSON from the params, the error is definitely being thrown.
image
image

I'm currently using MySQL.

avatar richard67
richard67 - comment - 17 Apr 2021

@sksuryan Well if you managed to get the error shown and verified that the pre-update check shows it, too, the it was a successful test. The empty json might be needed depending on the database driver. The subject of the test was to show that if the database checker shows an error, then the pre-update check added by this PR here shows it, too.

avatar sksuryan
sksuryan - comment - 17 Apr 2021

@richard67 another thing I want to add here is that I'm not able to fix the last error using Extensions - Manage - Database > fix. The result remains the same nevertheless.

avatar richard67
richard67 - comment - 17 Apr 2021

The fix button is also not subject of this PR. If it doesn’t work for that empty params error that’s another issue. I‘ll try to find the error and will fix it with this or another PR. But as said, it’s not related to what this PR here does.

avatar sksuryan
sksuryan - comment - 17 Apr 2021

Alright, got it.

avatar sksuryan sksuryan - test_item - 17 Apr 2021 - Tested successfully
avatar sksuryan
sksuryan - comment - 17 Apr 2021

I have tested this item successfully on 39fa88e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33080.

avatar richard67
richard67 - comment - 17 Apr 2021

@sksuryan Thanks for testing.

I think the reason why the fix doesn't work is that in the extensions record for com_content, there are no filters defined which could be used for the fix in this function:

https://github.com/joomla/joomla-cms/blob/3.10-dev/administrator/components/com_installer/models/database.php#L246

I.e. the following line of code is executed, but $contentParams->get('filters') returns and empty string (which is right because there is no filters property in paramts of the com_content extension);

https://github.com/joomla/joomla-cms/blob/3.10-dev/administrator/components/com_installer/models/database.php#L257

I think I have to adjust my testing instructions for that.

This particular database error (empty filter parameters for com_config) and the fix for it are only relevant when updating from old versions (2.5) to later and should meanwhile be obsolete anyway.

avatar richard67 richard67 - change - 17 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 17 Apr 2021
avatar richard67
richard67 - comment - 17 Apr 2021

@sksuryan I've updated the testing instructions to your finding. Thanks for reporting.

avatar sksuryan
sksuryan - comment - 17 Apr 2021

@sksuryan Thanks for testing.

I think the reason why the fix doesn't work is that in the extensions record for com_content, there are no filters defined which could be used for the fix in this function:

https://github.com/joomla/joomla-cms/blob/3.10-dev/administrator/components/com_installer/models/database.php#L246

I.e. the following line of code is executed, but $contentParams->get('filters') returns and empty string (which is right because there is no filters property in paramts of the com_content extension);

https://github.com/joomla/joomla-cms/blob/3.10-dev/administrator/components/com_installer/models/database.php#L260

I think I have to adjust my testing instructions for that.

This particular database error (empty filter parameters for com_config) and the fix for it are only relevant when updating from old versions (2.5) to later and should meanwhile be obsolete anyway.

oh, alright!

@sksuryan I've updated the testing instructions to your finding. Thanks for reporting.

Happy to help!

avatar richard67 richard67 - change - 17 Apr 2021
The description was changed
avatar richard67 richard67 - edited - 17 Apr 2021
avatar jwaisner jwaisner - test_item - 18 Apr 2021 - Tested successfully
avatar jwaisner
jwaisner - comment - 18 Apr 2021

I have tested this item successfully on 39fa88e

Tested on MySQL 8.0.16.

Everything checks out on the pre-update checks. Everything works as expected.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33080.

avatar jwaisner jwaisner - change - 18 Apr 2021
Status Pending Ready to Commit
avatar jwaisner
jwaisner - comment - 18 Apr 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/33080.

avatar zero-24
zero-24 - comment - 20 Apr 2021

Merging for now thanks @richard67

avatar zero-24 zero-24 - change - 20 Apr 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-04-20 18:18:03
Closed_By zero-24
Labels Added: ? ?
avatar zero-24 zero-24 - close - 20 Apr 2021
avatar zero-24 zero-24 - merge - 20 Apr 2021

Add a Comment

Login with GitHub to post a comment