Convert the update.inc file functions to a class

According to #43, this was postponed on a number of other issues. All those issues are now committed, so I think work can continue on this one.

Patch needs update as there is only left - update requirements and batch to run'em

Hi @andypost can you give more details about it ?

Updating the patch

Now everything goes to 11.x https://www.drupal.org/about/core/blog/new-drupal-core-branching-scheme-... →

Opened MR, rebased initial work against 11.x branch. Ready for the review.

Reran the tests and the 1 failure seems legit.

Did not review.

Addressed review comments

Wonder if the CR needs updating searching for

update_fix_compatibility
update_check_incompatibility
update_set_schema
update_replace_permissions

I don't find them in updates.inc maybe deprecated and removed already?

Added suggestion and tests should compare result of old vs new function calls

CR needs clean-up as since 8.7 core already deprecated and removed some functions, for example via related

I removed the todo in \Drupal\Core\Updater\Module::getSchemaUpdates because it didn't have an associated issue. Also, I can find no usages of the method either. That needs some investigation.

Ah, there is an issue for that. 📌 Remove unused function getSchemaUpdates() Fixed

The MR is updated and tests are passing. Time to get another round of review.

I have updated the CR,

Left some comments on the MR.

I am not sure about the testing being asked for. The testing being asked for is to compare result of the old function execution with the replacement result. However, there are no explicit tests of this legacy code. Any testing of it is implicit in other update tests which rely on this code. And we have all tests passing and I think that is the best we can do.

Then, that means that UpdateLegacyTest is only testing that the exception is thrown. I am not sure we need to do that in this case either. So, I discussed this with catch and he did say that UpdateLegacyTest can be removed.

Therefor, I have removed that test.

All green and my feedback was addressed by #66, so marking this.

This will be good to clean up. That said, though, the diff is massive -- 11 files, +868, −464. That is a total of 1332 changed lines, five to ten times as many lines as a reviewer can effectively review. (And it's exponential decay -- that means having this all lumped into one change set means a reviewer will find 1-5% of the bugs they would find if the scoping were better. Especially since it's moved lines, which git does not deal well with.

Is there any way we could split this up into stages?

I see your point @xjm but think the high number of changes is due to whole functions being moved. But if it's a blocker I imagine each function could be broken out to individual tickets.

Moved functions are harder to review than changed ones. So I really would suggest splitting this into logical steps. What has to happen first? What's second? Etc. You can use the original MR to make sure everything will still end up in the same final state, but a little git add -p etc. could make this easier to get in.

Opened
📌 Convert the update.inc file functions to a class pt1 Active
📌 Convert the update.inc file functions to a class pt2 Active
📌 Convert the update.inc file functions to a class pt3 Active
📌 Convert the update.inc file functions to a class pt4 Active

3 functions per ticket.

Updated this ticket to include the last 3, if the MR could be updated for that.