I believe terminating mid-request doesn't really match the new kernel module with onTerminate subscribers and everything. drupal_exit() and even worse plain exit calls should be replaced.

Files: 
CommentFileSizeAuthor
#32 drupal-exit-16231140-32.patch13.04 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,519 pass(es).
[ View ]
#30 drupal-exit-16231140-30.patch10.92 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 56,834 pass(es), 16 fail(s), and 3 exception(s).
[ View ]
#24 drupal-exit-1623114-18.patch8.85 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,265 pass(es).
[ View ]
#18 drupal-exit-die-1623114-18.patch11.27 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,523 pass(es), 68 fail(s), and 17 exception(s).
[ View ]
#18 interdiff.txt4.23 KBParisLiakos
#16 drupal-exit-die-1623114-16.patch10.99 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 55,200 pass(es), 313 fail(s), and 115 exception(s).
[ View ]
#14 drupal-exit-die-1623114-14.patch10.96 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-exit-die-1623114-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 1623114-9.patch8.33 KBmarvil07
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#6 core_remove_drupal_exit-1623114-6.patch8.17 KBchrisdolby
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#1 core_remove_drupal_exit-1623114-1.patch7.53 KBchrisdolby
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core_remove_drupal_exit-1623114-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

chrisdolby’s picture

Status:Active» Needs work
StatusFileSize
new7.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core_remove_drupal_exit-1623114-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Part way there, definitely needs more work. Here's a patch with most of the drupal_exit() calls removed, apart from the ones in ajax_get_form() and _form_test_submit_values_json(), which still need to be removed along with the function itself. Not touched the other exit calls as of yet.

I've tried to convert everything so far to a Response or RedirectResponse, apart from system_php(), where I've just returned phpinfo().

Niklas Fiekas’s picture

Status:Needs work» Needs review

Thanks @chrisdolby! (Just sending it to the testbot for testing - although not yet complete.)

Status:Needs review» Needs work

The last submitted patch, core_remove_drupal_exit-1623114-1.patch, failed testing.

sun’s picture

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.

pounard’s picture

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.

chrisdolby’s picture

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

This is mostly a re-post of what I'd posted before, which I'm pushing to the testbot.

The phpinfo() call needs an appropriate response closer (the page output still continues), still working on that, and the ajax_get_form() call still needs changing, as there are two potential security holes here.

I've put in a JsonResponse for form_test.module, in the hope it's going to do the job now.

Status:Needs review» Needs work

The last submitted patch, core_remove_drupal_exit-1623114-6.patch, failed testing.

Crell’s picture

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.

marvil07’s picture

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

Re-roll including suggestion from #8.

Status:Needs review» Needs work

The last submitted patch, 1623114-9.patch, failed testing.

catch’s picture

catch’s picture

Priority:Normal» Major

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

Berdir’s picture

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

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new10.96 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-exit-die-1623114-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-exit-die-1623114-14.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new10.99 KB
FAILED: [[SimpleTest]]: [MySQL] 55,200 pass(es), 313 fail(s), and 115 exception(s).
[ View ]

git pull

Status:Needs review» Needs work

The last submitted patch, drupal-exit-die-1623114-16.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new4.23 KB
new11.27 KB
FAILED: [[SimpleTest]]: [MySQL] 55,523 pass(es), 68 fail(s), and 17 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal-exit-die-1623114-18.patch, failed testing.

pdrake’s picture

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.

ParisLiakos’s picture

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

ParisLiakos’s picture

Crell’s picture

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

ParisLiakos’s picture

Title:Remove drupal_exit() and other exit calls» Remove drupal_exit()
Status:Needs work» Needs review
StatusFileSize
new8.85 KB
PASSED: [[SimpleTest]]: [MySQL] 55,265 pass(es).
[ View ]

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

Crell’s picture

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

ParisLiakos’s picture

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

Crell’s picture

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.

catch’s picture

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

ParisLiakos’s picture

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

ParisLiakos’s picture

StatusFileSize
new10.92 KB
FAILED: [[SimpleTest]]: [MySQL] 56,834 pass(es), 16 fail(s), and 3 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, drupal-exit-16231140-30.patch, failed testing.

ParisLiakos’s picture

Status:Needs work» Needs review
StatusFileSize
new13.04 KB
PASSED: [[SimpleTest]]: [MySQL] 55,519 pass(es).
[ View ]

missed some return statements on XMLRPC

Crell’s picture

Status:Needs review» Reviewed & tested by the community

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

catch’s picture

Title:Remove drupal_exit()» Change notice: Remove drupal_exit()
Priority:Major» Critical
Status:Reviewed & tested by the community» Active

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

ParisLiakos’s picture

Status:Active» Needs review
Crell’s picture

Title:Change notice: Remove drupal_exit()» Remove drupal_exit()
Priority:Critical» Major
Status:Needs review» Fixed

Looks good to me.

effulgentsia’s picture

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

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