Convert remaining 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 initial functions in update.inc file to a class
#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.

update_replace_permissions was removed in #3143605: Remove unused and broken update_replace_permissions(), which always throws an exception.

The remaining functions after #3391683: Convert initial functions in update.inc file to a class are 12 files changed, 677 insertions(+), 408 deletions(-), which is a bit large, but reviewable.

Blocker is in!

I interactive rebased to remove the squashed commit from #3391683: Convert initial functions in update.inc file to a class, and then resolved the conflicts by re-adding the promoted constructor arguments for UpdateRegistry and ModuleExtensionList.

I think this is almost ready, one question about the constructor.

I pulled it down and all references to the functions have been replaced except a couple of comments in the file that is being deprecated.

I also used the zebra git command to compare and all changes line up as expected they are faithful conversions.

I think we just need to move the new arguments to the end and add BC.

Nevermind about the constructor comment, that class is new and isn't in a release yet, as long as we get it in for 11.5 is clean!

Also forgot to mention I checked all of the deprecation messages and versions and they look right.

CR looks good too.

Seems like an unrelated random functional javascript failure in core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php

Rebased and the random FunctionalJavascript test failure went away. Still RTBC.

@larowlan ran this passed release managers, and we're no longer need able to make 12.0.0/11.5.0 for this so we need to do the following:

1. Split the change record into a new change record
2. Update the deprecation notices changed in this issue to deprecated in 12.1.0 instead and update the change record URL.

One task down, change record is split and set for 12.1.0 and 12.1.x

I added suggestions for the version and cr change!

Back to Needs review.

Moving to needs works cause technically we still need bc for the two new arguments, here is an example: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/user/...

The changes made look good other than that.

CR is good too.

Arguments are now nullable with a trigger_error for BC.

Perfect thanks!

I think this is ready again!

couple of things in the MR - thanks

Going back to Needs review first before RTBC.

All feedback has been addressed!