error_displayable() currently calls variable_get(), which can't throw an exception - it has no dependencies at all.

If we convert that to CMI, then it could do all kinds of things, which isn't possible in at least two of these functions:

http://api.drupal.org/api/drupal/includes%21errors.inc/function/calls/er...

It could move to settings.php, but then there's no UI for setting it any more.

This is one of a small number of variables which don't comfortably sit as either raw settings.php, globals, config, state or cache since they were completely reliant on the dual-function of variable_get() working both before and after variable_init().

I don't have any ideas on how to fix this at the moment. Since it blocks eventual variable removal opening as 'major'.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: -CMI +Configuration system

Correcting tag.

dawehner’s picture

chx’s picture

My current train of thought for these is to push them into the kernel. Language negotiation settings are similar so we might need some generic helpers which cache CMI settings into the kernel ie upon saving certain config values force a kernel rebuild and on kernel rebuilds set kernel parameters accordingly.

chx’s picture

Issue tags: +Stalking Crell
Anonymous’s picture

#3 scares me, because it increases the things that rebuild a kernel / DIC. this feels akin to triggering a registry rebuild on variable_set() in D7, which is scary.

i don't have a better idea (other than making CMI simple, low dependency system like it was many moons ago) :-\

sun’s picture

I agree with @beejeebus / #5.

Let's also bear in mind that we've force-disabled the Kernel's built-in error handling:
#1722694: Fixed Kernel::init() overrides Drupal's error handling configuration

Crell’s picture

I'd much rather switch over to the Kernel's error handling per #6, where it already has a mechanism in place. Let's cut to the chase and use it.

Anonymous’s picture

does using symfony's error handling help us with no GUI for configuring these settings? if so, yay, lets do that.

if it doesn't, i guess we need to decide on a) ditching the GUI requirement or b) living with rebuilding the DIC a lot more.

they both suck, but i think i hate a) less.

Crell’s picture

Symfony's "environment" parameter is implemented with multiple front controllers. So, in the stock fullstack package:

http://example.com/app.php/my/path // The default "production" environment (and the one that gets rewritten to by htaccess)
http://example.com/app_dev.php/my/path // The dev mode, all caching disabled, so the DIC is rebuilt every request
// And we could add more here.

We could also toggle the call to new Kernel() in index.php if we can set that value from earlier, but it would have to be via editing settings.php then.

Does that help? (Remember, most of Symfony fullstack is architected on the assumption that you change configuration by committing new yaml files to git, not via a GUI.)

Anonymous’s picture

#9 helps answer the question about a GUI to configure error handling. sf2 error handling doesn't help us at all with the GUI.

so we're back to a) or b) from #8. (or, you know, go back to a configuration system that can be used for read operations without a fully booted drupal.)

sun’s picture

Status: Active » Needs review
FileSize
1.88 KB

The discussion on revamping our error handling onto Symfony's should exclusively happen in #1722694: Fixed Kernel::init() overrides Drupal's error handling configuration only, IMO, since there are a lot more aspects involved, and we'd have to override and extend Symfony's error handling in various ways to meet our requirements.

That said, I'm not sure what remains to do for this issue, since error_displayable() was actually converted to config() already and off-hand I cannot really remember a situation during D8 development in which I experienced a horrible failure or fatal error due to that.

Instead, what I did experience are the following two issues:

  1. If system.logging is missing, then no errors are displayed, since error_displayable() does not apply a default value for that case.

  2. #1872522: Compiled data in PHP storage is cleared too late in drupal_flush_all_caches() — or: Bogus rebuild of the kernel/DIC, preventing the config service to be instantiated.

  3. Drupal's error/exception handlers getting invoked before any services are ready and thus blowing up, because they are registered before the services are actually available, and even before config.inc is loaded. ;)

    As contained in #1867972: Make HEAD2HEAD updates work and a range of other patches in the meantime.

    That said, this issue only applies to very low-level bootstrap errors, and we could reasonably argue that those can only happen during Drupal core code thaw; i.e., when the ultimate base bootstrap code under the hood is touched and changed; i.e., stuff that's before or within the very first bootstrap phase, DRUPAL_BOOTSTRAP_CONFIGURATION. After code thaw, these parts of the code base are expected to just Simply Work™ without exceptions.

Thus, I think the only thing we should do for this issue is to resolve 1) supplying a hard-coded default value for error_level.

For that, I'd propose to default to expose all errors, since that's the least of a WTF.

And we could additionally fix 3), since I guess it's more on-topic here than all of the other issues.

Anonymous’s picture

so that patch means any error before the config system is available will be shown on the screen? that doesn't seem safe.

sun’s picture

Well... here's a full stack conversion, accounting for all possibilities.

The only case in which the ultimate fallback value of _ALL is reached is when 1) system.logging does not exist or the config service(s) aren't available, AND 2) settings.php does not provide a value.

We can certainly change that final default value to _NONE, but I personally think that it doesn't make sense to do so, since the only situation in which you can reach it is when the entire system is wrecked. In that case, you normally want to see the error.

There's no parameter which would help us to decide differently, except two potential options:

1) Adding the environment parameter ('prod', 'dev', etc) directly to settings.php, and only falling back to _ALL/_VERBOSE if it's not 'prod'.

