Replace config() with Drupal::config()

This is liable to break every patch in core, so let's split it into smaller patches. Convert a module at a time or such.

Postponing on the main issue.

The problem is that there is never truly a simple search-and-replace, and the patch is still impossible to review regardless of how fast you make it. If you split this up, you can let novices handle it gradually, and let other patches be rerolled gradually. Each issue has a slight overhead, but the overall process is much less painful.

I think I agree with berdir here, I would rather see one patch for each method conversion, even though I managed to mess up the state() one :) That is mainly down to a lack of time and laziness. Yes, it is a large patch, but given a little time, it's relatively easy to go through.

If we split this up per method per module, we will end up with an issue count in the hundreds, which would take a monumental amount of time to get committed. It would be some sort of reroll hell for other patches; having to keep track of multiple issues doing this conversion, potentially.

OK, but I'm not reviewing it. :)

As a committer, I also prefer this "one patch per method" approach. And so I guess I'll sign up to review it. :)

I think we should mark the config function deprecated so that we can do this after feature freeze #2028149: The config() function should be deprecated in favour of Drupal::config()

That seems sensible to me, although I still think we should reconsider making this just one patch. After api freeze this will become easier anyway.

Fun!

I think the replacement went a bit OTT there

-  public static function \Drupal::config($name) {
+  public static function config($name) {

Also, there were a couple of missing parts etc... :)

And I guess this is not a meta issue anymore.

Oh, missed some stuff in ConfigImporter that needed changing from the original patch.

Sorry, unresolved merge conflict from rebasing.

We should replace Drupal::config // config() with $this->container->get('config.factory') in tests. And there are still a lot of places where we use Drupal::config() // config() in classes even though we could inject it properly.

Let's get it right in one go now that we are producing such a big patch anyways.

Changing all the places to inject instead is going to make an absolutely massive patch!

Edit: if that is the patch...not too bad.

No, that patch does not do the injection, it just changes the test usage of the config factory to go with $this->container->get('config.factory');

Edit: Oh, I forgot *TestBase.php :)

In that case I would say going with injection is not a good plan for this patch.

I think this should be it. There was just a very tiny amount of things can get the config factory injected. I will cover the ModuleHandler later as part of the disabled modules post-patch cleanup :).

@damiankloip even if it's just 5kb of the entire patch? (Which is 456kb large ;) ).

OK, fair enough. I don't mind...I'm not reviewing it :-)

I think the config query factory is just not passing in the new config factory param.

There is no $this->container at that stage when run() is called.

Next try...

Injecting in some more places and using $this->container->get('config.factory') in some base tests that we missed before.

Note: Per discussion with @damiankloip we are completely ignoring views plugins (or rather, any plugins) for now when it comes to injecting the config factory. Otherwise this patch would explode... Also, there will be dedicated cleanup patches for all those Views plugins anyways... We can solve that there.

This should be it now from my side. I just walked through all the changes once again and think we covered it all for now.

More fixes...

Should be green now I guess.

Reroll.

+++ b/core/core.services.ymlundefined
@@ -262,7 +262,7 @@ services:
+    arguments: ['@config.storage', '@config.factory']

I really really really like the idea to split up the injection part from the procedural code, as done afaik on the module handler. This just makes the life easy for other people.

