Currently the AMP PHP Library ( https://github.com/Lullabot/amp-library ) processes all HTML from rich text fields where various kinds of AMP Text formatters are used.

It would be a good idea for the final HTML generated by Drupal 8 to be also processed by the AMP Library. Analogously, this should happen in Drupal 7 also. This will result in HTML being more compliant with the AMP HTML standard. Illegal/disallowed attributes, tags and property value pairs would be removed from the HTML by the library.

The Drupal module side of the coding to send Full Page HTML to the AMP PHP library was done by Marc earlier but we did not enable it. The library should be sufficiently robust now so we should be able to do this.

One important feature is that if there were any custom elements found in the full html page, the AMP PHP Library should be able to add the custom javascript <script> tag in the head (if this slipped by the AMP module for various reasons). This has been accomplished (along with some other changes) -- please see https://github.com/Lullabot/amp-library/issues/20 to follow the library side of this work.

There are some improvements planned on Drupal module side:
- Users should be able to enable/disable Full Page processing of the HTML via AMP configuration. Currently this is disabled by default, but in a few days we will enable it by default. In any case users will always have the ability to enable or disable this in the AMP configuration screens.
- Users should be able to goto the Drupal Log and see what AMP warnings and fixes have been generated by the AMP PHP Library

CommentFileSizeAuthor
#8 time_taken.png50.46 KBsidharth_k
#8 amp_config.png113.58 KBsidharth_k
#6 time_taken.png50.46 KBsidharth_k
#2 drupal log amp library.png98.34 KBsidharth_k
#2 amp library config.png106.65 KBsidharth_k

Comments

sidharth_k created an issue. See original summary.

sidharth_k’s picture

sidharth_k commented
StatusFileSize
new amp library config.png106.65 KB
new drupal log amp library.png98.34 KB

This feature is merged to the 8.x-1.x branch and ready to test.

This was merged in https://www.drupal.org/commitlog/commit/81075/c524e4e81d84f5f653a1e05d5e...

IMPORTANT: Please ensure that the AMP library on your local machine is also upto date (commit 85d1361e7a or later) for this to work well.
You will find this library in your vendor/lullabot/amp folder in your drupal docroot.

To enable this feature click on the "experimental" checkbox and also optionally enable the last checkbox for debugging (See attached screenshots)

sidharth_k’s picture

sidharth_k commented
Title: *Full page* HTML processing of Drupal page output by the library » Full page HTML processing of Drupal page output by the library

  • sidharth_k committed c524e4e on 8.x-1.x 
    Issue #2685509 by sidharth_k: Drupal full page HTML amp library support...
mtift’s picture

mtift
he/him
Primary language English
Location Minnesota, USA
commented

Looks good!

sidharth_k’s picture

sidharth_k commented
StatusFileSize
new time_taken.png50.46 KB

  • sidharth_k committed 228f3ca on 8.x-1.x 
    Issue #2685509 by sidharth_k: Respond as late as possible for Full HTML...
  • sidharth_k committed 5418264 on 8.x-1.x
    Issue #2685509 by sidharth_k: Add ability to see statistics on Full Page...
sidharth_k’s picture

sidharth_k commented
StatusFileSize
new amp_config.png113.58 KB
new time_taken.png50.46 KB

I added some small tweaks and changes to further improve this:
- I added the ability to add some performance statistics at the bottom of the Drupal page. This is useful for general performance tracking and getting to know when a page was last cached (See screenshots attached to this comment)
- I decreased the priority of AmpHtmlResponseSubscriber to -1024. This way it will capture the final HTML to validate and fix

The library side of this work was done here: https://github.com/Lullabot/amp-library/issues/24

Caching

I explored caching implications of Full Page HTML conversion and here are my findings:

For anonymous users, the Full HTML conversion code in triggered by AmpHtmlResponseSubscriber runs only once. Once generated, the anonymous page is served out the Drupal page_cache module. This is fantastic because we don't want time to be repeatedly spent on Full HTML conversion for anonymous requests.

This is also the main use case for AMP sites. So we're all good here.

For non-anonymous users, I again explored the possibility of trying to cache the Full HTML conversion HTML. I delved deeply into this. For non-anonymous users the Drupal Dynamic Cache module is used for Full page HTML caching. We really can't validate+fix the HTML after the DynamicPageCacheSubscriber::onRespond because even though Dynamic Page Cache tries to cache the page as much as possible (but not everything!), it is full of placeholders that are programmatically str_replaced in the HtmlResponseSubscriber. Moreover other Kernel::RESPONSE listeners come after HtmlResponseSubscriber. We could cache that (final) HTML but since the DynamicPageCacheSubscriber is doing the actual caching here we can't do this in feasible way. (There is also the danger of caching just too many variants in logged in mode. And this is why the Dynamic Page Cache has placeholders for rapidly changing content )

So what happens is that the Full Page HTML conversion routine runs every page load for logged in users on AMP pages. The logged in user use case is not so important (as AMP is going to be for anonymous users in the overwhelming case) so again we should be good here.

In any case, Full HTML processing time taken is quite acceptable for even biggish pages. You can always turn on the statistics and track the performance in case you're interested in knowing how it doing. Remember to turn off xdebug and turn on opcaches to see true indicative performance.

All tasks are complete for Drupal 8. Closing this ticket.

sidharth_k’s picture

sidharth_k commented
Status: Active » Closed (fixed)
sidharth_k’s picture

sidharth_k commented

For those interested in performance measurement you can additionally read:

https://github.com/Lullabot/amp-library/issues/24#issuecomment-198667588
https://github.com/Lullabot/amp-library/issues/24#issuecomment-198667757