[4.2] Fix upgrade fatal error

[joomdonation]

Pull Request for Issue #37177.

Summary of Changes

Currently, if someone uses repeatable fields on their Joomla 3 installation, upgrade to Joomla 4.0.4 or newer version will get fatal error as described in the issue #37177 . This PR fixes that error.

Testing Instructions

1. Install Joomla 3.10.11 (latest Joomla 3 version)
2. Go to Content -> Fields, add a Repeatable field
3. Go to Content -> Articles, add an article, enter data for that Repeatable field
4. Try to upgrade the site to latest Joomla 4. You will see fatal error. You should make a backup of the current site before upgrading so that you don't have to setup it again for the next step
5. Restore the above site. Try to update your site to the update package generated by this PR which can be downloaded at https://ci.joomla.org/artifacts/joomla/joomla-cms/4.2-dev/39245/downloads/59693/Joomla_4.2.6-dev+pr.39245-Development-Update_Package.zip , or use the custom update URL for that package: https://ci.joomla.org/artifacts/joomla/joomla-cms/4.2-dev/39245/downloads/59693/pr_list.xml . Make sure upgrade went smooth.

Actual result BEFORE applying this Pull Request

Fatal error.

Expected result AFTER applying this Pull Request

No fatal error. Upgrade works well.

[joomdonation]

Ping @nikosdion for taking a look as we discussed yesterday. Just for information, after thinking more about it, I decided to add necessary code to restore_finalisation.php instead of to finalisation.php because these code is a legacy thing, so I don't want to mix it into finalisation.php which is the real code use by our update process.

[richard67]

Just for information, after thinking more about it, I decided to add necessary code to restore_finalisation.php instead of to finalisation.php because these code is a legacy thing, so I don't want to mix it into finalisation.php which is the real code use by our update process.

To me that seems to be a good idea.

[nikosdion]

I'm overall mostly satisfied, but for the fact that I want to make the code a bit more future-proof.

[joomdonation]

Thanks @nikosdion . I applied the suggested changes. Should be OK now.

[nikosdion]

I have tested this item ✅ successfully on 61d5dc4

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39245.}

[richard67]

I have tested this item ✅ successfully on 61d5dc4

@nikosdion Was that a real test with updating a 3.10 with repeatable fields in use? If not and it was only a code review, then the GitHub approval tells this already. The test counter of the issue tracker should only be used for tests according to the testing instructions.

[nikosdion]

@richard67 No, I was trying to see if I can get the approvals count go up on GitHub. Sorry. Can you remove my test? I can't find out how without marking a negative test 🤦🏽

[richard67]

@nikosdion You can use option "I have not tested this item" (or similar).

[nikosdion]

I have not tested this item.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39245.}

[nikosdion]

Thanks! I was blind to that text. The green and red text stuck out, the other option I didn't even notice until you pointed me to it. Ooooooops!

[richard67]

I think we should do real test not only with updating a 3.10.11 with repeatable field in use but also test updating a 4.0.4 or newer 4.x.y to be sure this is not broken somehow.

@nikosdion What do you think? Am I paranoid?

[richard67]

P.S.: I will test tomorrow.

[richard67]

