An alter hook that removes external javascript seems useful. Should only be active when in AMP mode (needs definition).

CommentFileSizeAuthor
#6 2588519-remove-js.patch3.86 KBrainbowarray

Comments

moshe weitzman created an issue. See original summary.

sokrplare’s picture

Agreed - a D7 start (from another site that handles JS aggregation externally to Drupal so this will need some work):

/**
 * Implements hook_js_alter().
 *
 */
function amp_js_alter(&$javascript) {
  $whitelisted_files = array(
    drupal_get_path('module', 'amp'),
  );
  foreach ($javascript as $data => $options) {
    // Allow any files in the whitelist array to be added. Inline JS is also
    // allowed for AMP JSON configs.
    foreach ($whitelisted_files as $whitelisted_file) {
      if (strpos($data, $whitelisted_file) !== FALSE || $options['type'] == 'inline') {
        $is_whitelisted_file = TRUE;
      }
    }
    // If the file isn't found to be whitelisted, remove it from the page.
    if (!$is_whitelisted_file) {
      unset($javascript[$data]);
    }
  }
}
rainbowarray’s picture

Version: » 8.x-1.x-dev

In Drupal 8, we're replacing the html_response.attachments_processor service with a custom AmpHtmlResponseAttachmentsProcessor class. This looks through the library files and only attaches those that start with 'amp/'. That allows us to pull in the JS necessary for amp components, but prevents other JS from being added to the site.

Thanks for the Drupal 7 code! We'll need to implement this for the D7 version too.

rainbowarray’s picture

Version: 8.x-1.x-dev » 7.x-1.x-dev
Assigned: Unassigned » rainbowarray

Moving this issue to the D7 branch. The code above looks like a good place to start.

rainbowarray’s picture

The only JS that can be included is JS for specific components like ads, analytics and iframes. Those are hosted externally on cdn.ampproject.org. I'm looking into the best way to load these (and conversely remove all JS not from that source).

Right now, I'm thinking the Libraries API CDN might be the way to go: https://www.drupal.org/project/libraries_cdn

As for pulling out JS, we'll need to make sure all JS is pulled out from #attached that isn't from this, probably pull things out with js_alter as well, and we'll probably have to check page_bottom and maybe even the $head variable in html.tpl.php, because there are definitely third-party modules that try to put JS up there to get higher in the load order.

rainbowarray’s picture

Project: Accelerated Mobile Pages (AMP) » AMP Theme
Category: Feature request » Task
StatusFileSize
new3.86 KB

I looked into this further. Neither drupal_add_js nor Libraries API CN allow adding some necessary attributes to the script tags needed for amp components. Namely, async and a custom-attribute='iframe', for example. There has been an issue open for years to allow for script attributes #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI), but it's still not in. The best workaround I've found is to manually add a script into $head like so:

  if (amp_is_amp_request()) {
    $element = array(
      '#tag' => 'script',
      '#type' => 'html_tag',
      '#attributes' => array(
        'src' => 'https://cdn.ampproject.org/v0/amp-iframe-0.1.js',
        'async' => "async",
        'custom-element' => 'amp-iframe'
      ),
    );
    drupal_add_html_head($element, 'amp-iframe');
  }

Is that ideal? Nope. In most cases that means the script is going to appear before the custom style tags. However, the script is set to async, so effectively that shouldn't matter, and the amp validator doesn't complain.

The upside of this is that simplifies what to do with all other possible avenues for third-party scripts. There's no need for a whitelist, we'll just remove them all, although we will check for scripts in $head for cdn.ampproject.org. Those can stay. This also allows other modules to add additional amp component js if they need to do so.

Right now this does not allow for adding inline js for component configuration. When we get to one of those components, we'll see if that ends up being a barrier, and if so, at that time we'll find a way to tackle that. For now, this should tighten up the ship for js.

One other note: I'm checking for js added as an element added directly within a region. It's always possible there's something hidden even further inside a render array with JS. If there are ideas for going further down that rabbit hole, I'm all ears.

  • mdrummond committed 912a25a on 7.x-1.x
    Issue #2588519 by mdrummond, sokrplare: Alter hook to strip 3rd party JS
    
rainbowarray’s picture

Status: Active » Fixed
sokrplare’s picture

Yeah, there isn't a great option here within Drupal. We used this which was custom for the AMP theme we made - same concept:

  // Required AMP font js.
  $variables['amp_head'][] = array(
    '#tag' => 'script',
    '#attributes' => array(
      'src' => url('https://cdn.ampproject.org/v0/amp-font-0.1.js', array('absolute' => TRUE)),
      'async' => NULL,
      'custom-element' => 'amp-font',
    ),
    // We need to add an empty string for #value so the script tag will be
    // properly closed with </script>.
    '#value' => '',
    '#theme' => 'html_tag',
  );
rainbowarray’s picture

I like that. We could add a $amp_scripts variable to html.tpl.php to better control where the scripts are placed. drupal_add_html_head might be handier though since we might end up needing to add the scripts either from a field formatter or a theme function. Or maybe we get the best of both worlds if we can collect the html_head scripts, remove them and move them to a $amp_scripts variable in preprocess_html. I'll be getting to that next week and can look if something like that might work.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.