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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kmcculloch’s picture

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.

kmcculloch’s picture

Status: Active » Needs review
FileSize
13.52 KB

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

Status: Needs review » Needs work

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

kmcculloch’s picture

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

assigning to self

kmcculloch’s picture

FileSize
30.73 KB

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

kmcculloch’s picture

FileSize
43.24 KB

modified the rest of the files

Status: Needs review » Needs work

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

kmcculloch’s picture

Status: Needs work » Needs review
FileSize
43.47 KB

fixed syntax error

Status: Needs review » Needs work

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

kmcculloch’s picture

FileSize
43.5 KB

slowly debuggin'

kmcculloch’s picture

Status: Needs work » Needs review
FileSize
43.5 KB

again

Status: Needs review » Needs work

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

attiks’s picture

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.

kmcculloch’s picture

Status: Needs work » Needs review
FileSize
12.76 KB

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.

kmcculloch’s picture

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.

kim.pepper’s picture

Issue tags: +Needs reroll

Tagging

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
4.06 KB

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.

kim.pepper’s picture

Tagging

kim.pepper’s picture

tagging

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

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

kim.pepper’s picture

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

Crell’s picture

Issue tags: +Stalking Crell

Retagging, you silly bot.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
2.6 KB

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.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Looks good visually. Bot can object if it wants.

kim.pepper’s picture

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.

kim.pepper’s picture

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.

valdo’s picture

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

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

kim.pepper’s picture

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.

valdo’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
646 bytes
7.87 KB

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
8.69 KB
16.57 KB

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

Crell’s picture

+++ 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.

kim.pepper’s picture

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.

kim.pepper’s picture

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

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, Kim!

YesCT’s picture

Issue tags: +RTBC July 1

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

hass’s picture

kim.pepper’s picture

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

hass’s picture

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.

kim.pepper’s picture

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

alexpott’s picture

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', '');
kim.pepper’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
15.46 KB

Made changes in #48.

Crell’s picture

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

And back.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

tstoeckler’s picture

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.

kim.pepper’s picture

@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

tstoeckler’s picture

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.

tstoeckler’s picture

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...

alexpott’s picture

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.

alexpott’s picture

Priority: Critical » Normal
Issue tags: -Needs tests

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

Crell’s picture

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. :-)

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
539 bytes
14.8 KB

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

kim.pepper’s picture

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

kim.pepper’s picture

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

kim.pepper’s picture

Issue tags: +Needs tests

Tagging

kim.pepper’s picture

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
3.53 KB
13.09 KB

Removed ajax.inc as per #63.

Replaced a few instances of calling deprecated functions.

tstoeckler’s picture

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).

alexpott’s picture

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';
kim.pepper’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.85 KB
13.19 KB

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

tim.plunkett’s picture

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

:\

kim.pepper’s picture

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

kim.pepper’s picture

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.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +RTBC July 1
alexpott’s picture

Status: Needs review » Fixed

Committed c1c38a9 and pushed to 8.x. Thanks!

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