make it easier to instantiate ConfigImporter

What about a factory class?

Well, services are shared and ConfigImporter stores internal state (errors, validated status) etc which suggests to me that it shouldn't be a service.

Here is an example with pseudo code: https://gist.github.com/johndevman/6f095d41e6d49ed0306bd9683789c905

Alternatively, you could perhaps do something similar to cache (not entirely sure if the objects are shared or not here though)

  cache.entity:
    class: Drupal\Core\Cache\CacheBackendInterface
    tags:
      - { name: cache.bin }
    factory: cache_factory:get
    arguments: [entity]

  cache_factory:
    class: Drupal\Core\Cache\CacheFactory
    arguments: ['@settings', '%cache_default_bin_backends%']
    calls:
      - [setContainer, ['@service_container']]

I wrote this up in a project.
Too lazy to create a patch atm :)

declare(strict_types = 1);

namespace Drupal\MYMODULE\Config;

use Drupal\Core\Config\ConfigImporter;
use Drupal\Core\Config\ConfigManagerInterface;
use Drupal\Core\Config\StorageComparerInterface;
use Drupal\Core\Config\TypedConfigManagerInterface;
use Drupal\Core\Extension\ModuleExtensionList;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\Extension\ModuleInstallerInterface;
use Drupal\Core\Extension\ThemeHandlerInterface;
use Drupal\Core\Lock\LockBackendInterface;
use Drupal\Core\StringTranslation\TranslationInterface;
use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;

/**
 * Factory for ConfigImporter.
 *
 * See https://www.drupal.org/project/drupal/issues/3123491.
 */
class ConfigImporterFactory {

  /**
   * Constructor.
   *
   * @param \Symfony\Contracts\EventDispatcher\EventDispatcherInterface $event_dispatcher
   * @param \Drupal\Core\Config\ConfigManagerInterface $config_manager
   * @param \Drupal\Core\Lock\LockBackendInterface $lock
   * @param \Drupal\Core\Config\TypedConfigManagerInterface $typed_config
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   * @param \Drupal\Core\Extension\ModuleInstallerInterface $module_installer
   * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
   * @param \Drupal\Core\StringTranslation\TranslationInterface $string_translation
   * @param \Drupal\Core\Extension\ModuleExtensionList $extension_list_module
   */
  public function __construct(
    private EventDispatcherInterface $event_dispatcher,
    private ConfigManagerInterface $config_manager,
    private LockBackendInterface $lock,
    private TypedConfigManagerInterface $typed_config,
    private ModuleHandlerInterface $module_handler,
    private ModuleInstallerInterface $module_installer,
    private ThemeHandlerInterface $theme_handler,
    private TranslationInterface $string_translation,
    private ModuleExtensionList $extension_list_module,
  ) {}

  /**
   * Creates a ConfigImporter instance.
   *
   * @param \Drupal\Core\Config\StorageComparerInterface $storage_comparer
   *
   * @return \Drupal\Core\Config\ConfigImporter
   */
  public function createConfigImporter(StorageComparerInterface $storage_comparer): ConfigImporter {
    return new ConfigImporter(
      $storage_comparer,
      $this->event_dispatcher,
      $this->config_manager,
      $this->lock,
      $this->typed_config,
      $this->module_handler,
      $this->module_installer,
      $this->theme_handler,
      $this->string_translation,
      $this->extension_list_module,
    );
  }

}

We could also make an interface for this.

Failure seems legit to the issue.

Added https://www.drupal.org/project/drupal/issues/3394450

Follow up issue has been resolved.

But could a CR be written for the new search, preferably with an example?

Also issue summary mentions more details pending?

Thanks much better! cR reads well too, thanks for adding an example

I want to avoid swapability at all costs here. Supporting alternate implementations of the ConfigImporter and StorageComparer makes me feel very uneasy. I think the idea of have a factory service is a good one as long as we make changes to show the scope is to use a \Drupal\Core\Config\StorageComparer object to generate a \Drupal\Core\Config\ConfigImporter object.

I wish \Drupal\Core\Config\StorageComparerInterface did not exist. Perhaps we can move all the docs to the StorageComparer and change the typehint on \Drupal\Core\Config\ConfigImporter::__construct() to StorageComparer|StorageComparerInterface and deprecate the interface.

The StorageInterface deprecation is definitely a follow-up.

I think my comments on the MR were pretty clear... make the factory final and @internal and instead of typehinting on StorageComparerInterface do it on StorageComparer instead.

1 small comment.

For the parameters could they be typehinted or would that cause issues since they're really being moved?

Ah this is great! For Config Split, I made a trait which gets the services from \Drupal:: in order to avoid people trying to abuse it by swapping it etc.
But with final and the concrete classes typehinted I think it is even nicer.

When reviewing I saw that obviously the constructor of the form classes changed, we remove some deprecation notice and I am wondering if we need to do that here too.. but since there is a create method I think we are fine, but it makes me wonder why it was done last the other time.
Also the change record looks good.

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.

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.

I think this is a great idea; offered some suggestions to reduce verbosity and pump up that diffstat even more. :)

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.

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.

Test coverage is present. Test-only test output:

