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

The following files need to be converted:

  • core/includes/ajax.inc
  • core/includes/batch.inc
  • core/includes/bootstrap.inc
  • core/includes/common.inc
  • core/includes/errors.inc
  • core/includes/form.inc
  • core/includes/install.core.inc
  • core/includes/install.inc
  • core/includes/language.inc
  • core/includes/mail.inc
  • core/includes/pager.inc
  • core/includes/session.inc
  • core/includes/tablesort.inc
  • core/tests/bootstrap.php
Files: 
CommentFileSizeAuthor
#69 1998696-core-include-request-68.patch13.03 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,498 pass(es).
[ View ]
#69 interdiff.txt806 byteskim.pepper
#67 1998696-core-include-request-67.patch13.19 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/tablesort.inc.
[ View ]
#67 interdiff.txt1.85 KBkim.pepper
#64 1998696-core-include-request-64.patch13.09 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,167 pass(es).
[ View ]
#64 interdiff.txt3.53 KBkim.pepper
#59 1998696-core-include-request-59.patch14.8 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,628 pass(es).
[ View ]
#59 interdiff.txt539 byteskim.pepper
#49 1998696-core-include-request-49.patch15.46 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,743 pass(es).
[ View ]
#49 interdiff.txt2.97 KBkim.pepper
#41 1998696-core-include-request-41.patch16.28 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,106 pass(es).
[ View ]
#41 interdiff.txt4.23 KBkim.pepper
#38 1998696-core-include-request-38.patch16.57 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,084 pass(es).
[ View ]
#38 interdiff.txt8.69 KBkim.pepper
#36 1998696-core-include-request-36.patch7.87 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 56,679 pass(es), 18 fail(s), and 1 exception(s).
[ View ]
#36 interdiff.txt646 byteskim.pepper
#32 1998696-core-include-request-32.patch8.5 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#32 interdiff.txt8.5 KBkim.pepper
#31 1998696-core-include-request-31.patch1.79 KBvaldo
PASSED: [[SimpleTest]]: [MySQL] 58,309 pass(es).
[ View ]
#26 1998696-core-include-request-26.patch2.6 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 57,544 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#26 interdiff.txt1.17 KBkim.pepper
#20 1998696-core-include-request-20.patch4.06 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#15 1998696_15.patch12.76 KBkmcculloch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1998696_15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 1998696_10.patch43.5 KBkmcculloch
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#10 1998696_10.patch43.5 KBkmcculloch
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#8 1998696_8.patch43.47 KBkmcculloch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]
#6 1998696_6.patch43.24 KBkmcculloch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]
#5 1998696_5.patch30.73 KBkmcculloch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]
#2 1998696_2.patch13.52 KBkmcculloch
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/common.inc.
[ View ]

Comments

Working on this at DrupalCon. core/includes/bootstrap.inc runs before the \Drupal object is loaded, so I can't access $_SERVER['foo'] via \Drupal::request()->server->get('foo'). I could go ahead and load \Drupal here, but I doubt that's a good idea. Skipping bootstrap.inc for now.

Status:Active» Needs review
StatusFileSize
new13.52 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/common.inc.
[ View ]

preliminary patch, addresses ajax.inc, batch.inc and common.inc

Status:Needs review» Needs work

The last submitted patch, 1998696_2.patch, failed testing.

Assigned:Unassigned» kmcculloch
Status:Needs work» Needs review

assigning to self

StatusFileSize
new30.73 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]

fixed previous bugs; adding errors.inc, form.inc, install.core.inc

StatusFileSize
new43.24 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]

modified the rest of the files

Status:Needs review» Needs work

The last submitted patch, 1998696_6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new43.47 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/install.core.inc.
[ View ]

fixed syntax error

Status:Needs review» Needs work

The last submitted patch, 1998696_8.patch, failed testing.

StatusFileSize
new43.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

slowly debuggin'

Status:Needs work» Needs review
StatusFileSize
new43.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

again

Status:Needs review» Needs work

The last submitted patch, 1998696_10.patch, failed testing.

Status:Needs work» Needs review

#10: 1998696_10.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1998696_10.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new12.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1998696_15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

rolling a patch including only inc files that have passed local testing or that I think unlikely to cause errors: ajax, batch, errors, language, mail, pager, tablesort and tests/bootstrap.php

Status:Needs review» Needs work

The last submitted patch, 1998696_15.patch, failed testing.

Status:Needs work» Needs review

#15: 1998696_15.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1998696_15.patch, failed testing.

Issue tags:+Needs reroll

Tagging

Status:Needs work» Needs review
StatusFileSize
new4.06 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This one is a potential can-o-worms. I've included just the common.inc in this patch to see if the bot gods are pleased.

Tagging

tagging

Status:Needs review» Needs work
Issue tags:-Stalking Crell+Needs reroll

