Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## main #4513 +/- ##
==========================================
- Coverage 83.85% 75.96% -7.89%
==========================================
Files 891 898 +7
Lines 15760 15938 +178
==========================================
- Hits 13215 12107 -1108
- Misses 2545 3831 +1286
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
434e594 to
5d40b05
Compare
|
This is great! As discussed, the initial goal can be provide sanitization similar to how the AMP Reader Mode with dev tools turned off works in the AMP plugin: ensure the full validity of stories while silently sanitizing any offending markup. This work also provides excellent input to the architecture/modularization work on the plugin side. |
b556f49 to
a08f8cd
Compare
This comment has been minimized.
This comment has been minimized.
e56381f to
b502c90
Compare
1f5f761 to
cdb6b26
Compare
This comment has been minimized.
This comment has been minimized.
|
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
|
High time for an update on this one! I know that this change wasn't the easiest to review, with over 4k changed LOC in one massive PR, even when most of the code is just copied from the AMP plugin and not too much our concern. In any case, it helped get a much better sense of what's needed to contribute all of this back to the AMP plugin. Plus, it's a giant leap forward for built-in AMP sanitization. Now it's crucial to get some real-world feedback on it, which is why I think it's high time to get this thing merged! We can then continue to iterate on it in follow-up tickets if anything arises. |
Co-authored-by: Weston Ruter <westonruter@google.com>
Co-authored-by: Weston Ruter <westonruter@google.com>
|
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
Summary
This PR addresses on of our longer standing and more pressing issues: built-in AMP sanitization and optimization for stories. See this doc for reference.
How it works:
HTML.php, calls the newSanitizationandOptimizationclasses which do the respective jobs.AMP_Theme_SupportRelevant Technical Choices
mcaskill/composer-exclude-files, to avoid loading any procedural code by the AMP plugin.For one because they'd otherwise cause conflicts if functions are already defined. But also because they often cause side-effects like accessing options from the database, or requiring
AMP__VERSIONto be set.DOMDocumentHTMLclass and removes duplicate logic.AMP_Theme_SupporttoSanitizationto run markup through sanitization and optimization.Meta_SanitizerA shameless copy of
AMP_Meta_Sanitizer, except that it does not callamp_get_boilerplate_stylesheets(). As mentioned above, we're not loading that function.Canonical_Sanitizer(#4193)Enforces adding a
link[rel=canonical]to documents to make sure they are always valid.The AMP plugin itself does that already in
\AMP_Theme_Support::ensure_required_markup(). We just do the same here.This sanitizer is only used when the AMP plugin is not active. It could be easily contributed to the AMP plugin, too.
Story_SanitizerThis new sanitizer was extracted from the
HTMLclass, and currently deals with setting publisher logos and poster images. Also removes theampattribute from the document if either is missing.Related: #4762
Avoiding Conflicts
See #4543 for how we're avoiding conflicts with the AMP plugin itself (or any other plugin using the same libraries) by prefixing them before shipping.
To-do
Not in the scope of this PR, but tasks to look at soon after.
AMP_Style_SanitizerCurrently disabled because add_poster_images calls
AMP_Options_Manager, which in turn requiresAMP__VERSIONto be defined. The method isprivate, so can't be overridden in an extended class.Aside: CSS used in stories is usually rather small, so not sure transient caching is actually needed
User-facing changes
Web Stories are sanitized and optimized by default, even when the AMP plugin is not active.
Testing Instructions
composer installor check out this branch on the QA siteFixes #4650
Fixes #4193