PHPUnit 11.5.34 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-3123491/core/phpunit.xml.dist
EE                                                                  2 / 2 (100%)
Time: 00:01.855, Memory: 8.00 MB
There were 2 errors:
1) Drupal\KernelTests\Core\Config\ConfigImporterMissingContentTest::testMissingContent
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "config.importer.factory".
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:1041
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:608
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:571
/builds/issue/drupal-3123491/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterMissingContentTest.php:82
2) Drupal\KernelTests\Core\Config\ConfigImporterMissingContentTest::testMissingBlockContent
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "config.importer.factory".
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:1041
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:608
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:571
/builds/issue/drupal-3123491/core/tests/Drupal/KernelTests/Core/Config/ConfigImporterMissingContentTest.php:82
ERRORS!
Tests: 2, Assertions: 0, Errors: 2.
HTML output directory sites/simpletest/browser_output is not a writable directory.
PHPUnit 11.5.34 by Sebastian Bergmann and contributors.
Runtime:       PHP 8.4.12
Configuration: /builds/issue/drupal-3123491/core/phpunit.xml.dist
.E                                                                  2 / 2 (100%)
Time: 00:01.497, Memory: 8.00 MB
There was 1 error:
1) Drupal\KernelTests\Core\Entity\ContentEntityNullStorageTest::testDeleteThroughImport
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "config.importer.factory".
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:1041
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:608
/builds/issue/drupal-3123491/vendor/symfony/dependency-injection/ContainerBuilder.php:571
/builds/issue/drupal-3123491/core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNullStorageTest.php:56
ERRORS!
Tests: 2, Assertions: 4, Errors: 1.
Exiting with EXIT_CODE=2

This seems as desired? I assume that the various other tests edited in the MR were edited to fix them after they were broken by the MR?

I have added several code review comments.

I see 2x unit tests failing but look unrelated. Some disagreement on coding standards in the comments. I think the comment reviews have not been set to 'you have to follow the comments' so moving to RTBTC.

The test fails are very much related...

Symfony\Component\DependencyInjection\Exception\RuntimeException: Cannot autowire service "Drupal\Core\Config\ConfigImporterFactory": argument "$lock" of method "__construct()" references interface "Drupal\Core\Lock\LockBackendInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "lock", "lock.persistent", "drupal.proxy_original_service.lock.persistent". in /builds/issue/drupal-3123491/vendor/symfony/dependency-injection/Compiler/DefinitionErrorExceptionPass.php:48

#53 Thanks @alexpott. I just spotted the autowire error and was changing the issue status back to needs work! Then I saw your change 50s ago..

I think can loop back to #52. There are again 2x tests failing but this time they definitely appear to be unrelated. Returning to RTBTC.

We start need to address the name. As I've pointed out we already have the pattern of get() from factories and we should use it here.

$config_importer = $this->configImporterFactory->createConfigImporter($storage_comparer); - repeating the words config importer twice here does not add anything. And choosing method names based on their searchability without taking into context the class name is not a rationale for the longer name.

Not to throw a wrench in the works here, but...could we not just have ConfigImporter implement ContainerInjectionInterface, and sidestep any need for a full factory service, in conjunction with a setStorageComparer() method? That would achieve the goal here and also make it much easier to instantiate: \Drupal::classResolver(ConfigImporter::class)->withStorageComparer($storage_comparer). And we could still take advantage of autowiring, by way of AutowireTrait, which should work on anything implementing ContainerInjectionInterface.

I don't feel strongly about it, to be honest, but I'd be remiss if I didn't raise this possible alternative.

Good idea, moreover it will help to fix injection of theme_installer and config.installer

Also it needs follow-up for PHP 8.5+ to fix reInjectMe() to use __serialize() as part of #3548971: Replace PHP soft-deprecated __sleep()/__wakeup() with __serialize()/__unserialize()

I guess the only issue with #59 is injectability - like injecting the class resolver is not the same as injecting a ConfigImporterFactory object. But I think it is a great idea. Factories usually make sets of things but the ConfigImporter is very much a single thing. And it gets around the whole discussion about the factory getter method name. @phenaproxima++

#59 approach seems to be in use now in NodePermissions.php?:

class NodePermissions implements ContainerInjectionInterface {

  use AutowireTrait;
  use BundlePermissionHandlerTrait;
  use StringTranslationTrait;

  public function __construct(
    protected ?EntityTypeManagerInterface $entityTypeManager = NULL,
  ) {
    if ($entityTypeManager === NULL) {
      @trigger_error('Calling ' . __METHOD__ . ' without the $entityTypeManager argument is deprecated in drupal:11.2.0 and it will be required in drupal:12.0.0. See https://www.drupal.org/node/3515921', E_USER_DEPRECATED);
      $this->entityTypeManager = \Drupal::entityTypeManager();
    }
  }

Or is this a different use case?

I'm not sure what you mean here -- why wouldn't we be able to mock it in tests?

Took a shot at implementing the ContainerInjectionInterface approach - would love reviews!

Oh, I see. Well, if you need to do something like that, you can certainly mock, or decorate, the class resolver to return a faked version of the config importer.

Mocking the whole class resolver feels rather heavy!

Why? It's an interface with a single method. How would that be any heavier than mocking the proposed factory?

I think the main reason to move away from the factory is that it was largely there to wrap around the very heavy dependency injection and autowiring requirements of ConfigImporter...which is pretty much obviated by the robust autowiring support you can already get for free from AutowireTrait and the ContainerInjectionInterface. It also seems to have found more support from a naming perspective, since ConfigImporter::create() is less verbose than ConfigImporterFactory::createConfigImporter(). From a mocking perspective, they're basically the same.

I don't think it's an earth-shattering difference or anything, but why add a whole factory if we don't really need to?

Thinking about this some more the optionality around storage comparer when it is not optional plus the additional API makes me think we should go back to the version before #55 and just change the long method name to "get"

Done! It was worth a shot but I'm not married to the ContainerInjectionInterface approach, and I agree the optionality was awkward.

I think this approach makes more sense as well. Everything looks right to me!

Opened #3554866: Deprecate StorageComparerInterface to deprecate StorageComparerInterface as per #31 and the comments after it.

Committed and pushed d594cf40b84 to 11.x and 51c0672b816 to 11.3.x. Thanks!