Make dpm() and friends pluggable (with Krumo, Kint, Ladybug etc)

Also related #1804998: Add LadyBug from Symfony to Core to allow debugging variables within templates

Just to be clear, you're proposing to expose the whole devel debug tools as a service, right?

I think it's a great addition as long as we provide (at least) one by default, but we could discuss which and how many should be packaged with devel.

It doesn't seems that the direction is to provide a service/pluggable system as Kint is in already and we're going to kill Krumo in: #2209705: Kill krumo and procedural debug functions

Move to 8.x.

Edit: the following patchs cover only the first part of IS - make dumper libraries pluggable

In my opinion this is the way forward for 8.x. Make the various dump functions pluggable make devel much more flexible, makes the various functions for the dump of the variables more consistent, we would avoid having to maintain procedural (and obsolete) code and we could allow developers to use their favorite libraries for debugging.

Attached a proof of concept:
- created DevelDumper plugin type
- created a plugin for kint and a default one (which use Doctrine\Common\Util\Debug)
- created a generic function devel_dump(), that relying on the plugin manager, dump variables based on the configuration
- made kdevel_print_object() and kprint_r() use the new plugin system

work is only partial, and all dump functions can be optimized for use the new plugin system but you can already see some results.

it is a draft and it can be improved a lot.. Suggestions/reviews/help is welcome/needed :)

ehem..the diff was not complete..

Removed devel_print_object() and _devel_print_object()

and the interdiff

some progress

Created DevelDumperBase as base class for dumper plugins (for now is empty); all plugins now extend this base class
All dumper plugin now must implement DevelDumperInterface
DevelDumperPluginManager now implements FallbackPluginManagerInterface

I'm thinking on how change the various dump functions but i have some dubts. We have :
dvr(), dvm() and dprint_r()(if $function = 'drupal_var_export') that should use Variable::export() (was drupal_var_export). Have these functions still reason to exist since Variable::export () is not able to export complex objects (eg export an entity result in Warning: var_export does not handle circular references)?
do we need to write a dumper to export variables in a Drupal-friendly way?

kpr(), kprint_r(), kdevel_print_object(), dpr() and dprint_r() do all more or less the same thing. We should deprecate all these functions except:
- kpr() dump variables using the plugin chosen during configuration
- dpr() dump variables forcing the use of doctrine plugin (plain text)
do you agree?

ChromePhp and FirePhp need a dedicated plugin. We could think to create a dedicated modules which already include the needed libraries

It would also be a good thing find a way to ensure that, for all plugins that make use of external libraries, the needed classes/functions exist and are included

I like this approach and would allow those really missing krumo to have it with Devel. I remember that this approach was discarded a while ago because it was overkill of a simple feature, assigning to Moshe for input.

Some reviews.

1. +++ b/config/install/devel.settings.yml
   @@ -15,3 +15,4 @@ rebuild_theme: FALSE
   +devel_dumper: 'doctrine'
   

   Do we need to call it doctrine? seems confusing

