Closes #5923: Prevent loading logger class without WP_ROCKET_DEBUG enabled

[jeawhanlee]

Description

Load logger classes conditionally only when WP_ROCKET_DEBUG is set and true.

Fixes #5923

Type of change

• [x] Enhancement (non-breaking change which improves an existing functionality)

Is the solution different from the one proposed during the grooming?

Slightly

How Has This Been Tested?

Automated tests

Checklist:

• [x] My code follows the style guidelines of this project
• [x] I have performed a self-review of my own code
• [x] My changes generate no new warnings
• [x] New and existing unit tests pass locally with my changes

[vmanthos]

@jeawhanlee Thank you for the PR.

The following classes aren't loaded :+1: :

• WP_Rocket\Logger\HTML_Formatter
• WP_Rocket\Logger\Stream_Handler
• Monolog\Logger

However, WP_Rocket\Logger\Logger is still loaded.

Checking the code, I see that it's used in the constructor and in other methods of the WP_Rocket\Buffer\Cache class - probably elsewhere as well, and therefore not loading it would cause fatal errors.

@piotrbak

1. Should we load WP_Rocket\Logger\Logger? If not, further changes will be required to this PR.

2. Also, the changes from this PR will not be added to the advanced-cache.php when updating to 3.14.2. They will when/if we regenerate the advanced-cache.php file, e.g. if "Separate cache files for mobile devices" is enabled/disabled.

Should we add the logic to regenerate the advanced-cache.php when updating to 3.14.2?

[piotrbak]

@Tabrisrp What's your opinion on that?

[remyperona]

• Not loading the Logger class while require additional changes indeed, to remove the coupling between the logger and the buffer classes (cache and optimization)
• Since we're doing changes to the advanced-cache file, we should regenerate it on update yes

[MathieuLamiot]

Hi team, I'm catching up on this topic, is it correct that:

• All debug classes except one are not loaded anymore (according to @vmanthos)
• For this to fully work, advanced-cache file must be updated. The logic has been added by @jeawhanlee but yet to be tested.
• To tackle the missing class, the work is more complex than anticipated.

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. A new task could be created (and added to an upcoming sprint if the value is interesting) to specifically handle the Logger/Logger class. With the acquired knowledge, a better grooming can be done.

What do you think?

[piotrbak]

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

[vmanthos]

@piotrbak @MathieuLamiot

I found that even on trunk only the WP_Rocket\Logger\Logger class is loaded when WP_ROCKET_DEBUG is set to false.

When enabling debugging the following classes are additionally loaded:

• WP_Rocket\Logger\HTML_Formatter
• WP_Rocket\Logger\Stream_Handler
• Monolog\Logger

@engahmeds3ed confirmed my findings. :pray:

So, this PR doesn't bring anything new, and the real issue to tackle is to avoid loading WP_Rocket\Logger\Logger unless WP_ROCKET_DEBUG is set to true.

[MathieuLamiot]

Thanks for the deep-dive @vmanthos. Let's block the issue for now and have a talk about it.