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

Extract post-processor logic into CMS-agnostic PHP library #2315

Closed
westonruter opened this issue May 14, 2019 · 5 comments
Closed

Extract post-processor logic into CMS-agnostic PHP library #2315

westonruter opened this issue May 14, 2019 · 5 comments
Labels
Epic Groomed Infrastructure Changes impacting testing infrastructure or build tooling WS:Core Work stream for Plugin core

Comments

@westonruter
Copy link
Member

westonruter commented May 14, 2019

There is need in Drupal, Joomla!, Typo3 among other PHP-based CMSes for an official library to do what the AMP WordPress plugin does in its post-processor phase. The Drupal AMP module is currently using amp-library for this purpose. but it is very out of date and it needs to be overhauled (see Lullabot/amp-library#231). A library which is maintained by the AMP project is needed to ensure it is kept up to date.

Such a library should include:

  • Converting HTML elements to their equivalent AMPHTML ones (e.g. img to amp-img).
  • Processing stylesheets (concatenating, minifying, tree-shaking).
  • Removing invalid markup and reporting what was removed.

This would essentially be extracting the entirety of the AMP_Theme_Support::prepare_response() method, minus the parts that are WordPress-specific. The extracted functionality would need to be configurable to plug in to any CMS, including the way it handles script dependencies and object caching.

As part of this effort, we should take the opportunity to refactor the sanitizers to clean up some legacy cruft and embrace a more modern PHP 5.6 world (which is WordPress's new minimum version). Likewise, the AMP_Theme_Support::ensure_required_markup() should be converted into a sanitizer, and a new sanitizer for implementing optimized/transformed AMP can be introduced (#958).

While not part of the post-processor phase, there would also be value in extracting the the oEmbed handling, defining a mapping of oEmbed URLs to how they should be converted into AMP.

@bmack
Copy link

bmack commented May 15, 2019

Hi Weston,

I think we should go with a "amp-php-migrator-engine" package or so. This could be ideally prepared for PHP 7+ but still support PHP 5.6 for WP - for now I'm fine coding the PHP 5.6-way.

We could then create "adapters" for each CMS as separate packages IMHO.

Do you have more docs on the amp-wp package, otherwise I can start migrating and look into a proper API so WP works with it.

@westonruter
Copy link
Member Author

Yes, agreed. The CMS-agnostic library would be in a separate package, naming TBD. The AMP plugin for WordPress would then use the package as it would adapt the library for a WordPress context, just as there would be similar adapters for other CMSes, like Typo3.

Do you have more docs on the amp-wp package, otherwise I can start migrating and look into a proper API so WP works with it.

The docs so far are almost all inline with the code. Part of this effort is to flesh out the documentation and better explain how it works, and clean up some technical debt. This would be an important part of the refactoring so that it is easier for you to integrate. You can see an example of @marcelovani experimenting with integrating the WordPress plugin's code into the AMP library used by the Drupal module here: Lullabot/amp-library#231 (comment).

@westonruter
Copy link
Member Author

I opened an issue specifically for the extraction of the CSS sanitizer (including the tree shaker): #5602.

@kmyram kmyram added the Epic label Feb 2, 2021
@kmyram kmyram added this to the v2.2 milestone Feb 2, 2021
@kmyram kmyram added the Groomed label Feb 2, 2021
@schlessera
Copy link
Collaborator

For people following this specific issue, I'd like to note that the work for this issue is already underway under the https://github.com/ampproject/amp-toolbox-php/ repository.

@westonruter
Copy link
Member Author

Specifically, see ampproject/amp-toolbox-php#194 and ampproject/amp-toolbox-php#195. Other issues (e.g. embed handlers) can be filed as issues in amp-toolbox-php as well.

@westonruter westonruter removed this from the v2.2 milestone Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic Groomed Infrastructure Changes impacting testing infrastructure or build tooling WS:Core Work stream for Plugin core
Projects
None yet
Development

No branches or pull requests

5 participants