Skip to content

Conversation

@Fedik
Copy link
Member

@Fedik Fedik commented Dec 30, 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

@richard67
Copy link
Member

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

@Fedik
Copy link
Member Author

Fedik commented Dec 30, 2022

Yes, sounds like it.

@richard67
Copy link
Member

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

@viocassel
Copy link
Contributor

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.

@MacJoom
Copy link
Contributor

MacJoom commented Dec 30, 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.

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 30, 2022
@SharkyKZ
Copy link
Contributor

This completely breaks request tracking.

@Fedik
Copy link
Member Author

Fedik commented Dec 30, 2022

This completely breaks request tracking.

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

@richard67
Copy link
Member

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.

@richard67
Copy link
Member

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.

@wilsonge
Copy link
Contributor

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

@richard67
Copy link
Member

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?

@Fedik
Copy link
Member Author

Fedik commented Dec 31, 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.

@joomdonation
Copy link
Contributor

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)

@richard67
Copy link
Member

Back to pending due to previous comments.


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

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Dec 31, 2022
@Fedik
Copy link
Member Author

Fedik commented Dec 31, 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.

@joomdonation
Copy link
Contributor

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

@Fedik
Copy link
Member Author

Fedik commented Dec 31, 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.

@joomdonation
Copy link
Contributor

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

@brianteeman
Copy link
Contributor

brianteeman commented Apr 21, 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

@brianteeman
Copy link
Contributor

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

@Fedik
Copy link
Member Author

Fedik commented Apr 21, 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 :)

@brianteeman
Copy link
Contributor

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

@joomdonation
Copy link
Contributor

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

@brianteeman
Copy link
Contributor

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

image

@joomdonation
Copy link
Contributor

@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

@Fedik
Copy link
Member Author

Fedik commented Apr 21, 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.

@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev May 2, 2023 16:29
@HLeithner
Copy link
Member

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

@Fedik
Copy link
Member Author

Fedik commented Jun 25, 2023

I suggest set it back to RTC and merge

@richard67
Copy link
Member

Solves also issue #41045 .

@richard67
Copy link
Member

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.

@richard67
Copy link
Member

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

richard67 added a commit to richard67/joomla-cms that referenced this pull request Jun 25, 2023
richard67 added a commit to richard67/joomla-cms that referenced this pull request Jun 25, 2023
@MacJoom
Copy link
Contributor

MacJoom commented Jun 25, 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.

@richard67
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 25, 2023
@obuisard obuisard added this to the Joomla! 4.3.3 milestone Jun 26, 2023
@obuisard obuisard merged commit 082e8a1 into joomla:4.3-dev Jun 26, 2023
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 26, 2023
@obuisard
Copy link
Contributor

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

@Fedik Fedik deleted the plg-debug-fake-session branch June 26, 2023 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Small A PR which only has a small change

Projects

None yet

Development

Successfully merging this pull request may close these issues.