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

Status: Needs review » Needs work

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

ddrozdik’s picture

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

Added some modifications to previous patch and also changed module_exists() to Drupal::moduleHandler()->moduleExists()

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/comment/comment.views.inc
@@ -141,7 +141,7 @@ function comment_views_data() {
 
-  if (module_exists('language')) {
+  if (\Drupal::moduleHandler()->moduleExists('language')) {

@@ -360,7 +360,7 @@ function comment_views_data() {
-  if (drupal_container()->get('module_handler')->moduleExists('translation_entity')) {
+  if (\Drupal::moduleHandler()->moduleExists('translation_entity')) {

In these two the leading \ could actually be omitted.

It's not wrong in any way though (I just think we loosely agreed on omitting in procedural code), so marking RTBC anyway.

ddrozdik’s picture

tstoeckler, ok, fixed in this patch

tstoeckler’s picture

Yay! +1

alexpott’s picture

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

No longer applies

curl https://drupal.org/files/2003498-replace-drupal_container-comment-module-2.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  5927  100  5927    0     0   2395      0  0:00:02  0:00:02 --:--:--  2581
error: patch failed: core/modules/comment/comment.views.inc:360
error: core/modules/comment/comment.views.inc: patch does not apply
error: patch failed: core/modules/comment/lib/Drupal/comment/Plugin/views/field/NcsLastCommentName.php:37
error: core/modules/comment/lib/Drupal/comment/Plugin/views/field/NcsLastCommentName.php: patch does not apply
ddrozdik’s picture

Status: Needs work » Needs review
FileSize
6.65 KB
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

This no longer applies for me.

pwieck’s picture

Status: Needs work » Needs review
FileSize
6.64 KB

Re-rolled @webchick this reroll successfully applied to today's build. Hope it passes.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yup.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NcsLastCommentName.phpundefined
@@ -37,7 +37,7 @@ public function query() {
-    $join = drupal_container()->get('plugin.manager.views.join')->createInstance('standard', $definition);
+    $join = \Drupal::service('plugin.manager.views.join')->createInstance('standard', $definition);

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/sort/NcsLastCommentName.phpundefined
@@ -28,7 +28,7 @@ public function query() {
-    $join = drupal_container()->get('plugin.manager.views.join')->createInstance('standard', $definition);
+    $join = \Drupal::service('plugin.manager.views.join')->createInstance('standard', $definition);

I think these can be injected into the plugin because it extends Drupal\views\Plugin\views\PluginBase

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.phpundefined
@@ -73,7 +73,7 @@ function testCommentEnable() {
+    $this->assertFalse(\Drupal::moduleHandler()->moduleExists('comment'), 'Comment module disabled.');

@@ -85,7 +85,7 @@ function testCommentEnable() {
+    $this->assertTrue(\Drupal::moduleHandler()->moduleExists('comment'), 'Comment module enabled.');

Should use $this->container->get

porchlight’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.86 KB

The patch no longer applied. Heres the reroll

tstoeckler’s picture

Status: Needs review » Needs work

Still needs work for #13.

kgoel’s picture

Status: Needs work » Needs review
FileSize
3.54 KB

I think these can be injected into the plugin because it extends Drupal\views\Plugin\views\PluginBase

Took care of this.

Should use $this->container->get

This was already implemented in core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.php.

ebeyrent’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs reroll...

git ac https://drupal.org/files/comment-2003498-16.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3629  100  3629    0     0   3419      0  0:00:01  0:00:01 --:--:--  7872
error: patch failed: core/modules/comment/comment.module:1325
error: core/modules/comment/comment.module: patch does not apply
Gaelan’s picture

Rerolled. alexpott: BTW this was my reroll script. :)

kgoel’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Reviewed & tested by the community

And back.

catch’s picture

#19: comment.dic_move.2003498.19.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +WSSCI Conversion, +CodeSprintUA

The last submitted patch, comment.dic_move.2003498.19.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Bot can object.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NcsLastCommentName.php
@@ -7,6 +7,7 @@
+use Drupal\views\Plugin\views\PluginBase;

For some reason there we're adding an unnecessary use

kgoel’s picture

Status: Needs work » Needs review
FileSize
620 bytes
5.12 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

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

Patch no longer applies.

star-szr’s picture

Issue tags: -WSSCI Conversion

Tag fix, WSSCI -> WSCCI.

star-szr’s picture

Issue tags: +WSCCI-conversion

Sorry for the noise, didn't check autocomplete.

disasm’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
5.87 KB

reroll! Also, replacing global $user with Drupal::currentUser() in comment.module as well.

Status: Needs review » Needs work

The last submitted patch, drupal8.comment-module.2003498-32.patch, failed testing.

LinL’s picture

Assigned: ddrozdik » Unassigned
Status: Needs work » Needs review
FileSize
3.87 KB

Re-rolled from #27 as the change to global user in #32 looks like scope creep? (And it's being done in #2061899: Remove references to global $user in Comment module)

LinL’s picture

Issue tags: -Needs reroll

Tag fix.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC.

alexpott’s picture

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

Patch no longer applies.

herom’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.98 KB

Rerolled.
Some of the changes were already fixed in HEAD. so, the patch size is smaller.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Crell’s picture

Issue tags: +Quick fix

Let's just get this in.

tstoeckler’s picture

Issue tags: -Quick fix

#38: comment-2003498-38.patch queued for re-testing.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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