drupal_container() is deprecated, and all calls in the ckeditor 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)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ebeyrent’s picture

Status: Active » Needs review
FileSize
6.28 KB

Status: Needs review » Needs work

The last submitted patch, 2003430-1.patch, failed testing.

ddrozdik’s picture

Assigned: ebeyrent » ddrozdik
Status: Needs work » Needs review
Issue tags: +CodeSprintUA
FileSize
6.95 KB

Attached new patch with some modifications.

podarok’s picture

Status: Needs review » Reviewed & tested by the community

#3 clean conversion
RTBC

ebeyrent’s picture

+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Tests/CKEditorTest.php
@@ -161,8 +160,7 @@ function testBuildContentsCssJSSetting() {
+    $internal_plugin = $this->container->get('plugin.manager.ckeditor.plugin')->createInstance('internal');

Is it confusing for developers new to Drupal 8 to see $this->container->get() vs. Drupal::service()?

ddrozdik’s picture

ebeyrent, No, we should use $this->container in tests instead Drupal::service(), and it's ok, because container already available from parent class and don't need to call again it, just look at parent clases.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll

curl https://drupal.org/files/2003430-replace-drupal_container-ckeditor-module.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7120  100  7120    0     0   3270      0  0:00:02  0:00:02 --:--:--  4020
error: patch failed: core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.php:49
error: core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.php: patch does not apply
pwieck’s picture

Status: Needs work » Needs review
FileSize
6.93 KB

Re-rolled

Status: Needs review » Needs work
Issue tags: -WSSCI Conversion, -CodeSprintUA

The last submitted patch, ckeditor-module-replace-drupal_container-2003430-8.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review
Issue tags: +WSSCI Conversion, +CodeSprintUA
pwieck’s picture

Bad testbot...Bad! Don't know why this failed so bad the first time. I guess submitting a test in the middle of the night is a not a good idea.

pwieck’s picture

Status: Needs review » Reviewed & tested by the community

ckeditor seems to work as it should without errors on this mornings build.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.phpundefined
@@ -49,7 +49,7 @@ public function getDefaultSettings() {
   public function settingsForm(array $form, array &$form_state, EditorEntity $editor) {
     $module_path = drupal_get_path('module', 'ckeditor');
-    $manager = drupal_container()->get('plugin.manager.ckeditor.plugin');
+    $manager = \Drupal::service('plugin.manager.ckeditor.plugin');
     $ckeditor_settings_toolbar = array(
       '#theme' => 'ckeditor_settings_toolbar',
       '#editor' => $editor,
@@ -114,7 +114,7 @@ public function getJSSettings(EditorEntity $editor) {

@@ -114,7 +114,7 @@ public function getJSSettings(EditorEntity $editor) {
     $language_interface = language(Language::TYPE_INTERFACE);
 
     $settings = array();
-    $manager = drupal_container()->get('plugin.manager.ckeditor.plugin');
+    $manager = \Drupal::service('plugin.manager.ckeditor.plugin');

Now that #1903346: Establish a new DefaultPluginManager to encapsulate best practices has landed lets do the inject properly here...

This means that CKEditorPluginManager will need to be converted to the new DefaultPluginManager class and then the CKEditor plugin will need to be converted to implement ContainerFactoryPluginBase.

This is because replacing drupal_container for \Drupal::service is just replacing a static with a static...

Wim Leers’s picture

Title: Replace drupal_container() with Drupal::service() in the ckeditor module » Convert CKEdito module to use DefaultPluginManager and ContainerFactoryPluginBase, remove drupal_container()
Issue tags: +Spark, +CKEditor in core

Updating title.

Are you still up for it, DmitryDrozdik or pwieck? I promise fast reviews if you still are! :)

Wim Leers’s picture

Title: Convert CKEdito module to use DefaultPluginManager and ContainerFactoryPluginBase, remove drupal_container() » Convert CKEditor module to use DefaultPluginManager and ContainerFactoryPluginBase, remove drupal_container()

Fix typo.

Wim Leers’s picture

Title: Convert CKEditor module to use DefaultPluginManager and ContainerFactoryPluginBase, remove drupal_container() » Convert CKEditor tests to use $this->container instead of drupal_container()
Assigned: ddrozdik » Wim Leers
Status: Needs work » Needs review
Issue tags: +sprint
FileSize
4.42 KB

#13:

Hence the scope of this issue has been reduced to just removing the leftover drupal_container() calls in CKEditor's tests.

Patch attached for just that.

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

The last submitted patch, ckeditor-module-replace-drupal_container-2003430-16.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
4.59 KB
ebeyrent’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Wim Leers’s picture

Issue tags: +sprint

.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks pretty straight-forward.

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.