2. +++ b/devel.module
   @@ -916,6 +771,42 @@ function devel_timer() {
   + * @param bool $return
   

   We're calling it 'export' everywhere else

3. +++ b/kint/src/Plugin/DevelDumper/KintDevelDumper.php
   @@ -0,0 +1,50 @@
   +  public function dump($input) {
   +    if ($this->init()) {
   +      \Kint::dump($input);
   +    }
   ...
   +  public function export($input) {
   +    if ($this->init()) {
   +      return @\Kint::dump($input);
   +    }
   

   Why are these different and why do we need the @?

4. +++ b/kint/src/Plugin/DevelDumper/KintDevelDumper.php
   @@ -0,0 +1,50 @@
   +    return kint_require();
   

   Are we using kint_require anywhere else?

5. +++ b/src/Plugin/DevelDumper/DoctrineDevelDumper.php
   @@ -0,0 +1,44 @@
   +class DoctrineDevelDumper extends DevelDumperBase {
   

   Why not just DevelDumper?

Needs a reroll. After that I would like to test the doctrine dumper and if that looks good, we can commit this.

This is way over-engineered for my taste, but thats true of a lot of D8. The IS complains "Devel is "polluting" the global namespace with functions like dpm() etc, thus preventing others from doing the same.". Yet the patch keeps this problem, and makes a pluggable system for what dpm() does. If I were engineering this, I would just assert that dpm() is a devel owned function and that other modules that prefer doctrine or krumo can make their own equivalent functions. If dpm() sounds like an especially valuable function name then you probably have been enjoying devel for years!

A couple key devel maintainers like this patch so I won't obstruct it. Lets commit it soon. I just want to voice my opinion.

@moshe thanks for looking at this.

some clarifications:

Needs a reroll. After that I would like to test the doctrine dumper and if that looks good, we can commit this.

I think the patch is not ready for to be committed. It needs some work and especially should be clarified the doubts expressed in 11

This is way over-engineered for my taste, but thats true of a lot of D8.

I can agree..

If I were engineering this, I would just assert that dpm() is a devel owned function and that other modules that prefer doctrine or krumo can make their own equivalent functions.

We are not talking about dpm() or dumping functions. We are talking to integrate devel with external libraries for debugging. Currently if I want integrate my favorite libraries with devel I can't. I should write a module and duplicate most of the functionality of devel just to change the dump output..

This issue should solve the problem and make devel much more flexible

Attached a reroll of #11.. I'm going to work on suggestions in #12

@pcambra

12.1 - Renamed to 'default'
12.2 - Fixed
12.3 - We need both. @ modifier force kint to return the rapresentation of object as string instead of dumping it. See Kint - Advanced usage
12.4 - yes it is used inside kint module so we can't move require in the KintDevelDumper class.
12.5 - DevelDumper is also the name of annotation class so could be confusing.. how about rename it in DefaultDevelDumper?

other changes from #16:
- deleted unused functions has_kint(), has_krumo() and merits_kint()
- remove kint install from DevelControllerTest::testRouteGeneration()

some work:

Changed the plugin's subdirectory from 'Plugin/DevelDumper' to 'Plugin/Devel/Dumper' for a better organization of plugins.

Added support for prefix label for the variable (as is in the current implementation)

Added a wrapper for SafeMarkup::set() in DevelDumperBase and used it where needed (since after #2273925: Ensure #markup is XSS escaped in Renderer::doRender() we need to set our debug output as safe or it is stripped and escaped)

Introduced Drupal\devel\RequirementsInterface for checking for plugins requirements (eg external libraries). Each plugin that implements this interface is checked in DevelDumperPluginManager::processDefinitions() and marked as supported if its requirements are satisfied. This should solves my concerns for the unchecked external plugin dependencies.

Made KintDevelDumper implement RequirementsInterface

Added ChromePhp and FirePHP plugin. They of course implement RequirementsInterface and are supported only if external lib are available. Moreover they check also at runtime if their dependencies are satisfied so we can avoid fatal errors.

Wrapped output of DoctrineDevelDumper::export() in a details element so a page with multiple dump is a lot simpler to read

Managed in SettingForm the dumper's radios for permit to select only supported plugins

quick reroll

rerolled

rerolled after #2576533: Rename SafeStringInterface to MarkupInterface and move related classes

Rerolled after #2620790: Remove Krumo library and Krumo integration from devel

Initial implementation of Symfony var-dumper integration here: https://www.drupal.org/sandbox/lussoluca/2631596

The previous patch obviously fails because #2620790: Remove Krumo library and Krumo integration from devel is not yet committed. Here's a patch with both #2620790: Remove Krumo library and Krumo integration from devel and the patch in #24

Patch regenerated with --binary

@luca in my opinion we should not merge the two issues. They are separate and specific taks and the code conflicts are very few (some functions that we want to remove in both the issues). Moreover if we do not merge the patches the code in this issue will remain more simple and easy to review.

The only reason for which I not have yet committed #2620790: Remove Krumo library and Krumo integration from devel is that currently we have an issue with the CI testbot and I want to avoid regressions (see #2629828: Wrong dependencies are used on Drupal CI for devel_entity_test module).

attached a reroll of the "clean" patch.

I want to discuss and go in deep with the issue before committ. I'd like to know what you think of the implementation in the patches and how do you think it can be improved. The currently the implemantation simply works :) but we can improve and make more solid the code.

Here some of the doubt that I have and that I would like to discuss:
- currently devel_dump() acts as a "manager". Is better to create, for the DX and testability, a service or a class with static method and replace this function?
- the access checks are scattered all around the code. Should we centralize they in a unique point (the manager)? and should the plugins to be able to define the rules to access to their own functionality?
- currently the plugins that I wrote check their requirements manually every time that are instantiated. Should we move the requirements check outside, in the manager, and make plugins simple implements the RequirementsInterface (a good improvement from the DX perspective)?

@willzyx I agree with you,
I'll wait that #2620790: Remove Krumo library and Krumo integration from devel comes in and meanwhile we could discuss on IRC how to better implement this one

#2620790: Remove Krumo library and Krumo integration from devel is in. Now we can proceed with this issue

Rerolled after #2620790: Remove Krumo library and Krumo integration from devel

Here my version. Some notes:

1. a new manager service (devel.dumper)
2. all functions in devel.module are now just wrappers to service methods
3. checkRequirements method is now in DevelDumperInterface so every dumper must implement it (just return TRUE for no requirements)
4. the manager checks for access and requirements

Great Luca! some observations:
+1 on removing code from devel.module and use OO where possible but some changes seem out of scope here: dargs, devel_render, dpq use the dumper system but they are not part of it and should not be included in DevelDumperManager. For moving the code out of devel.module we could open a separate issue :)

Also, the manager should contains generic methods, map 1:1 the devel.module functions in the manager seems wrong.. I think that all we need for debug variables is:
-dump : dump the variable
-export : return the string rapresentation of the variable
-message : set a message that contains the exported variable
-debug : write to file the exported variable

with these 4 methods and the dumper plugins we could cover all the functionality currently implemented.
I try to write a patch shortly

Here the patch. Note: I have not removed kprint_r() and kdevel_print_object() for now so the patch remains small and is more simple to review.

Would you please define the 4 methods more clearly?

- "dump" — where and in what format?
- "export" — in what string format? one-liner? JSON? PHP?
- "message" — the export?
- "debug" — why not call it "write"?

If we can write the "export" to a file, then I assume it's human readable, right? But then it does not have enough information to produce an interactive tree such as what we currently see from dpm().

I think we should discuss this and come up with a clear specification before jumping into code.

@salvis the 4 methods that i suggested are implemented in the DevelDumperManager and they are not thought for substitute the functions in devel.module. When called they trigger specific methods on the selected plugin; Think to them like a "bridge" for interact with the plugins. The hard work of extract and format the variables is totally done by plugins.

- "dump" — where and in what format?

Print a human-readable variable on the screen (ala print_r()). When called it triggers the ::dump() method in the selected plugin. It is the plugin that is responsible for formatting and display the variable.

In the patch is used by dcp, dfp, dpr($return=false),kpr($return=false), dvr($return=false)

- "export" — in what string format? one-liner? JSON? PHP?

Returns a string representation of a variable. When called it triggers the ::export() method in the selected plugin. It is the plugin that is responsible for formatting and return the string representation of the variable.

In the patch is used by dpr($return=true),kpr($return=true), dvr($return=true)

- "message" — the export?

When called it triggers the ::export() method in the selected plugin and the result string is used for set a message to display to the user (drupal_set_message())

In the patch is used by dpm(),dvm()

- "debug" — why not call it "write"?

When called it triggers the ::export() method in the selected plugin and the result string is written to a file (drupal_debug.txt). I have called it "debug" because it is used by drupal_debug() and dd(). If you prefer another name there are not objections for me :)

If we can write the "export" to a file, then I assume it's human readable, right? But then it does not have enough information to produce an interactive tree such as what we currently see from dpm().

The informations written to file are in a human readable format (no interactive tree obviously) like the informations written by the current implementation of drupal_debug().

Your review and your any criticisms are welcome :)

This looks good to me. Feel free to commit when you are ready.

+1 also for me, good tradeoff

I think that In order to commit this we need test coverage (and I really need help for this!)

Ok, I can look at this

First version of tests just to see if I'm on the right path. This will fail because of #2657032: Unused element in schema and config

No idea if it is useful here, but #2671034: Create plugin per file YAML discovery just landed.

@willzyx - any chance you can review the tests in #44?

Rerolled

With #2657032: Unused element in schema and config in all tests are green, for me is ready to be committed

Patch looks great to me. Lets give @willzyx a few days to review this before merge.

Can happen before or after merge:

1. More descriptive labels/descriptions so we guide the user in her choice of dumper plugin
2. Disabled plugins should give help to the user about how to enable them
3. I would think that Symfony VarDumper plugin should ship with Devel. Its OK that it adds a new Composer dependency.
4. I have not tried Ladybug. Ship it with Devel?

@moshe great to know and I'm ok to commit this! unfortunately I have no way to work on this patch or on a full review before the weekend..
for now a quick review (many of the observation are on the code that I wrote :p):

1. +++ b/src/DevelDumperPluginManager.php
   @@ -0,0 +1,76 @@
   +  public function isPluginSupported($plugin_id) {
   +    $definition = $this->getDefinition($plugin_id, FALSE);
   +    return $definition && $definition['supported'];
   +  }
   

   it is worth to add an interface for DumperPluginManager for this method and for the methods that in the future will be added to the manager?

2. +++ b/src/DevelDumperPluginManager.php
   @@ -0,0 +1,76 @@
   +    parent::__construct('Plugin/Devel/Dumper', $namespaces, $module_handler, 'Drupal\devel\DevelDumperInterface', 'Drupal\devel\Annotation\DevelDumper');
   

   Can we use DevelDumperInterface::class and DevelDumper::class instead of FQNS string?

3. +++ b/src/DevelDumperManagerInterface.php
   @@ -0,0 +1,92 @@
   \ No newline at end of file
   

   No newline at end of file

4. +++ b/src/DevelDumperManager.php
   @@ -0,0 +1,138 @@
   +    // The temp directory does vary across multiple simpletest instances.
   +    $file = 'temporary://drupal_debug.txt';
   

   maybe I'm wrong but I don't think that drush supports stream wrappers

5. +++ b/src/Plugin/Devel/Dumper/DoctrineDevelDumper.php
   @@ -0,0 +1,77 @@
   +  protected function decorate($input, $name = NULL) {
   +    $output['wrapper'] = array(
   +      '#type' => 'details',
   +      '#title' => $name ?: t('Variable'),
   +      '#attributes' => array('class' => array('container-inline', 'devel-dump-output')),
   +      '#open' => TRUE,
   +    );
   +    $output['wrapper']['dump'] = array(
   +      '#markup' => '<pre>' . $input . '</pre>',
   +    );
   +
   +    return \Drupal::service('renderer')->renderPlain($output);
   +  }
   

   This seems wrong.. the only work that the ::export() method on plugins should do is provide a string representation of a variable; a "clean" string representation of a variable in this way is inaccessible for developers.

   We could think to introduce in plugins a new method ::exportAsRenderable() for allow plugins to wrap the strings in a render array and if needed add asset (css/js) for enhance the navigation of the dumped variables.
   The new method can also improve the DX (eg):

   return $manager::exportAsRenderable($variable)

6. +++ b/kint/src/Plugin/Devel/Dumper/KintDevelDumper.php
   @@ -0,0 +1,62 @@
   +class KintDevelDumper extends DevelDumperBase {
   
   +++ b/src/Plugin/Devel/Dumper/ChromePhpDevelDumper.php
   @@ -0,0 +1,45 @@
   +class ChromePhpDevelDumper extends DevelDumperBase {
   
   +++ b/src/Plugin/Devel/Dumper/DoctrineDevelDumper.php
   @@ -0,0 +1,77 @@
   +class DoctrineDevelDumper extends DevelDumperBase {
   
   +++ b/src/Plugin/Devel/Dumper/DrupalDevelDumper.php
   @@ -0,0 +1,48 @@
   +class DrupalDevelDumper extends DevelDumperBase {
   
   +++ b/src/Plugin/Devel/Dumper/FirePhpDevelDumper.php
   @@ -0,0 +1,45 @@
   +class FirePhpDevelDumper extends DevelDumperBase {
   

   We can probably remove the DevelDumper suffix. Also Drupal dumper should renamed to something like DrupalVariable(?)

7. +++ b/src/DevelDumperBase.php
   @@ -0,0 +1,36 @@
   +abstract class DevelDumperBase extends PluginBase implements DevelDumperInterface {
   
   +++ b/src/DevelDumperInterface.php
   @@ -0,0 +1,51 @@
   +interface DevelDumperInterface {
   

   we can move these classes in dumper plugin folder and remove the Devel prefix(but this is only a matter of taste)

8. +++ b/src/DevelDumperManager.php
   @@ -0,0 +1,138 @@
   +class DevelDumperManager implements DevelDumperManagerInterface {
   
   +++ b/src/DevelDumperManagerInterface.php
   @@ -0,0 +1,92 @@
   +interface DevelDumperManagerInterface {
   
   +++ b/src/DevelDumperPluginManager.php
   @@ -0,0 +1,76 @@
   +class DevelDumperPluginManager extends DefaultPluginManager implements FallbackPluginManagerInterface {
   

   we can goup these classes in a /Dumper folder and remove the Devel prefix (but this is only a matter of taste)

I would think that Symfony VarDumper plugin should ship with Devel. Its OK that it adds a new Composer dependency.

I have not tried Ladybug. Ship it with Devel?

I think that devel should provide only the plugin management, plugins that works with libraries which are present in core and a default implementation, all other implementation should live in contrib modules (including ChromePhp and FirePhp ). This means that we should decide what implementation ship with devel (kint? VarDumper?) and drop all other implementations.

#52.1 added DevelDumperPluginManagerInterface
#52.2 done
#52.3 fixed
#52.4 reverted to the current dd() solution
#52.5 introduced the method ::exportAsRenderable() for plugins. Now plugins are able to return render arrays and this could bring great benefits as: use libraries, provide cache metadata, easily provide better output using the render system etc . The method is not used currently but IMHO in the long run this method could be used as default.
#52.6 removed the suffix for all plugins
#52.7 left as is for now
#52.8 left as is for now

also improved a little the tests

@all can you apply the patch and play a little with the dumper system to verify that it works as expected and that we have not lost any of the features currently offered by devel

Great work willzyx!
The patch works and for me it's ok. I've refactor a bit the tests just for readability (small tests with less assertions).

I'm not sure if it's worth to split the assertions into so many different test methods, because each test* methods in WebTestBase require a re-install of Drupal and consequently the global execution of tests is more slow...but this should not be a big problem

If you are ok I'm going to commit the patch in a couple of days

Yes you are right. #54 is just for readability so feel free to commit #53 if you prefer.

Thanks

@lussoluca reverted tests to #53

- Added an hook_update_n() for set kint as default dumper if kint module is available. In this way developer that currently use kint do not have to reconfigure devel.
- Improved a little the dumpers plugin description and the settings form description

Opened the following followup
#2703343: Remove usage of kprint_r() and kdevel_print_object()
#2703345: Improve labels/descriptions for dumper plugins
#2703347: Decide which dumper plugins ship with devel

Committed and pushed on 8.x. Thanks to all those who have worked on this issue