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
#41 1497162-41.patch1.67 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,130 pass(es). View
#39 eliminate_the_use_of-1497162-39.patch3.85 KBpjonckiere
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,790 pass(es). View
#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
FileSize
8.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

FileSize
9 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
FileSize
9.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:

/**
 * 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

FileSize
10.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

FileSize
8.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.
    		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

    FileSize
    11.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

    FileSize
    11.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
    FileSize
    7.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
    FileSize
    7.25 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,825 pass(es), 101 fail(s), and 93 exception(s). View
    763 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
    mgifford’s picture

    Assigned: CrazySquirrel » Unassigned

    There has been no new work on this issue in quite some time. So I'm assuming the person assigned is no longer being actively pursuing it. Sincere apologies if this is wrong.

    pjonckiere’s picture

    Status: Needs work » Needs review
    FileSize
    3.85 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,790 pass(es). View

    Let's start with a reroll and see what testbot thinks of this now.

    mgifford’s picture

    The bots like it. Just wondering if we've missed some now.

    I went looking after the patch with grep -ir 'exit;' * and found these:

    core/includes/errors.inc: exit;
    core/includes/install.core.inc: exit;
    core/includes/bootstrap.inc: exit;
    core/authorize.php: exit;
    core/modules/system/tests/modules/form_test/src/Form/FormTestVerticalTabsForm.php: exit;
    core/modules/system/tests/modules/form_test/src/Form/FormTestFormStateValuesCleanAdvancedForm.php: exit;
    core/modules/system/tests/modules/form_test/src/Form/FormTestFormStateValuesCleanForm.php: exit;
    core/rebuild.php: exit;
    core/lib/Drupal/Core/Test/TestKernel.php: exit;
    core/install.php: exit;

    jhedstrom’s picture

    FileSize
    1.67 KB
    PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,130 pass(es). View

    This is a reroll of #39.

    As for the remaining exit calls mentioned in #40, they either appear to be for security purposes (eg, not to leak information to attackers on production sites, as in the case of the core.install.inc), or in utility scripts to ensure no further code is executed.

    This is the current list of remaining exit calls once this patch is applied:

    authorize.php:  exit;
    authorize.php:      exit;
    includes/bootstrap.inc:      exit;
    includes/errors.inc:        exit;
    includes/errors.inc:      exit;
    includes/install.core.inc:    exit;
    includes/install.core.inc:        exit;
    includes/install.core.inc:  exit;
    install.php:  exit;
    lib/Drupal/Core/Test/TestKernel.php:      exit;
    modules/system/tests/modules/form_test/src/Form/FormTestFormStateValuesCleanAdvancedForm.php:    exit;
    modules/system/tests/modules/form_test/src/Form/FormTestFormStateValuesCleanForm.php:    exit;
    modules/system/tests/modules/form_test/src/Form/FormTestVerticalTabsForm.php:    exit;
    rebuild.php:  exit;
    scripts/drupal.sh:  exit;
    scripts/password-hash.sh:  exit;
    scripts/password-hash.sh:  exit;
    scripts/run-tests.sh:  exit;
    scripts/run-tests.sh:  exit;
    scripts/run-tests.sh:  exit;
    scripts/run-tests.sh:  exit;
    scripts/run-tests.sh:exit;
    scripts/run-tests.sh:        exit;
    scripts/run-tests.sh:    exit;
    scripts/run-tests.sh:        exit;
    scripts/run-tests.sh:          exit;
    scripts/run-tests.sh:    exit;
    scripts/update-countries.sh:  exit;
    
    jhedstrom’s picture

    The patch above also removed the seemingly-unrelated url_alter_test.module file, as noted in #35.

    Crell’s picture

    +++ b/core/includes/errors.inc
    +++ b/core/includes/errors.inc
    @@ -175,7 +175,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
    
    @@ -175,7 +175,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
           // Should not translate the string to avoid errors producing more errors.
           $response->setContent(html_entity_decode(strip_tags(SafeMarkup::format('%type: @message in %function (line %line of %file).', $error))). "\n");
           $response->send();
    -      exit;
    +      return;
         }
    

    I think this would result in potentially 2 responses getting sent, one here, one "wherever else". That seems unwise.

    The root problem here is that our trigger_error handling is still godawful and needs to be rewritten. I don't know if swapping exit for return here is going to make things better or worse until that gets done.

    dawehner’s picture

    The root problem here is that our trigger_error handling is still godawful and needs to be rewritten. I don't know if swapping exit for return here is going to make things better or worse until that gets done.

    What do you want to do. I mean when you have a fatal, you cannot continue, as you are not in a repariable state. Returning anything doens't help at all, right?

    Crell’s picture

    An E_FATAL isn't our problem anyway.

    An E_RECOVERABLE_ERROR, in PHP 7, becomes an exception. We should be doing the same for compatibility, and then handle it in the normal exception pipeline.

    Anything else is non-fatal, so we should be able to continue. Record a message/log/whatever and continue, but make sure the error message is recorded/displayed.

    dawehner’s picture

    An E_FATAL isn't our problem anyway.

    An E_RECOVERABLE_ERROR, in PHP 7, becomes an exception. We should be doing the same for compatibility, and then handle it in the normal exception pipeline.

    Anything else is non-fatal, so we should be able to continue. Record a message/log/whatever and continue, but make sure the error message is recorded/displayed.

    Ah yeah that is a good point. I'm a bit curious how we should ideally handle the CLI case

    joelpittet’s picture

    Category: Bug report » Task

    From what I can tell from the issue summary this is a task.

    Version: 8.0.x-dev » 8.1.x-dev

    Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

    Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.