2) As a last resort, fall back to ini_get('error_reporting') (but that's out of control for many users).

Anonymous’s picture

settings.php values are not available in config() without an event dispatcher and a wired up listener. is that available before a booted kernel?

chx’s picture

settings.php is loaded insanely early , in DRUPAL_BOOTSTRAP_CONFIGURATION (which, contrary to the name has nada to do with CMI) and $conf is global'd from there. No need for a kernel.

sun’s picture

re: #14: Yeah, nothing of that is available in case config() fails. However, if config() fails, then it's almost guaranteed that it fails altogether due to a bogus or non-existing kernel/container.

Anonymous’s picture

woops, chx is right, the second patch uses global conf directly.

Berdir’s picture

+++ b/core/includes/errors.incundefined
@@ -153,14 +153,46 @@ function _drupal_render_exception_safe($exception) {
+    // Otherwise, default to ERROR_REPORTING_DISPLAY_ALL.
+    else {
+      $error_level = ERROR_REPORTING_DISPLAY_ALL;

I would argue that we should default to showing no errors or maybe fallback on the display_errors php ini setting?

catch’s picture

This is depending on a custom one-off wrapper around the configuration system to check if it's initialized then behave differently depending on whether it is or not. That's clever but it makes me a bit sad that we're re-introducing what was apparently a very handy feature of variable_get() inside error_displayable().

Anonymous’s picture

re #19 - yeah. i wish we hadn't added events to config. i wish i'd been able to stop the 'make all the config things pluggable' code that landed a while back. beejeebus--

config is simply not usable before a bunch of other things are ready. and some of those things will depend on config. yay.

Damien Tournoud’s picture

Hm. I actually agree with #3: system.logging is one of the CMI variables that are required to bootstrap the kernel (the other is system.module). We should just dump it into the kernel during the configuration phase and rebuild the kernel if it changes (note: that happens precisely once in a blue moon).

Those bits of bootstrapping that are pre-kernel are meant to go away, so the least one-off things we do here the better.

gdd’s picture

Isn't this now a candidate for moving into $settings? #1833516: Add a new top-level global for settings.php - move things out of $conf

Crell’s picture

heyrocker: If we say that it cannot be edited from the UI, yes. If not, no.

dawehner’s picture

FileSize
2.5 KB
1.84 KB

Let's see what the testbot says about this. The patch currently still contains the UI but should certainly be removed later.

Status: Needs review » Needs work

The last submitted patch, drupal-1845646-24.patch, failed testing.

catch’s picture

One reason to remove the UI for this came up on a client site recently.

If you set the variable via the UI, but you get a PHP error when your database goes down (for example hitting max connections), then since variable_init() just failed, it falls back to the default, which is printing to the screen. If we provide an option for setting this, then it should work regardless of bootstrap level and whether it's possible to read from the configuration system (thinking of an error when actually compiling the container or similar).

If there's no UI it'd be good to default the setting to log rather than printing to screen though.

gdd’s picture

I'm personally fine with removing the UI and agree on the default.

tim.plunkett’s picture

I think the UI is actually rather important to a majority of Drupal users, wouldn't this mean that anyone with only UI access would never be able to turn on errors/notices? (Or worse, never be able to turn them off)

gdd’s picture

I guess I feel like anyone with the need for this functionality wouldn't have only UI access, that seems like a pretty big edge case to me. Would be interested in more thoughts on it though.

webchick’s picture

We can't leave the setting off/logging by default. That's a no-go. Switching it to showing all errors (to the screen) by default is the reason D7 contrib isn't a complete wasteland of crappy notice-laden code.

Switching this setting from on (which needs to be the default) to off (which it needs to be in production) is something that literally every site builder (at least the smart ones who follow best practices) is going to do, since you expose your site to information disclosure vulnerability otherwise (I got called out by Rasmus Lerdorf in Sunnyvale for this :P).

The vast majority of our userbase is non-coders. Therefore, turning this into a code-only setting is a no-go, at least from where I sit.

chx’s picture

Well, then #3?

webchick’s picture

catch found #1671318: Error messages are shown to users under certain circumstances despite setting ERROR_REPORTING_HIDE which is an interesting approach, to solving both #26 and this issue. It basically says "I'm not fully bootstrapped yet, so let's default to logging errors so we don't do anything funny, then later in the request let me get the value that's actually supposed to be used."

chx’s picture

That's a great idea -- let's default to show warnings and notices to solve #30 and when the database comes along it can set whatever it wants.

Crell’s picture

This smells like another case where leveraging different environments a la Symfony would be beneficial: #1537198: Add a Production/Development Toggle To Core. On in dev, Off in prod. If you really want to change that you can via code, but if you have an actual reason to change it, guess what, you're in code anyway.

chx’s picture

Except that toggle per #30 would be helpful from an UI and you have no place, none at all, to put an UI-changeable value which is required to boot the kernel.

Crell’s picture

That's why Symfony ships with multiple front-end files: app.php and app_dev.php, that are hard coded to "prod" and "dev" environments, respectively.

dlu’s picture

Moved to configuration system per #2050763-16: Refine "base system" component (notes on refactoring of "base system" category here: https://docs.google.com/a/acquia.com/spreadsheet/ccc?key=0AusehVccVSq2dF...).

mtift’s picture

Component: base system » configuration system

#37 confused me a bit, so I'm going to be a bit more specific -- it was because of #2050763-16: Refine "base system" component

alexpott’s picture

Status: Needs work » Closed (duplicate)

#1951068: install.php error reporting is broken refactored the call to config() in error_displayable() into _drupal_get_error_level() which wraps it in a try catch and replicates the can't fail functionality of variable_get().