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.
Comment | File | Size | Author |
---|---|---|---|
#51 | 1497162-51.patch | 1.67 KB | anish.a |
Comments
Comment #1
pdrake CreditAttribution: pdrake commentedComment #3
BerdirShouldn't the explicit header() call be removed too?
Comment #4
BerdirComment #5
pdrake CreditAttribution: pdrake commentedHopefully, this patch contains a little less fail.
Comment #6
pdrake CreditAttribution: pdrake commentedThis 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:
Comment #7
pdrake CreditAttribution: pdrake commentedDiscovered I failed to replace exit; calls in system_tests, so this patch adds those.
Comment #9
Lars Toomre CreditAttribution: Lars Toomre commentedSince a number of these functions have non-NULL return values now, the docblocks should be updated as appropriate as well.
Comment #10
Crell CreditAttribution: Crell commentedsend()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.
Comment #11
pdrake CreditAttribution: pdrake commentedIt 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.
Comment #12
pdrake CreditAttribution: pdrake commented#7: drupal-eliminate_exit_calls-1497162-7.patch queued for re-testing.
Comment #14
Crell CreditAttribution: Crell commentedIf 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.
Comment #15
Berdir@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?
Comment #16
Crell CreditAttribution: Crell commentedAh! 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?
Comment #17
pdrake CreditAttribution: pdrake commented@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.
Comment #18
Crell CreditAttribution: Crell commentedThat 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.
Comment #19
pdrake CreditAttribution: pdrake commentedAt 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.
Comment #19.0
pdrake CreditAttribution: pdrake commentedChanged issue details to provide all relative issue summary sections.
Comment #20
ParisLiakos CreditAttribution: ParisLiakos commentedJust closed #2230121: Remove exit() from FormBuilder as duplicate, so copying status/priority etc
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedSince it's been a while, I thought I'd start with a reroll from #7 to see what has changed since then.
edit: formatting
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #24
cosmicdreams CreditAttribution: cosmicdreams commentedmy attempt to reroll
Comment #25
cosmicdreams CreditAttribution: cosmicdreams commentedComment #26
cosmicdreams CreditAttribution: cosmicdreams commenteduploaded the wrong file.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commented@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?
Comment #30
cosmicdreams CreditAttribution: cosmicdreams commented@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.
Comment #31
martin107 CreditAttribution: martin107 commentedRe 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...
Comment #33
martin107 CreditAttribution: martin107 commentedAdding "use Symfony\Component\HttpFoundation\Response"
Fixes one test Drupal\system\Tests\Session\SessionTest
plus exchanging magic number for the appropriate CONST.
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedi 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
So in the end it doesnt look that bad.
Comment #36
andypostplease fix
Comment #37
CrazySquirrel CreditAttribution: CrazySquirrel commentedComment #38
mgiffordThere 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.
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedLet's start with a reroll and see what testbot thinks of this now.
Comment #40
mgiffordThe 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;
Comment #41
jhedstromThis 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 thecore.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:Comment #42
jhedstromThe patch above also removed the seemingly-unrelated
url_alter_test.module
file, as noted in #35.Comment #43
Crell CreditAttribution: Crell as a volunteer commentedI 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.
Comment #44
dawehnerWhat 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?
Comment #45
Crell CreditAttribution: Crell as a volunteer commentedAn 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.
Comment #46
dawehnerAh yeah that is a good point. I'm a bit curious how we should ideally handle the CLI case
Comment #47
joelpittetFrom what I can tell from the issue summary this is a task.
Comment #50
marvil07 CreditAttribution: marvil07 as a volunteer commentedThe 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.
Comment #51
anish.a CreditAttribution: anish.a at Axelerant commentedReroll of latest patch
Comment #52
anish.a CreditAttribution: anish.a at Axelerant commentedComment #53
anish.a CreditAttribution: anish.a at Axelerant commentedComment #55
dawehnerI 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...
Comment #56
catchThis 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.
Comment #57
catchMaybe we could throw an exception instead of exiting?
Comment #58
dawehnerWait, do you talk about our actual logger?
Comment #59
catchNo I'm talking about session_test_user_login().
Comment #60
dawehnerOh yeah that would totally work for me.