+++ b/core/includes/file.incundefined
@@ -1015,8 +1015,6 @@ function file_unmanaged_delete_recursive($path, $callback = NULL) {
-
-

Out of scope!

+++ b/core/includes/module.incundefined
@@ -24,7 +24,7 @@
+ *   data. Consider to add a Drupal::config($name, $cache = TRUE) argument to allow

> 80 chars

+++ b/core/lib/Drupal/Core/Asset/AssetDumper.phpundefined
@@ -15,6 +16,23 @@
   /**
+   * The config factory.
+   *
+   * @var \Drupal\Core\Config\ConfigFactory
+   */
+  protected $configFactory;

@@ -41,7 +59,7 @@ public function dump($data, $file_extension) {
+    if ($this->configFactory->get('system.performance')->get($file_extension . '.gzip') && extension_loaded('zlib')) {

I personally think that we should store the config object instead of the full factory.

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.phpundefined
@@ -58,10 +58,9 @@ class DrupalDateTime extends DateTimePlus {
-    $settings['langcode'] = !empty($settings['langcode']) ? $settings['langcode'] : language(Language::TYPE_INTERFACE)->id;
...
+    $settings['langcode'] = !empty($settings['langcode']) ? $settings['langcode'] : \Drupal::languageManager()->getLanguage(Language::TYPE_INTERFACE)->id;

This is out of scope, sorry.

+++ b/core/lib/Drupal/Core/Executable/ExecutablePluginBase.phpundefined
--- a/core/lib/Drupal/Core/Extension/ModuleHandler.php
+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -582,8 +582,8 @@ public function enable($module_list, $enable_dependencies = TRUE) {
+    $module_config = \Drupal::config('system.module');
+    $disabled_config = \Drupal::config('system.module.disabled');

@@ -751,8 +751,8 @@ function disable($module_list, $disable_dependents = TRUE) {
+    $module_config = \Drupal::config('system.module');
+    $disabled_config = \Drupal::config('system.module.disabled');

@@ -843,7 +843,7 @@ public function uninstall($module_list = array(), $uninstall_dependents = TRUE)
+    $disabled_config = \Drupal::config('system.module.disabled');

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.phpundefined
@@ -75,13 +75,13 @@ public function enable($module_list, $enable_dependencies = TRUE) {
+      $module_config = \Drupal::config('system.module');
...
+      \Drupal::config('system.module.disabled')

So why don't we inject it in here?

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -75,7 +75,7 @@ public function getConfig() {
+   * @todo This does not set a value in \Drupal::config(), so the name is confusing.

> 80 chars

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -58,8 +58,8 @@ function testCRUD() {
+    // Verify a call to $this->container->get('config.factory')->get() immediately returns the updated value.

@@ -88,36 +88,36 @@ function testCRUD() {
+    // Verify a call to $this->container->get('config.factory')->get() immediately returns the updated value.

> 80 chars.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigCRUDTest.phpundefined
@@ -147,7 +147,7 @@ function testNameValidation() {
     $name = 'config_test.herman_melville.moby_dick_or_the_whale.harper_1851.now_small_fowls_flew_screaming_over_the_yet_yawning_gulf_a_sullen_white_surf_beat_against_its_steep_sides_then_all_collapsed_and_the_great_shroud_of_the_sea_rolled_on_as_it_rolled_five_thousand_years_ago';

+1

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImporterTest.phpundefined
@@ -72,10 +72,10 @@ function testNoImport() {
+    // Verify that a bare $this->container->get('config.factory')->get() does not involve module APIs.

> 80 chars.

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigInstallWebTest.phpundefined
@@ -61,7 +61,7 @@ function testIntegrationModuleReinstallation() {
+    //   object in memory. Every new call to $this->container->get('config.factory')->get() MUST revert in-memory changes

> 80 chars.

+++ b/core/modules/contact/lib/Drupal/contact/CategoryFormController.phpundefined
@@ -21,7 +21,7 @@ public function form(array $form, array &$form_state) {
+    $default_category = \Drupal::config('contact.settings')->get('default_category');

@@ -108,7 +108,7 @@ public function save(array $form, array &$form_state) {
+    $contact_config = \Drupal::config('contact.settings');

+++ b/core/modules/contact/lib/Drupal/contact/CategoryListController.phpundefined
@@ -66,7 +66,7 @@ public function buildRow(EntityInterface $entity) {
+      $default_category = \Drupal::config('contact.settings')->get('default_category');

+++ b/core/modules/contact/lib/Drupal/contact/MessageFormController.phpundefined
@@ -187,7 +187,7 @@ public function save(array $form, array &$form_state) {
+    \Drupal::service('flood')->register('contact', \Drupal::config('contact.settings')->get('flood.interval'));

Why is there no injection applied? +1 to not do it in any example.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -80,7 +80,7 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
-    $field_prefix = config('field_ui.settings')->get('field_prefix');
+    $field_prefix = $this->configFactory->get('field_ui.settings')->get('field_prefix');

@@ -313,7 +313,7 @@ protected function validateAddNew(array $form, array &$form_state) {
-        $field_name = config('field_ui.settings')->get('field_prefix') . $field_name;
+        $field_name = $this->configFactory->get('field_ui.settings')->get('field_prefix') . $field_name;

This is not injected, so it simply does not work at all!

+++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterUnitTest.phpundefined
@@ -428,7 +428,7 @@ function testUrlFilter() {
+      // $protocols = $this->container->get('config.factory')->get('system.filter')->get('protocols')... (approx line 1555).

> 80 chars

+++ b/core/modules/layout/lib/Drupal/layout/Config/DisplayInterface.phpundefined
@@ -41,7 +41,7 @@
+   * block instance's configuration object name, and \Drupal::config() may be called on

> 80 chars.

+++ b/core/modules/locale/locale.bulk.incundefined
@@ -799,7 +799,7 @@ function locale_config_batch_build(array $names, array $langcodes, $options = ar
+    // so it is very expensive to initialize the Drupal::config() object on each request.

> 80 chars.

+++ b/core/modules/locale/tests/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -25,6 +25,13 @@ class LocaleTranslationTest extends UnitTestCase {
+   * @var \Drupal\Core\Config\ConfigFactory

I think we should also document the mock object, to not show that we have used an invalid method call.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -821,7 +821,7 @@ protected function setUp() {
+    $this->container->get('config.factory')->get('system.mail')->set('interface.default', 'Drupal\Core\Mail\VariableLog')->save();

Wow, it is odd to use a class in a setting. Shouldn't that just be a service?

+++ b/core/modules/user/lib/Drupal/user/Tests/UserTimeZoneTest.phpundefined
@@ -26,10 +26,11 @@ public static function getInfo() {
+

Out of scope.

+++ b/core/modules/user/user.installundefined
@@ -1121,7 +1121,7 @@ function user_update_8019() {
-  config("entity.form_mode.user.register")
+  Drupal::config("entity.form_mode.user.register")

We could also fix the double-quoting ;)

Yeah, I think I'm starting to agree. All the replacements - fine. Switching tests to use $this->container directly and injection, is a step too far imo.

Shall we just go back to this replacement then?

Inventing a new tag ;)

I'm not retrospectively refilling the old patch (which is a pain) to get an interdiff! :-) this is basically a reroll of #21.

+++ b/core/includes/config.inc
@@ -66,33 +66,11 @@ function config_get_storage_names_with_prefix($prefix = '') {
- */
-function config($name) {

Maybe we should better keep the api.

Yeah, you're right.

Sorry, forgot to rebase 8.x.

sorry.

Rerolled.

Just another reroll.

This looked fine already back in #46 (beside the config() removable) so a fast scan now haven't shown anything bad.

All right, awesome. As promised in #9, I read through this entire patch. I didn't spot any false-positives (that doesn't mean there aren't any :), just I didn't notice and was looking for them) and the regexes used even do fancy things like switch Drupal::config() for \Drupal::config() depending on whether or not it's in a class file. (That reminds me that I still really want #2053489: Standardize on \Drupal throughout core to happen, because the code looks wildly inconsistent right now to people who don't know that that's the trick.)

There's nothing currently tagged as "Avoid commit conflicts" so talked this situation over with catch, and he approved just getting this done and letting the chips fall where they may. Hopefully since this function was already deprecated, it won't break too much RTBC stuff in the queue.

So...

Committed and pushed to 8.x. Thanks!

Because we're after API freeze, let's get a small change notice out about this to notify module developers.

Also I went through quickly and updated all the change notices I could find with "config(" in them to do Drupal::config() instead.

Here is one https://drupal.org/node/2065313

Change notice looks great! I touched the language up slightly.

However, we missed a few spots. :)

./core/includes/common.inc:    $page_compressed = config('system.performance')->get('response.gzip') && extension_loaded('zlib');
./core/includes/config.inc: * @code config('book.admin') @endcode will return a configuration object in
./core/includes/config.inc: *   a configuration file. For @code config('book.admin') @endcode, the config
./core/lib/Drupal/Core/Controller/ControllerBase.php:   *   a configuration file. For @code config('book.admin') @endcode, the config
./core/lib/Drupal/Core/Controller/ControllerBase.php:   * (for example, the system maintenance message) should use config() instead.
./core/lib/Drupal/Core/Datetime/Date.php:      $this->country = config('system.date')->get('country.default');
./core/lib/Drupal.php:  public static function config($name) {
./core/modules/block/custom_block/custom_block.module.orig:    $config = config($config_name);
./core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php:    $config = config('system.performance');

Let's fix them.

Sorry, must have got lost/missed as the patch got rerolled!

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -59,13 +59,14 @@ protected function cache($bin = 'cache') {
-   * @code $this->config('book.admin') @endcode will return a configuration
+   * @code \Drupal::config('book.admin') @endcode will return a configuration

This is irreverent.

You are totally right!

Here we go. I think #57 is addressed so RTBC. It is just a comment change so I went ahead and RTBC it.

Committed and pushed that to 8.x too. Thanks!

I think we can close this out now.

--- a/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/PageCacheTest.php

@@ -45,7 +45,7 @@ function setUp() {
-    $config = config('system.performance');
+    $config = \Drupal::config('system.performance');

This needs follow-up a-la #2001206: Replace drupal_container() with Drupal::service() to replace \Drupal with $this->container

Yes, that for tests. And we need another follow up to inject config objects where we can instead of calling Drupal::config().

Filed #2066993: Use \Drupal consistently in tests
And still needs another one to address #65