User tests: Successful: Unsuccessful:
Pull Request for Issues #39403 and #39341 and #41045 .
It enough to have a dummy seesion for debug plugin.
Apply patch and follow #39403 or #41045 .
Error
No error
Category | ⇒ | Front End Plugins |
Status | New | ⇒ | Pending |
Yes, sounds like it.
I have tested this item
I have tested this item
Tested successfully on 4.3.0-dev / PHP 8.1.13
Without the patch i had this warning/error in php log:
[Fri Dec 30 11:55:37.275367 2022] [fcgid:warn] [pid 2628] [client 192.168.160.1:50011] mod_fcgid: stderr: PHP Warning: session_write_close(): Failed to write session data using user defined save handler. (session.save_path: /var/www/clients/client2/web19/tmp) in /var/www/clients/client2/web19/web/joomla-cms/libraries/vendor/joomla/session/src/Storage/NativeStorage.php on line 114, referer: http://bug4-3.test/joomla-cms/administrator/index.php?option=com_cpanel&view=cpanel&dashboard=system
Status | Pending | ⇒ | Ready to Commit |
Labels |
Added:
?
|
RTC
This completely breaks request tracking.
This completely breaks request tracking.
What exactly? please be more specific. I tested and it works for me.
I don't get it either. I've tested this PR, too, and when the PR is applied and I enable the "Track Request History" option, I see the request history when using the "folder" icon at the top right of the debug bar.
Labels |
Added:
?
|
I've restored the previous human test results in the issue tracker since the commit which invalidated the tests just was the removal of use statements for classes of which I've checked by review that they are not used anymore.
If the issue is that we’re storing too much data in the session the correct fix is to reduce the size of the session not to use a dummy driver for all data
If the issue is that we’re storing too much data in the session the correct fix is to reduce the size of the session not to use a dummy driver for all data
@wilsonge @Fedik I don’t think the issue was the size of the data as such because the issue happened when tracking of session metadata was switched off and not when it was switched on. The problem was that the data was still collected in the session when it was switched off. Or am I missing something?
The problem was that the data was still collected in the session when it was switched off. Or am I missing something?
Yes, that correct.
the correct fix is to reduce the size of the session not to use a dummy driver for all data
I would not bother with it. It does not realy need to store anything in session, in our use case.
Yes, that correct
That's not correct. It has nothing to do with Joomla session meta data. Our debug plugin only renders collected debug data when the request return a html document. For other types of requests such as Ajax request or request return Json data ...., the collected data will be stored in session and it will only be displayed for next Html request.
For example, when you go to System page, there are several ajax requests on that page, but the collected debug data is not rendered on that page, it is collected, stored in session and will only be rendered on the next request.
From that System page, you go to Articles page and look at debug area, you will see the request history in the dropdown like in the screenshot)
If you replace Joomla session with Dummy Session like in this PR, that stacked debug data feature will be lost, so it does not look like a right solution to me.
Not really understand how the debug plugin really works yet (first time looks at it's code :D ), however, I think one solution would be remove session from this array https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/system/debug/src/DataCollector/RequestDataCollector.php#L33 . We have our own SessionCollector already https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/system/debug/src/DataCollector/SessionCollector.php#L25 , so collecting data here again is useless, causes much more data need to be stored in session than needed (one of the reason causing the issue which we are trying to solve here)
Status | Ready to Commit | ⇒ | Pending |
Back to pending due to previous comments.
the collected data will be stored in session and it will only be displayed for next Html request.
the same but in other words
If you replace Joomla session with Dummy Session like in this PR, that stacked debug data feature will be lost, so it does not look like a right solution to me.
It does not matter when purpose of track request_history
is to disable tracking.
If you want to track the data you should set it track_request_history
On.
I see zero problem here.
I see zero problem here.
The problem is that you could not see the the collected debug data of ajax requests (which are pushed to session and displayed on next page without this change) without track_request_history
enabled. So a kind of loosing feature here. Not sure if there are something else loosing with this change. I haven't really used the debug plugin myself, so I'm unsure if it is really an issue.
Also, I think we should still look at the option I mentioned earlier, doing that solve the reported issue for me.
Remove session from this array https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/system/debug/src/DataCollector/RequestDataCollector.php#L33 . We have our own SessionCollector already https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/system/debug/src/DataCollector/SessionCollector.php#L25
The problem is that you could not see the the collected debug data of ajax requests
This is the whole point of setting track_request_history
to Off, to disable any tracking
When User need this data, then he/she can freely set it to On, it not something forbidden or anything scary or danger.
Also, I think we should still look at the option I mentioned earlier, doing that solve the reported issue for me.
Yeap, that can be addittion, and probably separately.
This is the whole point of setting track_request_history to Off, to disable any tracking
Not really the same. When we enable track_request_history, it will log every requests. When it is off and without dummy session as proposed in this PR, the none html requests will still be logged for debugging purpose, but just just temporarily until it is rendered. So still a feature lost here, just unsure if it is important or not. Someone would have to decide :D
Labels |
Removed:
?
|
Remove session from this array https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/system/debug/src/DataCollector/RequestDataCollector.php#L33 . We have our own SessionCollector already https://github.com/joomla/joomla-cms/blob/4.2-dev/plugins/system/debug/src/DataCollector/SessionCollector.php#L25
Turns out a Session in Request collector and in Session collector diferent things. "Session collector" actualy only "joomla session".
I have replaced joomla session in regular session.
But I will keep dummy session, I see no point to track the data even when track_request_history
is off.
But I will keep dummy session, I see no point to track the data even when track_request_history is off
By using dummy session, we loose stacked data feature which we implemented in debug plugin, that stacked data feature is not the same with track request history.
For me, the change which you made to plugins/system/debug/src/DataCollector/RequestDataCollector.php alone already solve the reported issue, so I would go with that for now.
But if it's just me, that's fine. Would love to hear feedback from others about this.
confirmed the issue - very annoying
not marking as a successful test as I dont understand the code but it does appear to resolve the reported issue
to be honest the stack data only being displayed on the system page when you go to a different page makes that pretty useless (i didnt know it existed until reading the comments here). Far more important not to get the out of memory issues
@brianteeman Could you please only apply change in the file plugins/system/debug/src/DataCollector/RequestDataCollector.php from this PR ? For me, that change alone solves the issue and also doesn't loose the said feature, so I think it is better.
@joomdonation If I just make that change then I still get
@brianteeman Thanks. For whatever reason, it works well for me. I don't want to block the fix, so please report your test result.
For whatever reason, it works well for me.
Try enable Loging, Deprecation, and Query loggin, profile, trace
Well it doesn't work as it results in out of memory issues
It will work only when you set track_request_history
"On", in Debug plugin parameters.
This pull request has been automatically rebased to 4.3-dev.
I suggest set it back to RTC and merge
Labels |
Added:
bug
PR-4.3-dev
?
Removed: ? |
I have tested this item
Solves also issue #41045 .
@viocassel @MacJoom Could one of you two do a quick re-test if this PR still works? @joomdonation , do you still have any objections ?
I have tested this item
Tested with PHP 8.1 - Debug On - Display Errors On - Max Error Report - Installed Sample Blog Data
Status | Pending | ⇒ | Ready to Commit |
RTC
Labels |
Added:
?
|
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-06-26 14:51:32 |
Closed_By | ⇒ | obuisard |
@Fedik This should also solve the other issue #39341 , right?