Updated: Comment #12

Problem/Motivation

Some of our new OO code wants to do things like decode exception messages etc... Like I found earlier doing an IMP patch (_drupal_decode_exception() is being called in the MigrateExecutable class). This is kind of annoying as it's pretty much standalone utility logic.

Proposed resolution

Move methods that make sense into a Drupal\Core\Utility\Error class. format_backtrace, _drupal_get_last_caller, _drupal_render_exception_safe, and _drupal_decode_exception I think make sense. Some of the other methods in there are dependent on watchdog and other things, so maybe leave them. Distilling the useful re-usable things.

Also, remove the _ suffixed functions above, as these are technically not a part of any API that people can rely on.

Remaining tasks

RTBC?

User interface changes

None

API changes

Addition of Drupal\Core\Utility\Error class, but wrapper procedural function will be kept, so BC is kept for error functions. _ (private) functions will be removed.

CommentFileSizeAuthor
#17 interdiff-2157691-17.txt2.4 KBdamiankloip
#17 2157691-17.patch28.77 KBdamiankloip
#12 interdiff-2157691-11.txt16.95 KBdamiankloip
#12 2157691-11.patch31.23 KBdamiankloip
#9 interdiff-2157691-7.txt1.59 KBdamiankloip
#9 2157691-7.patch16.38 KBdamiankloip
#7 interdiff-2157691-7.txt698 bytesdamiankloip
#7 2157691-7.patch16.25 KBdamiankloip
#5 interdiff-2157691-5.txt9.91 KBdamiankloip
#5 2157691-5.patch15.57 KBdamiankloip
utility-error.patch10.18 KBdamiankloip
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +PHPUnit 
+++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -0,0 +1,164 @@
+      // The first element in the stack is the call, the second element gives us the caller.

80 line chars limit.

Can we add some unit tests as well?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: Unassigned » damiankloip

Yes, I was think we def need some tests.

Doing this.

chx’s picture

chx CreditAttribution: chx commented

Superb. We could do more in #2160655: Improve error handling of \Drupal class or a followup of it if this happens.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/includes/errors.inc
@@ -322,31 +286,7 @@ function _drupal_get_error_level() {
 function _drupal_get_last_caller(&$backtrace) {

@@ -137,9 +103,7 @@ function _drupal_decode_exception($exception) {
 function _drupal_render_exception_safe($exception) {

@@ -89,42 +90,7 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
 function _drupal_decode_exception($exception) {

    We maybe should at a @deprecated? 

  2. +++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -0,0 +1,164 @@
+      $db_functions = array('db_query',  'db_query_range');
...
+      // or in one of its global functions.

    As a follow up we should research whether we also can support $connection->query()/$connection->select ... db_query_range though is certainly wrong 

  3. +++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -0,0 +1,164 @@
+    return String::checkPlain(strtr('%type: !message in %function (line %line of %file).', $decode));

    What about using String::format ?

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2157691-5.patch15.57 KB
interdiff-2157691-5.txt9.91 KB

Ok, here are those changes, as well as some unit tests. I have added tests for the getLastCaller and formatBacktrace methods. However, testing decodeException is a bit awkward - You can't create a new Exception without it being thrown (feature? ;)), you also cannot control the backtrace etc.. so it become messy. Any good suggestions on testing this? I guess we need to arrive at some sort of compromise here.

The last submitted patch, 5: 2157691-5.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Assigned: damiankloip » Unassigned
FileSize
2157691-7.patch16.25 KB
interdiff-2157691-7.txt698 bytes

Ah, so the actual raw assertion text it was looking for in ShutdownFunctionsTest is actually wrong. It was (and I guess has been for some time) double escaped as the message is passed through String::checkPlain() in decodeException, then again in _drupal_render_exception_safe().

The changes I have made to Error::renderExceptionSafe is correct, using String::format() now. As the message will actually utilise the output form decodeException now. So changed the shutdown function test assertion text accordingly.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Some minor issues.

  1. +++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -0,0 +1,176 @@
+namespace Drupal\Core\Utility;
...
+use Drupal\Component\Utility\String;

    Why is it not in compenent like other Utilitiy? Or Why is it in core? 

  2. +++ b/core/tests/Drupal/Tests/Core/Utility/ErrorTest.php
@@ -0,0 +1,161 @@
+    // Test with just one item. This should default to the function being main().

    more then 80 chars.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
FileSize
2157691-7.patch16.38 KB
interdiff-2157691-7.txt1.59 KB

Why is it not in compenent like other Utilitiy?

We have lots of things also in Core\Utility. I added this to Core namespace as stuff like decodeException is pretty specific to Drupal, rather than generalised - IMO. If people disagree though, I can move.

Also, the interdiff contains changes needed from rerolling after #2159459: Rebuild script triggers errors in error handler, fails to rebuild container went in, and made a change to format_backtrace code.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Just a note for a possible follow up: Symfony\Component\Debug\Exception\FlattenException has kind of similar functionaliy (ensures that there is no object in the backtrace.

+++ b/core/lib/Drupal/Core/Utility/Error.php
@@ -0,0 +1,178 @@
+      $db_functions = array('db_query',  'db_query_range');
+      while (!empty($backtrace[1]) && ($caller = $backtrace[1]) &&

Not really sure whether we should support db_query/db_query_range here, given that most calls should be converted already.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Beside that, this is fine for me!

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Reviewed & tested by the community » Needs review
FileSize
2157691-11.patch31.23 KB
interdiff-2157691-11.txt16.95 KB

Ok, spoke to chx on IRC. Let's just remove the _drupal_decode_exception, _drupal_render_exception_safe, and _drupal_get_last_caller methods here too, and just replace their usages (not too much).

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Issue summary: View changes
chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

I see no problems with nuking private functions at an alpha stage.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

$rtbc = TRUE;

The last submitted patch, 12: 2157691-11.patch, failed testing.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented
Status: Reviewed & tested by the community » Needs review
FileSize
2157691-17.patch28.77 KB
interdiff-2157691-17.txt2.4 KB

Ok, so we can't remove ExceptionController::decodeException (just yet) as it is relying solely on FlattenException, so has some different logic based around this.

Interdiff just to show changes to ExceptionController.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Let's git it in in the current working state.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Looks great. Committed/pushed to 8.x, thanks!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Looks like we could have used those methods now in ExceptionController::decodeException() and others there? Follow-up?

It actually did cover that in away, but that implementation is special, as @damiankloip just explained to me.

damiankloip’s picture

damiankloip CreditAttribution: damiankloip commented

Yeah, we did try to reuse it in ExceptionController, but it has a modified version of decodeException, just for dealing with FlattenException :/

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented

Created a follow up issue for some missing class names in comments #2170519: Fix missing class name in comments in TestBase

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.