Remove UpdateHookRegistryFactory and UpdateRegistryFactory services

Tests need work to accommodate this change.

Nope, above my brain capacity.

Spokje → changed the visibility of the branch 3419914-remove-updatehookregistryfactory-and to hidden.

Unsure about how to deprecate the changed arguments in Update[Hook]Registry.

There's not much to "repair" if they send a non-associative array and/or an KeyValueStoreInterface instead of an KeyValueFactoryInterface.

Seems some of the deprecations are targeting 10.1

fixed deprecations

My feedback has been addressed.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

catch → closed merge request !6761

Came across a BC in 10.3.x as Drush has the following call in \Drush\Commands\core\DeployHookCommands::getRegistry:

        $registry = new class (
            \Drupal::getContainer()->getParameter('app.root'),
            \Drupal::getContainer()->getParameter('site.path'),
            array_keys(\Drupal::service('module_handler')->getModuleList()),
            \Drupal::service('keyvalue')->get('deploy_hook')
        ) extends UpdateRegistry {
            public function setUpdateType(string $type): void
            {
                $this->updateType = $type;
            }
        };

The throws exception is about the changed 4th argument:

TypeError: Drupal\Core\Update\UpdateRegistry::__construct(): Argument #4 ($key_value_factory) must be of type Drupal\Core\KeyValueStore\KeyValueFactoryInterface, Drupal\Core\KeyValueStore\DatabaseStorage given, called in /var/www/html/vendor/drush/drush/src/Commands/core/DeployHookCommands.php on line 42 in /var/www/html/web/core/lib/Drupal/Core/Update/UpdateRegistry.php on line 90 #0 /var/www/html/vendor/drush/drush/src/Commands/core/DeployHookCommands.php(42): Drupal\Core\Update\UpdateRegistry->__construct()
#1 /var/www/html/vendor/drush/drush/src/Commands/core/DeployHookCommands.php(93): Drush\Commands\core\DeployHookCommands::getRegistry()
#2 [internal function]: Drush\Commands\core\DeployHookCommands->run()
#3 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(276): call_user_func_array()
#4 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback()
#5 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter()
#6 /var/www/html/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(391): Consolidation\AnnotatedCommand\CommandProcessor->process()
#7 /var/www/html/vendor/symfony/console/Command/Command.php(326): Consolidation\AnnotatedCommand\AnnotatedCommand->execute()
#8 /var/www/html/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run()
#9 /var/www/html/vendor/symfony/console/Application.php(324): Symfony\Component\Console\Application->doRunCommand()
#10 /var/www/html/vendor/symfony/console/Application.php(175): Symfony\Component\Console\Application->doRun()
#11 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(110): Symfony\Component\Console\Application->run()
#12 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(40): Drush\Runtime\Runtime->doRun()
#13 /var/www/html/vendor/drush/drush/drush.php(139): Drush\Runtime\Runtime->run()
#14 /var/www/html/vendor/drush/drush/drush(4): require('...')
#15 /var/www/html/vendor/bin/drush(119): include('...')
#16 {main}

Reverted, let's rework to not break Drush.

Spokje → reopened merge request !6761

Added a BC layer for Drupal\Core\Update\UpdateRegistry::__construct(): Argument #4

Hmm, Drush calls it with this:

\Drupal::service('keyvalue')->get('deploy_hook')

but we force it to be a different collection:

      $key_value_factory = \Drupal::service('keyvalue');
...
    $this->keyValue = $key_value_factory->get('post_update');

I assume Drush is extending this for its custom "deploy" hooks...

I added a small change that would make Drush continue to work.

I also created a PR for drush that should make this work regardless of what gets committed here.

@bircher in order to make fewer changes in the constructor and also Drush what about something like:

update.update_hook_registry:
  class: Drupal\Core\Update\UpdateHookRegistry
  arguments: ['%container.modules%', '@update.key_value.system_schema']

update.key_value.system_schema:
  factory: ['@key_value', 'get']
  arguments: ['system.schema']
  public: false

@longwave drush uses the post_update registry (ie \Drupal\Core\Update\UpdateRegistry) so the suggestion in #28 will not make a difference for drush. Drush has to anyway be updated so that can override the protected $updateType property to detect the deploy hooks with it. I would consider making this easier for drush in a follow up issue. Then drush could could use the factory or whatever we add to make this a usable API.

Oh sorry I was looking at the wrong service. But we could inject the keyvalue store directly and also set $updateType in the constructor as well? Then we make life a lot easier for Drush - and would prefer to do that here given we are already adding a deprecation, don't want to have to immediately deprecate the new changes...

good point going back and fourth on the deprecation is not great.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Seems to be failing all tests with

Fatal error: Uncaught Error: Class "Symfony\Component\Config\Loader\FileLoader" not found in /builds/issue/drupal-3419914/vendor/symfony/dependency-injection/Loader/FileLoader.php:36

@bircher the new test fails because $update_type is 'custom_update' and so this returns FALSE:

  protected function includeThemes(): bool {
    return $this->updateType === 'post_update';
  }

tests seem to pass now. The test now tests #35. So I think this is ready to be reviewed again.

Seems test issue has been resolved.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

baseline needs changes https://www.drupal.org/node/3426891 →

Rebased seems good

Imagine we are going to see a few of these with the phpstan change.

Let's try that again. Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Spokje → closed merge request !6761

Automatically closed - issue fixed for 2 weeks with no activity.