? bug PR-4.3-dev ? Pending

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
30 Dec 2022

Pull Request for Issues #39403 and #39341 and #41045 .

Summary of Changes

It enough to have a dummy seesion for debug plugin.

Testing Instructions

Apply patch and follow #39403 or #41045 .

Actual result BEFORE applying this Pull Request

Error

Expected result AFTER applying this Pull Request

No error

avatar joomla-cms-bot joomla-cms-bot - change - 30 Dec 2022
Category Front End Plugins
avatar Fedik Fedik - open - 30 Dec 2022
avatar Fedik Fedik - change - 30 Dec 2022
Status New Pending
avatar richard67
richard67 - comment - 30 Dec 2022

@Fedik This should also solve the other issue #39341 , right?

avatar Fedik
Fedik - comment - 30 Dec 2022

Yes, sounds like it.

avatar richard67 richard67 - change - 30 Dec 2022
The description was changed
avatar richard67 richard67 - edited - 30 Dec 2022
avatar richard67
richard67 - comment - 30 Dec 2022

@Fedik Ok, I was so free to add that to the description.

avatar viocassel viocassel - test_item - 30 Dec 2022 - Tested successfully
avatar viocassel
viocassel - comment - 30 Dec 2022

I have tested this item successfully on 678b799

?


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

avatar MacJoom MacJoom - test_item - 30 Dec 2022 - Tested successfully
avatar MacJoom
MacJoom - comment - 30 Dec 2022

I have tested this item successfully on 678b799

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


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

avatar richard67 richard67 - change - 30 Dec 2022
Status Pending Ready to Commit
Labels Added: ?
avatar richard67
richard67 - comment - 30 Dec 2022

RTC


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

avatar SharkyKZ
SharkyKZ - comment - 30 Dec 2022

This completely breaks request tracking.

avatar Fedik
Fedik - comment - 30 Dec 2022

This completely breaks request tracking.

What exactly? please be more specific. I tested and it works for me.

avatar richard67
richard67 - comment - 30 Dec 2022

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.

avatar Fedik Fedik - change - 30 Dec 2022
Labels Added: ?
avatar richard67 richard67 - alter_testresult - 30 Dec 2022 - viocassel: Tested successfully
avatar richard67 richard67 - alter_testresult - 30 Dec 2022 - MacJoom: Tested successfully
avatar richard67
richard67 - comment - 30 Dec 2022

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.

avatar wilsonge
wilsonge - comment - 30 Dec 2022

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

avatar richard67
richard67 - comment - 30 Dec 2022

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?

avatar Fedik
Fedik - comment - 31 Dec 2022

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.

avatar joomdonation
joomdonation - comment - 31 Dec 2022

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)

stackeddata

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)

avatar richard67 richard67 - change - 31 Dec 2022
Status Ready to Commit Pending
avatar richard67
richard67 - comment - 31 Dec 2022

Back to pending due to previous comments.


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

avatar Fedik
Fedik - comment - 31 Dec 2022

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.

avatar joomdonation
joomdonation - comment - 31 Dec 2022

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

avatar Fedik
Fedik - comment - 31 Dec 2022

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.

avatar joomdonation
joomdonation - comment - 31 Dec 2022

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

avatar Fedik Fedik - change - 31 Dec 2022
Labels Removed: ?
avatar Fedik
Fedik - comment - 31 Dec 2022

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.

avatar joomdonation
joomdonation - comment - 8 Jan 2023

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.

avatar brianteeman
brianteeman - comment - 21 Apr 2023

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

avatar brianteeman
brianteeman - comment - 21 Apr 2023

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

avatar Fedik
Fedik - comment - 21 Apr 2023

It mainly for advanced users, and it is relatively new feuture #37465.
However whoever added track_request_history parameter wanted it to be explicitly disabled. So I do not really understand all arguments here, against the fix :)

avatar brianteeman
brianteeman - comment - 21 Apr 2023

It mainly for advanced users, and it is relatively new feuture #37465.

Well it doesn't work as it results in out of memory issues

avatar joomdonation
joomdonation - comment - 21 Apr 2023

@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.

avatar brianteeman
brianteeman - comment - 21 Apr 2023

@joomdonation If I just make that change then I still get
image

image

avatar joomdonation
joomdonation - comment - 21 Apr 2023

@brianteeman Thanks. For whatever reason, it works well for me. I don't want to block the fix, so please report your test result.

debug

avatar Fedik
Fedik - comment - 21 Apr 2023

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.

avatar HLeithner
HLeithner - comment - 2 May 2023

This pull request has been automatically rebased to 4.3-dev.

avatar Fedik
Fedik - comment - 25 Jun 2023

I suggest set it back to RTC and merge

avatar richard67 richard67 - change - 25 Jun 2023
Labels Added: bug PR-4.3-dev ?
Removed: ?
avatar richard67
richard67 - comment - 25 Jun 2023

Solves also issue #41045 .

avatar richard67 richard67 - test_item - 25 Jun 2023 - Tested successfully
avatar richard67
richard67 - comment - 25 Jun 2023

I have tested this item successfully on a99826e

Solves also issue #41045 .


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

avatar richard67
richard67 - comment - 25 Jun 2023

@viocassel @MacJoom Could one of you two do a quick re-test if this PR still works? @joomdonation , do you still have any objections ?

avatar richard67 richard67 - change - 25 Jun 2023
The description was changed
avatar richard67 richard67 - edited - 25 Jun 2023
avatar MacJoom MacJoom - test_item - 25 Jun 2023 - Tested successfully
avatar MacJoom
MacJoom - comment - 25 Jun 2023

I have tested this item successfully on a99826e

Tested with PHP 8.1 - Debug On - Display Errors On - Max Error Report - Installed Sample Blog Data


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

avatar richard67 richard67 - change - 25 Jun 2023
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 25 Jun 2023

RTC


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

avatar obuisard obuisard - change - 26 Jun 2023
Labels Added: ?
avatar obuisard obuisard - close - 26 Jun 2023
avatar obuisard obuisard - merge - 26 Jun 2023
avatar obuisard obuisard - change - 26 Jun 2023
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
avatar obuisard
obuisard - comment - 26 Jun 2023

Thank you Fedir @Fedik for correcting those long lasting issues!

Add a Comment

Login with GitHub to post a comment