In drupal_get_js's current state, we can't have inline scripts appearing above external scripts. This is important for many external third party web services where they expect local variables to be initiated before using their external script....

drupal_add_js('/* This should appear above the external script! */', array(
  'type' => 'inline',
  'scope' => 'footer',
  'weight' => JS_THEME - 10,
));
drupal_add_js('http://example.com/example.js', array(
  'type' => 'external',
  'scope' => 'footer',
  'weight' => JS_THEME + 10,
));

Expected output is......

< script type="text/javascript">/* This should appear above the external script! */</script>
< script type="text/javascript" src="http://example.com/example.js"></script>

....But it results in the reverse order.

< script type="text/javascript" src="http://example.com/example.js"></script>
< script type="text/javascript">/* This should appear above the external script! */</script>
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Uhhhh, we expect......

<script type="text/javascript">/* This should appear above the external script! */</script>
<script type="text/javascript" src="http://example.com/example.js"></script>

....But it results in the reverse order.

<script type="text/javascript" src="http://example.com/example.js"></script>
<script type="text/javascript">/* This should appear above the external script! */</script>

This was as a result from #721400: Order JS files according to weight, don't change filenames for aggregated JS/CSS.

pwolanin’s picture

So I thought we did not want to mix inline with the rest, but guess I was wrong - this should be an easy fix, I think.

mfer’s picture

@pwolanin - The way it used to work was that inline and non-preprocess (external and internal) were intermixed by weight. It used to not mix preprocessed and non-preprocessed.

This makes me wish this was all documented well.

mfer’s picture

Priority: Normal » Critical

Since this messes with JavaScript dependencies I'd call it critical.

pwolanin’s picture

Status: Active » Needs review
FileSize
567 bytes

I think the fix here is 1-line.

Status: Needs review » Needs work
Issue tags: -Performance, -JavaScript, -frontend performance

The last submitted patch, 746676-inline-js-by-weight-5.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review

#5: 746676-inline-js-by-weight-5.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Performance, +JavaScript, +frontend performance

The last submitted patch, 746676-inline-js-by-weight-5.patch, failed testing.

pwolanin’s picture

This is a different fail than last time - feel like testbot is a game of roulette now...

pwolanin’s picture

Status: Needs work » Needs review

#5: 746676-inline-js-by-weight-5.patch queued for re-testing.

RobLoach’s picture

FileSize
2.1 KB

Does botman like the test?

Status: Needs review » Needs work

The last submitted patch, 746676.patch, failed testing.

RobLoach’s picture

Status: Fixed » Reviewed & tested by the community

Ignore the testing bot. The new JavaScript test with the mixed external/inline tags passed. The weighting is retained. Nice one, Peter ;-) .... I posted a follow up: #748794: Aggregate side-by-side inline JavaScript.

Dries’s picture

Status: Needs work » Fixed

Committed to CVS HEAD. Thanks.

Status: Reviewed & tested by the community » Closed (fixed)
Issue tags: -Performance, -JavaScript, -frontend performance

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