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

Refactor AMP_Theme_Support::ensure_required_markup() into sanitizers #3763

Closed
8 tasks
schlessera opened this issue Nov 15, 2019 · 4 comments
Closed
8 tasks
Labels
Groomed P1 Medium priority Sanitizers WS:Perf Work stream for Metrics, Performance and Optimizer

Comments

@schlessera
Copy link
Collaborator

Feature description

The code found in AMP_Theme_Support::ensure_required_markup() does a lot, and does it after the sanitizers have run:

/**
* Ensure the markup exists as required by AMP and elements are in the optimal loading order.
*
* Ensure meta[charset], meta[name=viewport], and link[rel=canonical] exist, as the whitelist sanitizer
* may have removed an illegal meta[http-equiv] or meta[name=viewport]. For a singular post, core only outputs a
* canonical URL by default. Adds the preload links.
*
* @since 0.7
* @link https://www.ampproject.org/docs/reference/spec#required-markup
* @link https://amp.dev/documentation/guides-and-tutorials/optimize-and-measure/optimize_amp/
* @todo All of this might be better placed inside of a sanitizer.
* @todo Consider removing any scripts that are not among the $script_handles.
*
* @param DOMDocument $dom Document.
* @param string[] $script_handles AMP script handles for components identified during output buffering.
*/
public static function ensure_required_markup( DOMDocument $dom, $script_handles = [] ) {

This code should be refactored into separated sanitizers so that all of that work is part of what the sanitizers do.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The AMP_Theme_Support::ensure_required_markup() can be removed as it is not needed anymore.

Implementation brief

  • Create new AMP_Meta_Sanitizer to deal with meta tags => WIP PR: Add AMP_DOM_Document & meta tag sanitizer #3758
  • Move script handling into AMP_Script_Sanitizer
  • Move rel links and preconnect/preload/prefetch handling to AMP_Link_Sanitizer
  • Create AMP_Title_Sanitizer to deal with title tags (or include in AMP_Meta_Sanitizer?)
  • Move script re-ordering/deduplication and render-delaying logic to AMP_Script_Sanitizer
  • Move AMP runtime logic to AMP_Script_Sanitizer
  • Move custom style reordering logic in AMP_Style_Sanitizer
  • Move the AMP boilerplate logic into AMP_Style_Sanitizer

For the above, we might face an issue where we cannot do the ordering at the point in time where the particular sanitizer is run. In case we hit such an issue, we should still put the generation of said elements into the corresponding sanitizers, and then add a final AMP_Head_Sanitizer to go over the order of things in one go.

QA testing instructions

Demo

Changelog entry

@westonruter
Copy link
Member

These sanitizers should be forcibly included among the list of sanitizers, in the same way that we currently force AMP_Style_Sanitizer and AMP_Tag_And_Attribute_Sanitizer to always be included and always run at the end.

@westonruter
Copy link
Member

As we discussed previously, the sanitizers that come out of this should align with the Transformers as outlined in the AMP SSR Toolkit Implementation Guide.

@westonruter
Copy link
Member

Much of this work has been accomplished in the Optimizer and will be part of upcoming modularization work.

@amedina amedina added the P1 Medium priority label May 14, 2020
@kmyram kmyram added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 5, 2020
@kmyram kmyram added the Groomed label Nov 24, 2020
@westonruter
Copy link
Member

A good bit of the code was moved in the amp-toolbox-php. More could be refactored, but it's not a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groomed P1 Medium priority Sanitizers WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
Archived in project
Development

No branches or pull requests

4 participants