The last submitted patch, 1998696-core-include-request-20.patch, failed testing.

The web UI installer is working fine. Just failing from drush install.

Issue tags:+Stalking Crell

Retagging, you silly bot.

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new2.6 KB
FAILED: [[SimpleTest]]: [MySQL] 57,544 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Looks like there is an issue converting POST in common.inc. The request object isn't available from the container when installing with Drush.

This patch removes that conversion.

Status:Needs review» Reviewed & tested by the community

Looks good visually. Bot can object if it wants.

Status:Reviewed & tested by the community» Needs work

Sorry Crell. This is only the common.inc file. We need to add the rest of the files too.

Status:Needs work» Needs review
Issue tags:-Needs reroll, -Stalking Crell

Status:Needs review» Needs work
Issue tags:+Needs reroll, +Stalking Crell

The last submitted patch, 1998696-core-include-request-26.patch, failed testing.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,309 pass(es).
[ View ]

Patch re-rolled. The changes in drupal_goto() have been discarded as this function doesn't exist anymore.

StatusFileSize
new8.5 KB
new8.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Added the remaining conversions.

Status:Needs review» Needs work
Issue tags:-Stalking Crell

The last submitted patch, 1998696-core-include-request-32.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Stalking Crell

The last submitted patch, 1998696-core-include-request-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new646 bytes
new7.87 KB
FAILED: [[SimpleTest]]: [MySQL] 56,679 pass(es), 18 fail(s), and 1 exception(s).
[ View ]

Reverted the form.inc conversions, as they break the installer.

Status:Needs review» Needs work

The last submitted patch, 1998696-core-include-request-36.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.69 KB
new16.57 KB
PASSED: [[SimpleTest]]: [MySQL] 58,084 pass(es).
[ View ]

Dug around in the tests and found some that were using $_GET causing test failures. Injecting mock requests into the container now.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/TableSortExtenderUnitTest.php
@@ -53,38 +54,44 @@ function testTableSortInit() {
+    $request->query->replace(array());
+    $request->query->add(array(
       'sort' => 'DESC',
       // Add an unrelated parameter to ensure that tablesort will include
       // it in the links that it creates.
       'alpha' => 'beta',
-    );
+    ));

