The following conditional checks if the $mod_base_url exists anywhere within the external js file path, rather than the actual base url of the filepath, which returns true for a url such as http://my.externaldomain.com/foo/{mod_base_url}/path/to/file.js, which is incorrect.

      // If type is external and starts with http, https, or // but points to
      // this host change to file, but move it to the top of the aggregation
      // stack.
      if ($value['type'] === 'external'
        && stripos($value['data'], $mod_base_url) !== FALSE
        && (stripos($value['data'], 'http://') === 0
          || stripos($value['data'], 'https://') === 0
          || strpos($value['data'], '//') === 0
          )
      ) {

This should be validating that the $mod_base_url is actually the base url to the resource, and not just contained anywhere within $value['data'].

Suggested fix:

      // If type is external and starts with http, https, or // but points to
      // this host change to file, but move it to the top of the aggregation
      // stack.
      if ($value['type'] === 'external'
        && preg_match('#^(https?:)?//' . preg_quote($mod_base_url) . '#i', $value['data']) === 1
      ) {
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ZhuRenTongKu created an issue. See original summary.

mikeytown2’s picture

Going to test patch against latest dev

Status: Needs review » Needs work

The last submitted patch, 3: advagg-2864421-3-fix-external-js-detection-logic.patch, failed testing.

mikeytown2’s picture

Try again without escaping

Status: Needs review » Needs work

The last submitted patch, 5: advagg-2864421-5-fix-external-detection-logic.patch, failed testing.

mikeytown2’s picture

Status: Needs work » Needs review

The last submitted patch, 2: fix-external-js-type-option-logic-2864421.diff, failed testing.

The last submitted patch, 2: fix-external-js-type-option-logic-2864421.diff, failed testing.

The last submitted patch, 2: fix-external-js-type-option-logic-2864421.diff, failed testing.

mikeytown2’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: advagg-2864421-6-fix-external-detection-logic.patch, failed testing.

ZhuRenTongKu’s picture

Have updated patch by using 7.x-2.x branch

git clone git://git.drupal.org/project/advagg.git
applied changes to advagg.module
git diff > advagg-2864421-14-fix-external-detection-logic.patch

ZhuRenTongKu’s picture

Status: Needs work » Needs review
mikeytown2’s picture

Typo (double slash) will fail with the new version
http://my.externaldomain.com/foo//{mod_base_url}/path/to/file.js

Going to look at this more.

mikeytown2’s picture

ZhuRenTongKu’s picture

Thanks for your prompt attention to this MikeyTown2 - Applying this to my codebase today, will report back shortly!

ZhuRenTongKu’s picture

This seems to do the trick for me!

advagg-2864421-17-fix-external-detection-logic.patch = working as expected.

  • mikeytown2 committed 790ab08 on 7.x-2.x
    Issue #2864421 by ZhuRenTongKu, mikeytown2: Fix external JS type option...
mikeytown2’s picture

Status: Needs review » Fixed

Good to hear! It's been committed.

Status: Fixed » Closed (fixed)

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