Automattic / Automattic/nginx-http-concat

External scripts are not enqueued with necessary dependencies

Open
#18 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
PHP
Stars
120
Forks
37
PR merge metrics
No merged PRs in 30d

Description

[`do_item()`](https://github.com/Automattic/nginx-http-concat/blob/master/jsconcat.php#L117) is called to let core handle external scripts, since they don't get concatenated. Unfortunately, `$this->do_concat` is true, so the external script tag is [saved for later instead of being `echo'd` at the proper time for its dependents/dependencies](https://github.com/WordPress/WordPress/blob/4.5.2/wp-includes/class.wp-scripts.php#L358)

I've got a patch (below) but I'm not 100% sure enough to know that it wouldn't break other things. It likely needs cleaned up and tested more.

```
diff --git a/jsconcat.php b/jsconcat.php
index b46a4b5..ae8c61a 100644
--- a/jsconcat.php
+++ b/jsconcat.php
@@ -39,6 +39,7 @@ class WPcom_JS_Concat extends WP_Scripts {
$handles = false === $handles ? $this->queue : (array) $handles;
$javascripts= array();
$siteurl = site_url();
+ $external_scripts = array();

$this->all_deps( $handles );
$level = 0;
@@ -73,8 +74,10 @@ class WPcom_JS_Concat extends WP_Scripts {
$do_concat = true;

// Don't try to concat externally hosted scripts
- if ( ( isset( $js_url['host'] ) && ( preg_replace( '/https?:\/\//', '', $siteurl ) != $js_url['host'] ) ) )
+ if ( ( isset( $js_url['host'] ) && ( preg_replace( '/https?:\/\//', '', $siteurl ) != $js_url['host'] ) ) ) {
+ $external_scripts[] = $handle;
$do_concat = false;
+ }

// Concat and canonicalize the paths only for
// existing scripts that aren't outside ABSPATH
@@ -114,8 +117,17 @@ class WPcom_JS_Concat extends WP_Scripts {

foreach ( $javascripts as $js_array ) {
if ( 'do_item' == $js_array['type'] ) {
- if ( $this->do_item( $js_array['handle'], $group ) )
+ // Disable "concat" for external scripts, otherwise do_item() may print it at the wrong time
+ if ( in_array( $js_array['handle'], $external_scripts, true ) ) {
+ $this->do_concat = false;
+ }
+ if ( $this->do_item( $js_array['handle'], $group ) ) {
$this->done[] = $js_array['handle'];
+ }
+ // Re-enable "concat"
+ if ( in_array( $js_array['handle'], $external_scripts, true ) ) {
+ $this->do_concat = true;
+ }
} else if ( 'concat' == $js_array['type'] ) {
array_map( array( $this, 'print_extra_script' ), $js_array['handles'] );

```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start by reading do_item() in jsconcat.php alongside the referenced WordPress class.wp-scripts.php implementation. Trace how external scripts and their dependencies are processed, then verify that external tags are emitted in dependency order while concatenated scripts still behave as before.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, php, wordpress
Domain
backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.