@nikosdion P.S.: Your review and approval might not count as a human test result in the issue tracker, but of course it counts somehow (I would count it like a maintainer's approval which I would require anway for all changes on the updater), and it means much to us (at least me and I am sure also @joomdonation ) . We are really happy to have you in our boat. Just wanted to say because I felt like it.

[richard67]

First test with updating a current 3.10-dev branch was successful. I could reproduce the issue without the PR, and with the PR the update worked, and the field was properly converted. Will continue with testing other scenarios before posting a test result, but I am optimistic.

[richard67]

Unfortunately on a shared hosting with opcache being in use I get an internal server error 500 with an error 0 "no active transaction" at the finalisation step. Last entry in the log file is "Deleting removed files and folders.", i.e. the database updates succeeded. I have to investigate if it also happens with an update package without this PR and if it's related to opcache. When that error happens, the backend is shown without any styling, but after reloading the page the backend looks ok and reports the update suceeded.

Will report back when I know more.

[richard67]

It seems to be a problem with PDO only, not with MySQLi, and it's very likely not related to this PR, see https://www.php.net/manual/en/migration80.incompatible.php#migration80.incompatible.pdo-mysql and e.g. doctrine/migrations#1202 . Will continue to investigate if related to this PR or not.

[richard67]

Hmm, with a nightly 4.2.6-dev without this PR under the same conditions (PDO and MariaDB 10.6) the error did not happen. So maybe it is somehow related to this PR, or not caused by the changes in this PR but unmasked by them?

[richard67]

@nikosdion @joomdonation As far as I could find out, the workaround alone doesn't help with PDO, at least not with PostgreSQL. The error happens inside the $fieldModel->save method, so we end up here:

joomla-cms/administrator/components/com_admin/script.php

Line 356 in 7b4a59e

throw new \Exception($error);

I have a fix which works, but it is a fix for the PDO drivers in the database framework. The fix is not to start a transaction when the "inTransaction" of the PDO connection says we are already inside a transaction. I'll post here when I have a PR ready for the framework.

Update: My fix for the framework seems to work, but I think it is not right, so if may take a bit longer until I have something.

[richard67]

As the error happens somewhere with the $fieldModel->save call, one way out could be not to use the model but to save with a query.

[joomdonation]

@richard67 If fix the transaction error is that hard, I would remove the usage of transaction code in uninstallRepeatableFieldsPlugin method.

[richard67]

@richard67 If fix the transaction error is that hard, I would remove the usage of transaction code in uninstallRepeatableFieldsPlugin method.

@joomdonation Either this, or we move the transaction start with Nik’s workaround to below the loop where we use the model for writing. I am at work and don’t have much time now, but I can explain all later and try to provide a fix without framework changes.

[joomdonation]

OK. Thanks @richard67 for your hard work on this issue.

[richard67]

Now I have the time to write more details.

As said, the error with transaction start on PostgreSQL happens with the $fieldModel->save call. I think (but haven't found yet where it happens) that there is a table lock made, possibly somewhere in the admin model or the table class.

The PostgreSQL driver starts a transaction when locking tables because table locks are only allowed inside transactions on PostgreSQL, see https://github.com/joomla-framework/database/blob/2.0-dev/src/Pgsql/PgsqlDriver.php#L506 . Other drivers don't do that.

The way out for now would be either not to use transactions at all in the uninstallRepeatableFieldsPlugin method, or we use a transaction only for that part coming after the foreach loop, i.e. only for unprotecting the plugin and then uninstalling it, like we have it in the uninstallEosPlugin. We still could need some of @nikosdion 's workarounds or all of them.

I'll work on it, but I'd like to have opinions to see if there are maybe better ideas.

[richard67]

@joomdonation I've made a PR for you with the necessary fixes to make it work with any database and driver: joomdonation#5
But it means we do not use a transaction for the field and field value changes.
See the description in that PR.
Please review and comment there. I would value your and @nikosdion 's opinion.

[richard67]

@wilsonge Would be happy to have your opinion, too, on my proposed change for this PR here: joomdonation#5 . For understanding the issue with transactions you would have to read all my previous comments. Maybe you have an idea for a better fix.

[richard67]

I have meanwhile a better fix with transactions for each field's migration so we have consistent data in case of a failure. I just have to test it.

[richard67]

@joomdonation I've updated my PR for you so it uses single transactions for each field's migration's SQL statements and another transaction for unprotecting and uninstalling the plugin. This leaves consistent data in case of a failure so with another attempt to update (or reinstall core files, if that runs the finalization step, too, of which I am not sure now) it could be fixed. It works now with all databases and drivers, and it doesn't need @nikosdion 's workarounds for transaction stuff. The drivers handle that already.

[nikosdion]

@richard67 I prefer your approach.

[richard67]

I have tested this item ✅ successfully on 1039baf

I could reproduce the issue and verify that this PR fixes it.

The repeatable fields are correctly converted on update.

The update log "administrator/logs/joomla_update.php" shows at the end the deleting files and the cleanup steps after the successful end of SQL updates and the final completion message:

End of SQL updates.
Deleting removed files and folders.
Cleaning up after installation.
Update to version 4.2.6-dev+pr.39245 is complete.

I've tested both the live update with the custom URL and upload and update with the update package, and I've tested on different Linux environments (local VM and shared hosting on IONOS) with different database types (MySQL 8, MariaDB 10.6, PostgreSQL 14.5) and in case of MariaDB with both the "MySQLi" and the "MySQL (PDO)" drivers.

And I've tested with several repeatable and not repeatable fields in several articles.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39245.}

[joomdonation]

Today I tested upgrade again with latest change from @richard67 and it worked well for both MySQL and PDO MySQL and it worked well. Unfortunately, I don't have Postgresql installed here to test.

[fancyFranci]

Thank you very much for your important contribution!