In order to depend on using the Request object to determine caching, we need to ensure that everything uses it, rather than raw PHP variables for $_SERVER, $_REQUEST, $_GET, $_POST.

Conversion Guide

Remaining tasks: One last patch to remove all last $_GET, $_POST, $_COOKIE, $_REQUEST references. (Ignore $_SESSION for now, as it's being addressed elsewhere.)

Here are the methods on the Request object that map to PHP variables.

  • $request->query The GET parameters
  • $request->request The POST parameters
  • $request->attributes The request attributes (parameters parsed from the PATH_INFO, ...)
  • $request->cookies The COOKIE parameters
  • $request->files The FILES parameters
  • $request->server The SERVER parameters

See the Symfony documentation for an overview or a full list of convenience methods

To get the request object:

  1. If it is a route controller method, you can add a Request parameter as the last param in the method signature, and it will magically be passed in
  2. If you are in a new PSR-0, namespaced class, use \Drupal::request() (with a leading backslash)
  3. If you are in old-skool functional land (e.g. includes) use Drupal::request() (without a leading backslash)

Sub-issues

Name Issue
Includes #1998696: Use Symfony Request for core includes Assigned to: kmcculloch
Core classes #1998700: Use Symfony Request for core classes Assigned to: mhagedon
Action module #1998704: Use Symfony Request for action module Assigned to: chertzog
Aggregator module #1999338: Use Symfony Request for aggregator module Assigned to: chertzog
Block module #1998708: Use Symfony Request for block module
Comment module #1999340: Use Symfony Request for comment module Assigned to: chertzog
Edit module #1999344: Use Symfony Request for edit module
Field UI module #1999346: Use Symfony Request for fieldui module Assigned to: atchijov
File module #1999370: Use Symfony Request for file module Assigned to: chertzog
Filter module #1999376: Use Symfony Request for filter module Assigned to: chertzog
Image module #1999384: Use Symfony Request for image module Assigned to: arknoll
Language module #1999388: Use Symfony Request for language module
Locale module #1999394: Use Symfony Request for locale module
Menu module #1999398: Use Symfony Request for menu module Assigned to: chertzog
Node module #1999404: Use Symfony Request for node module
Overlay module #1999408: Use Symfony Request for overlay module Assigned to: atchijov
Path module #1999424: Use Symfony Request for path module
Search module #1999426: Use Symfony Request for search module
Shortcut module #1999428: Use Symfony Request for shortcut module Assigned to: chrishks
Simpletest module #1999430: Use Symfony Request for simpletest module Assigned to: chertzog
System module #1999434: Use Symfony Request for system module
Taxonomy module #1999436: Use Symfony Request for taxonomy module Assigned to: sidharthap
Toolbar module #1999442: Use Symfony Request for toolbar module Assigned to: chertzog
Translation module #1999444: Use Symfony Request for translation module Assigned to: chertzog
User module #1999448: Use Symfony Request for user module Assigned to: chertzog
Views module #1999450: Use Symfony Request for views module

..

Files: 
CommentFileSizeAuthor
#65 1998638.65.patch71.35 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,199 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#65 interdiff.txt3.95 KBalexpott
#44 globals-1998638.patch69.36 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,078 pass(es).
[ View ]
#44 interdiff.txt654 bytesdawehner
#42 interdiff.txt1.65 KBdawehner
#41 globals-1998638.41.patch69.78 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 59,075 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#41 interdiff.txt955 byteslarowlan
#39 interdiff.txt1.58 KBkim.pepper
#39 1998638-superglobals-39.patch69.67 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 59,046 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#37 1998638-superglobals-37.patch69.69 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 59,037 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#33 1998638-33.patch69.56 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 59,037 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#31 1998638-31.patch67.92 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 60,039 pass(es), 6 fail(s), and 4 exception(s).
[ View ]
#27 1998638-27.patch65.15 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 59,799 pass(es), 13 fail(s), and 6 exception(s).
[ View ]
#22 interdiff-1998638-22.txt3.05 KBdamiankloip
#22 1998638-22.patch63.65 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 57,561 pass(es), 485 fail(s), and 315 exception(s).
[ View ]
#20 1998638-last.patch61.2 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 1998638-superglobals.patch1.98 KBDamien Tournoud
FAILED: [[SimpleTest]]: [MySQL] 56,423 pass(es), 0 fail(s), and 10 exception(s).
[ View ]
#3 1998638_3.patch7 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#1 1998638_2.patch1.2 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] 55,668 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Comments

cosmicdreams’s picture

StatusFileSize
new1.2 KB
FAILED: [[SimpleTest]]: [MySQL] 55,668 pass(es), 5 fail(s), and 0 exception(s).
[ View ]

Here's an example of what Kim's talking about. This converts common.inc's use of $_SERVER variables. I just want to see if this blows anything up.

cosmicdreams’s picture

Status:Active» Needs review

go testbot.

kim.pepper’s picture

Issue summary:View changes

Added conversion guide

cosmicdreams’s picture

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

This patch converts all $_SERVER['REQUEST_METHOD']

cosmicdreams’s picture

Issue summary:View changes

Added getMethod() note

kim.pepper’s picture

Issue summary:View changes

Added link to symfony docs.

Damien Tournoud’s picture

StatusFileSize
new1.98 KB
FAILED: [[SimpleTest]]: [MySQL] 56,423 pass(es), 0 fail(s), and 10 exception(s).
[ View ]

Alternatively, we could decide to do something like this.

There is nothing wrong in using those globals, as long as we enforce that only one request is active at the same time, which is already true because we have Drupal::request().

Damien Tournoud’s picture

Issue summary:View changes

Added link to request api page

kim.pepper’s picture

Issue summary:View changes

Added more files

Status:Needs review» Needs work

The last submitted patch, 1998638-superglobals.patch, failed testing.

Damien Tournoud’s picture

About #3: you need to use $request->getMethod() here. Reading from ->server directly is almost never the right thing to do.

cosmicdreams’s picture

@Damien: Yea, Kim pointed that out to me after I did it. This seems like a great issue to spread out during code sprints.

kmcculloch’s picture

I am working on some of the subissues here at DrupalCon. It's easy to get $_SERVER, $_GET and $_POST from the Symfony Request object, but there does not seem to be an equivalent for $_REQUEST. Am I missing it? If it doesn't exist, what should I do: roll a $_REQUEST by hand by merging get, post and cookie, or try to figure out whether it's the get or the post that contains the id that would otherwise be passed into $_REQUEST?

Crell’s picture

$_REQUEST is an unreliable value anyway. Its data may change depending on server configuration. It shouldn't be used. You always care about whether data is coming in from query parameters or the request body (GET vs POST), and it's a bug in PHP that they let you merge them at all.

kmcculloch’s picture

I can't decide the best solution here. In the interest of moving forward, I'm going to avoid tampering with the $_REQUEST variables.

That said, based on my research, I see two general approaches. (I'm assuming that we don't need cookie variables, but I'm not sure if that's a safe assumption.)

The first is to merge the $query (i.e. GET) and $request (i.e. POST) objects:

$request = array_merge(\Drupal::request()->request->all(), \Drupal::request()->query->all());
$foo = $request['foo'];

The second is to check the request method and fetch an array conditionally:

if (\Drupal::request()->isMethod('GET')) {
$request = \Drupal::request()->query->all();
} else {
$request = \Drupal::request()->request->all();
}
$foo = $request['foo'];

The main problem I don't know how to address is order of precedence when $_GET and $_POST both contain the same array key. In a given PHP environment, the order of $_REQUEST depends on the variables_order configuration directive. The default value of this is 'GPCS' (at least, that's what came out of the box with my PHP), meaning $_GET comes before $_POST. But it could be changed. Then again, Drupal currently calls $_REQUEST without checking precedence (as far as I can tell), so this may be a non-issue.

