Replacement for Krumo?

code will follow *another day*. I really want feedback on the idea first.

Cool!
PSR-0 or sth PHP 5.2 compatible?

I made a sandbox where we can play around, before this goes into devel. You have access.
I think this is easier than patches on devel itself.

The working title is "Krumo NG" / "krumong".
Currently the two interesting commands are
krumong()->dpm($data);
krumong()->consoleLog($data);

For this phase the main thing was to build a solid and nice architecture for the tree traversal and rendering, to get rid of the mess. I think this is going quite well.

Still to do:
- agree on new identifiers for everything
- detect the calling function from backtrace
- permission management
- behavior on error pages, where drupal_add_js / css does not work.
- behavior if Drupal displays a dpm() from a previous request, and does not have our css and js included.
- add more useful functions, to be as feature-complete as krumo.
- merge into devel.

Ehm, a link would be useful :)
http://drupal.org/sandbox/donquixote/1853562

I had a similar idea.
Not to totally let go of drupal_set_message(), but to keep the data in a temporary storage (session), and then do the drupal_set_message() in the next request.

The good thing with drupal_set_message() is that it is supported in almost all themes. If we find another way to sneak the message into the page (e.g. preprocess page), I am happy to drop it.

New stuff in the sandbox:
- Detect calling function.
- breadth-first recursion check, instead of depth-first.
- recursion shows the tree position of the duplicate.

Breadth-first recursion check:
E.g.

$referenced = array('foo');
$root = array();
$root['a']['b'] =& $referenced;
$root['x'] =& $referenced;
$root['y'] =& $referenced;

Depth-first (as in krumo) would give us (pseudo-yaml)

a:
  b: ['foo']
x: Recursion({trail of keys: ['a', 'b']})
y: Recursion({trail of keys: ['a', 'b']})

Breadth-first gives us (pseudo-yaml)

a:
  b: Recursion({trail of keys: ['x']})
x: ['foo']
y: Recursion({trail of keys: ['x']})

New stuff in sandbox:
http://drupalcode.org/sandbox/donquixote/1853562.git/commit/930b85fdf830...
tag: 7.x-1.x-messages-in-session

Now messages are kept in the session, and fired on hook_process_status_messages().

function krumong_process_status_messages($vars) {
  krumong()->flushMessages();
}

The system also shows a separator to indicate which dpm() are from other requests, showing $_GET['q'] and timestamp.
It uses drupal_render() with #attached for the css/js.

TODO:
Factor the messages logic into a separate class/object.

And.. cleaned up, refactored.

Done:
- detect the calling function from backtrace
- behavior if Drupal displays a dpm() from a previous request, and does not have our css and js included.

Still to do:
- agree on new identifiers for everything
- permission management
- behavior on error pages, where drupal_add_js / css does not work.
- add more useful functions, to be as feature-complete as krumo.
- merge into devel.

I think it's time for some feedback..

xautoload:
A solution I use in Crumbs is to provide a built-in class loader, and let it do nothing if xautoload is present.
Crumbs uses a PHP 5.2 compatibility pattern that is unique to xautoload.

As we rather go full PSR-0, we have choice between xautoload and classloader module.
xautoload is my baby, so I am kinda biased.
I'd say we add none of these as dependency, and just ship our own built-in loader, which can be quite trivial to implement.

// Not sure if that's the correct place for it, or if we launch this from whenever we need it.
function devel_boot() {
  if (!module_exists('xautoload') && module_exists('classloader')) {
    spl_autoload_register('_devel_autoload');
  }
}

