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.

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

Turn the AMP Optimizer into an injectable service #6065

Merged
merged 9 commits into from
Apr 14, 2021

Conversation

schlessera
Copy link
Collaborator

@schlessera schlessera commented Apr 11, 2021

Summary

This PR turns the AMP Optimizer into an injectable service and uses the injector configuration to configure it from a central location.

It also hooks the WordPress filters into the Optimizer's configuration object.

Fixes #6064
Depends on ampproject/amp-toolbox-php#150

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@schlessera schlessera added Infrastructure Changes impacting testing infrastructure or build tooling Optimizer Tech Debt Deprecations, inefficiencies, code health labels Apr 11, 2021
@schlessera schlessera force-pushed the fix/6064-turn-optimizer-into-service branch from c3807c8 to 4562e6c Compare April 12, 2021 06:20
@schlessera schlessera marked this pull request as ready for review April 12, 2021 06:21
@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #6065 (af79fa2) into develop (6b1af52) will increase coverage by 0.05%.
The diff coverage is 96.03%.

❗ Current head af79fa2 differs from pull request most recent head 70dced8. Consider uploading reports for the commit 70dced8 to get more accurate results
Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #6065      +/-   ##
=============================================
+ Coverage      75.38%   75.44%   +0.05%     
- Complexity      5706     5714       +8     
=============================================
  Files            218      227       +9     
  Lines          17265    17335      +70     
=============================================
+ Hits           13016    13079      +63     
- Misses          4249     4256       +7     
Flag Coverage Δ Complexity Δ
javascript 79.84% <94.82%> (-0.22%) 0.00 <0.00> (ø)
php 75.24% <97.67%> (+0.05%) 0.00 <11.00> (ø)

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

Impacted Files Coverage Δ Complexity Δ
...lidation/components/amp-validation-status/index.js 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...alidation/components/sidebar-notification/index.js 100.00% <ø> (ø) 0.00 <0.00> (ø)
...s/src/block-validation/components/sidebar/index.js 0.00% <ø> (ø) 0.00 <0.00> (?)
...udes/sanitizers/class-amp-core-theme-sanitizer.php 36.28% <ø> (ø) 228.00 <0.00> (ø)
src/Optimizer/Transformer/AmpSchemaOrgMetadata.php 85.71% <ø> (ø) 4.00 <0.00> (?)
.../Transformer/AmpSchemaOrgMetadataConfiguration.php 0.00% <ø> (ø) 4.00 <0.00> (?)
src/Optimizer/Transformer/DetermineHeroImages.php 91.04% <ø> (ø) 25.00 <0.00> (?)
...k-validation/hooks/use-post-dirty-state-changes.js 96.96% <75.00%> (-3.04%) 0.00 <0.00> (ø)
.../block-validation/hooks/use-amp-document-toggle.js 80.00% <80.00%> (ø) 0.00 <0.00> (?)
includes/class-amp-theme-support.php 86.19% <83.33%> (-0.03%) 244.00 <2.00> (-1.00)
... and 23 more

@github-actions
Copy link
Contributor

github-actions bot commented Apr 12, 2021

Plugin builds for 70dced8 are ready 🛎️!

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Looks good, other than the test coverage complaints.

Comment on lines +36 to +41
* Note: They will only be applied once, when this method is hit for the first time.
*/
public function apply_filters() {
if ( $this->already_applied ) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Any chance this would be undesirable? What if you wanted to change the configuration with a filter change on a subsequent call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The alternative is to run the filters on every get() and has(), which can quickly become a performance issue.

If you want to change the configuration with a later filter call, you can instantiate a new configuration object instead.

Also, this should actually only be triggered when the optimizer is being run, which happens in the shutdown hook that collects the final output buffer after WordPress has finished. I don't think plugins should do anything after the optimizer has run.

@westonruter westonruter added this to the v2.1 milestone Apr 14, 2021
@westonruter westonruter merged commit 113afdf into develop Apr 14, 2021
@westonruter westonruter deleted the fix/6064-turn-optimizer-into-service branch April 14, 2021 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes impacting testing infrastructure or build tooling Optimizer Tech Debt Deprecations, inefficiencies, code health
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor Optimizer integration to make use of the injector
2 participants