Problem/Motivation

Drupal core currently generates headers and/or prints content directly in various places. With the introduction of HttpKernel and Response objects, these exit; calls should be eliminated. This is necessary in order to return properly formatted error pages, ensure event subscribers are called and to facilitate code testing (it can be difficult to execute tests on code that calls exit). Additionally, we are very close to having the ability to run Drupal as a long-running process, and/or fork drupal processes to handle sub-requests and one of the few remaining steps is to eliminate exit; calls.

Proposed resolution

Replace exit calls with return. In instances where header/print was used to send content to the client, return an appropriately formed Response object. As the installer does not yet use the HttpKernel object, this proposed resolution does not cover/include the installer.

Remaining tasks

Reviews needed.

User interface changes

No user interface changes.

API changes

No API changes.

Files: 
CommentFileSizeAuthor
#7 drupal-eliminate_exit_calls-1497162-7.patch10.8 KBpdrake
FAILED: [[SimpleTest]]: [MySQL] 47,481 pass(es), 189 fail(s), and 181 exception(s).
[ View ]
#6 drupal-eliminate_exit_calls-1497162-6.patch9.04 KBpdrake
FAILED: [[SimpleTest]]: [MySQL] 47,405 pass(es), 148 fail(s), and 137 exception(s).
[ View ]
#5 drupal-eliminate_exit_calls-1497162-5.patch9 KBpdrake
FAILED: [[SimpleTest]]: [MySQL] 47,436 pass(es), 145 fail(s), and 127 exception(s).
[ View ]
#1 drupal-eliminate_exit_calls-1497162-1.patch8.81 KBpdrake
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/form_test/form_test.module.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new8.81 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/tests/modules/form_test/form_test.module.
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-eliminate_exit_calls-1497162-1.patch, failed testing.

Status:Needs work» Needs review

+++ b/core/includes/bootstrap.incundefined
@@ -618,7 +618,7 @@ function drupal_environment_initialize() {
     if (!drupal_valid_http_host($_SERVER['HTTP_HOST'])) {
       // HTTP_HOST is invalid, e.g. if containing slashes it may be an attack.
       header($_SERVER['SERVER_PROTOCOL'] . ' 400 Bad Request');
-      exit;
+      return new Response('Bad Request', 400);

Shouldn't the explicit header() call be removed too?

Status:Needs review» Needs work

StatusFileSize
new9 KB
FAILED: [[SimpleTest]]: [MySQL] 47,436 pass(es), 145 fail(s), and 127 exception(s).
[ View ]

Hopefully, this patch contains a little less fail.

Status:Needs work» Needs review
StatusFileSize
new9.04 KB
FAILED: [[SimpleTest]]: [MySQL] 47,405 pass(es), 148 fail(s), and 137 exception(s).
[ View ]

This patch changes how a submit handler in the form_test.module works. It feels like I'm doing it wrong, but it does pass tests on my local system. I would welcome feedback on this - the bit of code I'm uncertain about is:

<?php
/**
* Form submit handler for form_state_values_clean() test form.
*/
function form_test_form_state_values_clean_form_submit($form, &$form_state) {
 
form_state_values_clean($form_state);
 
$response = new JsonResponse($form_state['values']);
 
$response->send();
  return
FALSE;
}
?>

StatusFileSize
new10.8 KB
FAILED: [[SimpleTest]]: [MySQL] 47,481 pass(es), 189 fail(s), and 181 exception(s).
[ View ]

Discovered I failed to replace exit; calls in system_tests, so this patch adds those.

Status:Needs review» Needs work

The last submitted patch, drupal-eliminate_exit_calls-1497162-7.patch, failed testing.

Since a number of these functions have non-NULL return values now, the docblocks should be updated as appropriate as well.

send()ing a response yourself is wrong, because then it doesn't get properly prepare()d in index.php. That is necessary to normalize headers, caching, etc. based on the request.

I'm not sure what the correct way to deal with breaking out of Form API is, but it's not that. :-)

I think drupal_environment_initialize can just go away entirely, or if not then it should. Any parts of it we still need should either be refactored or pushed upstream to Symfony.

It looks like Replace drupal_goto() with RedirectResponse rethinks the way drupal_build_form responses work to some extent. With a little tweaking, this may allow form submission handlers to return a response object, but given that we can have multiple submit handlers for a single form, I'm not sure how we would reconcile multiple response objects.

Status:Needs work» Needs review
Issue tags:-symfony

Status:Needs review» Needs work
Issue tags:+symfony

The last submitted patch, drupal-eliminate_exit_calls-1497162-7.patch, failed testing.

If a given submit handler does a print/exit itself, then any submit handlers after that don't fire. To mimic that behavior, we'd say that if a submit handler returns a Response then that response is returned up the chain and subsequent submit handlers don't fire.

That sounds like a particularly stupid way of doing it, since that means whether or not your submit handler is fired is non-deterministic at code time, but it sounds like it would be consistent with the current behavior. Submit handlers ought to be setting the redirect property instead to play nice.

@Crell: Submit handlers do not print/exit? If they do, that's already considered a bug and incorrect API usage IMHO that could lead to problems.

They just set $form_state['redirect'], which can be altered by any following submit handler. why not just keep that and instead of it being a string/array, use a response object? then a following submit calback can extend or replace it just like it can replace the string right now?

Ah! I misunderstood. Ignore #14. :-)

Would it make more sense to allow submit handlers to specify a response object (which could then be a non-redirect response), or to let them continue to specify a path and we turn that into a response object at the end (where we would otherwise be calling drupal_goto())? And which do we want to do on the assumption that paths here will go away in favor of generated URLs sooner or later?

@Berdir: I would agree that it is incorrect API usage, but at least one core submit handler does a print and exit today (form_test_form_state_values_clean_form_submit) . This is the place where I (temporarily) created a response object and called send() to mimic the existing behavior.

@Crell: I do like the idea of allowing forms to manipulate the response object directly as this seems to make sense, but they could do so by registering a listener, could they not? Perhaps this single use-case could be resolved in that manner.

That would require there being an event for them to listen to, unless you mean letting a module register a response listener. The challenge there is that any response listener fires for every response, which would run into a performance issue if every form registers a listener that only occasionally does anything.

At the moment, only one module in core is directly modifying the response and in this one case, it may be more appropriate to store the object in question and redirect to a display page (as is the intended flow for forms) assuming that simpletest can gracefully handle that redirect. Can you recommend a persistent storage mechanism appropriate for storing the form value object in this test module? It could be stored in the session or state controller, perhaps.

That said, it is probably still a legitimate question as to whether form submit handlers should have a mechanism by which to affect the Response object. Using an event subscriber, it would mean that the subscriber would fire on every response (assuming response is the event it subscribes to) and this could become a performance problem if it becomes prevalent. It would be possible to extend the API, adding hook_form_FORM_ID_response_alter or something similar, but manipulating the response object seems like a more generic need subsequent to the introduction of the Response object, so establishing the pattern in a more generic manner is probably wise.

Issue summary:View changes

Changed issue details to provide all relative issue summary sections.

Title:eliminate the use of exit in coreEliminate the use of exit in core
Category:Feature request» Bug report
Issue summary:View changes
Priority:Normal» Critical