function _devel_autoload($class) {
  if (substr($class, 0, strlen('Drupal\devel\')) === 'Drupal\devel\') {
    ...
  }
}

MessageSystem: (#11 point 9.)
That was following the idea from #6.
Personally I'm ok with always including the js/css. Although, maybe on a test site you want to show *some* messages to anonymous, without changing permissions. E.g. by not using dpm() but an equivalent that goes without permission check.

#13:
Using an existing class loader system, if one is available, has (minor) performance benefits.
Otherwise, if every module ships with its own solution, PHP has to loop through all of them for each class. Drupal 8 / classloader kinda have the same loop over all namespaces, just within one class. xautoload does an array lookup instead. And any of these put a cache layer on top, which avoids the loop.
Based on some arbitrary made-up number of classes and namespaces, I came to 40ms difference of loop vs array lookup.

I think the conditional registration with module_exists() is a decent "best practice" for existing D7 modules which want to avoid dependencies.

#11
1. "number of characters in a string"
fine to remove it. for the start, I wanted to be feature-complete as possible, but now that is proven, we can drop stuff we don't like.

2. FirePHP vs consoleLog
I never really looked at FirePHP. Will check.

3. "Your browser doesn't have one? C'mon..."
That message was for the "proof of concept" / demo purpose.
I wonder if we should put this javascript into the messages area at all, or rather do sth else (or use FirePHP)
Btw, even if we use drupal_add_js() with inline option, we could use the MessageSystem to delay it to the next request. So, another use for the MessageSystem (but maybe then we can rename it RequestDelaySystem)

4. Adding a label
yep.

5. permissions
yes, we can use those already existing in devel.

6. views and recursion.
Yep, it catches the recursion just fine.
Something I notice, though, is that strings are not sanitized. You get to see the html. Bad!
Maybe we should do this on js level.
At least the js DOM building will fix any broken html..

7. Tests
yes.
And a lot of that can be unit tests! And we can mock the MessageSystem - which is not possible with drupal_set_message().
And again, xautoload will help, as it can detect PSR-0 tests (and if it is disabled, we don't care, then people just don't see the tests).

8. xautoload
see previous post.

9. MessageSystem
I already plan on adding a HtmlPrintSystem for the pages that are so broken that they don't have a regular header. So on first call we print the css and js, and further calls just print the data.
Also, we can mock this thing for unit testing!! Not easily possible with drupal_set_message().

#16
"we can mock the MessageSystem"
hm, maybe not such a big gain, as all we test by this is the Main class. The rest is already unit-testable.

Related:
#1855468: Krumo side effect: Object vars become references

#20
I was too lazy to count. Should be

if (substr($class, 0, 13) === 'Drupal\devel\')

I would imagine that is faster than strpos(), especially in the majority case where the class is not within devel. I did not really do any benchmarks, so you are free to correct me. But, I'd prefer to postpone this for late optimization :)

#22:
You can git clone the sandbox on a test site and krumong()->dpm(...).
But, I do not guarantee any API stability. Any update might change how it works.

I don't think you should hack devel's dpm() at this stage. Better wait until krumong is finished.

@moshe, salvis:
I have been thinking about alternatives to drupal_set_message().

drupal_set_message() has a few flaws:
- messages in hook_preprocess_page() are delayed to the next page.
- you don't see easily which message was generated in which request. E.g. you might see one message from a hook_preprocess_page() show up above a message from hook_init(), and be totally confused, if you don't realize the first one is from the previous request. The same is for messages from ajax requests (e.g. the one that fetches the admin_menu xml)
- it can blow up the page html
- you can't attach js and css to a drupal_set_message(). drupal_add_js() won't help, if the message is shown on the next page.

----------

So, here is one alternative I came up with.
- each request has a (pseudo)unique identifier, e.g. a random string (that's as good as a hash in git).
- krumong messages are stored in our own session variable, sorted by request id.
- the message area contains just a basic <div class="krumong-monitor" style="display:none;"></div>, where messages can be appended with javascript.
- message content is fetched with ajax, at krumong/ajax/%krumong_request, where the parameter is the request id.
- the UI lets you browse back/forward between the messages of different requests, and also displays meta information (url, timestamp) for those requests.

Difficulties:
- How do we know if a page needs a krumong-monitor div or not? If we just always print it, won't this have an undesired side effect on page.tpl.php, making it always print a messages div? We don't even know how the messages div will be named in a particular theme..
- When do we know that we can delete a message from session, to free up memory/storage? With drupal_set_message() they are deleted the moment they are printed to the screen..
- Does this work ok on pages or systems that are kinda broken? Do we care at all?

FirePHP vs krumong()->consoleLog():
I had a look at FirePHP. It seems quite a nice tool, but it does cost a few steps to install. You need a browser extension, and you need a PHP library. And then you do it again for Chrome.

I'd say we keep both krumong()->consoleLog() and FirePHP / ChromePHP.
We don't introduce a shortcut (such as dfb) for consoleLog, but we keep it as an option or even a fallback, for people who don't like to or can't install FirePHP / ChromePHP (e.g. if you are surfing on a friend's computer).

#28
This is not necessarily the D8 way, this is my way.
I might still change some of the cache/factory stuff or ditch it altogether, seeing how the rest shapes up.

But if your observation is that I tend to use more (and smaller) classes than others, then you are correct.
And the split of TreeRenderer vs TreeTheme is absolutely intended.

I did a bunch of changes.
(tag: 2012-12-08)
(known issues below)

Narrowed it down to the following methods:

// The following methods on krumong('main') go without access checking!
// To have access checking, use krumong()->... instead.

// Print krumong dump directly to stdOut,
// and also print the necessary js and css inclusions.
// This will work even if no regular Drupal head and body follows.
// E.g. you could call this and then just die().
krumong('main')->kPrint($data, $name = NULL);

// Equivalent to dpm().
// Postpone to the next time that drupal messages are printed.
// It will also take care to include the necessary css and js once that happens.
krumong('main')->kMessage($data, $name = NULL, $type = 'status');

// console.log() statement printed directly to stdOut.
krumong('main')->jPrint($data, $name = NULL);

// console.log() statement queued in $_SESSION.
// To be included with drupal_add_js(.., 'inline') the next time we print status messages.
// This is necessary, because any drupal_add_js() that comes too late in the request would be ignored otherwise.
krumong('main')->jMessage($data, $name = NULL);

// dump the data and return as html.
// This html will only work with the css/js included.
// With the $add_css_js = TRUE, it will include the css/js automatically.
krumong('main')->dump($data, $name = NULL, $add_css_js = TRUE);

// ------------------------

// Same as above, but access-checked.
krumong()->kPrint(..);
krumong()->kMessage(..);
krumong()->jPrint(..);
krumong()->jMessage(..);
krumong()->dump(..);

// Equivalent to original devel functions
krumong('devel')->ddebug_backtrace(..);
krumong('devel')->dpm(..);
...

Known issues / TODO:
- Still want to rethink some identifiers.
- Some classes are indeed overly small. I generally like that, but in this case a few of them might indeed be overkill. I will think about it.
- However, I will not kill the ServiceCache / ServiceFactory. I also don't think we should use the DIC of D8, as it would only make the system more fragile.
- Original devel functions always check whether krumo is enabled and available. I don't do any of this. We should decide if this is still desired.
- I will rethink Moshe's suggestion to always include the css and js. However, this would have to be for all roles, because some krumo is not protected by 'access devel information'.
- More tests needed. And the existing one needs work.

Shortcuts: Yes.
I consider this a step to do when krumong actually becomes part of devel. Until then, I prefer to have clearly distinguishable and transparent names rather than shortcuts.

Permissions:
Imo the "only use devel locally" is a myth. You can expect that on a lot of projects you don't even have the separation of dev/test/alpha/staging vs production. The "access devel information" permission does exist for a reason (and I didn't invent it).

Having two separate objects, one with and one without access checks, is just one way to avoid having user_access() all over the place, and making it more transparent what is access checked and what isn't. And actually it should be faster, because we only call user_access() once on setup, and then never again.
But then, performance of this one call does not really matter. What matters much more is the tree traversal and assembly of the dump output.

Btw, I might change the key for this "no access checks" thing from 'main' to 'public'.
Atm it would be
krumong('mainAccessChecked')->... with access check, and
krumong('main')->... without access check.
krumong('public')->... alias for 'main'.
I just made the first thing the default argument, so you can write
krumong()->...

And for all identifiers I have the policy that I use whatever comes to my mind, until a better idea shows up..

Hi,
I have been quite busy with other projects the recent weeks.
If anyone wants to help me out writing some tests, go ahead!
Otherwise, I'm sure I can pick this up again some time in January.

Another thing that can be done without waiting for me is thinking about a perspective for integration in Devel. There are different alternatives imaginable, so we should evaluate and make a choice.

Due to our D8-first policy, no merging between branches is possible. Too bad.

Here is a preview D7 patch for devel, which uses krumong if available, traditional devel/krumo otherwise.
I set this to 7.x-1.x and "needs review" only to see if testbot has any opinion.
No intent to have this committed.

We should switch back to D8 soon after this.

I don't really know how I should write the tests.
All the components are kinda unit-testable for themselves, but that doesn't tell us much.
To test the entire thing, we would need to execute the javascript and test the result. Is this even possible with Drupal?

Also, I would like to see some progress on PSR-0 tests on D7 (*):
#1693398: Allow PSR-0 test classes to be used in D7 (followups)
#1751540-15: Expose test classes to Simpletest using hook_simpletest_alter()

(*) EDIT:
Or finally make a D8 krumong :)

Another integration patch, this time not intended for testbot, only for people to have a look.
I only "converted" dpm(), to keep the patch nice and small and reviewable.

I don't really know if the function_exists() on every dpm() and friends has a cost that we would have to worry about. I almost think no, so probably #37 is the better choice.

The idea is to have an object that provides the debug methods, and this object is replaceable by another module (in this case, the choice of modules is hardcoded, it can only be krumong)

#1891908: Write tests issue on krumong

#42, Yes it does :)

So, here is the thing:
I personally care very little about how exactly the new krumong stuff is to be integrated into Devel.
My personal concern is mostly what is the easiest for me to maintain and work on.
For the integration, I will mostly accept whatever the maintainers of Devel consider the best.

For the time being, it is easier for me to continue working on KrumoNG as a separate module, where I don't have to wait for patches being reviewed etc, where I can commit stuff without having to fear it will break Devel, and where we have a dedicated issue queue.

Proposed plan:
- I make KrumoNG a full project.
- We agree on some integration code that lets Devel use KrumoNG as a separate module, such as #37, and "freeze" the public API functions in krumong that can be used by devel. I would like to get some more feedback here. Or, not just feedback, but a concrete proposal.
- We / I continue to work on KrumoNG. This does also include a D8 release for KrumoNG, and the writing of unit and web tests.
- Discussion about specific features happen in the KrumoNG issue queue.
- After some time we can reconsider whether we want to package krumong with devel.

Does this sound ok?

The integration code in #37 replaces a lot of Devel functionality, e.g. it replaces just about the entire ddebug_backtrace(). Why? Shouldn't replacing krumo_ob() be sufficient?

I suppose you are right :)
I think it was mostly for proof-of-concept and checking completeness, that I reproduced all the major devel stuff in Drupal\krumong\Devel.

The important thing is that krumong() does more than just return some html. It delays messages to the next request, and it controls the timing js and css inclusion. It knows whether you are on a broken page where js and css need to be printed directly to stdout, or on a regular Drupal page where Drupal takes care of that.

I would prefer some sort of plug-in architecture or even just a hook that does not hard-wire us to a specific module name like 'krumong' but instead allows any ambitious module to take over the role of the built-in krumo.

I am not opposed to that.
Maybe those modules can register their plugins, and then the user can choose.

It could be like in #40, where at some point in the request, we register an object (handler/plugin) that can handle those things.

We might even swap out this object later in the request.
E.g. someone calls dpm() at a time when krumong is not available yet, so we use krumo or whatever low-level built-in thingie. Then some time later in hook_boot() we replace the handler.
I'm not sure if this is ever necessary, probably not..

Is there any benefit in adding a configuration mechanism?

Not that I can think of.
The current devel gives the user a choice about using krumo or not. I don't know the motivation for this choice.

If krumong needs additional information beyond the $object being passed to krumo_ob(), then we surely want to provide that information.

It is more about the return value.
krumo_ob() is a function that does something and then returns a string. What you do with this string, is your choice entirely. It is your responsibility to delay the output to the next request via drupal_set_message(), and to add the krumo css when it is output.

krumong methods can work like this, but the output won't do anything without the css and js. So you would need to call krumong again to tell it to include the css and js.
(yes, Moshe suggested that we simply always include this css and js - but this has some limitations)

Instead, the intended way to use krumong is to let it handle all this stuff. So krumong can replace krumo_ob(), but it is rather designed to replace dpm() and friends, so all the drupal_set_message() and drupal_add_js() happens outside of Devel.
This may also be the reason why I made a krumong version of ddebug_backtrace() - but I don't remember if this is strictly necessary.

I would very much prefer this to be an all or nothing choice though. Using different krumo handlers during various phases would give us headaches and never-ending discussions about why and which one at which point, and which one is at fault if something goes wrong.

Quick reference: http://api.drupal.org/api/devel/devel.module/function/has_krumo/7

The timing we can and should aim for on a regular site is when both devel.module and/or krumong.module have been included. We should not wait for hook_boot() to fire.

Unfortunately we don't know which module is included first, devel or krumong. Or do we?
http://api.drupal.org/api/devel/devel.install/function/devel_enable/7
Currently devel aims to be the last module to be included.
Can we rely on this?
So, devel could assume that all other modules that provide krumo-like functionality will be already included at this point.

Or, we could wait with this until the first person actually calls dpm() or other krumo stuff.

(For those who need it, we could provide a devel.early.inc, that can be hard-included in settings.php. At this time we would have no access to the database, module_exists(), variable_get() etc, devel cannot determine whether krumong is available or not and where it is installed, so it would need to activate the regular krumo instead. Maybe krumong could solve this by providing a krumong.early.inc itself, so you would need to include both after each other, to get early-bootstrap debugging functions.

It's already too difficult (at least for me) to keep track of when dpm() works and when it doesn't...

I'm curious, what is the current status on this with existing krumo?

has_krumo() is where the drupal_add_js() is done, and this needs to be delegated, too, of course, but I'm not so happy to delegate dpm() and ddebug_backtrace(). There's quite a bit of logic that I'd like to keep within the Devel process.

Which logic is it that you have feelings about?

What about this:

• dpm() and friends all become methods in a class/object within devel.
  The procedural versions of those methods need to obtain a cached instance of the object.
  #40 was a first attempt at this, but I am very open for any suggestions about how to name this class and where to place it.
• Modules like krumong can inherit from this class and override some or all of those methods. So it is still open, how much of dpm() or other things is being replaced, and how much stays within devel.
• (Instead of inheritance, we could also think about something else - e.g. we maintain a number of such objects, and then find out which of them has a certain method. Not really sure I would want this, inheritance seems like the more natural choice.)
• We need an API where modules like krumong can declare they have an alternative implementation for dpm() and stuff.

I can't claim to understand every detail about how dpm() interacts with kprint_r(), merits_krumo(), etc., and you probably can't either.

I dare to disagree :)
I did have a close look at these things, I think I understand those problems, and krumong does indeed solve them.

http://drupalcode.org/sandbox/donquixote/1853562.git/blob/refs/heads/7.x...
This is basically an implementation of Moshe's idea in #6. It may still be changed and refactored, but it works.

The cool stuff:
- With ..\Output\RequestCache::message(), krumong.js and krumong.css do not get included immediately, but when those messages are actually printed, using hook_process_status_messages().
- Likewise, ..\Output\RequestCache::jsInline() can be called during a form submit request, and the inline js will be delayed until the next real request.
- ..\Output\StdOut::html() prints to stdout, and will include the js directly in stdout, instead of drupal_add_js().
- ..\Output\CssJs has a printCssJsOnce() and a addCssJsOnce(), and makes sure it never gets added twice.

Typical problem in Devel:

ddebug_backtrace();
die();

This gives you a page with pretty unreadable html, because krumo.css is missing.

On the other hand, with krumong you get the html with all the css and js.

krumong('devel')->ddebug_backtrace();
die();

there exists a large number of code paths and we cannot know which ones are used to what degree.

The goal is that dpm() and friends behave exactly the same to the outside world as they used to.
And, the functions themselves should all remain within devel module, only the implementation does change.

We introduce tight coupling that makes maintenance and porting much harder.

Where do you see tight coupling?

Deriving from a class is about the tightest coupling you can get.

Probably yes.
Only beaten by procedural code calling each other all over the place :)