Why not just replace(array('alpha'=>'beta'))? (Same for the other places this patch does this.

Otherwise this looks OK to me visually.

Crell, because we are using the current request and at least in UI testing, we have a load of cruft in the url from batch api, overlay etc, that needs to be cleared. Not sure if there is a better way of doing that.

StatusFileSize
new4.23 KB
new16.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,106 pass(es).
[ View ]

After speaking with @Crell in IRC I actually understand what he means now. Removed the redundant use of query->replace(array()).

Status:Needs review» Reviewed & tested by the community

Thanks, Kim!

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

I don't believe so. It deals with the how we are using the request, not the response.

If you want to check values in the response, have a look at the last example on https://drupal.org/node/2023537. This shows how to implement an EventSubscriber for the Response event, where you can get access to it.

Kim

Thanks for the hint, but event subscriber looks not like a possible solution for Google Analytics as I need to know the response at a specific moment and I do not need to run code when the event occurs. The event occurence is not the moment when I need the information.

Ok. Probably best to move the discussion from here to your specific issue.

Status:Reviewed & tested by the community» Needs work

+++ b/core/includes/pager.incundefined
@@ -26,7 +26,8 @@
+  $request = Drupal::request();
+  $page = $request->query->get('page', '');
@@ -136,7 +137,8 @@ function pager_default_initialize($total, $limit, $element = 0) {
+    $request = Drupal::request();
+    $query = drupal_get_query_parameters($request->query->all(), array('page'));
@@ -345,7 +347,8 @@ function theme_pager_link($variables) {
+  $request = Drupal::request();
+  $page = $request->query->get('page', '');
+++ b/core/includes/tablesort.incundefined
@@ -100,7 +100,8 @@ function tablesort_cell($cell, $header, $ts, $i) {
+  $request = Drupal::request();
+  return drupal_get_query_parameters($request->query->all(), array('sort', 'order'));
@@ -115,7 +116,8 @@ function tablesort_get_query_parameters() {
+  $request = Drupal::request();
+  $order = $request->query->get('order', '');

lets do these changes without creating a $request variable...

eg for the last one...

  $order = Drupal::request()->query->get('order', '');

Status:Needs work» Needs review
StatusFileSize
new2.97 KB
new15.46 KB
PASSED: [[SimpleTest]]: [MySQL] 56,743 pass(es).
[ View ]

Made changes in #48.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Stalking Crell

And back.

Status:Reviewed & tested by the community» Fixed

Committed 127b54a and pushed to 8.x. Thanks!

Status:Fixed» Needs review

+++ b/core/includes/ajax.inc
@@ -235,12 +235,14 @@ function ajax_render($commands = array()) {
-    if (empty($_POST['ajax_page_state'][$type])) {
+    $state = $request->request->get('ajax_page_state[' . $type . ']', FALSE, TRUE);
+    if ($state) {

Either the Symfony Request object is trickier than I thought, or the conversion is missing the empty() check. Even if so, this definitely needs a comment, so I'm re-opening this for "needs review" again.

@tstoeckler we return FALSE if the value does not exist, so if ($state) doesn't need a check for empty. See http://api.symfony.com/2.2/Symfony/Component/HttpFoundation/ParameterBag...

Kim

Right, so in the case that $_POST['ajax_page_state'][$type] is not set empty($_POST['ajax_page_state'][$type]) will return TRUE (previous behavior) and $state (as defined in #52) returns FALSE. So this changes the behavior, no? Sorry, if I'm missing something obvious.

Status:Needs review» Needs work

Per @alexpott in IRC what I'm trying to say is that it should be if (!$state) not if ($state).

This sadly means that we're also missing test coverage for ajax_render(). The best of luck to whomever wants to tackle that beast...

Priority:Normal» Critical
Issue tags:+Needs tests

So I've revert the commit... we obviously have no test coverage here...

Nice catch @tstoeckler

Committed f17f9cf and pushed to 8.x.

Priority:Critical» Normal
Issue tags:-Needs tests

Now not critical... because I reverted...

Why is ajax_render() even still a thing? That should be gone entirely by now. If not, I'm sure there's an issue somewhere to remove it. If not, there should be. :-)

Status:Needs work» Needs review
StatusFileSize
new539 bytes
new14.8 KB
PASSED: [[SimpleTest]]: [MySQL] 57,628 pass(es).
[ View ]

Fixed the logic in #52. No extra tests yet. Seeing what the bot says.

ajax_render() is still called from Drupal\Core\EventSubscriber\ViewSubscriber::onAjax()

...and Drupal\Core\EventSubscriber\ViewSubscriber::onIframeUpload()

Issue tags:+Needs tests

Tagging

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

Given that #1959574: Remove the deprecated Drupal 7 Ajax API is a critical, I suggest we

1) Remove the ajax.inc changes from this issue
2) Remove the requirement for ajax tests

Status:Needs work» Needs review
StatusFileSize
new3.53 KB
new13.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,167 pass(es).
[ View ]

Removed ajax.inc as per #63.

Replaced a few instances of calling deprecated functions.

Status:Needs review» Reviewed & tested by the community

OK, that looks good. I opened #2074995: Missing test coverage in ajax_render() for the missing test coverage which might or might not be crucial, depending on the fate of ajax_render() and also the equivalent code in AjaxController (i.e. if that has proper test coverage).

Status:Reviewed & tested by the community» Needs work

+++ b/core/includes/common.inc
@@ -4077,7 +4077,8 @@ function show(&$element) {
+  $request = Drupal::request();
+  if (!$request->isMethodSafe() || !$cid = drupal_render_cid_create($elements)) {
@@ -4109,7 +4110,8 @@ function drupal_render_cache_get($elements) {
+  $request = Drupal::request();
+  if (!$request->isMethodSafe() || !$cid = drupal_render_cid_create($elements)) {
+++ b/core/includes/errors.inc
@@ -222,7 +222,8 @@ function _drupal_log_error($error, $fatal = FALSE) {
+  $request = Drupal::request();
+  if ($request->isXmlHttpRequest()) {

Why bother with assigning the $request variable?

+++ b/core/includes/tablesort.inc
@@ -150,8 +151,9 @@ function tablesort_get_order($headers) {
+  $sort = Drupal::request()->query->get('sort');
+  if (isset($sort)) {
+    return (strtolower($sort) == 'desc') ? 'desc' : 'asc';

I think we can improve the readability of the code here. Assigning the $sort variable and then checking if it's set just looks odd. How about something like this?

  $query = Drupal::request()->query;
  if ($query->has('sort')) {
    return (strtolower($query->get('sort')) == 'desc') ? 'desc' : 'asc';

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new1.85 KB
new13.19 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/tablesort.inc.
[ View ]

Thanks for the feedback. This makes the changes in #66. Assume RTBC is green.

+++ b/core/includes/tablesort.inc
@@ -165,6 +165,6 @@ function tablesort_get_sort($headers) {
-  }
+  }q

:\

StatusFileSize
new806 bytes
new13.03 KB
PASSED: [[SimpleTest]]: [MySQL] 58,498 pass(es).
[ View ]

Gah! Somehow added an extra character. Also missed one of the items in #66.

Status:Reviewed & tested by the community» Needs review

Status:Needs review» Needs work
Issue tags:-RTBC July 1

The last submitted patch, 1998696-core-include-request-68.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+RTBC July 1

Status:Needs review» Fixed

Committed c1c38a9 and pushed to 8.x. Thanks!

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