Convert the update.inc file functions to a class

Change call update_do_one to Update::doOne in triggerBatch()

Change to Update::class

Added deprecations

Fix deprecations errors

Add legacy tests for update_fix_compatibility() and update_check_incompatibility()

Add tests for 3 functions. Intediff has #13 and #14.

Add new test methods and fix exists methods

Add new test methods and fix exists methods

Add new test methods and fix exists methods

Initial review, thanks for great work!

- lots of \Drupal:: calls inside loops - needs refactor
- probably this class needs split into static methods (batch related) and service to properly inject dependencies, guess multiple classes
- usage needs better analysis, if there will be a service it needs injection
- naming - requirements could use "get" or "build"... like #2620304: htaccess functions should be a service

will check deeper later

+++ b/core/lib/Drupal/Core/Update/Update.php
@@ -0,0 +1,714 @@
+    $extension_config = \Drupal::configFactory()->getEditable('core.extension');
...
+      $themes = \Drupal::service('theme_handler')->rebuildThemeData();
+      $modules = system_rebuild_module_data();
...
+    $system_schema = drupal_get_installed_schema_version('system');
...
+    $requirements = \Drupal::moduleHandler()->invokeAll('requirements', ['update']);
...
+    \Drupal::keyValue('system.schema')->set($module, $schema_version);
...
+    require_once \Drupal::root() . '/core/includes/module.inc';
+    system_list_reset();
...
+    module_load_include('php', $module, $module . '.post_update');
...
+          \Drupal::service('update.post_update_registry')->registerInvokedUpdates([$function]);
...
+    $modules = drupal_get_installed_schema_version(NULL, FALSE, TRUE);
...
+      $updates = drupal_get_schema_versions($module);
...
+        $last_removed = \Drupal::moduleHandler()->invoke($module, 'update_last_removed');
...
+      $updates = drupal_get_schema_versions($module);
...
+    foreach (\Drupal::keyValue('system.schema')->getAll() as $module => $schema) {
...
+    $role_names = \Drupal::service('config.storage')->listAll($prefix);
...
+      $config = \Drupal::configFactory()->getEditable("user.role.$rid");

All this dependencies makes me think about service, OTOH there still procedural things used that means it will be hard to test this new class

Checked new class and I really like to split static methods from real service
Also new class should not use module_load_include() and others functions which supposed to be deprecated in other "include conversions" - so better to add protected methods for them with @todo to related conversion issue

+++ b/core/lib/Drupal/Core/Update/Update.php
@@ -0,0 +1,715 @@
+    $extension_config = \Drupal::configFactory()->getEditable('core.extension');
...
+    $system_schema = drupal_get_installed_schema_version('system');
...
+    $requirements = \Drupal::moduleHandler()->invokeAll('requirements', ['update']);
...
+    \Drupal::keyValue('system.schema')->set($module, $schema_version);
+    \Drupal::service('extension.list.profile')->reset();
+    \Drupal::service('extension.list.module')->reset();
+    \Drupal::service('extension.list.theme_engine')->reset();
+    \Drupal::service('extension.list.theme')->reset();
+    drupal_static_reset('drupal_get_installed_schema_version');
...
+      drupal_set_installed_schema_version($module, $number);
...
+    module_load_include('php', $module, $module . '.post_update');
...
+          \Drupal::service('update.post_update_registry')->registerInvokedUpdates([$function]);
...
+    $modules = drupal_get_installed_schema_version(NULL, FALSE, TRUE);
...
+        $last_removed = \Drupal::moduleHandler()->invoke($module, 'update_last_removed');
...
+      $updates = drupal_get_schema_versions($module);
...
+    return $number <= drupal_get_installed_schema_version($module);
...
+    foreach (\Drupal::keyValue('system.schema')->getAll() as $module => $schema) {
...
+      module_load_install($module);
...
+    $role_names = \Drupal::service('config.storage')->listAll($prefix);
...
+      $config = \Drupal::configFactory()->getEditable("user.role.$rid");

All this services & functions will need injection means this class can't be static

It needs reroll but maybe postpone it on other conversions?

I mean this 2
- #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry)
- #2908886: Split off schema management out of schema.inc

+++ b/core/includes/update.inc
@@ -109,14 +82,15 @@ function update_check_requirements() {
+  @trigger_error("update_set_schema() is deprecated in Drupal 8.7.0-dev and will be removed before Drupal 9.0.0. Use  \Drupal\Core\Update\Update::setSchema() instead. See https://www.drupal.org/node/3013060", E_USER_DEPRECATED);
+  Update::setSchema($module, $schema_version);

It slightly related to #2908886-65: Split off schema management out of schema.inc

+++ b/core/tests/Drupal/KernelTests/Core/Update/UpdateLegacyTest.php
@@ -0,0 +1,242 @@
+   * Tests update_check_incompatibility() function.
...
+    $this->assertTrue(update_check_incompatibility('incompatible_module'));

I filed separate issue to remove the function without replacement #3150726: Deprecate update_check_incompatibility as unused

The next hunk to decouple is update_system_schema_requirements(), update_check_requirements(), _update_fix_missing_schema() - the only usage found is \Drupal\system\Controller\DbUpdateController::handle() so this requirements could be moved to system module

EDIT there's \Drupal\Core\Updater\Module::getSchemaUpdates() so part of conversion could be moved to extension lists

Opened #3210900: Deprecate update_set_schema() for removal as I don't think that function is needed at all.

Re #38 - there's usage of update_check_requirements() in drush https://github.com/drush-ops/drush/blob/dbdb6733655231687d8ab68cdea6bf9f...

Probably better to file separate issue as well

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 ?

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

Reran the tests and the 1 failure seems legit.

Did not review.

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. #3050578: Remove unused function getSchemaUpdates()

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
#3391680: Convert the update.inc file functions to a class pt1
#3391683: Convert the update.inc file functions to a class pt2
#3391684: Convert the update.inc file functions to a class pt3
#3391685: Convert the update.inc file functions to a class pt4

3 functions per ticket.

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