Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- install_drupal() contains this:
try { install_begin_request($install_state); $output = install_run_tasks($install_state); } catch (Exception $e) { // When an installation error occurs, either send the error to the web // browser or pass on the exception so the calling script can use it. if ($install_state['interactive']) { install_display_output($e->getMessage(), $install_state); } else { throw $e; } }
When an exception is thrown in the interactive installer, only the exception message is printed to the screen.
Any additional info (file, line, backtrace) is missing. Not helpful.
Goal
- Use the regular error/exception handler in the installer:
Comment | File | Size | Author |
---|---|---|---|
#20 | drupal8.install-error-handler.20.patch | 7.78 KB | sun |
#19 | drupal8.install-error-handler.19.patch | 7.75 KB | sun |
#14 | interdiff.txt | 4.4 KB | sun |
#14 | drupal8.install-error-handler.14.patch | 7.81 KB | sun |
#11 | drupal8.install-error-handler.11.patch | 6.41 KB | sun |
Comments
Comment #1
webchickSince this prevents people from solving their own problems (or at the very least from posting a bug report anyone else could have a hope of solving), escalating to major.
Comment #2
valthebaldCould $e->getTraceAsString() be a better solution?
Comment #3
sunIdeally, the (interactive) installer should just simply use the regular error/exception handlers (which are registered already), so errors are printed to the screen as usual. I don't really see why that is not the case.
Studying the situation and call chains, that is actually the case already. However, our (procedural/oldskool) exception handler is completely b0rked right now.
The necessary fix only underlines further that a lot of critical base system → HttpKernel refactoring is still required for D8, because our current code couldn't be any more fragile. In light of what I'm seeing, it's very confusing that "beta" is even a word at this point.
Attached patch "fixes" the procedural exception handler in the installer... but it can't really get any more dirty.
I found two closely related issues, which might be obsolete with this patch:
#2153401: Uncaught InvalidArgumentException sometimes thrown by _drupal_error_handler during install
#2091501: When install fails: Fatal error: Call to undefined function theme() in install.core.inc on line 954
Comment #4
fietserwinLast 2 lines of install_display_output() (not within any if or other condition):
So I'm afraid that this is not going to solve the related issue about the undefined theme() function, or that it will help at all. Also the exit prevents setting the status code to 500 and using the response object at all.
if ($fatal) within an if ($fatal)? OK, it is not code that is touched by this patch, but still. So shouldn't we just remove the inner if?
Comment #5
sunWell spotted. Indeed, all of that makes no sense. :)
Attached patch removes the senselessly duplicated $fatal condition and clarifies that install_display_output() exits.
Comment #7
fietserwinSo, we accept that:
- when the installer is running we do not return status code 500, but try to display the error within the installer "wizard".
- #2091501: When install fails: Fatal error: Call to undefined function theme() in install.core.inc on line 954 will not be solved by this patch. A patch for that issue could just check if the theme() function exists before calling it and exiting. If it does not exist, just return and let _drupal_log_error() finish the work.
Personally, I'm fine with that. So RTBC for me.
Note: all exceptions that are thrown between the start of install_begin_request() and the call to drupal_maintenance_theme(); will not be themable. And there is a lot that can go wrong. (But this remark is more for the other issue).
Comment #8
sunComment #9
alexpottI've manually tested this by throwing an exception in SystemListingInfo::profile() on the first call to it. The patch definitely improves the situation.
Without the patch
With the patch
The new message now shows exactly where I threw the exception. In HEAD it's totally obscured.
Comment #10
catchCommitted/pushed to 8.x, thanks!
Comment #11
sunThanks!
Here's a small follow-up patch to fix some additional error handling problems in the installer, which I discovered while debugging #2096741: Installation failure: Missing configuration files and some other installer related issues.
These additional changes
It is very obvious that our error/exception handling is a total mess right now. This patch does not make it prettier. However, I'd highly recommend to commit these adjustments as-is for now, because:
In short:
Comment #12
fietserwinWhat makes you think that this will never throw an exception. In other words: why not leave the try - catch intact but change the code to return _VERBOSE in the catch?
Question: if ...get('error_level') returns, will it always return a proper value or should we check that as well? (During error handling I become even more paranoid than I already am in normal situations).
I had a hard time understanding this comment. only after looking at the diff I got it a bit. The problem with the comment is that it talks about installer, but subsequently the condition is when not in the installer. So change to something like: The installer initializes a maintenance theme at the earliest possible point in time. Do not unset that.
(Or describe why we would want to unset it when we are not in the installer)
How sure can we be that this will not fail fatally?(undefined function ... and we have no useful error information at all anymore)
What happens if it fails with an error/exception?
I can agree that the error handling as is, is complex, is spaghetti code, and has to many special cases, So my review was not about understanding the code but understanding (the implications of) the changes and if they could make sense. Therefore my remarks are more posed as questions than as things that must be changed.
What would be typical places in the code to insert throwing an exception to see the changes before and after?
Comment #13
alexpottWith the latest patch
Review
Another case for \Drupal::has() :) - but actually I don't understand why we don't just change the return in the catch.
Why is this change necessary?
So this change ensures a verbose message if an secondary exception is thrown in _drupal_exception_handler or if an exception is thrown in _drupal_shutdown_function(). Seems like a good idea.
Comment #14
sunThanks for the reviews! Addressed all concerns in code + code comments.
Comment #16
sun14: drupal8.install-error-handler.14.patch queued for re-testing.
Comment #17
sunComment #18
fietserwinI now understand that this will never fail with an exception. So I tend to be fine with removing that try-catch. But OTOH, I still think that it would be cleaner and more readable to do use a try-catch. Many people after me will wonder (and loose time on) if this can fail?
I also had a situation in which it returned NULL. (Upgrading to HEAD more often seems to render my local site unusable than not. Is there something I can do to prevent that?). Should that situation be handled? and if so, how? Should returning NULL mean that we're in serious trouble thus return ERROR_REPORTING_DISPLAY_ALL?
All other parts of the patch are OK.
Comment #19
sunAttached patch addresses both of your concerns. Let's move forward? :-)
Comment #20
sunOopsie, forgot to initialize the new $error_level variable. Will cancel the test for the previous patch in a moment.
Comment #22
fietserwinThanks, I'm fine with the patch now.
Comment #23
alexpottCommitted 643dab0 and pushed to 8.x. Thanks!