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.

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.

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

I have tested this item ✅ successfully on 61d5dc4

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.

@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 ??

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

I have not tested this item.

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!

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?

P.S.: I will test tomorrow.

@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.

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.

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.

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.

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?

I have tested this item ? unsuccessfully on 61d5dc4

Like @richard67 I also received "0 no active transaction" on update. Using MySQL PDO on MySQL 8.0.22.

However, the site was upgraded correctly and the repeatable field converted to a subform with my sole field in it converted to a subform-only field.

In my experience, this is definitely linked to using PDO instead of MySQLi. If you try to run $db->transactionCommit() without an active transaction PDO throws a fatal whereas MySQLi simply returns false IIRC.

I would suggest fixing that problem as well since the objective here is to fix the update experience for Joomla 3 upgraders.

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?

The thing is that the nightly 4.2.6-dev does not have the changes in the restoration.php, therefore it does not run any of the database upgrade code post-upgrade. It makes sense to me that you don't get a database error when the database code never ran ?

But, seriously now, somewhere we have an errant ->transactionCommit() which should be wrapped in a try-catch block. That's the problem to solve. See my test result description for some pointers.

@nikosdion Do you have an idea how we can fix that? Or is it something to be fixed in the database driver fro the framework?

I see I was too slow with typing

@richard67 Tee-hee!

You can set Debug Site to Yes so the error give you a trace. You should then see where it comes from, tell me, and I can try to parse it into something actionable. I'm off testing other PRs now.

I will continue tomorrow

@nikosdion @richard67 Thanks for testing the PR and providing details feedback. I believe the issue comes from the block of code which handle uninstalling repeatable fields https://github.com/joomla/joomla-cms/blob/4.2-dev/administrator/components/com_admin/script.php#L242-L470

I haven't used db transaction before, so no idea why that error happens yet. Will try to look at it later today. In the mean time, if you have any idea/instructions to solve that error, please suggest.

Many thanks !

OK. I wrap $db->transactionCommit(); in a try ... catch block and it should address the error now. Would be great if you could review my last commit to see if there could be any problem with it. Thanks @nikosdion @richard67

@joomdonation I've allowed myself to update the links to the update package and to the custom update URL (which I had allowed myself to add yesterday) to the last build so they point to the right package.

@joomdonation Now it works with the MySQL (PDO) driver and MariaDB, too. But meanwhile I found the time to test it also with PostgreSQL, and there I still get another exception: "0 There is already an active transaction". That could mean that we have to wrap also the $db->transactionStart(); in line 243 into a try catch, or we commit the previous transaction, if any, which we also would have to wrap into a try catch.

I have tested this item ? unsuccessfully on 7b4a59e

It works now for "MySQL (PDO)" but not for PostgreSQL, see my previous comment.

Wrapping the transactionStart will not be enough. You will need to do two separate things there:

// Make sure there is no other transaction present
try {
  $db->transactionCommit();
} catch (\Throwable $e) {
  // No worries if there was no other transaction present.
}

// Try to start a new transaction
try {
  $hasTransaction = true;
  $db->transactionStart();
} catch (\Throwable $e) {
  $hasTransaction = false;
}

Towards the end of the method we should only call transactionRollback and transactionCommit (wrapped in their own try-catch blocks) only if the $hasTransaction flag is enabled.

This kinda simulates what the Joomla MySQLi driver does internally for transactions. You don't really have to use the flag, it will just avoid triggering the try-catch blocks when we are absolutely sure that there is actually no transaction present.

@nikosdion Regarding the error "0 There is already an active transaction" I've found following: https://stackoverflow.com/a/41749323 . That means we maybe should fix our transaction handling in the PDO drivers in the database framework. For the mean time your suggested workaround to be used here would be a solution.

@richard67 Look, MySQL does not support nested transactions. Ideally, the PDO database driver should catch this error by itself and auto-commit the last transaction when we ask it to to open a new one. Then again, this might actually be worse because it can mask bugs.

In this case we are discussing, most likely there is something else which has already started a transaction but never committed or rolled it back. Ideally, this is what we need to fix. However, what I proposed is a crude but effective workaround.

@nikosdion Do we run the SQL update scripts in a transaction? If so, this would be a problem since they contain not only DML but also DDL, which depending on the server settings may cause an auto-commit of previous DML, and so it may depend on if the last SQL statement is a DML or a DDL if there is an open transaction or not at the end of the SQL update scripts.

@richard67 Off the top of my head, I don't think we do. Even if we did, though, a DDL query immediately commits and closes the transaction. Therefore we would not have an exception calling transactionStart, we'd have an exception calling transactionCommit as there would be no open transaction anymore.

@richard67 Off the top of my head, I don't think we do. Even if we did, though, a DDL query immediately commits and closes the transaction. Therefore we would not have an exception calling transactionStart, we'd have an exception calling transactionCommit as there would be no open transaction anymore.

Not if the last statement in the last update SQL is a DML statement, which is the case.

@joomdonation I'm just preparing here a modified patch with the changes suggested by @nikosdion . If that works, I can make a PR against your branch with it.

@joomdonation I'm just preparing here a modified patch with the changes suggested by @nikosdion . If that works, I can make a PR against your branch with it.

Great. Thanks @richard67

I've added @nikosdion 's workaround to both methods "uninstallRepeatableFieldsPlugin" and "uninstallEosPlugin" in script.php, but it still fails with the "There is already an active transaction" error. I continue to investigate.

The problem could be that the installer itself also does a transaction start in its "updateSchemaTable" method.

Does it commit the transaction...?

Ha .. the "updateSchemaTable" method in the installer does a transaction in which it first deletes the schema record and then inserts it. This being failing could cause things like we had been reported in past about the database checker not working due to a missing schema record in the db, of which we (Brian T.) always said it is caused by the provider's custom installer, but at least one case was reported by Troy (Bear) where no custom installer had been used.

Does it commit the transaction...?

@nikosdion Yes, it does, but it doesn't handle the other issues which we try to workaround here, so we might have to add it there. Look here where the problem is: https://github.com/joomla/joomla-cms/blob/4.2-dev/libraries/src/Installer/Installer.php#L1339-L1369

DELETE and INSERT are both allowed in a transaction. They DML. Only TRUNCATE TABLE is considered DDL (as it essentially drops and creates the table afresh).

If it does not commit the transaction, though, the DELETE and INSERT won't take place until there is another DDL or a transactionCommit executed. That would indeed explain such issues.

Makes sense?

@nikosdion I understood that all, it's not that I wouldn't know that. The problem with PostgreSQL could be caused by the driver. It has here a start transaction in its "lockTable" method: https://github.com/joomla-framework/database/blob/2.0-dev/src/Pgsql/PgsqlDriver.php#L506 , but it does not do any commit or roll back. The MySQLi driver doesn't have that, also not in the subsequently called method: https://github.com/joomla-framework/database/blob/2.0-dev/src/Mysqli/MysqliDriver.php#L840

P.S.: The "unlockTable" method of the PostgreSQL driver does a commit. But maybe somewhere a table is locked but not unlocked?

of which we (Brian T.) always said it is caused by the provider's custom installer,

I know for a fact that a providers custom installer was a source as it was missing a step

of which we (Brian T.) always said it is caused by the provider's custom installer,

I know for a fact that a providers custom installer was a source as it was missing a step

@brianteeman I haven't said that this was not right. I have only said that in case of Troy it might have been a different cause.

@nikosdion The call stack says that the problem happens here:

@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:

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.

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.

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

@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.

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

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.

@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.

@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.

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.

@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.

@richard67 I prefer your approach.

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.

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.

Thank you very much for your important contribution!