make it easier to instantiate ConfigImporter

What about a factory class?

How do you envisage that would look? What would be the advantage over a service?

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

Ah, yes, I hadn't thought of that!

Symfony actually allows services to not be shared, but we probably don't want to introduce that extra complexity to Drupal core services.

Updated the summary. Could you add details on how a factory class would work?

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

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,
    );
  }

}

Cool, I was halfway through writing this while tests were running a few weeks ago. I'll compare what I had with what you have and make a MR.

We could also make an interface for this.

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Pushed my WIP to a fork, will start to combine with @donquixote's work later.

Still not finding time to work on this, but I've added the actual factory service to the Field Tools module based on @donquixote's code (with a few tweaks), so the code can be grabbed from that when someone gets round to doing more work here.

Setting to NW since there's actually some code done on this issue.

Failure seems legit to the issue.

I'm stuck because there's no docs on why the test fails and how to do services in core now.

Added https://www.drupal.org/project/drupal/issues/3394450 🐛 Improve the failure message from Drupal\KernelTests\Core\DependencyInjection\AutowireTest Active

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?

Done the CR and cleaned up the IS.

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

Haha, it was just copy-pasted from the MR diff!

Thanks for the review :)

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.

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

That sounds like a follow-up to me? Or should it happen here to prevent swappability?

Could you explain what changes you want to this MR please? I'm not clear.

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.

Made a follow-up: 📌 deprecate StorageComparerInterface Active

Made the fixes.

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.

Addressed some of the comments & rebased on 11.x.

Regarding the deprecation for constructors in the two form classes -- form classes are not part of core's API and these two classes specifically have an '@internal' tag. On top of that, given it's faffy, I'd say we should leave it.

Rebased and resolved comments.

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.