Exclude internal functions from backtrace

Is this patch meant to solve the problem at #2643392: dpm()'s / Kint shows the file and line from devel module, not from where dpm() was called? It's not doing that for me -- dsm() output says:

> Called from /8-drupal/modules/contrib/devel/kint/src/Plugin/Devel/Dumper/Kint.php:32 [Drupal\kint\Plugin\Devel\Dumper\Kint->export()]

Shouldn't these functions be included in the list, because this issue isn't only about kint.

backtrace_error_handler
dargs
dcp
ddebug_backtrace
dfb
dfbt
dpq

1. +++ b/src/DevelDumperBase.php
   @@ -40,4 +40,41 @@ abstract class DevelDumperBase extends PluginBase implements DevelDumperInterfac
   +   * The list returned from this method could be very useful for dumpers that
   

   'could be very useful' doesn't read right for actual code. How about 'is intended for'

2. +++ b/src/DevelDumperBase.php
   @@ -40,4 +40,41 @@ abstract class DevelDumperBase extends PluginBase implements DevelDumperInterfac
   +   *   An array of methods and functions which will be considered internal.
   

   Needs to document the nested structure of the array.

Revised patch

• Adds code to make it work for kint.
• Fixes comments #5, #6-1.
• Return a simple array instead of nested - php doesn't really make a distinction of method/function - they are all function, callable

I think the test fail is unrelated

Let's rename to getInternalFunctions

Done #9.
New patch also fixes typo on the quote position for some of the functions in the list.

> Called from /8-drupal/modules/contrib/devel/kint/src/Plugin/Devel/Dumper/Kint.php:32 [Drupal\kint\Plugin\Devel\Dumper\Kint->export()]

Couldn't we just check for '/devel/' (and maybe some other fragments) in the file path?

IAC, it should be a variable, so that it can be configured locally without hacking Devel.

(Also, I'm not sure against which branch this should go.)

Thanks @salvis

1. Couldn't we just check for '/devel/' (and maybe some other fragments) in the file path?

No I think it doesn't work that way. It is the dumper plug-in that does the check. In the case of kint like this:

\Kint::$aliases = $this->getInternalFunctions();

I don't see any way to pass a regex.

2. IAC, it should be a variable, so that it can be configured locally without hacking Devel.

Please can you explain what scenario you are thinking of and what should be a variable? Any base dumper class can easily override getInternalFunctions() without any hacking of Devel.

3. (Also, I'm not sure against which branch this should go.)

Normally in Drupal we commit to the newest branch then maybe back-apply.

Setting back to "needs review" for response from reviewer.

2. IAC, it should be a variable, so that it can be configured locally without hacking Devel.

Please can you explain what scenario you are thinking of and what should be a variable? Any base dumper class can easily override getInternalFunctions() without any hacking of Devel.

I mean configuration. People might want to add to or remove from the list.

As I see it the purpose of this issue is to ensure that the internal functions of devel are excluded from backtrace. The problem is explained clearly in #2643392: dpm()'s / Kint shows the file and line from devel module, not from where dpm() was called - which I just noticed that you raised:-). When you add a dump line of code to a function my_func_to_debug, then the trace should reports the backtrace starting from my_func_to_debug, and not including the debug line of code that you added such as ksm(). So it seems to me that the list of internal functions is determined by the code present in this module - i.e. they are internal to this module (or whatever module the dumper plugin is in).

I mean configuration. People might want to add to or remove from the list.

Ah I see, thanks for clarifying. Do you have an example in mind of when this would be required?

I don't really have time to extend the patch for configuration. I propose that the patch as it stands is useful so why don't we commit it. We can raise a follow on issue for further enhancements.

Someone may want to wrap, say, dpm(), for whatever reason, and want to exclude their wrapper as well. Hard-coded lists of function names just don't feel right...

Someone may want to wrap, say, dpm(), for whatever reason, and want to exclude their wrapper as well.

Ah yes I see, I agree that is a valid possibility. However I would say that less than 1% of people will want to do that.

As I mentioned, unfortunately I don't have time for that. The patch as it stands is a significant improvement. Nothing in this patch makes any obstacle to adding the ability to configure extra functions later. How about if we raise a separate issue to add configuration?

I guess the alternative is to set this needs work and wait hoping that someone will feel inspired to do the work.

I'm fine to leave it at NR.

Thanks @salvis

Thank you, AdamPS! :-)

Many thanks, @AdamPS.

Thanks @moshe weitzman

Here's the patch backported to 8.x-2.x, for the benefit of everyone still running that version.

Updated 8.x-2.x patch -- I finally figured out the problem with warning messages being removed!

Anyone interested in reviewing #25 with Drupal 8.6?

If someone else can verify that this works on 2.x branch, I'll merge it.

I just scanned the issue queue and it looks to me like 8.x-3.x is in good shape now - very little left in "Needs Review" or "RTBC", thanks Moshe for the recent commits. So another alternative that could be considered more forward-looking is to create a 3.x release then mark 2.x as unsupported soon after.

Just created a 3.x release - https://www.drupal.org/project/devel/releases/8.x-3.0-beta1. Final release will come soon as well. I'll leave this open for a bit in case someone really wants to backport and can verify it works.

Excellent, many thanks @moshe. I have been running with local patches to devel for 2 years but now running clean, the latest release is great.