I still wonder what practical issues this would have.
Typically what we have to worry about with inheritance is
- any internal change (protected methods etc) in the parent class may break the deriving class.
- attribute values (state) used by parent class methods are also accessible to methods of the deriving class.

So yes, this could be a bad thing.

#54 (myself)

(Instead of inheritance, we could also think about something else - e.g. we maintain a number of such objects, and then find out which of them has a certain method. Not really sure I would want this, inheritance seems like the more natural choice.)

Would this be an option?
E.g. (simplified)

function dpm($data) {
  // Loop through handlers provided by other modules.
  foreach (devel_get_handlers() as $handler) {
    if (method_exists($handler, 'dpm')) {
      return $handler->dpm($data);
    }
  }
  ... // fallback code.
}

(This code should rather live in a class by itself, but for now who cares)

----------

I realize you probably prefer a clear set of functions (in your taste, just krumo_ob()), that a contrib module can replace all-or-nothing. These could be baked into an interface, that the replacement class would have to implement.

I can see the motivation, but atm I would prefer something more flexible, so I can slowly figure out which methods I want to replace.

We already have a porting problem. You develop for D7 but we cannot go forward with D7 and leave D8 behind.

We will get there.
I think a lot of the krumong code will be quite similar in D7 and D8.

Just released a D8 version.
It seems to work, with very little modifications. Although:

• When trying to dpm() a node (this happens in the krumong_example), I get
  Objects returned by Drupal\node\Plugin\Core\Entity\Node::getIterator() must be traversable or implement interface Iterator
  This smells very much like a core bug to me, not a bug in KrumoNG.
• After doing a git pull on my 8.x core, the site explodes, and update.php doesn't work either.
  Doesn't seem to be related to krumong, just makes D8 quite unpleasant as a platform for module development.
  (and this is not the first time I saw this happening)

I plan to continue supporting D8, but I don't care much if issues are reported and fixed in D7 first. With git merge and cherry-pick, there is little reason to insist on a "D8 first" policy.

@mmucklo:
I thought of doing the same thing with krumong.
I am going to "watch" your github repo :)

If you have time for it, it would be great to have an overview of the main differences, and possible shortcomings in either.

What about moving all the dpm(), dpq() etc into a separate file? Then other modules could tell devel to not include this file, and provide their own equivalents for these functions.

It seems generally a polite thing to be able to disable the global namespace-squatting (no offense)

Little problem: This would mean a module can either replace all or none of the functions. Maybe something more fine-grained would be needed?

See #2191395: Make dpm() and friends pluggable (with Krumo, Kint, Ladybug etc)

So far I did not give Kint the attention that it might deserve. When I wrote the KrumoNG stuff, the main incentive was to have something that looks and behaves like Krumo to the user, but sucks less from a developer perspective.

I'm ok with Kint or Ladybug or whatever, but I think we should take this chance to somehow make this pluggable.