For some reason, i don't want my javascript files to be served from CDN.
So, I added *.js in Exception for the blacklisted file path.
But, my js files are not aggregating even if I enabled " Aggregate JavaScript files." on performance page.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nileshlohar created an issue. See original summary.

nileshlohar’s picture

Here is the patch for it

nileshlohar’s picture

Status: Active » Needs review

Issue appears only when I add "*.js" in exception ie when I don't want any of my js files to be loaded from CDN.
if I add any other path specific exception it works well.
my patch resolves the issue and works in both conditions.

Status: Needs review » Needs work

The last submitted patch, 2: js_aggregation_issue-2684289-2.patch, failed testing.

nileshlohar’s picture

Status: Needs work » Needs review
FileSize
582 bytes

Updated patch for failed testing.

nileshlohar’s picture

More correct version.

Wim Leers’s picture

Priority: Critical » Normal
Issue tags: +Needs steps to reproduce

Please provide steps to reproduce, I don't yet understand how JS aggregation is supposedly broken in version 2.7 and how this would fix it.

Wim Leers’s picture

Status: Needs review » Postponed (maintainer needs more info)
nileshlohar’s picture

Steps to Reproduce:

  1. CDN mode is "Origin Pull" with Far Future expiration disabled.
  2. I have blacklisted *.js in File URL Exceptions.
  3. And Enabled "Aggregate JavaScript files".

Result:

JS files are not aggregated.

Reason:


/**
 * Implements hook_js_alter().
 *
 * Ensure that CDN-blacklisted JS files are not aggregated, so that the JS
 * aggregates can still be served from the CDN.
 */
function cdn_js_alter(&$javascript) {
  if (!cdn_status_is_enabled()) {
    return;
  }

  foreach (array_keys($javascript) as $key) {
    // Skip $type = 'inline'.
    if (is_numeric($key)) {
      continue;
    }
    // Skip $type = 'setttings'.
    elseif ($key === 'settings') {
      continue;
    }
    // Handle $type = 'file' and 'external'.
    elseif (!cdn_check_file($key)) {
      $javascript[$key]['preprocess'] = FALSE;
    }
  }
}

This code makes sure that CDN-blacklisted JS files are not aggregated.
but as I am blacklisting all my js file (*.js)
It's blocking aggregation of all JS files and as a result, No js files are aggregated.

Resolution:

I am adding a condition to check if all JS files are blacklisted then further code doesn't gets executed to block all JS files aggregation.

Hope this helps....

nileshlohar’s picture

Status: Postponed (maintainer needs more info) » Needs review
Issue tags: -Needs steps to reproduce
Wim Leers’s picture

Title: JS Aggregation not working » When blacklisting all *.js or *.css files, aggregation stops working
Component: Module » Origin Pull mode
Priority: Normal » Minor

I see now. Thanks for the explanation!

This bug was introduced by #2499967: CSS/JS Aggregation and blacklisting of individual CSS/JS files within the aggregates.

And in fact, the same problem exists for CSS aggregates: if you blacklist *.css. But nobody is probably doing that.

Wim Leers’s picture

Marked it minor because you have to blacklist all JS files to hit this edge case.

Wim Leers’s picture

FileSize
1007 bytes

Can you please test this patch?

nileshlohar’s picture

FileSize
1.14 KB

Working fine with the small update.

I'm attaching updated patch with:

  • Updated *.css to *.js in cdn_js_alter().
  • And combined If conditions.

Status: Needs review » Needs work

The last submitted patch, 14: 2684289-14.diff, failed testing.

nileshlohar’s picture

Status: Needs work » Needs review
FileSize
1.15 KB

Updated Patch after solving Errors.

Status: Needs review » Needs work

The last submitted patch, 16: 2684289-15.diff, failed testing.

nileshlohar’s picture

FileSize
1.04 KB

Again... Updated Patch.

nileshlohar’s picture

Status: Needs work » Needs review
Wim Leers’s picture

FileSize
1006 bytes

You're right in #14 in that the patch in #13 indeed has *.css where it should have *.js. But instead of fixing that, you changed the patch completely.

So, bringing back #13, because the code in there is cleaner/simpler. And fixing the bug you noticed. Can you please test this patch? :) Thanks!

nileshlohar’s picture

Status: Needs review » Reviewed & tested by the community

Tested and it is fixing the Bug.

  • Wim Leers committed 1568cd1 on 7.x-2.x authored by nileshlohar
    Issue #2684289 by nileshlohar, Wim Leers: When blacklisting all *.js or...
Wim Leers’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! Credited you :)

nileshlohar’s picture

Thanks :)

Wim Leers’s picture

Thank you! :)

Status: Fixed » Closed (fixed)

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