kmcculloch’s picture

Just spoke to Kim. (Had no idea he was here in the room with us!) His recommendation is to use \Drupal::request()->get(), which pulls (in order) from GET, PATH, POST. This is slower than calling directly to GET or POST, so it's better to use those if you can figure out the context.

kmcculloch’s picture

Issue summary:View changes

Added subissues

kim.pepper’s picture

Issue summary:View changes

Added more sub-tasks

kim.pepper’s picture

Issue summary:View changes

Added more links

kim.pepper’s picture

Issue summary:View changes

more links

kim.pepper’s picture

Issue summary:View changes

More links

kim.pepper’s picture

Issue summary:View changes

More links

kim.pepper’s picture

Issue summary:View changes

typo

Crell’s picture

I still hold that anywhere we're using $_REQUEST now is a pre-existing bug. Whether we fix that here or later is a debatable point. :-)

kmcculloch’s picture

I'm inclined to agree with you, Crell. I went through and patched all of the core/includes files yesterday, and the only place I found $_REQUEST was in batch.inc:

mac:~/drupal8/core/includes$ fgrep -rn "\$_REQUEST[" *
batch.inc:24: *   the relevant ID is found in $_REQUEST['id'].
batch.inc:48:  if (!isset($_REQUEST['id'])) {
batch.inc:54:    $batch = batch_load($_REQUEST['id']);
batch.inc:73:  $op = isset($_REQUEST['op']) ? $_REQUEST['op'] : '';

These lines all occur within a function called _batch_page(), which is sprinkled throughout core:

mac:~/drupal8/core$ fgrep -rn "_batch_page" *
authorize.php:137:    $output = _batch_page();
includes/batch.inc:45:function _batch_page() {
includes/batch.inc:489: * @see _batch_page()
includes/install.core.inc:607:      $output = _batch_page();
modules/system/system.admin.inc:1386:function system_batch_page() {
modules/system/system.admin.inc:1388:  $output = _batch_page();
modules/system/system.module:1016:    'page callback' => 'system_batch_page',
update.php:521:      $output = _batch_page();

I will return to the core/includes sub-issue later this week and study the context of _batch_page() to see if I can use a direct reference to the POST or GET data.

kmcculloch’s picture

Issue summary:View changes

Removed files as they are now in the issues.

kim.pepper’s picture

Issue summary:View changes

Added more info about the request

Crell’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -158,6 +160,42 @@ public function boot() {
   /**
+   * Overrides Symfony\Component\HttpKernel\HttpKernel::handle().
+   */
+  public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQUEST, $catch = true) {

Wait, what? This doesn't make any sense to me at all...

Cottser’s picture

Status:Needs work» Active

Since this is a meta, setting to active. Patching should be happening in the sub-issues.

Cottser’s picture

Issue summary:View changes

.

Crell’s picture

Issue tags:+WSCCI

Tagging

Crell’s picture

Issue summary:View changes

Refreshing links

kim.pepper’s picture

Issue summary:View changes

Refreshing statuses

kim.pepper’s picture

Issue summary:View changes

Updating status

webchick’s picture

I committed the final two issues in the issue summary:

#1999388: Use Symfony Request for language module
#1999430: Use Symfony Request for simpletest module

However, it looks like we've managed to re-introduce some in the meantime. In addition to $_SESSION being used everywhere, there are stragglers in core/includes/ajax.inc, core/includes/bootstrap.inc, core/includes/install.core.inc, among others.

webchick’s picture

Issue summary:View changes

Remove OpenID as it is no longer a core module.

cosmicdreams’s picture

$_SESSION is to be handled after we complete #1858196: [meta] Leverage Symfony Session components and is not apart of this issue.

Crell’s picture

Title:[Meta] Refactor raw PHP variables (e.g $_SERVER, $_REQUEST, $_GET, $_POST) with Symfony Request object» Replace all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object
Issue summary:View changes

Refocusing the issue to finish this off.

damiankloip’s picture

Status:Active» Needs review
StatusFileSize
new61.2 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Ok, so here is an initial attempt to 'finish it off', and one question:

Shall we do something with the 'lazy_session' global in this patch?

Status:Needs review» Needs work

The last submitted patch, 20: 1998638-last.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 20: 1998638-last.patch, failed testing.

damiankloip’s picture

StatusFileSize
new63.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,561 pass(es), 485 fail(s), and 315 exception(s).
[ View ]
new3.05 KB

Let's try this.

damiankloip’s picture

Status:Needs work» Needs review

F%&^

dawehner’s picture

  1. +++ b/core/includes/common.inc
    @@ -522,7 +523,7 @@ function drupal_get_destination() {
    + *   The URL string to parse, i.e \Drupal::request()->query->get('destination').

    yeah 80 chars!

  2. +++ b/core/includes/install.core.inc
    @@ -1550,9 +1555,9 @@ function install_select_language(&$install_state) {
    -  if (!empty($_POST['langcode'])) {
    +  if ($request_params->has('langcode')) {

    So you kind of jump from using !empty and ->has() all over the place. is there some logic behind that?

  3. +++ b/core/includes/pager.inc
    @@ -109,10 +109,11 @@ function pager_find_page($element = 0) {
    diff --git a/core/includes/session.inc b/core/includes/session.inc

    I wonder whether it is a good idea to break the session patch, which will change all the things anyway.

  4. +++ b/core/lib/Drupal/Component/Utility/Url.php
    @@ -118,13 +119,13 @@ public static function filterQueryParameters(array $query, array $exclude = arra
    +   *   $options = Url::parse(\Drupal::request->query->get('destination'));
    ...
    +   *   The URL string to parse, i.e \Drupal::request->query->get('destination').

    Note: this cannot work, as () is missing.

  5. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
    @@ -76,7 +76,7 @@ protected function ajaxRender(Request $request) {
    -      // It is highly suspicious if $_POST['ajax_page_state'][$type] is empty,
    +      // It is highly suspicious if $ajax_page_state[$type] is empty,

    Can we keep somehow $request->request so it is more clear where the ajax page state was set?

  6. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -202,7 +202,8 @@ public function buildForm($form_id, &$form_state) {
    +      $request = \Drupal::request();
    +      $form_state['input'] = $form_state['method'] == 'get' ? $request->query->all() : $request->request->all();

    For D9 we could maybe think about using a parameter bag instead of an array here, so it is a bit better to use.

  7. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -1465,9 +1466,10 @@ protected function handleInputElement($form_id, &$element, &$form_state) {
    -        // To make it easier to handle $_FILES in file.inc, we place all
    +        // To make it easier to handle files in file.inc, we place all

    Is it just me that removing "$_" in comments makes things harder to understand? Maybe we should just not hide that to the people, given that this is valuable information.

  8. +++ b/core/modules/search/search.module
    index a4c7b27..813019e 100644
    --- a/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php

    --- a/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php

    +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    +++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
    @@ -1138,8 +1138,9 @@ protected function curlExec($curl_options, $redirect = FALSE) {

    @@ -1138,8 +1138,9 @@ protected function curlExec($curl_options, $redirect = FALSE) {
         // so you can't debug actual running code. In order to make debuggers work
         // this bit of information is forwarded. Make sure that the debugger listens
         // to at least three external connections.
    -    if (isset($_COOKIE['XDEBUG_SESSION'])) {
    -      $cookies[] = 'XDEBUG_SESSION=' . $_COOKIE['XDEBUG_SESSION'];
    +    $cookie_params = \Drupal::request()->cookies;
    +    if ($cookie_params->has('XDEBUG_SESSION')) {
    +      $cookies[] = 'XDEBUG_SESSION=' . $cookie_params->get('XDEBUG_SESSION');
         }

    Just wondering whether you have tested this change.

Status:Needs review» Needs work

The last submitted patch, 22: 1998638-22.patch, failed testing.

Crell’s picture

  1. +++ b/core/includes/bootstrap.inc
    @@ -457,25 +457,28 @@ function config_get_config_directory($type = CONFIG_ACTIVE_DIRECTORY) {
    -      $_SERVER['HTTP_HOST'] = $url['host'];
    +      $server_vars['HTTP_HOST'] = $url['host'];

    Can be replaced with http://api.symfony.com/2.3/Symfony/Component/HttpFoundation/Request.html...

  2. +++ b/core/includes/bootstrap.inc
    @@ -457,25 +457,28 @@ function config_get_config_directory($type = CONFIG_ACTIVE_DIRECTORY) {
    -      $_SERVER['SCRIPT_NAME'] = $url['path'];
    +      $server_vars['SCRIPT_NAME'] = $url['path'];

    Can be replaced with:

    http://api.symfony.com/2.3/Symfony/Component/HttpFoundation/Request.html...

  3. +++ b/core/includes/common.inc
    @@ -522,7 +523,7 @@ function drupal_get_destination() {
    - *   The URL string to parse, f.e. $_GET['destination'].
    + *   The URL string to parse, i.e \Drupal::request()->query->get('destination').

    f.e. means "for example". i.e translates to "that is". Changing from f.e. to i.e. means that the string to parse is always the destination.

    If you don't like f.e., then e.g. would be the alternative. Or, simply remove that clause from the sentence as it is rather pointless.

  4. +++ b/core/includes/install.core.inc
    @@ -253,9 +253,19 @@ function install_state_defaults() {
    +  // Create a minimal container for t() and request to work. This container will
    +  // be overriden but it needed for the very early installation process when
    +  // database tasks run.

    I think you need "for t() and $request to work", otherwise "request" seems like a verb, not a noun. And instead of "for", try "so that", which is more precise in this case.

  5. +++ b/core/includes/session.inc
    @@ -267,7 +268,7 @@ function drupal_session_initialize() {
    -      $_COOKIE[$insecure_session_name] = $session_id;
    +      $cookies->set($insecure_session_name, $session_id);

    Why do we set the cookie on the $request here....

  6. +++ b/core/includes/session.inc
    @@ -323,7 +324,8 @@ function drupal_session_commit() {
    -        setcookie($insecure_session_name, $_COOKIE[$insecure_session_name], $expire, $params['path'], $params['domain'], FALSE, $params['httponly']);
    +        $cookie_params = \Drupal::request()->cookies;
    +        setcookie($insecure_session_name, $cookie_params->get($insecure_session_name), $expire, $params['path'], $params['domain'], FALSE, $params['httponly']);

    But use setcookie() here?

  7. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
    @@ -154,8 +154,9 @@ public function getForm($form_arg);
    +   *     \Drupal::Request()->request or \Drupal::Request()->query, depending on

    Drupal::request(); lowercase method name.

    That said, I don't know if we should be directly referring to the \Drupal class in all of these comments. Shouldn't it be something more like "the request", or...? (This applies in many places.)

  8. +++ b/core/modules/system/system.module
    @@ -2743,7 +2744,8 @@ function system_default_region($theme) {
    - * inspect $_POST to see if an action was confirmed.
    + * inspect $_POST or \Drupal::request90->request to see if an action was
    + * confirmed.

    I think your shift key is on strike. :-)

  9. +++ b/core/modules/views_ui/lib/Drupal/views_ui/ViewEditFormController.php
    @@ -289,9 +289,6 @@ public function submit(array $form, array &$form_state) {
    -          // @todo For whatever reason drupal_goto is still using $_GET.
    -          // @see http://drupal.org/node/1668866
    -          unset($_GET['destination']);

    Yay!

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new65.15 KB
FAILED: [[SimpleTest]]: [MySQL] 59,799 pass(es), 13 fail(s), and 6 exception(s).
[ View ]
new10.04 KB

OK, thanks for the reviews. Most of the feedback addressed in this patch, I hope this should fix alot of the fails too.

I have not made the docs changes yet.

Status:Needs review» Needs work

The last submitted patch, 27: 1998638-27.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 27: 1998638-27.patch, failed testing.

damiankloip’s picture

Fixing..

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new67.92 KB
FAILED: [[SimpleTest]]: [MySQL] 60,039 pass(es), 6 fail(s), and 4 exception(s).
[ View ]
new4.18 KB

Let's see how this gets on.

Status:Needs review» Needs work

The last submitted patch, 31: 1998638-31.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new69.56 KB
FAILED: [[SimpleTest]]: [MySQL] 59,037 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

Let's start with a reroll.

Status:Needs review» Needs work

The last submitted patch, 33: 1998638-33.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review

33: 1998638-33.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 33: 1998638-33.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new69.69 KB
FAILED: [[SimpleTest]]: [MySQL] 59,037 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

Re-roll

Status:Needs review» Needs work

The last submitted patch, 37: 1998638-superglobals-37.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new69.67 KB
FAILED: [[SimpleTest]]: [MySQL] 59,046 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
new1.58 KB

This fixes the Form Storage tests, but not the AccessDenied test.

The last submitted patch, 39: 1998638-superglobals-39.patch, failed testing.

larowlan’s picture

StatusFileSize
new955 bytes
new69.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,075 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This fixes the exceptions not the access denied, dawehner has fix for them

dawehner’s picture

StatusFileSize
new1.65 KB

I was to optimistic, though here is a analyze, for the access denied one

  • On exceptions our ExceptionController creates a new subrequest which is 'get', so we never end up having the post data, unless we use $_POST directly
  • In the ExceptionController we still not have the proper request, because the ExceptionListener creates a new request for the exception.
    We might have to also provide an alternative implementation of the exception listener which also copies over the request type (post vs. get) and potentially passes along all the query attributes / post attributes.

    @see ExceptionListener::64/65
    @see ExceptionController::94

  • Additional your request service should be maybe better synchronized on the form builder, see attached interdiff

The last submitted patch, 41: globals-1998638.41.patch, failed testing.

dawehner’s picture

StatusFileSize
new654 bytes
new69.36 KB
PASSED: [[SimpleTest]]: [MySQL] 59,078 pass(es).
[ View ]

Let's revert that little change in form.inc and fix it properly in #2147153: Replace the last instance of $_GET/$_POST; Create a special exception listener / exception controller which allows to use POST requests as this involves way more places than you would expect.

larowlan’s picture

+1 rtbc

Crell’s picture

Status:Needs review» Reviewed & tested by the community

Well then mark it as such. :-)

Ashleigh Thevenet’s picture

Status:Reviewed & tested by the community» Fixed

Sorry, updated the wrong issue - reverting

Ashleigh Thevenet’s picture

Status:Fixed» Reviewed & tested by the community
damiankloip’s picture

This looks good to me too. Thanks all for finishing this off, haven't had too much time recently..

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Great work, folks! I feel cleaner reading Drupal 8 code already. :)

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title:Replace all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object» Change notice: Replace all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object
Priority:Normal» Major
Status:Fixed» Active
Issue tags:+Needs change record

Actually, since this marks the end of superglobals other than $_SESSION (which is coming later), we should issue a change notice for this (with before/after code), so people don't introduce them in future patches.

kim.pepper’s picture

Suggested change notice:

PHP Super-globals replaced with Symfony Request object

In Drupal 7, you would access GET, POST, SERVER and COOKIE variables using the PHP super-globals $_GET, $_POST, $_SERVER and $_COOKIE.

Drupal 7:

$query = $_GET['q']; // query string param
$myparam = $_POST['myparam']; // form param
$request_method = $_SERVER['REQUEST_METHOD']; // server variable
$mycookie = $_COOKIE['mycookie']; // cookie

In Drupal 8, you need to use the Symfony Request object.

Drupal 8:

$query = \Drupal::request()->query->get('q'); // query string param
$name = \Drupal::request()->request->get('name'); // form param
$request_method = \Drupal::request()->server->get('REQUEST_METHOD'); // server variable
$mycookie = \Drupal::request()->cookies->get('mycookie'); // cookie

More information on the Request object is available on the Symfony2 documentation pages. http://api.symfony.com/2.2/Symfony/Component/HttpFoundation/Request.html

jibran’s picture

It seems good to me. Please create a change notice for this and use php tag instead of code.

kim.pepper’s picture

Change notice posted https://drupal.org/node/2150267

jibran’s picture

Title:Change notice: Replace all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object» Replace all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object
Priority:Major» Normal
Status:Active» Fixed
Issue tags:-Needs change record

Thanks for the change notice.

alexpott’s picture

This has broken drush site-install and has not actually delivered on what the title promises "Replace all remaining superglobals...". For example, $_SERVER is still used in mail.inc, conf_path(), run-tests.sh

Either we should roll this back or we go forward with #2150279: Inconsistent use of request object in drupal_override_server_variables() and conf_path()

webchick’s picture

Status:Fixed» Needs work

I would be fine rolling this back, since it's only a "normal" task, except that it no longer cleanly rolls back. :(

system.module.rej:
***************
*** 2722,2729 ****
   *
   * If the submit handler for a form that implements confirm_form() is invoked,
   * the user successfully confirmed the action. You should never directly
-  * inspect $_POST or \Drupal::request()->request to see if an action was
-  * confirmed.
   *
   * Note - if the parameters $question, $description, $yes, or $no could contain
   * any user input (such as node titles or taxonomy terms), it is the
--- 2722,2728 ----
   *
   * If the submit handler for a form that implements confirm_form() is invoked,
   * the user successfully confirmed the action. You should never directly
+  * inspect $_POST to see if an action was confirmed.
   *
   * Note - if the parameters $question, $description, $yes, or $no could contain
   * any user input (such as node titles or taxonomy terms), it is the

So the race is on, to either provide a rollbackable patch here and fix the rest of the superglobals here, or to get #2150279: Inconsistent use of request object in drupal_override_server_variables() and conf_path() RTBC.

webchick’s picture

Issue tags:+Needs tests

Oh, nevermind. That conflict is because confirm_form() no longer exists, so is irrelevant. Very well!

Rolled back until this issue actually does what it says on the tin. :P

Also, this implies we need some kind of tests that could've caught this. Not sure exactly how that works.

Mile23’s picture

+1bn.

Eagerly awaiting #68387164: Refactor \Drupal Away.

damiankloip’s picture

Why are we reverting the whole patch? That seems mad to me. So now we need to worry about keeping the ~70k patch applied, rather than just a small follow up. Let's make more work for ourselves.

Also, when do we revert stuff because it breaks drush? If I checkout before the revert, I can install drupal (using drush si) fine.

Also, this implies we need some kind of tests that could've caught this. Not sure exactly how that works.

What is meant to be tested here?

webchick’s picture

I reverted the entire patch because normal tasks are not allowed to introduce critical bugs. If there's a subset of this patch I can commit that does not break drush si, let me know.

webchick’s picture

And as far as testing, I'm not sure. Hopefully alexpott can provide some guidance, as he understands much better than I what broke. :)

damiankloip’s picture

I'm not sure how drush si broke, hopefully alexpott can enlighten us on that. Usually it is drush that should catch up with core... So not sure where the critical bug is here?!

dawehner’s picture

Title:Replace all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object» Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object

We also did not blocked the module disable patch to be applied due to the same reason.

So I applied the patch, ran my script which uses drush si and it still worked (twice), so what about renaming the title
for now to be more honest?
All of the $_SERVER variables are probably hard to fix as it is code running really early in the installer.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new3.95 KB
new71.35 KB
FAILED: [[SimpleTest]]: [MySQL] 59,199 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Patch attached merges and improves fix from #2150279: Inconsistent use of request object in drupal_override_server_variables() and conf_path() and includes @Dave Reid's great idea on how to fix the warnings from mail.inc

drush site-install works for me with this patch.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

drush sistill works for me.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Awesome! Thanks for the fast turnaround on this, alexpott!

Re-committed and pushed to 8.x. Thanks!

damiankloip’s picture

Woop! Thanks all, that is some efficient resolution work.

Status:Fixed» Needs work

The last submitted patch, 65: 1998638.65.patch, failed testing.

alexpott’s picture

Status:Needs work» Fixed

Wonder where #69 came from? Drupal\system\Tests\Form\FormTest passes locally.

amateescu’s picture

It was a memory issue on the testbot:

Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 268435456 bytes) in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php on line 336
FATAL Drupal\system\Tests\Form\FormTest: test runner returned a non-zero error code (255).

Status:Fixed» Closed (fixed)

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