drupal_container() is deprecated, and all calls in the system module need to be replaced with Drupal::service(), except for where the module_handler service is requested, which needs to be replaced with Drupal::moduleHandler() (see #1957154: Replace calls to drupal_container()->get('module_handler') service with Drupal::moduleHandler())

This task a part of #2001206: Replace drupal_container() with Drupal::service()

CommentFileSizeAuthor
#73 replace-drupal-container-with-drupal-service-2014017-73.patch6.42 KBLinL
#64 replace-drupal-container-with-drupal-service-2014017-64.patch7.62 KBjlbellido
#64 interdiff.txt1.86 KBjlbellido
#62 replace-drupal-container-with-drupal-service-2014017-62.patch7.71 KBjlbellido
#59 replace-drupal-container-with-drupal-service-2014017-59.patch7.61 KBjlbellido
#59 interdiff.txt859 bytesjlbellido
#56 replace-drupal-container-with-drupal-service-2014017-56.patch7.55 KBjlbellido
#49 patch-interdiff.txt4.28 KBpenyaskito
#49 replace-drupal-container-with-drupal-service-2014017-49.patch8.24 KBpenyaskito
#45 replace-drupal-container-with-drupal-service-2014017-45.patch8.97 KBjlbellido
#45 interdiff.txt5.66 KBjlbellido
#44 replace-drupal-container-with-drupal-service-2014017-44.patch8.91 KBjlbellido
#37 replace-drupal-container-with-drupal-service-2014017-38.patch10.67 KBjlbellido
#33 replace-drupal-container-with-drupal-service-2014017-33.patch10.71 KBjlbellido
#33 interdiff.txt763 bytesjlbellido
#28 replace-drupal-container-with-drupal-service-2014017-28.patch10.71 KBjlbellido
#26 replace-drupal-container-with-drupal-service-2014017-26.patch10.66 KBjlbellido
#22 replace-drupal-container-with-drupal-service-2014017-22.patch14.71 KBalvar0hurtad0
#20 replace-drupal-container-with-drupal-service-2014017-20.patch14.23 KBalvar0hurtad0
#16 replace-drupal-container-with-drupal-service-2014017-16.patch14.3 KBalvar0hurtad0
#14 2014017-14.patch2 MBalvar0hurtad0
#13 replace-drupal-container-with-drupal-service-2014017-13.patch12.86 KBjlbellido
#8 replace-drupal-container-with-drupal-service-2014017-8.patch12.86 KBjlbellido
#6 replace-drupal-container-with-drupal-service-2014017-7.patch12.85 KBundertext
#4 replace-drupal-container-with-drupal-service-2014017-2.patch13.33 KBundertext
#3 replace-drupal-container-with-drupal-service-2014017-2.patch13.33 KBundertext
#1 replace-drupal-container-with-drupal-service-2014017-1.patch12.84 KBbenjy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

benjy’s picture

Status: Active » Needs review
FileSize
12.84 KB

Initial patch to get things started. Pretty much a find and replace however I changed Drupal::service('request') for Drupal::request()

Status: Needs review » Needs work

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-1.patch, failed testing.

undertext’s picture

Use statements added.

undertext’s picture

Status: Needs work » Needs review
FileSize
13.33 KB
Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Path/UrlAlterFunctionalTest.phpundefined
@@ -7,6 +7,7 @@
+use Drupal;

@@ -120,7 +121,7 @@ protected function assertUrlOutboundAlter($original, $final) {
-    $result = drupal_container()->get('path.alias_manager')->getSystemPath($original);
+    $result = Drupal::service('path.alias_manager')->getSystemPath($original);

Don't use Drupal, use \Drupal. Same for those below.

undertext’s picture

Status: Needs work » Needs review
FileSize
12.85 KB

Ok. As I understand from https://drupal.org/node/1353118

Classes and interfaces without a backslash \ inside their fully-qualified name (for example, the built-in PHP Exception class) must be fully qualified when used in a namespaced file. For example: new \Exception();.

kgoel’s picture

Status: Needs review » Needs work

It looks like there are a lot of calls in test files that need to be converted to this->container->get().

jlbellido’s picture

jlbellido’s picture

Status: Needs work » Needs review
jlbellido’s picture

I was looking for more instances of drupal_container()->get instances and i didn't find one (i applied #8 patch before search). This is the instances list of drupal_container that i found in system module:

lib/Drupal/system/Tests/Plugin/CacheDecoratorTest.php:    drupal_container()->set("cache.$this->cacheBin", new MemoryBackend($this->cacheBin));
lib/Drupal/system/Tests/Bundle/BundleTest.php:    $this->assertTrue(drupal_container()->getDefinition('file.usage')->getClass() == 'Drupal\\bundle_test\\TestFileUsage', 'Class has been changed');
lib/Drupal/system/Tests/Bundle/BundleTest.php:    $this->assertTrue(drupal_container()->has('bundle_test_class'), 'The bundle_test_class service has been registered to the DIC');
lib/Drupal/system/Tests/Bundle/BundleTest.php:    $this->assertFalse(drupal_container()->has('bundle_test_class'), 'The bundle_test_class service does not exist in the DIC.');
lib/Drupal/system/Tests/Bundle/BundleTest.php:    $this->assertTrue(drupal_container()->has('bundle_test_class'), 'The bundle_test_class service exists in the DIC.');
lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.php:    $this->containerBuild(drupal_container());
lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.php:      $container = drupal_container();
system.module:  if (drupal_container()->isScopeActive('request')) {
ddrozdik’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
error: patch failed: core/modules/system/language.api.php:191
error: core/modules/system/language.api.php: patch does not apply

needs reroll.

jlbellido’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Rerolled #8

jlbellido’s picture

Sorry, attached .patch

alvar0hurtad0’s picture

FileSize
2 MB

Rerolled and tested (I hope)

Status: Needs review » Needs work

The last submitted patch, 2014017-14.patch, failed testing.

alvar0hurtad0’s picture

Re-trying upload the patch now following the steps at https://drupal.org/patch/reroll

RobLoach’s picture

+++ b/core/modules/system/system.moduleundefined
@@ -1027,13 +1027,45 @@ function system_plugin_ui_form($form, &$form_state, $plugin, $facet = NULL) {
+/**
+>>>>>>> Applying patch from https://drupal.org/node/2014017#comment-7608725
  * Checks access for a given plugin using the plugin's access() method.
  *
  * @todo This needs more explanation, some @see, and parameter documentation.

Should this be there?

alvar0hurtad0’s picture

Where?

benjy’s picture

This line

+++ b/core/modules/system/system.module
@@ -1027,13 +1027,45 @@ function system_plugin_ui_form($form, &$form_state, $plugin, $facet = NULL) {
+>>>>>>> Applying patch from https://drupal.org/node/2014017#comment-7608725
alvar0hurtad0’s picture

Status: Needs work » Needs review
FileSize
14.23 KB

Ups,

You're right, Sorry and thank you very much :)

Trying again.

Status: Needs review » Needs work

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-20.patch, failed testing.

alvar0hurtad0’s picture

Status: Needs work » Needs review
FileSize
14.71 KB

Ok,

I know it's an easy task but it's my first patch.

Trying again

Status: Needs review » Needs work

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-22.patch, failed testing.

benjy’s picture

@alvar0hurtad0 not to worry everyone is here to help.

Do you have a another patch applied in this? It seems there is a new test added and some other stuff?

jlbellido’s picture

I have reviewed #22 and i think that this are the mistakes :

- This must be deleted , because in HEAD version it isn't:

+
+    // Check in a rendered page.
+    $this->drupalGet('common-test/query-string');
+    $this->assertPattern('@<script>.+drupalSettings.+"currentPath":"common-test\\\/query-string"@s', 'currentPath is in the JS settings');
+    $path = array('source' => 'common-test/query-string', 'alias' => 'common-test/currentpath-check');
+    \Drupal::service('path.crud')->save($path['source'], $path['alias']);
+    $this->drupalGet('common-test/currentpath-check');
+    $this->assertPattern('@<script>.+drupalSettings.+"currentPath":"common-test\\\/query-string"@s', 'currentPath is in the JS settings for an aliased path');

- This code :

-    $user1->pass = drupal_container()->get('password')->hash(trim($user1->pass_raw));
-    db_query("UPDATE {users} SET pass = :pass WHERE uid = :uid", array(':pass' => $user1->pass, ':uid' => $user1->id()));
+    $user1->pass = \Drupal::service('password')->hash(trim($user1->pass_raw));
+    db_query("UPDATE {users} SET pass = :pass WHERE uid = :uid", array(':pass' => $user1->pass, ':uid' => $user1->uid));

must be :

-  $user1->pass = drupal_container()->get('password')->hash(trim($user1->pass_raw));
+ $user1->pass = \Drupal::service('password')->hash(trim($user1->pass_raw));

Because db_query function hasn't drupal_container function and we must preserve HEAD version.

- This code must be deleted too because this code isn't in HEAD version:

+    $config_to_import['views.view.frontpage'] = 'node';
+  }
+  foreach ($config_to_import as $config_name => $module) {
+    $module_config_path = drupal_get_path('module', $module) . '/config';
+    $module_filestorage = new FileStorage($module_config_path);
+    $config_storage = Drupal::service('config.storage');
+    // If this file already exists, something in the upgrade path went
+    // completely wrong and we want to know.
+    if ($config_storage->exists($config_name)) {
+      throw new ConfigException(format_string('Default configuration file @name of @module module unexpectedly exists already before the module was installed.', array(
+        '@module' => $module,
+        '@name' => $config_name,
+      )));
+    }
+    $config_storage->write($config_name, $module_filestorage->read($config_name));

- This code must be deleted too for the same reason:

+ * Page callback: Autocompletes any plugin system tied to a plugin UI plugin.
+ *
+ * The passed plugin_id indicates the specific plugin_ui plugin that is in use
+ * here. The documentation within the annotation of that plugin will contain a
+ * manager for the plugins that need to be autocompleted allowing this function
+ * to autocomplete plugins for any plugin type.
+ *
+ * @param $plugin_id
+ *   The plugin id for the calling plugin.
+ *
+ * @return object JsonResponse
+ */
+function system_plugin_autocomplete($plugin_id) {
+  $string_typed = Drupal::request()->query->get('q');
+  $string_typed = drupal_explode_tags($string_typed);
+  $string = drupal_strtolower(array_pop($string_typed));
+  $matches = array();
+  if ($string) {
+    $plugin_ui = Drupal::service('plugin.manager.system.plugin_ui')->getDefinition($plugin_id);
+    $manager = Drupal::service($plugin_ui['manager']);
+    $titles = array();
+    foreach($manager->getDefinitions() as $plugin_id => $plugin) {
+      $titles[$plugin_id] = $plugin[$plugin_ui['title_attribute']];
+    }
+    $matches = preg_grep("/\b". $string . "/i", $titles);
+  }
+
+  return new JsonResponse($matches);
+}
+
+/**

Don't worry,this reroll is very dificult to start. Try it one more time with this changes, i hope that it works.

Regards!

jlbellido’s picture

Status: Needs work » Needs review
FileSize
10.66 KB

Rerolled and run test.

Status: Needs review » Needs work

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-26.patch, failed testing.

jlbellido’s picture

Status: Needs work » Needs review
FileSize
10.71 KB

I did #26 reroll past Tuesday , it seems that something has changed again.Trying one more time with a new reroll, this can be applied locally

alvar0hurtad0’s picture

@Jlbellido you are my heroe.

alvar0hurtad0’s picture

Status: Needs review » Fixed

It applyes ok

jlbellido’s picture

Status: Fixed » Needs review
benjy’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Path/UrlAlterFunctionalTest.phpundefined
@@ -112,7 +112,7 @@ protected function assertUrlOutboundAlter($original, $final) {
+   *   Drupal::service('path.alias_manager')->getSystemPath().

This should be absolute as per: https://drupal.org/node/1353118

jlbellido’s picture

Status: Needs work » Needs review
FileSize
763 bytes
10.71 KB

Changed "Drupal::service('path.alias_manager')->getSystemPath()" to "\Drupal::service('path.alias_manager')->getSystemPath()"

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed with emphasis on "Drupal" vs "\Drupal" and IMHO looks nice now.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-33.patch, failed testing.

jlbellido’s picture

Status: Needs work » Needs review
FileSize
10.67 KB

Rerolled #33 and run test.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-38.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
Issue tags: +Novice
penyaskito’s picture

Unrelated failure, retesting.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

ebeyrent’s picture

Looks good to me.

benjy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/lib/Drupal/system/Plugin/PluginUIBase.phpundefined
@@ -27,7 +27,7 @@ public function form($form, &$form_state) {
+      $manager = \Drupal::service($plugin_definition['manager']);

All OO code should have their dependencies injected.

+++ b/core/modules/system/lib/Drupal/system/Tests/Path/UrlAlterFunctionalTest.phpundefined
@@ -112,7 +112,7 @@ protected function assertUrlOutboundAlter($original, $final) {
+   *   \Drupal::service('path.alias_manager')->getSystemPath().

In tests we can use $this->container->get($service_id);

jlbellido’s picture

Status: Needs work » Needs review
FileSize
8.91 KB

It needs be rerolled first, atached the rerolled patch again and running tests

jlbellido’s picture

Changed "\Drupal::service($service_id)" to "$this->container->get($service_id)" in tests.
However i think that : All OO code should have their dependencies injected. is out of scope.

benjy’s picture

Status: Needs review » Needs work

We don't really want \Drupal::service() calls in our OO code at all because it couples the class to the global Drupal class.

We should either fix the OO stuff here or remove the OO changes (outside of tests) in this patch and create a follow up to do so.

penyaskito’s picture

Let's do it here, please :-)
@jlbellido: if you need help ping me on IRC.

andypost’s picture

Issue tags: +Needs reroll
  1. @@ -27,7 +27,7 @@ public function form($form, &$form_state) {
    -      $manager = drupal_container()->get($plugin_definition['manager']);
    +      $manager = \Drupal::service($plugin_definition['manager']);
    

    no more

  2. @@ -963,7 +963,6 @@ function system_menu() {
    -
    

    out of scope

penyaskito’s picture

Rerolled pairing phisically with @jlbellido, still needs DI.

penyaskito’s picture

Status: Needs work » Needs review

Test!

penyaskito’s picture

DI was only needed in the already removed class, so this should be ready for review.

penyaskito’s picture

Issue tags: -Novice, -Needs reroll +WSCCI

Clearing tags and tagging with WSCCI.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I don't think this is strictly WSCCI, but it's RTBC either way. :-)

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +WSCCI

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-49.patch, failed testing.

jlbellido’s picture

Status: Needs work » Needs review
FileSize
7.55 KB

Rerolled #49 and running tests

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Path/UrlAlterFunctionalTest.php
@@ -112,7 +112,7 @@ protected function assertUrlOutboundAlter($original, $final) {
    * @param $original
    *   A string with the aliased or un-normal path that is run through
-   *   drupal_container()->get('path.alias_manager')->getSystemPath().
+   *   $this->container->get('path.alias_manager')->getSystemPath($original).

Lets fix this comment. It is not helpful. How about

    * @param string $original
    *   The original path before it has been altered by inbound URL processing.
jlbellido’s picture

Status: Needs work » Needs review
FileSize
859 bytes
7.61 KB

Updated the comment in core/modules/system/lib/Drupal/system/Tests/Path/UrlAlterFunctionalTest.php with Alexpott's suggestion.

Thanks Alexpott! :D

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Back over to you, Alex.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

jlbellido’s picture

Status: Needs work » Needs review
FileSize
7.71 KB

Relloled #59

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/system.module
@@ -2201,7 +2201,7 @@ function system_page_build(&$page) {
+    $request = Drupal::request();

+++ b/core/modules/system/tests/modules/design_test/design_test.module
@@ -95,8 +95,8 @@ function design_test_menu_local_tasks_alter(&$data, $router_item, $root_path) {
-    $default_theme = \Drupal::config('system.theme')->get('default');
+    $selected_theme = Drupal::request()->query->get('theme');
+    $default_theme = Drupal::config('system.theme')->get('default');
 

@@ -129,5 +129,5 @@ function design_test_menu_local_tasks_alter(&$data, $router_item, $root_path) {
-  return drupal_container()->get('request')->query->get('theme');
+  return Drupal::request()->query->get('theme');

This should be \Drupal instead

jlbellido’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
7.62 KB

Updated with suggested changes of dawehner , thanks!

Status: Needs review » Needs work

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-64.patch, failed testing.

jlbellido’s picture

Status: Needs work » Needs review

Retesting

jlbellido’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

And back up.

David Hernández’s picture

mcrittenden’s picture

Issue tags: -Needs reroll

Tags

LinL’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +WSCCI

The last submitted patch, replace-drupal-container-with-drupal-service-2014017-64.patch, failed testing.

LinL’s picture

Status: Needs work » Needs review
FileSize
6.42 KB

Rerolled without the core/modules/system/tests/modules/design_test/design_test.module part as this module has now been deleted from core #2037569: Remove design_test module

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Sigh.

catch’s picture

Status: Reviewed & tested by the community » Fixed

I think there's an emerging standard to use \Drupal instead of $this->container in simpletest, but not sure if that's documented yet - at least this makes it easier to find all the $this->container so I went ahead and committed/pushed to 8.x, thanks!

catch’s picture

I think there's an emerging standard to use \Drupal instead of $this->container in simpletest, but not sure if that's documented yet - at least this makes it easier to find all the $this->container so I went ahead and committed/pushed to 8.x, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.