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

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.

go testbot.

This patch converts all $_SERVER['REQUEST_METHOD']

Added getMethod() note

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

Added link to request api page

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

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

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?

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

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.

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.

Added subissues

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

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.

Removed files as they are now in the issues.

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

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

.

Tagging

Refreshing links

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.

Remove OpenID as it is no longer a core module.

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

Refocusing the issue to finish this off.

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?

Let's try this.

F%&^

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.

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!

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.

Fixing..

Let's see how this gets on.

Let's start with a reroll.

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

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

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

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.

+1 rtbc

Well then mark it as such. :-)

Sorry, updated the wrong issue - reverting

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

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

Committed and pushed to 8.x. Thanks!

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.

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

Thanks for the change notice.

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

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.

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.

+1bn.

Eagerly awaiting #68387164: Refactor \Drupal Away.

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?

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.

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

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?!

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.

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.

drush sistill works for me.

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

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

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

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

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