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
#33 interdiff-31-33.txt763 bytesmartin107
#33 eliminate-exit-calls-1497162-33.patch7.25 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,825 pass(es), 101 fail(s), and 93 exception(s).
[ View ]
#31 eliminate-exit-calls-1497162-31.patch7.14 KBmartin107
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,779 pass(es), 104 fail(s), and 97 exception(s).
[ View ]
#26 1497162_24.patch11.67 KBcosmicdreams
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 47,754 pass(es), 12,279 fail(s), and 994 exception(s).
[ View ]
#21 eliminate-exit-calls-1497162-21.patch8.03 KBpjonckiere
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,111 pass(es), 149 fail(s), and 171 exception(s).
[ View ]

Comments

pdrake’s picture

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.

Berdir’s picture

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?

Berdir’s picture

Status:Needs review» Needs work
pdrake’s picture

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.

pdrake’s picture

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;
}
?>
pdrake’s picture

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.

Lars Toomre’s picture

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

Crell’s picture

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.

pdrake’s picture

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.

pdrake’s picture

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.

Crell’s picture

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.

Berdir’s picture

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

Crell’s picture

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?

pdrake’s picture

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

Crell’s picture

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.

pdrake’s picture

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.

pdrake’s picture

Issue summary:View changes

Changed issue details to provide all relative issue summary sections.

ParisLiakos’s picture

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

Just closed #2230121: Remove exit() from FormBuilder as duplicate, so copying status/priority etc

pjonckiere’s picture

StatusFileSize
new8.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,111 pass(es), 149 fail(s), and 171 exception(s).
[ View ]

Since it's been a while, I thought I'd start with a reroll from #7 to see what has changed since then.

  • update.php: supported php version number was changed
  • ajax.inc:
  • bootstrap.inc
    • removal of get_magic_quotes_gpc check: i removed this since it was not on the current dev branch, but I cannot find where/why it was removed.
    • <?php
             
      if (get_magic_quotes_gpc() || get_magic_quotes_runtime()) {
                  return new
      Response("PHP's 'magic_quotes_gpc' and 'magic_quotes_runtime' settings are not supported and must be disabled.", 500);
              }
             
      ?>
    • kept the response handling and just replaced exit with return
    • cf http.php change
  • http.php: that logic moved to bootstrap.inc::drupal_handle_request(), i can't find the issue page though
  • https.php: cf http.php (this was copy-paste code)
  • form_test.module:
    • removal of form_test_menu() since those are moved to menu_links.yml files (cf https://drupal.org/node/2228089)
    • adjusted _form_test_submit_values_json() by changing exit to return
    • adjusted form_test_form_state_values_clean_form_submit() to use the JsonResponse object
  • session_test.module: in session_test_user_login(): used the change from the patch
  • system_test.module
  • taxonomy.pages.inc: removal of taxonomy_autocomplete() (cf https://drupal.org/node/1987858)

edit: formatting

pjonckiere’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 21: eliminate-exit-calls-1497162-21.patch, failed testing.

cosmicdreams’s picture

StatusFileSize
new11.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 47,802 pass(es), 12,235 fail(s), and 993 exception(s).
[ View ]

my attempt to reroll

cosmicdreams’s picture

Status:Needs work» Needs review
cosmicdreams’s picture

StatusFileSize
new11.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 47,754 pass(es), 12,279 fail(s), and 994 exception(s).
[ View ]

uploaded the wrong file.

Status:Needs review» Needs work

The last submitted patch, 26: 1497162_24.patch, failed testing.

The last submitted patch, 24: 1497162_24.patch, failed testing.

pjonckiere’s picture

@cosmicdreams: the patches from #24 and #26 seem identical. Could it be that the patch from #26 is the same wrong file as in #24?

cosmicdreams’s picture

@pjonckiere: No, I just tried to approach the removal of exit() a different way. I'm not trying to reroll actually, I'm trying to approach the issue anew. My has many more issues so ignore me for now. I'll learn what I'm doing wrong.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new7.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,779 pass(es), 104 fail(s), and 97 exception(s).
[ View ]

Re Rolling the active patch ( #21 )

No conflicts to resolve just merging and a few 3 ways!

Note #21 had 149 fail(s), and 171 exception(s).

Lots has changed in a month, lets see what testbot has to say...

Status:Needs review» Needs work

The last submitted patch, 31: eliminate-exit-calls-1497162-31.patch, failed testing.

martin107’s picture

Status:Needs work» Needs review
StatusFileSize
new7.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,825 pass(es), 101 fail(s), and 93 exception(s).
[ View ]
new763 bytes

Adding "use Symfony\Component\HttpFoundation\Response"
Fixes one test Drupal\system\Tests\Session\SessionTest

plus exchanging magic number for the appropriate CONST.

Status:Needs review» Needs work

The last submitted patch, 33: eliminate-exit-calls-1497162-33.patch, failed testing.

ParisLiakos’s picture

Priority:Critical» Major
+++ b/core/modules/system/tests/modules/session_test/session_test.module
@@ -1,13 +1,15 @@
diff --git a/core/modules/system/tests/modules/url_alter_test/url_alter_test.module b/core/modules/system/tests/modules/url_alter_test/url_alter_test.module

diff --git a/core/modules/system/tests/modules/url_alter_test/url_alter_test.module b/core/modules/system/tests/modules/url_alter_test/url_alter_test.module
new file mode 100644

new file mode 100644
index 0000000..4637454

index 0000000..4637454
--- /dev/null

--- /dev/null
+++ b/core/modules/system/tests/modules/url_alter_test/url_alter_test.module

+++ b/core/modules/system/tests/modules/url_alter_test/url_alter_test.module
+++ b/core/modules/system/tests/modules/url_alter_test/url_alter_test.module
@@ -0,0 +1,70 @@

i dont see what this file has to do with the patch..merge conflict?

Also, after some thought, i do not think this is really critical.
The exit calls happen

  • when we dont really have a kernel yet
  • if there is a fatal error
  • in various tests
  • the FormBuilder, which terminates the kernel properly before exiting
  • when handling page caching, which needs refactoring before this can happen, with already open issues

So in the end it doesnt look that bad.

andypost’s picture

+++ b/core/modules/system/tests/http.php
@@ -20,4 +20,4 @@
\ No newline at end of file

+++ b/core/modules/system/tests/https.php
@@ -22,4 +22,4 @@
\ No newline at end of file

please fix

CrazySquirrel’s picture

Assigned:Unassigned» CrazySquirrel