-
Notifications
You must be signed in to change notification settings - Fork 210
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
Closes #5923: Prevent loading logger class without WP_ROCKET_DEBUG enabled #6028
base: develop
Are you sure you want to change the base?
Closes #5923: Prevent loading logger class without WP_ROCKET_DEBUG enabled #6028
Conversation
@jeawhanlee Thank you for the PR. The following classes aren't loaded 👍 :
However, Checking the code, I see that it's used in the constructor and in other methods of the
Should we add the logic to regenerate the |
@Tabrisrp What's your opinion on that? |
|
Hi team,
If I understood that correctly, then @piotrbak would you be OK with reducing the scope of the original task and accept it without the Logger/Logger class being handled? It would allow to move forward with the current implementation ot be tested by QA, and deliver value quickly. What do you think? |
@MathieuLamiot Yes, that's acceptable while taking into the consideration complexity. Let's deliver this one and create a low priority issue after the release about the missing class. |
…lass-without-wp-rocket-debug-enabled
I found that even on When enabling debugging the following classes are additionally loaded:
@engahmeds3ed confirmed my findings. 🙏 So, this PR doesn't bring anything new, and the real issue to tackle is to avoid loading |
Thanks for the deep-dive @vmanthos. Let's block the issue for now and have a talk about it. |
Description
Load logger classes conditionally only when WP_ROCKET_DEBUG is set and true.
Fixes #5923
Type of change
Is the solution different from the one proposed during the grooming?
Slightly
How Has This Been Tested?
Automated tests
Checklist: