Problem/Motivation

While debugging some test failures I came across the notice ... Undefined index args .... The location information (file and line number) were absolutely misleading.
After some digging I figured out that this is an error introduced by #1158322: Add backtrace to all errors.
Currently the code assumes that the args index is always available in the backtrace. Unfortunately this doesn't seem to be the case.

Proposed resolution

Add a check to determine if args is available in the backtrace.
The attached patch should do the trick.

Remaining tasks

Review the attached patch.
Clarify if there are other keys that are conditionally available.

User interface changes

none

API changes

none

CommentFileSizeAuthor
#40 drupal_1832300_40.patch1.32 KBxano
#40 interdiff.txt1.19 KBxano
#33 drupal-more_defensive_error_handler-1832300-33-tests-only.patch1.84 KByesct
#33 drupal-more_defensive_error_handler-1832300-33.patch2.95 KByesct
#33 interdiff-18-33.txt1.56 KByesct
#18 core-error-handler-test.patch1.84 KBifux
#18 core-more-defensive-error-handler-test.patch2.71 KBifux
#16 core-error-handler-test.patch1.81 KBifux
#16 core-more-defensive-error-handler-test.patch2.68 KBifux
#12 core-more-defensive-error-handler-test.patch1.91 KBifux
#11 core-more-defensive-error-handler-with-test.patch2.77 KBifux
#11 core-more-defensive-error-handler-test-inter-diff.txt1.91 KBifux
#2 core-more-defensive-error-handler-2.patch889 bytesifux
core-more-defensive-error-handler.patch919 bytesdas-peter

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: +Novice

Related: #1739808: Notice: Undefined index: file in Drupal\Core\Database\Log->findCaller() showed that sometimes file/line is also not present. this is however not used here.

+++ b/core/includes/errors.incundefined
@@ -346,12 +346,15 @@ function format_backtrace(array $backtrace) {
+    $call['args'] = array();

$call['args'] is already initialized to an empty array a few lines above. So this can be removed.

This can be removed. Tagging as novice for that.

ifux’s picture

ifux commented
Status: Needs work » Needs review
StatusFileSize
new core-more-defensive-error-handler-2.patch889 bytes

if that's all you need!?! yeah I can remove a line :-) ;-)

greets ifux

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Yes that's all :)

@das-peter, were you able to identify a way to produce this notice so that we could add test coverage for it?

das-peter’s picture

das-peter commented

@ifux Thanks!
@Berdir Unfortunately not.

I can only guess that this could happen if the an error happens in a function without parameters or in code that's not part of a function.

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, core-more-defensive-error-handler-2.patch, failed testing.

das-peter’s picture

das-peter commented
Status: Needs work » Needs review
Issue tags: +Novice

#2: core-more-defensive-error-handler-2.patch queued for re-testing.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

$call['args'] needs to be defined so the implode statement doesn't give an error.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

It's not visible in the patch, but it is, see http://api.drupal.org/api/drupal/core%21includes%21errors.inc/function/f....

I guess one way to test this function would be to write a unit test that manually builds up some backtrace-like arrays and passes it to that function.

Anonymous’s picture

Anonymous (not verified) commented

It's not visible in the patch, but it is

I was blind when I check the git source. Sorry for the noise.

das-peter’s picture

das-peter commented

I think I figured out a way how to reproduce this case.
I've overwritten a class method which had a typed parameter. But I forgot to add the use part - thus the type of the parameter was "unknown" and the class autoloader failed.
However, I'm not sure this is the easiest way to reproduce it ;)

ifux’s picture

ifux commented
StatusFileSize
new core-more-defensive-error-handler-test-inter-diff.txt1.91 KB
new core-more-defensive-error-handler-with-test.patch2.77 KB

berdir told me to implement the following test

ifux’s picture

ifux commented
StatusFileSize
new core-more-defensive-error-handler-test.patch1.91 KB

and the test only patch to show that it's not working without patch from above

Status: Needs review » Needs work

The last submitted patch, core-more-defensive-error-handler-test.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

test-only failed as we expected it to.

das-peter’s picture

das-peter commented
Status: Needs review » Needs work 
+++ b/core/modules/system/lib/Drupal/system/Tests/Error/FormatBacktraceUnitTest.php
@@ -0,0 +1,68 @@
+    //debug(format_backtrace($backtrace));
...
+//    $this->assertTrue(format_backtrace($backtrace));

I think these lines could be removed, right?

Everything else looks really good to me - thanks!

ifux’s picture

ifux commented
StatusFileSize
new core-more-defensive-error-handler-test.patch2.68 KB
new core-error-handler-test.patch1.81 KB

removed them.

1. just the test patch -> should fail
2. complete patch -> should pass

ifux’s picture

ifux commented
Status: Needs work » Needs review
ifux’s picture

ifux commented
StatusFileSize
new core-more-defensive-error-handler-test.patch2.71 KB
new core-error-handler-test.patch1.84 KB

hopefully last try!

some unnecessary lines removed

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, core-more-defensive-error-handler-test.patch, failed testing.

ifux’s picture

ifux commented
Status: Needs work » Needs review

#18: core-more-defensive-error-handler-test.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, core-more-defensive-error-handler-test.patch, failed testing.

ifux’s picture

ifux commented
Status: Needs work » Needs review

#18: core-more-defensive-error-handler-test.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, core-more-defensive-error-handler-test.patch, failed testing.

das-peter’s picture

das-peter commented
Status: Needs work » Needs review

#18: core-more-defensive-error-handler-test.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, core-more-defensive-error-handler-test.patch, failed testing.

das-peter’s picture

das-peter commented
Status: Needs work » Needs review

#18: core-more-defensive-error-handler-test.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, core-more-defensive-error-handler-test.patch, failed testing.

das-peter’s picture

das-peter commented
Status: Needs work » Needs review
Issue tags: +Novice

#18: core-more-defensive-error-handler-test.patch queued for re-testing.

das-peter’s picture

das-peter commented
Status: Needs review » Reviewed & tested by the community

@ifux I'm absolutely happy with that.
I say RTBC.

catch’s picture

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

OK the patch looks great, but we could do with a comment explaining why args is sometimes empty I think.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

comment #10 is about the only idea that we have so far. Officially, this shouldn't happen, as far as I know.

das-peter’s picture

das-peter commented

I don't have an idea either. As berdir pointed out I experienced the error only in the in #10 described case.
Does this missing comment really block this patch? It's quite annoying to have this error instead the real one when running tests on new stuff ;)

yesct’s picture

yesct commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-18-33.txt1.56 KB
new drupal-more_defensive_error_handler-1832300-33.patch2.95 KB
new drupal-more_defensive_error_handler-1832300-33-tests-only.patch1.84 KB

I added the comment based on the info in #10
I also fixes some small standards stuff

/**
 * @file
 * Contains \Fully\Qualified\Namespace\And\NameOfTheClass.
 */

from: http://drupal.org/node/1354#file

and whitespace/new lines at the start and end of class (edit: http://drupal.org/node/1353118 namespaces standards doc has an example that shows them)

mitron’s picture

mitron commented

Why are there more passing tests on the "FAILED" test (49,316) than on the "PASSED" test (49,231)? Something else, other than the patch, seems to have changed between the two test runs. Is it reasonable to re-test?

yesct’s picture

yesct commented

#33: drupal-more_defensive_error_handler-1832300-33-tests-only.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The number of assertions is always slightly different, because for example the number of batch steps for the upgrade tests differ depending on how fast the testbot is.

Anonymous’s picture

Anonymous (not verified) commented

I have reviewed and applied both patches. The first version (drupal-more_defensive_error_handler-1832300-33-tests-only.patch) fails as expected, the second version (drupal-more_defensive_error_handler-1832300-33.patch) runs successful.

It's working fine for me. The patch contains a test case and the changed logic i documented.

das-peter’s picture

das-peter commented

#33: drupal-more_defensive_error_handler-1832300-33.patch queued for re-testing.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented 
+++ b/core/includes/errors.inc
@@ -347,12 +347,18 @@ function format_backtrace(array $backtrace) {
+    // Args should never be empty, but it might be if a class method had a
+    // typed parameter, but the use part was not added. Thus, the type of the
+    // parameter would be "unknown" and the class autoloader would have empty

http://nl1.php.net/manual/en/function.debug-backtrace.php describes the array keys as follows:

Returns an array of associative arrays. The possible returned elements are as follows:

This means that the official PHP documentation admits these keys, including args, are optional. As it does not provide any reasons, though, I suggest we simply point developers there rather than giving one possible explanation of why the array key might not exist.

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
StatusFileSize
new interdiff.txt1.19 KB
new drupal_1832300_40.patch1.32 KB

Oh well.

socketwench’s picture

socketwench commented
Issue summary: View changes
Status: Needs review » Postponed
Issue tags: -Novice

Novice issue cleanup. (https://drupal.org/core-mentoring/novice-tasks) Also attempted to reroll the patch, it appears that most of changes are no longer relevant to code. Setting the issue to postponed as a result.

mgifford’s picture

mgifford
he/him
Primary language English
commented

I'm unclear what this is waiting for? Can we close the issue or does it need to be completely re-written?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

quietone’s picture

quietone commented
Status: Postponed » Closed (outdated)
Issue tags: +Bug Smash Initiative

#41 is correct this is no longer relevant, format_backtrace was removed in Drupal 8.0.x in #2300891: Remove format_backtrace() from error.inc as deprecated.