Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikeytown2’s picture

Status: Active » Needs review
FileSize
2.14 KB

Status: Needs review » Needs work

The last submitted patch, 1: devel-2376849-1-fix-krumo-D7.patch, failed testing.

mikeytown2’s picture

Status: Needs work » Needs review
FileSize
4.99 KB

Patch that doesn't ignore whitespace changes....

mikeytown2’s picture

Might as well fix all the tabs...

The last submitted patch, 3: devel-2376849-3-fix-krumo-D7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: devel-2376849-4-fix-krumo-D7.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: devel-2376849-4-fix-krumo-D7.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: devel-2376849-4-fix-krumo-D7.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: devel-2376849-4-fix-krumo-D7.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: devel-2376849-4-fix-krumo-D7.patch, failed testing.

Status: Needs work » Needs review
Albert Volkman’s picture

Status: Needs review » Reviewed & tested by the community

Works well for me.

travelertt’s picture

Works for me as well.

dasginganinja’s picture

Thanks. I used this fix on 7.x-1.5 to restore the Krumo is not defined and the krumo.over errors associated with a dsm() call.

salvis’s picture

Status: Reviewed & tested by the community » Needs review

I wish we could get the testbot to give a result. I'm unable to apply the patch to 7.x-1.x-dev...

Please explain in a few words where this bug comes from and why we suddenly need this patch.

salvis’s picture

Might as well fix all the tabs...

Krumo doesn't follow the Drupal coding conventions in many ways, because it used to live as a separate project. We wanted to keep our version as close to the original one.

This still holds and we want to keep maintenance of Krumo at an absolute minimum. Please do not introduce any unnecessary diffs. AFAICS (at least in #1 you've changed only three lines and you shouldn't have more diffs than that.

Here's a re-roll of #1 — since I can't apply #4 I can't tell whether you've changed more than those three lines...

The question is still open: what are we fixing here? How can we reproduce the issue? Does it take a specific PHP version?

jcnventura’s picture

I'm not sure why this is needed, but indeed it works.. Krumo simply doesn't expand without this patch.

Albert Volkman’s picture

Status: Needs review » Reviewed & tested by the community
mxr576’s picture

rtbc++
#21 patch worked for me too.

osopolar’s picture

Patch from #21 works for me too, thanks.

salvis’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

I won't commit this as long as I cannot at least reproduce the error that it's supposed to fix.

Albert Volkman’s picture

I just figured out what was causing the issue in my case. If you have AdvAgg installed, navigate here-

admin/config/development/performance/advagg

Selecting anything but "Move JS to the footer" > "All (might break things)" resolved the issue. Perhaps there's an issue with how the JS is added?

mxr576’s picture

Blaming Advagg also makes sense to me too. I had this problem only on those sites where advagg and "Move JS to the footer" is enabled.

mikeytown2’s picture

Project: Devel » Advanced CSS/JS Aggregation
Version: 7.x-1.x-dev » 7.x-2.x-dev
Component: krumo » Modifier
Status: Postponed (maintainer needs more info) » Active

I'll take another look at this in regards to making this work when the JS is in the footer and/or defered. Sometimes this can't be fixed in AdvAgg and it must be fixed in the JS file.

hey_germano’s picture

I also needed this patch to get Krumo to expand when using the dev release of Devel - the one in #21 worked for me until I enabled JS aggregation this afternoon. We're just using Drupal core aggregation though, not AdvAgg.

With JS aggregation enabled, I started getting the "TypeError: (intermediate value)(...) is not a function" error and traced it back to krumo.js. The fix was to add semicolons after all the functions in there.

This is a better explanation of why that was needed than I could give - http://stackoverflow.com/a/1834674/1940172 - but the gist is that while you don't need the semicolons in a JS file itself, you might need them when combining files so that functions are explicitly terminated and not accidentally combined.

Anyways, here's another patch that includes the changes in #21, plus adding semicolons after each function in krumo.js.

mikeytown2’s picture

AdvAgg will auto add semicolons to the aggregated files when combining, which is why it wasn't needed in the patch.

@hey_germano
What happens if you just add the semicolons to the krumo.js file? Will it work in your case?

  • mikeytown2 committed afc481f on 7.x-2.x
    Issue #2376849 by mikeytown2: ReferenceError: krumo is not defined
    
mikeytown2’s picture

Status: Active » Fixed
FileSize
840 bytes

The issue can be fixed in AdvAgg. The setting that triggers this issue is the "Put a wrapper around inline JS if it was added in the content section incorrectly" checkbox. The following patch has been committed and should make devel work without a devel patch.

Albert Volkman’s picture

Works for me. Thanks!!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Ievgen Antonovych’s picture

Update changes for devel 7.1.7

Ievgen Antonovych’s picture

Project: Advanced CSS/JS Aggregation » Devel
Version: 7.x-2.x-dev » 7.x-1.x-dev
Component: Modifier » devel