? Success
Pull Request for # 5826

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
27 Jun 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.

avatar mbabker mbabker - open - 27 Jun 2015
avatar mbabker mbabker - change - 27 Jun 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 27 Jun 2015
Labels Added: ?
avatar zero-24 zero-24 - change - 27 Jun 2015
Category Plugins
avatar zero-24 zero-24 - change - 27 Jun 2015
Rel_Number 0 5826
Relation Type Pull Request for
Easy No Yes
avatar zero-24
zero-24 - comment - 27 Jun 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.

avatar zero-24 zero-24 - test_item - 27 Jun 2015 - Tested successfully
avatar mbabker
mbabker - comment - 27 Jun 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.

avatar zero-24
zero-24 - comment - 27 Jun 2015

Thanks.

BTW. Travis want to have a @return tag. :smile:

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

avatar mbabker
mbabker - comment - 27 Jun 2015

Fixed

avatar wilsonge wilsonge - change - 27 Jun 2015
Milestone Added:
avatar jwaisner
jwaisner - comment - 29 Jun 2015

@test

PR works as intended.


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

avatar jwaisner jwaisner - test_item - 29 Jun 2015 - Tested successfully
avatar zero-24 zero-24 - change - 29 Jun 2015
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 29 Jun 2015

RTC Thanks :smile:


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

avatar zero-24 zero-24 - change - 29 Jun 2015
Labels Added: ?
avatar wilsonge
wilsonge - comment - 2 Jul 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?

avatar mbabker
mbabker - comment - 2 Jul 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.

avatar Kubik-Rubik
Kubik-Rubik - comment - 6 Jul 2015

Thank you @mbabker! Merged.

avatar mbabker mbabker - change - 6 Jul 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-07-06 13:20:53
Closed_By mbabker
avatar mbabker mbabker - close - 6 Jul 2015
avatar zero-24 zero-24 - close - 6 Jul 2015
avatar mbabker mbabker - close - 6 Jul 2015
avatar Kubik-Rubik Kubik-Rubik - change - 6 Jul 2015
Milestone Added:
avatar Kubik-Rubik Kubik-Rubik - change - 6 Jul 2015
Milestone Removed:
avatar mbabker mbabker - head_ref_deleted - 6 Jul 2015
avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?
avatar infograf768
infograf768 - comment - 11 Nov 2015

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

Add a Comment

Login with GitHub to post a comment