Add a link to the error settings form on "website encounted an error" message

That link would then be presented to all end users… which I don't think we want.

The main problem is that many people who are actively working on Drupal 8 core are unaware of the extra step they have to take when installing Drupal 8 (the enabling of settings.local.php). Despite the change notice (https://www.drupal.org/node/2259531).

How can we make people who are helping Drupal 8 get ready to be more aware of this? I wonder if we can do something like:

1. detect if the site is running on localhost ($_SERVER['SERVER_ADDR'] === '127.0.0.1')
2. detect if the site is a git checkout (file_exists(DRUPAL_ROOT . '/.git' directory
3. detect if the current user is the root user ($user->id() == 1
4. if all of the above are true, show a message like "You may want to enable development mode [etc. etc.]", perhaps linking to a handbook page.

if (localhost && uid == 1) works for me.

I would rather go with || tbh.

+1

+++ b/core/lib/Drupal/Core/Controller/ExceptionController.php
@@ -321,6 +321,14 @@ public function on500Html(FlattenException $exception, Request $request) {
+      $content .= t('In order to see the error message try to enable display errors under "logging and errors" on the admin section or enable it via a settings.local.php, see settings.php.');

$this->t

1. +++ b/core/includes/errors.inc
   @@ -246,6 +246,13 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +      if ((\Drupal::hasRequest() && array_intersect(['127.0.0.1', '::1', '::1'], \Drupal::request()->getClientIps())) || (\Drupal::hasService('current_user') && \Drupal::service('current_user')->accountInitialized() && \Drupal::service('current_user')->id() == 1)) {
   

   Isn't '::1' in there twice?

2. +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
   @@ -48,4 +48,11 @@ public function getAccount();
   +  public function accountInitialized();
   

   This method is not implemented?

Rerolled patch on 8.3.x (and about to do some more work on it)

Re: the failing test:

$subscriber = new DefaultExceptionSubscriber($config_factory);

Our patch means this now needs two parameters. The second is:

* @param \Drupal\Core\Session\AccountProxyInterface $user

*   The account proxy.

Not clear:
- what correct way is to generate user for tests (docs link welcome)
- if it matters which type of user for this one

Changes:

- use PHP filter to check against full IPv4 private range, not just 127.0.0.1 (struggling to find IPv6 equivalent)
- move ip address check to separate function and generally try to make that conditional easier to follow
- "In order to see the error" text was being shown even when they could already see it, fix this
- improve wording of the message
- coding style/standards

(Test will still fail, see #21, so skipping Testbot)

Re-rolled patch from #22, removed code from contrib directory. If we want we should add a PHPUnit test case but not a contrib module.

Fixed issue with failing tests.

@dawehner Ah sorry, I hadn't spotted there was more than one check...

re-roll with fixes for comments in #26

@nikunjkotecha:

You've kept the translation in on /DefaultExceptionSubscriber.php, but there's a message in errors.inc saying it's not used because it could create further errors.
Also, in #22, I reworded the message text slightly and wrapped it in a conditional to check that error display was actually turned off - i.e. pointless telling people how to view an error they can already see.

I'd just started working on the patch again when you posted #28, but feel free to do those changes if you like. (Thanks.)

@wturrell I'm already using your patch in #22 as base :), I saw your comment but I had already started and was waiting for the tests to finish in local before uploading. Just having enough free time today :)

submitting for re-test, output on dispatcher is not really helpful

No, I realise you used #22. What I mean is there are several changes - besides _is_private_ip(), which you've done – in errors.inc than need to be replicated in DefaultExceptionSubscriber.php:

- it doesn't use a string translation because, depending on what's broken, that might produce a further error whilst trying to print the error
- I've changed the message text in errors.inc slightly from what it was before
- I added an if (!error_displayable($error) conditional

(BTW, that problem with your first test run was a random Testbot failure – it lost a connection.)

* Removed use of $this->t() in DefaultExceptionSubscriber
* Refactored code a bit
* Copied message from errors.inc to DefaultExceptionSubscriber
* Added interdiff with 22 as base

Added a condition to avoid issues with the call to \Drupal::request()->getClientIps() in _is_private_ip()

Tested the patch. Applies cleanly and addresses the concerns in #35. Would be a welcome addition to the DX.

8.3.x is in RC.

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.

This seems more like a feature request then task

Tagging for tests as the new message is currently not checked.
Also tagging for a change record as we are changing an existing service.

1.

-  public function __construct(ConfigFactoryInterface $config_factory) {
+  public function __construct(ConfigFactoryInterface $config_factory, AccountProxyInterface $user) {
     $this->configFactory = $config_factory;
+    $this->user = $user;
   }

personally I think think $user should default to NULL and a trigger should be added that it will be required in D11

2.

+function _is_private_ip() {
+  // If it is not on screen and end user is not checking the message, there is
+  // no point showing the way to enable details.
+  if (!\Drupal::hasRequest()) {
+    return FALSE;
+  }

This will need to be typehinted as a new function

3.

+  public function accountInitialized() {
+    return isset($this->account);
+  }

same