User tests: Successful: Unsuccessful:
Pull Request for Issue # .
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.
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.
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;
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.
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:
Database problems found, tool-tip when hovering:
At least the following two things should to be done with other PRs for the pre-update checks:
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.
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.
The pre-update check documentation needs to be updated with the change made by this PR.
Status | New | ⇒ | Pending |
Category | ⇒ | Administration com_joomlaupdate Language & Strings |
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.
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.
Its fine for me it was just an first idea, but your reasoning makes sense to me too. Lets get this one here tested
Labels |
Added:
?
?
|
@richard67 I was testing the for the last test case i.e. the one with empty JSON and these were the results.
Although, if I remove the empty JSON from the params, the error is definitely being thrown.
I'm currently using MySQL.
@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.
@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.
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.
Alright, got it.
I have tested this item
@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:
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);
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.
@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:
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);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!
I have tested this item
Tested on MySQL 8.0.16.
Everything checks out on the pre-update checks. Everything works as expected.
Status | Pending | ⇒ | Ready to Commit |
RTC
Merging for now thanks @richard67
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:
?
?
|
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?