Per #1875086: Improve DX of drupal_container()->get() all use of drupal_container() has been deprecated.

See also: #1938334: META: Replace uses of drupal_container()

Files: 
CommentFileSizeAuthor
#10 8-10-interdiff.txt3.42 KBalexpott
#10 1938338.cmi_.container.10.patch10.51 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938338.cmi_.container.10.patch. Unable to apply patch. See the log in the details link for more information. View
#8 5-8-interdiff.txt4.87 KBalexpott
#8 1938338.cmi_.container.8.patch13.33 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 53,136 pass(es). View
#5 drupal-n1938338-5.patch8.44 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] 52,517 pass(es), 48 fail(s), and 11 exception(s). View
#4 drupal-n1938338-4.patch8.09 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#3 drupal-n1938338-3.patch8.15 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#2 drupal-n1938338-2.patch8.19 KBDamienMcKenna
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Comments

DamienMcKenna’s picture

Title: Replace drupal_container() in config() in core/includes/config.inc » Replace drupal_container() in core/includes/config.inc

Lets keep this general to cover all uses in this file.

DamienMcKenna’s picture

Status: Active » Needs review
FileSize
8.19 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Lets start with a simple search/replace.

DamienMcKenna’s picture

FileSize
8.15 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

#facepalm.

DamienMcKenna’s picture

FileSize
8.09 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Another attempt.

There's still the following code that I'm not sure how it should be replaced:

  if (drupal_container()->has($context_name)) {
    $context = Drupal::service($context_name);
  }
DamienMcKenna’s picture

FileSize
8.44 KB
FAILED: [[SimpleTest]]: [MySQL] 52,517 pass(es), 48 fail(s), and 11 exception(s). View

This version includes a solution to the if(thingy->has(whatsit)) thanks to a suggestion from msonnabaum.

Status: Needs review » Needs work

The last submitted patch, drupal-n1938338-5.patch, failed testing.

alexpott’s picture

Issue tags: +Configuration system

Tagging...

alexpott’s picture

Title: Replace drupal_container() in core/includes/config.inc » Replace drupal_container() in Configuration system
Status: Needs work » Needs review
FileSize
13.33 KB
PASSED: [[SimpleTest]]: [MySQL] 53,136 pass(es). View
4.87 KB

Broadened the scope of the patch to replace usages of drupal_container() in the Configuration system (core/lib/Drupal/Config, config.inc, and the config module).

One interesting thing is the usage of config() and Drupal::service() in \Drupal\Core\Config\Entity\ConfigStorageController... I think we should be injecting the config factory into the storage controller... but that is way beyond the scope of this patch as (afaics) that probably means that the EntityManager has to become containeraware...

mtift’s picture

+++ b/core/modules/config/config.admin.incundefined
+++ b/core/modules/config/config.admin.incundefined
@@ -93,8 +93,8 @@ function config_admin_sync_form(array &$form, array &$form_state, StorageInterfa

 function config_admin_import_form($form, &$form_state) {
   // Retrieve a list of differences between last known state and active store.
-  $source_storage = drupal_container()->get('config.storage.staging');
-  $target_storage = drupal_container()->get('config.storage');
+  $source_storage = Drupal::service('config.storage.staging');

Is it necessary to have a "use Drupal;" prior to calling Drupal::service()? It is not mentioned in the API docs:

http://api.drupal.org/api/drupal/core%21lib%21Drupal.php/class/Drupal/8

But I noticed it used elsewhere in this patch:

--- a/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -12,6 +12,7 @@
 use Drupal\Core\Entity\EntityMalformedException;
 use Drupal\Core\Entity\EntityStorageControllerInterface;
 use Drupal\Core\Config\Config;
+use Drupal;

Then again, the config.admin.inc portion of this patch might not need to be included here, if I am understanding #1945398: Convert config_admin_import_form to a new FormInterface implementation and routing definition. correctly.

alexpott’s picture

FileSize
10.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938338.cmi_.container.10.patch. Unable to apply patch. See the log in the details link for more information. View
3.42 KB

... I think this is ready to fly.

salvis’s picture

I've suggested to #1957804: Add a nothrow option to Drupal::service().

This will allow us to write

  if (!$context = Drupal::service($context_name, TRUE)) {
    if (class_exists($context_name) && in_array('Drupal\Core\Config\Context\ContextInterface', class_implements($context_name))) {
      $context = Drupal::service('config.context.factory')->get($context_name);
    }
    else {
      throw new ConfigException(sprintf('Unknown config context service or class: %s', $context_name));
    }
  }

rather than

  try {
    $context = Drupal::service($context_name);
  }
  catch (InvalidArgumentException $e) {
    if (class_exists($context_name) && in_array('Drupal\Core\Config\Context\ContextInterface', class_implements($context_name))) {
      $context = Drupal::service('config.context.factory')->get($context_name);
    }
    else {
      throw new ConfigException(sprintf('Unknown config context service or class: %s', $context_name));
    }
  }

Exceptions should be used for exceptional program flow — in normal program flow it's better to avoid them.

ddrozdik’s picture

Some work in this direction has already been made here #2001206: Replace drupal_container() with Drupal::service()

jibran’s picture

Issue tags: -Configuration system

#10: 1938338.cmi_.container.10.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1938338.cmi_.container.10.patch, failed testing.

ianthomas_uk’s picture

Issue summary: View changes

I think this is a dupe of #2001206: Replace drupal_container() with Drupal::service() (or the config module subissue #2003576: Replace drupal_container() with Drupal::service() in the config module) and has therefore been fixed.

Is there any cleanup still to do, or can this be closed?

alexpott’s picture

Status: Needs work » Closed (duplicate)

Can be closed :)