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

System - Debug relies on __destruct which is never triggered (Fix #5826) #7279

Closed
wants to merge 1 commit into from
Closed

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jun 27, 2015

This PR addresses #5826 by changing the method that renders the system profiler data to listen to the onAfterRespond event, the absolute last thing that Joomla does before PHP should start shutting down the application.

Testing Instructions

Make sure the profile data still renders correctly.

@zero-24
Copy link
Contributor

zero-24 commented Jun 27, 2015

hmm after applying the patch i can't see any issues with the debug plugin output.

the absolute last thing that Joomla does before PHP should start shutting down the application.

hmm not sure if/how we can test this.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7279.

@mbabker
Copy link
Contributor Author

mbabker commented Jun 27, 2015

Just what you did. Instead of the debug data being rendered while PHP is going through the shutdown process (this is when __destruct() magic methods are called), it is instead rendered by a proper system event before Joomla technically finishes executing. For an advanced test case, I'd suggest trying out what the original issue reporter was doing with the use of PHP's register_shutdown_function(). I don't though see anything in PHP's documentation about the behavior he ran into. Either way, this can at best be considered a good practice.

@zero-24
Copy link
Contributor

zero-24 commented Jun 27, 2015

Thanks.

BTW. Travis want to have a @return tag. 😄

FILE: /home/travis/build/joomla/joomla-cms/plugins/system/debug/debug.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 187 | ERROR | Missing @return tag in function comment
--------------------------------------------------------------------------------
UPGRADE TO PHP_CODESNIFFER 2.0 TO FIX ERRORS AUTOMATICALLY
--------------------------------------------------------------------------------

How to test the advanced test case

@mbabker
Copy link
Contributor Author

mbabker commented Jun 27, 2015

Fixed

@wilsonge wilsonge added this to the Joomla! 3.5.0 milestone Jun 27, 2015
@jwaisner
Copy link
Member

@test

PR works as intended.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7279.

@zero-24
Copy link
Contributor

zero-24 commented Jun 29, 2015

RTC Thanks 😄


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/7279.

@zero-24 zero-24 added the RTC This Pull Request is Ready To Commit label Jun 29, 2015
@wilsonge
Copy link
Contributor

wilsonge commented Jul 2, 2015

So the thing with this is now the debug plugin wants to load first to ensure that all the loggers are primed. But also last when dumping data out to ensure the timers in onAfterRespond are as accurate as possible (and db queries aren't missing etc). I'm not sure if there's some sort of workaround for this or it's just something we're going to have to sucker up and document appropriately.

I mean what's the ideal even? The plugin being first in the list so the loggers are active?

@mbabker
Copy link
Contributor Author

mbabker commented Jul 2, 2015

The actual fix in this PR is that the plugin renders the debug console while PHP shuts down and if you're using custom code to have a different shutdown operation, then the destructor does not get triggered. Since plugins are prioritized by class and not by event (another inherent weakness in JEvent), there is no way to re-prioritize the execution orders.

onAfterRespond is literally the last thing JApplicationCms::execute() does. So the only thing that could possibly come after this is another plugin also listening to that event. The only change that changes is if that plugin somehow manipulates the application profiler (adds a mark for example), that won't be included in the console's output any longer.

A proper long term fix is going to mean refactoring the event system to properly subscribe plugins to events with method level prioritization, not just class level.

@Kubik-Rubik
Copy link
Member

Thank you @mbabker! Merged.

@mbabker mbabker closed this in 7ed4bb9 Jul 6, 2015
@mbabker mbabker deleted the fix5826 branch July 6, 2015 14:04
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
@infograf768
Copy link
Member

@mbabker
This has broken some aspects of debug.
See #8362

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

Successfully merging this pull request may close these issues.

7 participants