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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pdrake’s picture

Status: Active » Needs review
FileSize
8.81 KB

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

Hopefully, this patch contains a little less fail.

pdrake’s picture

Status: Needs work » Needs review
FileSize
9.04 KB

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

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

Anonymous’s picture

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

    Anonymous’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

    cosmicdreams’s picture

    Status: Needs work » Needs review
    cosmicdreams’s picture

    FileSize
    11.67 KB

    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.

    Anonymous’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

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

    Anonymous’s picture

    Status: Needs work » Needs review
    FileSize
    3.85 KB

    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

    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.

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

    Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

    marvil07’s picture

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

    The patch no longer applies, hence marking as NW.

    The list with the grep output in comment 41 have not changed much, so the hunks in the last patch should be still relevant.

    anish.a’s picture

    Version: 8.2.x-dev » 8.3.x-dev
    Issue tags: -Needs reroll
    FileSize
    1.67 KB

    Reroll of latest patch

    anish.a’s picture

    Status: Needs work » Needs review
    anish.a’s picture

    Version: 8.3.x-dev » 8.4.x-dev

    Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    dawehner’s picture

    Status: Needs review » Reviewed & tested by the community
    +++ b/core/includes/errors.inc
    @@ -184,7 +184,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
           $response->setContent(html_entity_decode(strip_tags(SafeMarkup::format('%type: @message in %function (line %line of %file).', $error))) . "\n");
           $response->send();
    -      exit;
    +      return;
         }
       }
     
    @@ -196,7 +196,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
    
    @@ -196,7 +196,7 @@ function _drupal_log_error($error, $fatal = FALSE) {
             $response->setContent(SafeMarkup::format('%type: @message in %function (line %line of %file).', $error));
             $response->send();
           }
    -      exit;
    +      return;
    

    I had a lock at a couple of error handlers and they indeed just return and don't exit: https://github.com/filp/whoops/blob/master/src/Whoops/Run.php#L334 https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Deb... https://github.com/TYPO3/TYPO3.CMS/blob/master/typo3/sysext/core/Classes...

    catch’s picture

    Status: Reviewed & tested by the community » Needs work
    +use Symfony\Component\HttpFoundation\Response;
    +
     /**
      * @file
      * Test module.
    @@ -11,8 +13,8 @@
     function session_test_user_login($account) {
       if ($account->getUsername() == 'session_test_user') {
         // Exit so we can verify that the session was regenerated
    -    // before hook_user_login() was called.
    -    exit;
    +    // before hook_user() was called.
    +    return new Response('OK', Response
    

    This no longer exits, it's returning early.

    Why the change from hook_user() to hook_user_login()? It's hook_user_login() we're in.

    Also nothing is done with the return value of hook_user_login(), so returning a Response object won't make it finish early. The idea of the exit() here (without going through git blame, just looking at this comment) is to prevent execution beyond the hook invocation to ensure the session is already created. I can't remember why it tests like this, but pretty sure this is introducing a false-negative into that test now.

    catch’s picture

    Maybe we could throw an exception instead of exiting?

    dawehner’s picture

    Maybe we could throw an exception instead of exiting?

    Wait, do you talk about our actual logger?

    catch’s picture

    No I'm talking about session_test_user_login().

    dawehner’s picture

    Oh yeah that would totally work for me.

    Version: 8.4.x-dev » 8.5.x-dev

    Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    Version: 8.5.x-dev » 8.6.x-dev

    Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    Version: 8.6.x-dev » 8.7.x-dev

    Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    Version: 8.7.x-dev » 8.8.x-dev

    Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    Version: 8.8.x-dev » 8.9.x-dev

    Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

    Version: 8.9.x-dev » 9.1.x-dev

    Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

    Version: 9.1.x-dev » 9.2.x-dev

    Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

    Version: 9.2.x-dev » 9.3.x-dev

    Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 9.3.x-dev » 9.4.x-dev

    Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 9.4.x-dev » 9.5.x-dev

    Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 9.5.x-dev » 10.1.x-dev

    Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

    Version: 10.1.x-dev » 11.x-dev

    Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.