mithra62 / ee_debug_toolbar

Adds an unobtrusive interface for debugging an ExpressionEngine site
52 stars 10 forks source link

PHP error (warning) in Hookah branch #21

Closed croxton closed 11 years ago

croxton commented 11 years ago

Seeing this at the top of each pop-up panel in the toolbar:

A PHP Error was encountered Severity: Warning Message: Creating default object from empty value Filename: ee_debug_toolbar/ext.ee_debug_toolbar.php Line Number: 390

I'm running PHP 5.3.

ckimrie commented 11 years ago

Are you explicitly enabling error_reporting(E_STRICT) in code or in your php.ini ?

That line in the debug toolbar extension (390) is simply setting a property on the Template library:

        $this->EE->TMPL->debugging = FALSE;

According to that error, either EE, TMPL or debugging properties don't exist. I am leaning towards there being an issue with the EE property. I've noticed that we're not setting it explicitly as a class property. I'll apply a quick patch to do so and see if that fixes it.

johndwells commented 11 years ago

Actually, I'm seeing this with PHP 5.4.4, not 5.3 (5.3.14 to be exact).

Chris, explicitly declaring $EE as a class property didn't fix this error for me - I think it's actually TMPL that doesn't exist at the time the hook is being run... so may be lines 390 - 392 aren't necessary? I haven't studied the code close enough to know, so just guessing...

ckimrie commented 11 years ago

OK, thanks @johndwells

I am just wondering what the variable is here. Both @mithra62 and I are running PHP 5.3.x and haven't encountered that error, even when specifying error_reporting(E_STRICT).

After all, our extension is running right at the end of the CodeIgniter app lifecycle, after the EE CI Controller has finished, so it could be that the packages loaded by EE are being unloaded. Not sure what would cause this, however.

It could be a server configuration. Either of you using APC caching or similar?

croxton commented 11 years ago

This fixes it for me:

    if ( ! class_exists('EE_Template'))
    {
        require APPPATH.'libraries/Template.php';
        $this->EE->TMPL = new EE_Template();
    }

Not sure why it's necessary :/

croxton commented 11 years ago

Not running APC or any opcode cache btw. Using E_STRICT.

croxton commented 11 years ago

Ah, because it an ACT for ajax. Template class is only loaded when EE parses templates. Like John said you should be able to delete 390 - 392

ckimrie commented 11 years ago

No access to the files at this very moment, since I'm out of the office.

Can either of you confirm that it works as expected when lines 390-392 are removed?

johndwells commented 11 years ago

Well spotted Mark. Yep I can confirm that nothing appears to break when those lines are removed.

But... how does it work as an ACT for the current page that's being profiled, wouldn't that be a different request altogether?

On 21 March 2013 16:04, Christopher Imrie notifications@github.com wrote:

No access to the files at this very moment, since I'm out of the office.

Can either of you confirm that it works as expected when lines 390-392 are removed?

— Reply to this email directly or view it on GitHubhttps://github.com/mithra62/ee_debug_toolbar/issues/21#issuecomment-15247652 .

Pura Vida John D Wells http://johndwells.com

ckimrie commented 11 years ago

When your extension is called to create the panel, you are on the same request as the user. The toolbar takes your panel and caches the contents on disk. When you click a toolbar button, JS fetches the cached content (via an ACT) and inserts it into the panel.

This keeps the panel really lightweight when you're not interacting with it, preventing increased page load times.

mithra62 commented 11 years ago

This should be fixed now. Can any of you guys confirm the error is gone?

johndwells commented 11 years ago

Yep, that fixed 'er up!

Very clever btw with the whole ACT approach - it means a lot that you guys are placing effort on keeping things lightweight. Thanks.

On 21 March 2013 19:24, Eric Lamb notifications@github.com wrote:

This should be fixed now. Can any of you guys confirm the error is gone?

— Reply to this email directly or view it on GitHubhttps://github.com/mithra62/ee_debug_toolbar/issues/21#issuecomment-15259933 .

Pura Vida John D Wells http://johndwells.com

ckimrie commented 11 years ago

Thanks Jon!

We identified early on that the default method used to display debug data was causing excessive load times since the browser had to parse 5000+ lines or so of extra content, most of which was not even seen on most page loads.

Keep in mind, that you can completely override this default behaviour though! Good example is the bundled Memory History extension (which has just been updated with some new features). It moves away from the 'panel' paradigm and shows up as a persistent chart of sql, execution time and memory usage for the past 20 pages displayed in the top right of the page. It chooses to be loaded immediately on the page instead of being initialised on a panel click and also uses eedt.js library (which any panel can use) to load the Google Chart API and also communicate with the server to fetch data. Check it out!

For now I'll mark this as closed though. Thanks!