Use ConfigFactoryInterface to type hint ConfigFactory

#2186845: ConfigInstaller should expect a ConfigFactoryInterface not a ConfigFactory. was a duplicate, working on converting all places, puh!

There we go.

I cannot think of something which causes the memory issue in this patch. This is either a tip of the iceberg or a random failure.

Rerolled.

jibran opened a new pull request for this issue.

larowlan posted a new comment on https://github.com/klausi/drupal/pull/17#discussion_r9571902
Path: core/tests/Drupal/Tests/UnitTestCase.php

@@ -122,9 +122,7 @@ public function getConfigFactoryStub(array $configs = array()) {
     }
     // Construct a config factory with the array of configuration object stubs
     // as its return map.
-    $config_factory = $this->getMockBuilder('Drupal\Core\Config\ConfigFactory')
-      ->disableOriginalConstructor()
-      ->getMock();
+    $config_factory = $this->getMock('Drupal\Core\Config\ConfigFactoryInterface');

I like what you've done here and want to test comments back on d.o

+++ b/core/modules/search/tests/Drupal/search/Tests/SearchPageRepositoryTest.php
@@ -76,7 +76,7 @@ public function setUp() {
+    $this->configFactory = $this->getMockBuilder('Drupal\Core\Config\ConfigFactoryInterface')
       ->disableOriginalConstructor()
       ->getMock();

This can just use getMock() directly now. No need to disable the constructor.

Otherwise this looks fine - just straight forward find replacing for the interface.

Rerolled. Thank you for your review.

Setting to RTBC now.

Ups.

Just a note: when you have "@return $this" you do not need a description under it, and in fact I think it makes the docs less maintainable and less readable in this case.

Since this touches roughly a billion files anyway, could we take this opportunity to fix typehinting for a few more classes? Each of these classes will also need their own mega-patches otherwise:

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -66,10 +66,10 @@ class BlockFormController extends EntityFormController {
-  public function __construct(EntityManagerInterface $entity_manager, QueryFactory $entity_query_factory, LanguageManagerInterface $language_manager, ConfigFactory $config_factory) {
+  public function __construct(EntityManagerInterface $entity_manager, QueryFactory $entity_query_factory, LanguageManagerInterface $language_manager, ConfigFactoryInterface $config_factory) {

QueryFactoryInterface

+++ b/core/modules/user/lib/Drupal/user/AccountSettingsForm.php
@@ -27,12 +27,12 @@ class AccountSettingsForm extends ConfigFormBase {
-  public function __construct(ConfigFactory $config_factory, ModuleHandler $module_handler) {
+  public function __construct(ConfigFactoryInterface $config_factory, ModuleHandler $module_handler) {

ModuleHandlerInterface

+++ b/core/modules/user/lib/Drupal/user/UserAutocomplete.php
@@ -51,10 +51,10 @@ class UserAutocomplete {
-  public function __construct(Connection $connection, ConfigFactory $config_factory, EntityManager $entity_manager, QueryFactory $entity_query) {
+  public function __construct(Connection $connection, ConfigFactoryInterface $config_factory, EntityManager $entity_manager, QueryFactory $entity_query) {

EntityManagerInterface

+++ b/core/lib/Drupal/Core/ImageToolkit/ImageToolkitManager.php
@@ -34,10 +34,10 @@ class ImageToolkitManager extends DefaultPluginManager {
-  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ConfigFactory $config_factory) {
+  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ConfigFactoryInterface $config_factory) {

LanguageManagerInterface

@jibran: But since each of these patches have to touch so many files, isn't it going to be still going to be hell each time we do this? And each time one of these patches is committed, other patches will need re-rolls. I'm hoping we save future work by doing this as few times as possible.

I would suggest removing the description docs below @return $this. IMO @return $this is a lot clearer than what is currently in the docs there.

@Les Lim
In case we would just fix a couple of places I would totally agree but we have a >100k patch which per construction are hard to review, so let's try to move forward.

1. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
   @@ -53,9 +53,7 @@ class ConfigSync extends FormBase {
   -   * The configuration manager.
   -   *
   -   * @var \Drupal\Core\Config\ConfigManagerInterface;
   +   * @var \Drupal\Core\Entity\EntityManagerInterface;
   

   This is wrong

2. +++ b/core/modules/config/lib/Drupal/config/Form/ConfigSync.php
   @@ -67,6 +65,13 @@ class ConfigSync extends FormBase {
   +   * The UUID service.
   +   *
   +   * @var \Drupal\Component\Uuid\UuidInterface
   +   */
   +  protected $uuidService;
   

   Nope

Reroll gone bad? I have not checked the patch throughly. But I suspected there are more problems.

Gr, damnit I want that to be in for my contrib project.

The docs in this patch are still missing descriptions on a few @return tags. They are required except in the case of "@return $this", in which case they should be removed (which this patch doesn't do either in all cases). The ones I saw were right at the top of the patch file, in class Drupal and ConfigFactoryInterface.

I also think this docs change is not great on the constructor for ConfigStorageController:

-   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
-   *   The entity type definition.
-   * @param \Drupal\Core\Config\ConfigFactory $config_factory
+   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_info
+   *   The entity info for the entity type.
+   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory

"The entity info for the entity type" is not really better than "The entity type definition" IMO. "entity info", what is that? I don't think the parameter rename is appropriate either.

The docs in this patch are still missing descriptions on a few @return tags.

Done.

I also think this docs change is not great on the constructor for ConfigStorageController:

I removed this change as it seemed out of scope. We should probably not delay this issue because of it.

I also merged in the changes from #2191911: Improve @param and @return variable types for \Drupal\Core\Config\ConfigFactoryInterface.

--dawehner

+++ b/core/lib/Drupal/Core/Config/ConfigFactoryInterface.php
@@ -35,7 +34,7 @@ public function setOverrideState($state);
-   *   Get the override state.
+   *   Whether the configuration is overridden.

@@ -59,7 +58,7 @@ public function get($name);
-   * @return array
+   * @return \Drupal\Core\Config\Config[]

@@ -67,12 +66,11 @@ public function loadMultiple(array $names);
-   * @param string $name
+   * @param string|null $name

@@ -114,20 +112,18 @@ public function getCacheKeys($name);
-   * @param \Drupal\Core\Language\Language $language
+   * @param \Drupal\Core\Language\Language|null $language

@@ -139,15 +135,15 @@ public function setLanguage(Language $language = NULL);
-   * Gets the language Used to override configuration.
+   * Gets the language used to override configuration.
...
+   *   The language used to override configuration.

@@ -178,7 +174,7 @@ public function getLanguageConfigNames(array $names);
-   *   Returns false if the name already starts with the language config prefix.
+   *   Returns FALSE if the name already starts with the language config prefix.

This issue is not "clean up all docblocks in files where we need to switch ConfigFactory typehints to ConfigFactoryInterface". Please remove all of this and let's just get this small, simple, and well-scoped issue in.

I removed the code quoted in #47 as that was added after some confusion with #2191911: Improve @param and @return variable types for \Drupal\Core\Config\ConfigFactoryInterface.

Heh, turns out that was mostly at the top of the file. I had just given up and stopped scrolling thinking it was throughout.

That looks much better, thanks for the quick turnaround!

Committed 932e4d9 and pushed to 8.x. Thanks!

Thank you, especially jibran for fixing the horrible merges.

:( We/I should have rechecked it :(

λ d8 → ħ git 8.x* → ag "ConfigFactory " | grep -v "ConfigFactoryInterface"
modules/web_profiler/lib/Drupal/webprofiler/Form/ConfigForm.php:33:   * @param ConfigFactory $config_factory
modules/web_profiler/lib/Drupal/webprofiler/Form/ConfigForm.php:36:  public function __construct(ConfigFactory $config_factory, Profiler $profiler) {
modules/ds/lib/Drupal/ds/Form/SettingsForm.php:30:   * @param \Drupal\Core\Config\ConfigFactory $config_factory
modules/ds/lib/Drupal/ds/Form/SettingsForm.php:35:  public function __construct(ConfigFactory $config_factory, ModuleHandlerInterface $module_handler) {
modules/ds/lib/Drupal/ds/Form/EmergencyForm.php:45:   * @param \Drupal\Core\Config\ConfigFactory $config_factory
modules/ds/lib/Drupal/ds/Form/EmergencyForm.php:52:  public function __construct(ConfigFactory $config_factory, ModuleHandlerInterface $module_handler, State $state) {
modules/ds/modules/ds_ui/lib/Drupal/ds_ui/Form/FieldFormBase.php:56:   * @param \Drupal\Core\Config\ConfigFactory $config_factory
modules/ds/modules/ds_ui/lib/Drupal/ds_ui/Form/FieldFormBase.php:67:  public function __construct(ConfigFactory $config_factory, ContextInterface $context, EntityManager $entity_manager, CacheBackendInterface $cache_backend, ModuleHandler $module_handler) {
modules/ds/modules/ds_ui/lib/Drupal/ds_ui/Form/FieldDeleteForm.php:49:   * @param \Drupal\Core\Config\ConfigFactory $config_factory
modules/ds/modules/ds_ui/lib/Drupal/ds_ui/Form/FieldDeleteForm.php:52:  public function __construct(CacheBackendInterface $cache_backend, ConfigFactory $config_factory) {
modules/devel/lib/Drupal/devel/EventSubscriber/DevelEventSubscriber.php:45:  public function __construct(ConfigFactory $config, AccountInterface $account, ModuleHandlerInterface $module_handler) {
core/lib/Drupal/Core/Config/ConfigFactory.php:115:      // @todo Explore making ConfigFactory a listener to the config.delete
core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php:342:      // - The object needs to be renamed/copied in ConfigFactory and reloaded.
core/modules/statistics/lib/Drupal/statistics/StatisticsSettingsForm.php:29:   * @param \Drupal\Core\Config\ConfigFactory $config_factory
core/modules/statistics/lib/Drupal/statistics/StatisticsSettingsForm.php:34:  public function __construct(ConfigFactory $config_factory, ModuleHandlerInterface $module_handler) {
core/modules/field/lib/Drupal/field/FieldInfo.php:131:   * @param \Drupal\Core\Config\ConfigFactory $config
core/modules/field/lib/Drupal/field/FieldInfo.php:138:  public function __construct(CacheBackendInterface $cache_backend, ConfigFactory $config, ModuleHandlerInterface $module_handler, FieldTypePluginManagerInterface $field_type_manager) {
core/modules/config/lib/Drupal/config/Tests/ConfigModuleOverridesTest.php:41:    $this->assertTrue($config_factory->getOverrideState(), 'By default ConfigFactory has overrides enabled.');
core/modules/config/lib/Drupal/config/Tests/ConfigModuleOverridesTest.php:46:    $this->assertFalse($config_factory->getOverrideState(), 'ConfigFactory can disable overrides.');
core/modules/config/lib/Drupal/config/Tests/ConfigModuleOverridesTest.php:51:    $this->assertTrue($config_factory->getOverrideState(), 'ConfigFactory can enable overrides.');
core/tests/Drupal/Tests/UnitTestCase.php:100:   *   A MockBuilder object for the ConfigFactory with the desired return values.
core/includes/install.core.inc:474:    // ConfigFactory to try to load language override configuration which is not

.

Looks good.

Committed 094c6e8 and pushed to 8.x. Thanks!