Fix installer hint "/** CAN FAIL **/" for update SQL scripts not working with MySQLi driver#39391
Conversation
|
I have tested this item ✅ successfully on 814bc90 Tested with a custom component, but followed the testing instructions of this pr. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39391. |
|
I have tested this item ✅ successfully on 814bc90 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39391. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39391. |
|
Thanks guys for testing. As the upcoming 4.2.6 is already being prepared, this PR will very likely go into 4.2.7. So don’t be surprised about that. |
Co-authored-by: SharkyKZ <sharkykz@gmail.com>
|
Back to pending after changes. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39391. |
This comment was marked as resolved.
This comment was marked as resolved.
|
@ReLater Can it be that you have forgotten to select the test result? |
|
I have tested this item ✅ successfully on 56b8071 Tested with a custom component, but followed the testing instructions of this pr. Plus several other sql lines. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39391. |
|
I have tested this item ✅ successfully on 56b8071 Joomla 4.2.6-rc1 Works, BUT... does really any driver return RuntimeException if execute() fails? Eg. in PDODriver.php an ExecutionFailureException will be thrown; is it a kind of RuntimeException and will be catched within installer.php if only RuntimeException is defined? Just my 2 cents, maybe I'm wrong... I'm not very familiar with RuntimeExceptions... This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39391. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39391. |
|
Remove updates requested label This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39391. |
|
@richard67 User @dawe78 asked this:
Now I wonder if we should use |
@roland-d All the possible exceptions extend from runtime exceptions. Of course exception would also work. Feel free to provide a better fix, then I close my PR in favor of that. |
In that case, I am fine and we do not need any further changes. |
|
@roland-d You can consider it as a workaround because on the long run we indeed have to revisit exception handling in the framework in the database drivers. There have been made changes in the PHP clients (mysqli and PDO) with 8.0 and 8.1 so it can be that they throw exceptions in these versions and returned false in other versions for certain methods. But that’s a bigger piece of work so for the meantime we can fix it here. |
|
P.S.: In principle the right fix would be like suggested in that comment here #39333 (comment) by @dawe78 , but it would need to investigate if that should also be done in other methods and for other drivers for making a clean solution in the database drivers. That’s why I did not use that suggestion but fixed it here now. |
|
Thank you |
…ing with MySQLi driver (joomla#39391) * Catch also RuntimeException when update SQL fails * Fix missing backslash for global name space * Catch only \RuntimeException Co-authored-by: SharkyKZ <sharkykz@gmail.com> * Remove obsolete use statement Co-authored-by: SharkyKZ <sharkykz@gmail.com>
Pull Request for #39333 (comment) .
Summary of Changes
With Pull Request (PR) #36506 , the possibility to add an installer hint
/** CAN FAIL **/to SQL statements in update SQL scripts has been added, so that the installer ignores the failure and doesn't abort the SQL updates but continues with the next statement.This makes it possible to handle e.g. SQL errors when trying to add columns which already exist, which can happen if an update SQL scripts is executed multiple times or the update history of a component is complicated. The reason why this is needed in that use case is because "IF EXISTS" or "IF NOT EXISTS" clauses on adding or removing columns is only supported by MariaDB but not by MySQL.
However, depending on the database type (MySQL or MariaDB) and the version and which database client (MySQLi or PostgreSQL) are used, and also on the kind of SQL statement, this doesn't work in all cases since the database client throws different kinds of exceptions depending on all that. In case of the MySQLi driver, the client doesn't return false but raises an exception when executing the SQL statement fails.
See the discussion beginning with this comment: #39333 (comment) .
This PR here fixes that by adding
\RuntimeExceptionto the try catch where we check the installer hint.Testing Instructions
This test needs to be executed with a MySQL or MariaDB database with the MySQLi database driver and with PHP 8.1.
With PDO or with PHP 7.4 and the MySQLi driver, everything works without this PR.
Download and install the following extension zip package: https://test5.richard-fath.de/com_testupdatesql_1.0.0.zip
This will install version 1.0.0 of a dummy component named "Test Update SQL" (com_testupdatesql), which has a database table named
#__testupdatesql_table_1(with#__equal to your database prefix).The table has an
idand atitlecolumn and one record withid=1 andtitle='Record 1'.In phpMyAdmin, execute the following SQL statement to add a new columd
descriptionto that table:(replace
#__by your database prefix).This will simulate the situation after a failed update attempt to the next version 1.0.1 (see next step below) or the situation having a remainder from a previously installed version 1.0.1 where the database table has not been properly removed on uninstallation, or the situation when the extension developer had not provided SQL scripts in the right way so.
Update to version 1.0.1 of that component by downloading and installing the following extension zip package: https://test5.richard-fath.de/com_testupdatesql_1.0.1.zip
This update will add a new column
descriptionto the database table, then update the existing first record by a suitable value for that column, and then insert a new, 2nd record into the table.Result:
When this PR is not applied, the update fails with SQL error "Duplicate column name 'description'", see section "Actual result BEFORE applying this Pull Request"
below.
When this PR is applied, the update succeeds, see section "Expected result AFTER applying this Pull Request"
below.
Verify the table structure and content in phpMyAdmin.
Examine the installation and update SQL scripts of the 2 versions mentioned above and make sure you understand them and that they are doing everything right like it should be in update SQL scripts for Joomla 4.
Hint: I could have created the 2nd zip in a way so it causes the error immediately without the need for executing step 2, but I wanted the scenario to use correct packages so testers understand what happens.
Actual result BEFORE applying this Pull Request
The update fails with SQL error "Duplicate column name 'description'".
The SQL statement to add the column and all statements after that haven't been executed, so the table still has one record only and no
descriptioncolumn.Expected result AFTER applying this Pull Request
The update succeeds.
All database changes have been applied, so the table has two records with the right
descriptionvalues.Link to documentations
Please select:
Documentation link for docs.joomla.org:
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed
Additional remarks on documentation
It doesn't need to update any existing documentation because we don't have any which tells extension developers in detail how their update SQL scripts should be so that they are accepted by the database checker, too, and how to use the "/** CAN FAIL **/" installed hint or "INSERT IGNORE".
We should create such a documentation, but this should not stop this PR here.