Make WordPress Core

Opened 11 years ago

Last modified 14 months ago

#36448 new defect (bug)

When concatenating scripts in script-loader dependencies may not be honoured.

Reported by: gitlost Owned by:
Priority: normal Milestone:
Component: Script Loader Version: 4.4.2
Severity: normal Keywords:
Cc: Focuses: javascript

Description

This is a follow on from 36392#comment:13. Ticket #15833 looks to be related.

When concatenating scripts, dependencies may not be honoured for scripts that trigger the use of $print_html (ie those outside the default directories or having conditionals or (if available!) inline scripts).

For instance this test where three depends on one and one has a conditional:

<?php
public function test_wp_script_conditional_concat_dependency() {
        global $wp_scripts;

        $wp_scripts->do_concat = true;
        $wp_scripts->default_dirs = array( '/directory/' );
        $wp_scripts->default_version = 1;

        wp_enqueue_script( 'one', '/directory/one.js', array(), 1 );
        wp_enqueue_script( 'two', '/directory/two.js', array(), 1 );
        wp_enqueue_script( 'three', '/directory/three.js', array( 'one' ), 1 );

        wp_script_add_data( 'one', 'conditional', 'blah' );

        $wp_print_scripts = get_echo( 'wp_print_scripts' );
        $print_scripts = get_echo( '_print_scripts' );

        $expected  = "<!--[if blah]>\n";
        $expected .= "<script type='text/javascript' src='/directory/one.js?ver=1'></script>\n";
        $expected .= "<![endif]-->\n";
        $expected .= "<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&amp;load%5B%5D=two,three&amp;ver=1'></script>\n";

        $this->assertEquals( $expected, $print_scripts );
        $this->assertEquals( '', $wp_print_scripts );
}

will fail, as the conditional stuff will always be outputted after the concatenated scripts, resulting in one appearing after three:

<script type='text/javascript' src='/wp-admin/load-scripts.php?c=0&amp;load%5B%5D=two,three&amp;ver=1'></script>
<!--[if blah]>
<script type='text/javascript' src='/directory/one.js?ver=1'></script>
<![endif]-->

The same is also true, mutatis mutandis (ahem), for styles, but I'll open a separate ticket for that.

Attachments (1)

36448.batch.3.patch​ (28.0 KB ) - added by gitlost 11 years ago.
Version of "batch" demo with better treatment of before/after inline code.

Download all attachments as: .zip

Change History (3)

@gitlost
11 years ago

Version of "batch" demo with better treatment of before/after inline code.

#1 @gitlost
11 years ago

The above patch is version 3 (see #36392 "batch" patches) of a way around this, that puts the concatenate data into batches ordered by dependency, and then outputs each batch in order.

It includes all the previous tests plus some extra.

It also includes the jquery-core tests by @jeremyfelt (36392#comment:19), for which this version of the batching has been changed to pass, and shows that my 36392#comment:14 re being unwanted behaviour was false.

#2 @jonsurrell
14 months ago

  • Focuses javascript added

Are conditional scripts necessary to reproduce this?

#63821 proposes dropping support for conditional scripts.

Note: See TracTickets for help on using tickets.