Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prevent loading the Logger classes if WP_ROCKET_DEBUG is not enabled #5923

Open
2 of 6 tasks
DahmaniAdame opened this issue May 16, 2023 · 5 comments · May be fixed by #6028
Open
2 of 6 tasks

Prevent loading the Logger classes if WP_ROCKET_DEBUG is not enabled #5923

DahmaniAdame opened this issue May 16, 2023 · 5 comments · May be fixed by #6028
Labels
effort: [XS] < 1 day of estimated development time module: Logger needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement

Comments

@DahmaniAdame
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version 3.13.2
  • Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
Multiple Logger classes are loaded on the advanced-cache.php file.

wp-rocket/views/cache/advanced-cache.php

Lines 33 to 52 in 9822ca2

$rocket_classes = [
'WP_Rocket\\Buffer\\Abstract_Buffer' => $rocket_path . 'inc/classes/Buffer/class-abstract-buffer.php',
'WP_Rocket\\Buffer\\Cache' => $rocket_path . 'inc/classes/Buffer/class-cache.php',
'WP_Rocket\\Buffer\\Tests' => $rocket_path . 'inc/classes/Buffer/class-tests.php',
'WP_Rocket\\Buffer\\Config' => $rocket_path . 'inc/classes/Buffer/class-config.php',
'WP_Rocket\\Logger\\HTML_Formatter' => $rocket_path . 'inc/classes/logger/class-html-formatter.php',
'WP_Rocket\\Logger\\Logger' => $rocket_path . 'inc/classes/logger/class-logger.php',
'WP_Rocket\\Logger\\Stream_Handler' => $rocket_path . 'inc/classes/logger/class-stream-handler.php',
'WP_Rocket\\Traits\\Memoize' => $rocket_path . 'inc/classes/traits/trait-memoize.php',
];
if ( isset( $rocket_classes[ $class ] ) ) {
$file = $rocket_classes[ $class ];
} elseif ( strpos( $class, 'Monolog\\' ) === 0 ) {
$file = $rocket_path . 'vendor/monolog/monolog/src/' . str_replace( '\\', '/', $class ) . '.php';
} elseif ( strpos( $class, 'Psr\\Log\\' ) === 0 ) {
$file = $rocket_path . 'vendor/psr/log/' . str_replace( '\\', '/', $class ) . '.php';
} else {
return;
}

Since logging WP Rocket only happens when the WP_ROCKET_DEBUG, we should make loading all related classes conditional to that.

The impact on performance won't be significant, but it's still not

To Reproduce
N/A

Expected behavior
WP Rocket shouldn't load code/libraries not being used.

Screenshots
N/A

Additional context
Slack - https://wp-media.slack.com/archives/C43T1AYMQ/p1684220879897639?thread_ts=1684217755.309949&cid=C43T1AYMQ

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement module: Logger priority: medium Issues which are important, but no one will go out of business. labels Jun 1, 2023
@piotrbak
Copy link
Contributor

piotrbak commented Jul 2, 2023

Acceptance Criteria:

  1. Logger classes loaded only if the constant is set
  2. Logger working without regressions

@piotrbak piotrbak added priority: low Issues that can wait and removed priority: medium Issues which are important, but no one will go out of business. labels Jul 3, 2023
@jeawhanlee
Copy link
Contributor

Scope a solution ✅

Load classes conditionally in https://github.com/wp-media/wp-rocket/blob/9822ca221e2070d39ecd1a8e74fec20b4351f3d5/views/cache/advanced-cache.php:
if ( rocket_get_constant( 'WP_ROCKET_DEBUG', false ) ) {

wp-rocket/views/cache/advanced-cache.php

Lines 31 to 58 in 9822ca2

spl_autoload_register(
function( $class ) use ( $rocket_path ) {
$rocket_classes = [
'WP_Rocket\\Buffer\\Abstract_Buffer' => $rocket_path . 'inc/classes/Buffer/class-abstract-buffer.php',
'WP_Rocket\\Buffer\\Cache' => $rocket_path . 'inc/classes/Buffer/class-cache.php',
'WP_Rocket\\Buffer\\Tests' => $rocket_path . 'inc/classes/Buffer/class-tests.php',
'WP_Rocket\\Buffer\\Config' => $rocket_path . 'inc/classes/Buffer/class-config.php',
'WP_Rocket\\Logger\\HTML_Formatter' => $rocket_path . 'inc/classes/logger/class-html-formatter.php',
'WP_Rocket\\Logger\\Logger' => $rocket_path . 'inc/classes/logger/class-logger.php',
'WP_Rocket\\Logger\\Stream_Handler' => $rocket_path . 'inc/classes/logger/class-stream-handler.php',
'WP_Rocket\\Traits\\Memoize' => $rocket_path . 'inc/classes/traits/trait-memoize.php',
];
if ( isset( $rocket_classes[ $class ] ) ) {
$file = $rocket_classes[ $class ];
} elseif ( strpos( $class, 'Monolog\\' ) === 0 ) {
$file = $rocket_path . 'vendor/monolog/monolog/src/' . str_replace( '\\', '/', $class ) . '.php';
} elseif ( strpos( $class, 'Psr\\Log\\' ) === 0 ) {
$file = $rocket_path . 'vendor/psr/log/' . str_replace( '\\', '/', $class ) . '.php';
} else {
return;
}
if ( file_exists( $file ) ) {
require $file;
}
}
);

}

Estimate the effort ✅

[XS]

@jeawhanlee jeawhanlee added effort: [XS] < 1 day of estimated development time and removed needs: grooming labels Jul 5, 2023
@Tabrisrp
Copy link
Contributor

Tabrisrp commented Jul 5, 2023

rocket_get_constant() function is not available at the time advanced-cache.php is loaded. But the logic is the same, using the core PHP defined() function instead.

@jeawhanlee jeawhanlee self-assigned this Jul 5, 2023
@jeawhanlee
Copy link
Contributor

@Tabrisrp Many Thanks 🙏

@jeawhanlee jeawhanlee linked a pull request Jul 5, 2023 that will close this issue
Open
5 tasks
@piotrbak piotrbak added this to the 3.15 milestone Jul 14, 2023
@MathieuLamiot
Copy link
Contributor

Following the post-mortem, we agreed to:

  • drop the current implementation
  • Put back the task to "Needs grooming"
  • When grooming is started by a software engineer, it will be important to discuss added value vs. technical cost ie. is the added value worth the technical cost (like maintainability & codebase logic/quality)?

@piotrbak piotrbak added the needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. label Jul 25, 2023
@piotrbak piotrbak removed this from the 3.15 milestone Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
effort: [XS] < 1 day of estimated development time module: Logger needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. priority: low Issues that can wait type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Closes #5923: Prevent loading logger class without WP_ROCKET_DEBUG enabled wp-media/wp-rocket
5 participants
@DahmaniAdame @Tabrisrp @MathieuLamiot @piotrbak @jeawhanlee