Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Follow-up to updating the Krumo library #135

Open
jenlampton opened this issue Jan 24, 2022 · 8 comments · May be fixed by #153
Open

Follow-up to updating the Krumo library #135

jenlampton opened this issue Jan 24, 2022 · 8 comments · May be fixed by #153

Comments

@jenlampton
Copy link
Member

jenlampton commented Jan 24, 2022

This is a follow-up to #102

There's obviously more work to do here, like deciding what removed customisations are still needed (and working out how to add them back in, preferably without hacking Krumo again).

From that issue (thank you @BWPanda)

It seems Devel has hacked Krumo and changed a bunch of stuff in Krumo's files - including adding the addCssJs() function.

Here's a diff I made of Krumo v0.2.1a downloaded from their GitHub repo, and Devel's copy (diff -uprwiNEB --ignore-file-name-case krumo-0.2.1a/ krumo/): krumo.diff.txt

@ghost
Copy link

ghost commented Jan 24, 2022

One of the main differences I've noticed between this new, clean version and the old, hacked version is the calling function listed at the bottom of the Krumo output.

My limited testing shows it always seems to say Devel is the caller. One of the hacks we had before was to filter the backtrace by skipping functions with 'devel' in the name, obviously for this reason.

Not sure how to address this now. Maybe a feature request to Krumo that adds the ability to specify additional backtrace skips...? (it already skips 'krumo' functions).

@jenlampton
Copy link
Member Author

Not sure how to address this now. Maybe a feature request to Krumo that adds the ability to specify additional backtrace skips...? (it already skips 'krumo' functions).

That sounds like a great idea. Maybe someone else has opened a similar issue in their queue before?

@docwilmot
Copy link
Member

I tried the latest dev and did a dpm() on a views row $fields variable and keep getting Error: Maximum function nesting level of '256' reached, aborting! in is_callable() (line 1109 of modules\devel\lib\krumo\class.krumo.php), which doesnt happen if I revert to previous version

@argiepiano
Copy link
Contributor

I can confirm this problem dumping a view structure. To reproduce:

$v = views_get_view('node_admin_content');
$v->set_display('page');
$v->execute();
dpm($v);

You get a 500 error in the browser's console, and the nesting error posted above by @docwilmot.

@docwilmot
Copy link
Member

Got the same nesting error yesterday on a FormAPI $variables array. I wonder if this is a more general issue than just Views.

It's much much faster though.

@argiepiano
Copy link
Contributor

Agreed. Perhaps some of the krumo customizations that the upgrade removed were necessary. I'm wondering if it may be worth looking at the current D7 version, which is fast and works well with views and other complex structures.

@jenlampton
Copy link
Member Author

I would love some help with this. Last I looked at the Drupal 7 version it was still using the older customized version of the Krumo library. I'm going to roll back this change until we can resolve some of these issues, and make a new release with the other fixes first.

@jenlampton
Copy link
Member Author

jenlampton commented Apr 21, 2023

Okay, here is a branch with the krumo update and the linux line-endings fix @quicksketch added: #153

We should keep working on this here and can merge once enough people feel it's ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants