Part of #1998638: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object

Files that need converting are:

  • core/modules/comment/comment.admin.inc
  • core/modules/comment/comment.module
  • core/modules/comment/comment.pages.inc
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chertzog’s picture

Assigned: Unassigned » chertzog
Status: Active » Needs review
FileSize
1.88 KB

Status: Needs review » Needs work

The last submitted patch, 1999340_1-replace-raw-variables-comment.patch, failed testing.

chertzog’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.module
@@ -434,7 +434,7 @@ function comment_permalink(Comment $comment) {
-    $_GET['page'] = $page;
+    Drupal::request()->query->set('page', $page);
     return drupal_container()->get('http_kernel')->handle($subrequest, HttpKernelInterface::SUB_REQUEST);

Slight scope creep, but as long as we're in here let's change drupal_container()->get() to Drupal::service('http_kernel').

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
2.17 KB

Fix in #4 plus replaced uses of deprecated drupal_container().

Status: Needs review » Needs work

The last submitted patch, 1999340-comment-request-5.patch, failed testing.

kim.pepper’s picture

"Failed to write configuration file" Re-testing.

kim.pepper’s picture

Status: Needs work » Needs review

#5: 1999340-comment-request-5.patch queued for re-testing.

Crell’s picture

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

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

effulgentsia’s picture

+++ b/core/modules/comment/comment.module
@@ -430,12 +430,12 @@ function comment_permalink(Comment $comment) {
     // @todo: Convert the pager to use the request object.
-    $_GET['page'] = $page;
-    return drupal_container()->get('http_kernel')->handle($subrequest, HttpKernelInterface::SUB_REQUEST);
+    $request->query->set('page', $page);
+    return Drupal::service('http_kernel')->handle($subrequest, HttpKernelInterface::SUB_REQUEST);

Should we remove that @todo now or is there something else needed for that?

Crell’s picture

I think that would require rewriting the pager system, which has needed one for a few years now but still hasn't gotten it. :-(

effulgentsia’s picture

Does that mean the patch that got committed broke the pager when used on a page with comments? Sorry, I'd check myself, but trying to focus on other issues, so leaving this comment in case someone else wants to investigate.

Status: Fixed » Closed (fixed)

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