这是indexloc提供的服务，不要输入任何密码
Skip to content

Add error logger to the PX engine tools #26

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
Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add error logger to the PX engine tools #26

wants to merge 12 commits into from

Conversation

ediamin
Copy link
Collaborator

@ediamin ediamin commented Mar 16, 2022 •
edited
Loading

Fixes #19.

This PR adds PSR-3 standard logger support for the analyzer and the optimizer tools that can be provided by the Engine object and used by the PX engine consumer.

$logger = new Logger(); // Logger class that implements Psr\Log\LoggerInterface interface.
$pxEngine = new \PageExperience\Engine($logger);
$profile = new ConfigurationProfile();
$analysis = $pxEngine->analyze($url, $profile, );

To-do:

  • Add psr\log package via composer.
  • Adapt logger interface in tools.
  • Adapt in testing classes.
  • Add an implementation in CLI command classes.

@ediamin ediamin requested a review from schlessera March 16, 2022 14:14
@codecov-commenter
Copy link

codecov-commenter commented Mar 17, 2022 •
edited
Loading

Codecov Report

Merging #26 (3f17422) into main (bb64da0) will increase coverage by 0.36%.
The diff coverage is 73.52%.

@@             Coverage Diff              @@
##               main      #26      +/-   ##
============================================
+ Coverage     55.26%   55.62%   +0.36%     
- Complexity      409      412       +3     
============================================
  Files            61       62       +1     
  Lines           950      960      +10     
============================================
+ Hits            525      534       +9     
- Misses          425      426       +1
Flag Coverage Δ
php 55.62% <73.52%> (+0.36%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Cli/Command/Analyze.php 0.00% <0.00%> (ø)
src/Engine/ToolStack/ParallelToolStack.php 0.00% <0.00%> (ø)
src/Cli/Command/Optimize.php 90.00% <100.00%> (+0.52%) ⬆️
src/Cli/Logger.php 100.00% <100.00%> (ø)
src/Engine.php 100.00% <100.00%> (ø)
src/Engine/Tool/AmpOptimizer.php 100.00% <100.00%> (ø)
src/Engine/Tool/AmpValidator.php 100.00% <100.00%> (ø)
src/Engine/Tool/PageSpeedInsights.php 17.27% <100.00%> (ø)
src/Engine/ToolStack/SequentialToolStack.php 83.33% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb64da0...3f17422. Read the comment docs.

@ediamin ediamin force-pushed the add/19-transport-error-logger branch from cec8b19 to d9ca658 Compare March 21, 2022 10:12
@ediamin ediamin marked this pull request as ready for review March 29, 2022 06:20
schlessera
schlessera requested changes Mar 29, 2022
View reviewed changes
src/Engine.php Outdated
* @return Analysis Page experience analysis.
*/
public function analyze($url, ConfigurationProfile $profile)
public function analyze($url, ConfigurationProfile $profile, LoggerInterface $logger)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logger should not be injected by calling the analyze() or optimize() methods, but rather be injected via the constructor and stored as a property $this->logger.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the constructor, the ToolStackFactory should also be instantiated with the logger.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schlessera Updated the code. Please take a look.

@ediamin ediamin requested a review from schlessera March 31, 2022 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schlessera schlessera Awaiting requested review from schlessera

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add logging to transport error state from individual tools to the final consumer
3 participants
@ediamin @codecov-commenter @schlessera