-
Notifications
You must be signed in to change notification settings - Fork 385
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
Use external amp-toolbox library instead of lib/ subfolder #5582
Use external amp-toolbox library instead of lib/ subfolder #5582
Conversation
Plugin builds for c0d83e6 are ready 🛎️!
|
@schlessera one more change to be made: diff --git a/docs/src/Cli/GenerateCommand.php b/docs/src/Cli/GenerateCommand.php
--- a/docs/src/Cli/GenerateCommand.php (revision c0c7c7245f3f547d42a9675cb3cebfe22cf1aa29)
+++ b/docs/src/Cli/GenerateCommand.php (date 1604685464461)
@@ -246,7 +246,6 @@
private function get_excluded_dirs() {
return [
'#^.*/amp/(assets|bin|build|docs|node_modules|tests|vendor)/*#',
- '#^.*/amp/lib/(common|optimizer)/*#',
];
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few places where ampproject/optimizer
is still in use:
amp-wp/includes/deprecated.php
Line 213 in f972507
* @deprecated Boilerplate is now automatically added via the ampproject/optimizer library. |
* @package ampproject/optimizer |
* @package ampproject/optimizer |
02183ca
to
1e487ef
Compare
Co-authored-by: Pierre Gordon <[email protected]>
Changes look OK. I'm rebasing the PR since there's a merge conflict with the |
Also updating the patch to allow for the PHP 8 job to run since it's outdated. |
78d5d9f
to
c0d83e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 😄
diff --git a/composer.json b/composer.json | ||
index 4d24ebd44..2a98f63ce 100644 | ||
index 5a150f830..f670267f8 100644 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside: do we even need this context? It will get out of date often.
Summary
This PR removes the
lib/
subfolder and adapts the Composer file and the build instructions to use the externalampproject/amp-toolbox-php
library instead.This PR does not add namespace scoping to the library, assuming that any other WordPress plugin/theme should rely on the plugin's presence if they need access to the library => TBD!
Fixes #4567
Checklist