Remove drupal_exit()

Hm. There can be cases, in which removing a [drupal_]exit() could lead to security issues, because any following code in calling or even distant calling functions is not expected to be run.

We have to be sure that this is not the case for each of the removals/replacements happening here.

I think removing the early drupal_exit() may some minor cause performance regression, but I'm not sure about security. But we have to get rid of them, because handling two different paths for closing request messes up badly with some write delayed incremental caches that could be wrote but are ignored because of that, and may cause other unexpected behavior.

The easiest way to deal with phpinfo(), I think, is to wrap it in an output buffer, shove the results into a new Response object, and return that. We can maybe twiddle with the settings on it for caching and stuff, but that doesn't have to happen right now.

Re-roll including suggestion from #8.

Cross-linking #1860026: Remove hook_exit() for cached page requests.

Shouldn't ship with two ways to do something, so this feels at least major.

+++ b/core/includes/common.incundefined
@@ -697,12 +698,8 @@ function drupal_goto($path = '', array $options = array(), $http_response_code =
   $url = url($path, $options);
 
-  header('Location: ' . $url, TRUE, $http_response_code);
-
-  // The "Location" header sends a redirect status code to the HTTP daemon. In
-  // some cases this can be wrong, so we make sure none of the code below the
-  // drupal_goto() call gets executed upon redirection.
-  drupal_exit($url);
+  // The "Location" header sends a redirect status code to the HTTP daemon.
+  return new RedirectResponse($url);

That will never work ;)

We need to fix #1668866: Replace drupal_goto() with RedirectResponse first.

git pull

i would love some help with openid tests, its the only that fail

This issue and http://drupal.org/node/1497162 appear to be duplicates. One should probably be closed and efforts combined. While http://drupal.org/node/1497162 is an earlier issue, this one has more recent work.

there is no way we get rid of exit in current core's state. maybe after all forms convert to the FormInterface and the callbacks that call drupal_goto to routes
there is already a critical issue for that #1971384: [META] Convert page callbacks to controllers, but we should move forward here if we want to get rid of drupal_goto

relevant #556380-70: Remove OpenID from core

Can we reroll this issue to ignore OpenID (and leave the method in place for now but marked for deletion), and see what happens?

i am retitling this issue..removing exit calls can happen after code freeze, but not removing drupal_exit..and i believe this patch is all that it takes now that openid is history

we can remove exit on #1497162: Eliminate the use of exit in core

+++ b/core/includes/install.inc
@@ -861,10 +862,11 @@ function drupal_install_fix_file($file, $mask, $message = TRUE) {
+  RedirectResponse::create($base_url . '/' . $path, 302, $headers)->send();

By convention, we've always been using new RedirectResponse() rather than ::create(). That's not a requirement but just the convention we've been using. If we need to reroll, it's probably good to switch to that.

+++ b/core/modules/system/system.admin.inc
@@ -1311,8 +1311,9 @@ function system_status($check = FALSE) {
 function system_php() {
+  ob_start();
   phpinfo();
-  drupal_exit();
+  return new Response(ob_get_clean());

Urk. This will conflict with #1987824: Convert system_php() to a new style controller, which got to RTBC first, so we should probably wait for that to be committed and then adjust this patch accordingly.

+++ b/core/modules/xmlrpc/xmlrpc.server.inc
@@ -131,10 +133,13 @@ function xmlrpc_server_error($error, $message = FALSE) {
 function xmlrpc_server_output($xml) {
   $xml = '<?xml version="1.0"?>' . "\n" . $xml;
-  drupal_add_http_header('Content-Length', strlen($xml));
-  drupal_add_http_header('Content-Type', 'text/xml');
-  echo $xml;
-  drupal_exit();
+  $headers = array(
+    'Content-Length' => strlen($xml),
+    'Content-Type' => 'text/xml'
+  );
+  $response = new Response($xml, 200, $headers);
+  // @todo remove once converted to new routing system.
+  $response->send();

I'm not sure it's necessary to send() here. If you return a response object from a page callback, it should still work, shouldn't it?

I'm not sure it's necessary to send() here. If you return a response object from a page callback, it should still work, shouldn't it?

Yes it should work, but if i am dont send it, xmlrpc tests fail a lot..
we plan to fix xmlrpc module here #1979040: Rewrite XML RPC module to services and plugins

Weird, but OK. So I think we just need to reroll for the conflict and tweak the new/create issue, and then we're done here.

+++ b/core/includes/common.incundefined
@@ -727,12 +728,17 @@ function drupal_goto($path = '', array $options = array(), $http_response_code =
 
-  header('Location: ' . $url, TRUE, $http_response_code);
+  if (drupal_get_bootstrap_phase() == DRUPAL_BOOTSTRAP_FULL) {
+    drupal_session_commit();
+  }
+
+  // @todo We should not send the response here: http://drupal.org/node/1668866
+  RedirectResponse::create($url, $http_response_code)->sendHeaders();
 
   // The "Location" header sends a redirect status code to the HTTP daemon. In
   // some cases this can be wrong, so we make sure none of the code below the
   // drupal_goto() call gets executed upon redirection.
-  drupal_exit();

I don't think removing drupal_exit() then copying its contents to other functions is a good idea. Either it's ready to remove or it's not - we can mark it deprecated, then the two worst cases are 1. it's a wrapper for exit 2. it still has that session logic in it. I'm not sure what's going to happen to deprecated functions after code freeze, I'd lean towards removing them up until RC - just having the function there isn't such a massive blight compared to other things that are going to be left in 8.x.

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -570,8 +570,10 @@ function overlay_preprocess_page(&$variables) {
 function overlay_deliver_empty_page() {
   $empty_page = '<html><head><title></title>' . drupal_get_css() . drupal_get_js() . '</head><body class="overlay"></body></html>';
-  print $empty_page;
-  drupal_exit();
+  $response = new Response($empty_page);
+  // @todo sending the response is a bad practice. Fix this.
+  $response->send();
+  exit;

This doesn't have the session handling - does it need it? Again we could just leave drupal_exit() for now and not have to worry about this until it can be nuked properly.

about drupal_goto: #1668866: Replace drupal_goto() with RedirectResponse will make it not needing drupal_exit..i was hoping to remove drupal_exit here and then move on there, since the patch there is quite big.

about overlay: i suppose i should fix it here then

ok, since #1987824: Convert system_php() to a new style controller committed, this patch fixes overlay and xmlrpc
It also proves that drupal_exit is only needed by drupal_goto at the moment. the other two form callbacks, one from test module and other from locale need to be converted to _form routes, so the @todo will be removed there

missed some return statements on XMLRPC

I'm satisfied with the plan here. Let's do.

Looks much better now. Committed/pushed to 8.x, thanks!

change notice https://drupal.org/node/2017339

Looks good to me.

This surfaced an error in the installer. See #1929812-14: Fatal error on web-based install.