Abort script strategy logical checks when CONCATENATE_SCRIPTS is true

[10upsimon]

Description

Script concatenation, although rarely used, can be enabled in WordPress via the CONCATENATE_SCRIPTS config constant. When enabled, scripts will be concatenated by WordPress and served as a single response. This makes handling the script loading strategies at a per script level unfeasible. When script concatenation is enabled for a WordPress site, the script loading strategy should be treated as the default blocking state.

Acceptance Criteria

• Enhance the existing WP_Scripts class in areas that currently handle checks for the new $args array (where strategies are defined) and return early if CONCATENATE_SCRIPTS is defined as true.

Unit Tests

• Unit tests created as part of Milestones 2 and 3 should be enhanced and assert that the strategy be blocking for any/all script handles when CONCATENATE_SCRIPTS is defined as true.

Designs

No response

Describe alternatives you've considered

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[kt-12]

@10upsimon This makes sense. LGTM!

[eclarke1]

Assigning to @joemcgill to please check ACs here. Once happy, we can get that issue moved to 'To Do' and formulate a plan for engineering alongside the remaining 'Milestone 3' issues please.

[joemcgill]

This seems correct from my PoV too. Thanks, @10upsimon.

[10upsimon]

@joemcgill @kt-12 @felixarntz I started looking into this, and after some confusion and core code hunting, I actually think there may be no work needed here.

• For starters, this flag/constant and script concat in general only applies to the WordPress admin area
• By default, script concatenation is enabled so there is no need to even define this as true and in fact quite the opposite you'd define it as false if having script concat issues in the admin area
• It only applies to editor scripts, not general scripts enqueued for admin use
• Even with default concatenation enabled, it is not applied when registering and enqueuing scripts via the admin_enqueue_scripts

Given that our work touches on the wp_register_script and wp_enqueue_script functions only, and taking into account the above points, I don't actually believe there is any work we need to do here.

Please let me know if you feel I've missed the boat on this one, but I don't believe I have.

[spacedmonkey]

I think we should also look into #57836 while looking at this issue.

[adamsilverstein]

Given that our work touches on the wp_register_script and wp_enqueue_script functions only, and taking into account the above points, I don't actually believe there is any work we need to do here.

This seems fine for now given the current scope. I would like to see this feature expanded in the future to cover admin scripts as well so we can leverage it in wp-admin.

By default, script concatenation is enabled so there is no need to even define this as true and in fact quite the opposite you'd define it as false if having script concat issues in the admin area

I'm not sure concatenation is still considered best practice given how browsers load resources these days, it would be worth considering changing this.

[joemcgill]

I've spent some time reading through a lot of the logic that is effected by the CONCATENATE_SCRIPTS constant, and I think there is more here to consider than just the value of that constant. In reality, we need to enhance the WP_Scripts::do_item method, so it is aware of loading strategies when deciding if the WP_Scripts::do_concat property is true. While the CONCATENATE_SCRIPTS constant is one of the factors that ends up setting the do_concat property, it's not the only way that property can be set to true, and when true, that property will affect the way scripts are handled. There is already some logic in WP_Scripts::do_item that stops concatenation mid-process under certain conditions (see reference). We will likely need to do something similar to check certain conditions related to async/defer attributes.

Also, I have assumed that the changes we're making can also impact admin scripts if someone calls wp_enqueue_script from the admin_enqueue_scripts hook (or any non-standard method). Did I miss something in the requirements that ensures this is only added in non-admin contexts?

By default, script concatenation is enabled

This misses some of the additional complexities here:

• WordPress determines whether concatenation is enabled based on a global $concatenate_scripts variable that could be set/changed at any time, even though the main way this global is set is via the CONCATENATE_SCRIPTS constant. (reference)
• If not set at all the default is true except for the following conditions (in script_concat_settings()):
  • Script debug is true
  • Is not admin or a login form
• The $concatenate_scripts global determines which script at files are registered by tinymce in wp_register_tinymce_scripts, and possibly other code in core or third party integrations, so if we're not completely deprecating this global, we'll at least need to think of some testing scenarios for this.

[westonruter]

Have you seen Core-57548 (Stop concatenating scripts and stylesheets in wp-admin and retire load-scripts.php and load-styles.php)? If script concatenation were removed entirely from core, this would seem to greatly simplify the considerations needed for Script Loading Strategies. The two changes could ship in the same release.

[jonoalderson]

+1 for removing these and killing them with fire.

[felixarntz]

I would suggest we move this conversation over to the Trac ticket to get that moving. Discussing in this fork isn't really good for visibility, and also in terms of the script loading strategy work this fork is more or less retired now that there's the core PR WordPress#4391 and discussion and subsequent work is happening there.