Problem/Motivation

In #3392616-5: Update to Symfony 6.4 we've found out that the Symfony\Component\DependencyInjection\ContainerAwareTrait and Symfony\Component\DependencyInjection\ContainerAwareInterface are being deprecated in Symfony 6.4.

Both UpdateHookRegistryFactory and UpdateRegistryFactory are ContainerAware.

Steps to reproduce

Proposed resolution

Remove both services. Inject the correct services and parameters directly into the UpdateHookRegistry and UpdateRegistry services.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#38 3419914-nr-bot.txt90 bytesneeds-review-queue-bot
#32 3419914-nr-bot.txt3.54 KBneeds-review-queue-bot

Issue fork drupal-3419914

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

longwave opened merge request !6495

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs work

Tests need work to accommodate this change.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned

Nope, above my brain capacity.
Might have grown some brain cells.

Spokje opened merge request !6761

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

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review
spokje’s picture

spokje
Primary language Dutch
commented

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.

This is the best I can come up with.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Seems some of the deprecations are targeting 10.1

andypost made their first commit to this issue’s fork.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review

fixed deprecations

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

My feedback has been addressed.

  • catch committed 36f5209c on 10.3.x 
    Issue #3419914 by Spokje, longwave, andypost, smustgrave: Remove...

  • catch committed e5c3c870 on 11.x 
    Issue #3419914 by Spokje, longwave, andypost, smustgrave: Remove...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

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

catch closed merge request !6761

jurgenhaas’s picture

jurgenhaas
he/him
Primary language German
Location Gottmadingen
commented

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}

  • longwave committed c9f896b8 on 10.3.x 
    Revert "Issue #3419914 by Spokje, longwave, andypost, smustgrave: Remove...

  • longwave committed f41e428e on 11.x 
    Revert "Issue #3419914 by Spokje, longwave, andypost, smustgrave: Remove...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Fixed » Needs work

Reverted, let's rework to not break Drush.

Spokje reopened merge request !6761

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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; maybe we need to retain the factory services, but just inject the keyvalue factory into the update factory?

bircher made their first commit to this issue’s fork.

bircher’s picture

bircher
Location 🇨🇿
commented

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

bircher’s picture

bircher
Location 🇨🇿
commented

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@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
bircher’s picture

bircher
Location 🇨🇿
commented

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

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

bircher’s picture

bircher
Location 🇨🇿
commented

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3419914-nr-bot.txt3.54 KB

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.

bircher’s picture

bircher
Location 🇨🇿
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

@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';
  }
bircher’s picture

bircher
Location 🇨🇿
commented
Status: Needs work » Needs review

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems test issue has been resolved.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3419914-nr-bot.txt90 bytes

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.

andypost’s picture

andypost
he/him
Primary language Russian
commented

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Rebased seems good

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

  • catch committed 634f4624 on 10.3.x 
    Issue #3419914 by Spokje, longwave, bircher, andypost, smustgrave, catch...

  • catch committed ccd89c90 on 11.x 
    Issue #3419914 by Spokje, longwave, bircher, andypost, smustgrave, catch...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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

Spokje closed merge request !6761

Status: Fixed » Closed (fixed)

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