is_valid_strategy and _doing_wrong check #56

kt-12 · 2023-04-13T15:35:14Z

Trac ticket:

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

10upsimon

@kt-12 minor feedback comments from my side, mostly looking good to me though!

src/wp-includes/class-wp-scripts.php

tests/phpunit/tests/dependencies/scripts.php

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

adamsilverstein · 2023-04-14T16:50:34Z

src/wp-includes/class-wp-scripts.php

+	 * @return bool True if the strategy is of an allowed type, false otherwise.
+	 */
+	private function is_valid_strategy( $strategy ) {
+		$allowed_strategies = array( 'blocking', 'defer', 'async' );


Could a filter be applied here so plugins could add experimental strategies like 'worker'? Could also be added later, but I'd love to ensure this is extensible.

Personally, I'd be ok with leaving this as is for the initial release and add the additional filter based on developer feedback rather than adding it already. I believe if someone really had a use case where they were wanting to support additional strategies, they would be better off extending the whole class and replacing the global WP_Scripts object, rather than trying to filter Core's implementation.

Agree that a filter here could be useful, but we would also need to consider the side effects if someone were to filter this list to remove some/all of our supported strategies as a defensive move. Perhaps only make the filter additive, like this pseudocode example?

$additional_strategies = apply_filter( 'wp_add_script_loading_strategy', array() ) $allowed_strategies = array_merge( array( 'blocking', 'defer', 'async' ), $additional_strategies );

I share @joemcgill's sentiment here, but from a slightly different perspective of what does it mean for the rest of the process? We do a lot of deciding on what the most eligible/applicable strategy for a script is based on the known strategies of defer, async or blocking. What does it means for the dependency tree when a developer introduces a custom/other strategy? Does it mean we need to abort any logical checks and simply output the custom strategy? I have the same question for inline script handling.

I agree with @joemcgill here. There were many instances in the past where people created a plugin to stop a feature; the classic example is Classic editor plugins for Gutenberg. Even though this is not the same case, there will be some people who want to seize this opportunity and build custom plugins to do so. Core strategies should not be removed at all, as they can opt-out by not using any strategy. Again, as @10upsimon rightly suggested, there is much more to consider in terms of expected behaviour before we add this filter.

joemcgill

Looks good to me pending resolution of whether we want to add a filter. That could also be handled in a follow-up task, in my opinion

joemcgill · 2023-04-18T18:20:55Z

src/wp-includes/class-wp-scripts.php

+	 * @return bool True if the strategy is of an allowed type, false otherwise.
+	 */
+	private function is_valid_strategy( $strategy ) {
+		$allowed_strategies = array( 'blocking', 'defer', 'async' );


Personally, I'd be ok with leaving this as is for the initial release and add the additional filter based on developer feedback rather than adding it already. I believe if someone really had a use case where they were wanting to support additional strategies, they would be better off extending the whole class and replacing the global WP_Scripts object, rather than trying to filter Core's implementation.

Agree that a filter here could be useful, but we would also need to consider the side effects if someone were to filter this list to remove some/all of our supported strategies as a defensive move. Perhaps only make the filter additive, like this pseudocode example?

$additional_strategies = apply_filter( 'wp_add_script_loading_strategy', array() ) $allowed_strategies = array_merge( array( 'blocking', 'defer', 'async' ), $additional_strategies );

joemcgill · 2023-04-18T18:24:26Z

tests/phpunit/tests/dependencies/scripts.php

+	 *
+	 * @ticket 12009
+	 */
+	public function test_script_strategy_doing_it_wrong() {


joemcgill

One question, but looks good to me. Thanks!

joemcgill · 2023-04-20T14:03:41Z

src/wp-includes/class-wp-scripts.php

@@ -863,6 +863,18 @@ private function get_dependents( $handle ) {
 		return $dependents;
 	}

+	/**
+	 * Determines if a scripts defined loading strategy is valid.


Suggested change

* Determines if a scripts defined loading strategy is valid.

* Determines if a script loading strategy is valid.

@joemcgill @kt-12 I addressed this as part of a recent commit. I also changed the param description to be more reflective of your suggestion @joemcgill

joemcgill · 2023-04-20T14:06:05Z

src/wp-includes/class-wp-scripts.php

@@ -871,8 +883,24 @@ private function get_dependents( $handle ) {
 	 */
 	private function get_intended_strategy( $handle ) {
 		$script_args = $this->get_data( $handle, 'script_args' );
+		$strategy    = isset( $script_args['strategy'] ) ? $script_args['strategy'] : false;


What happens here is someone passes an empty string or false as a strategy? Do we ever hit this codepath or do we bail beforehand?

@joemcgill passing an empty or otherwise illegal value will have no ill bearing on the process. If no strategy is passed, it will still proceed to the next part of logic, which calls $this->is_valid_strategy( $strategy ). At this point, it will fail and return false, and the default blocking strategy will be used as part of the get_eligible_loading_strategy() method call.

Arguably, we are probably better off with a ! empty( $script_args['strategy'] ) as opposed to the current isset( $script_args['strategy'] ) check, as an empty string would still default to true for an isset() check, but either one is not changing the final outcome 2 lines down from the check.

The only side effect of using ! empty() would be if 'strategy' as a key was never set, but that is pretty much impossible in our logic given that we normalize the args. Either way though, what we have now is logical so I'm going to proceed with this merge, so that we have a final codebase in the base PR to do one final test against.

…strategy method

is_valid_script

4e59471

kt-12 requested a review from 10upsimon April 13, 2023 15:35

kt-12 added the Script Loading Strategy Issues relating to the Script Loading Strategy (WPP) label Apr 13, 2023

kt-12 mentioned this pull request Apr 13, 2023

DRAFT: PR for enhancing WP_Scripts with a loading strategy #54

Closed

kt-12 added 5 commits April 13, 2023 16:38

fix grammar

a75c67d

bug fix get_intended_strategy

bfc27f9

return false if invalid strategy used.

06514c4

invalid script strategy test

59d2503

script strategy doing it wrong.

fb93f94

kt-12 marked this pull request as ready for review April 14, 2023 04:16

kt-12 requested review from adamsilverstein and joemcgill April 14, 2023 04:16

10upsimon requested changes Apr 14, 2023

View reviewed changes

kt-12 and others added 4 commits April 14, 2023 11:57

code cleanup

909714e

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

fix grammar.

4534440

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

fix grammar

f5d4b55

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

update comment

4be6d42

Co-authored-by: Simon Dowdles <72872375+10upsimon@users.noreply.github.com>

10upsimon approved these changes Apr 14, 2023

View reviewed changes

adamsilverstein reviewed Apr 14, 2023

View reviewed changes

joemcgill approved these changes Apr 18, 2023

View reviewed changes

joemcgill approved these changes Apr 20, 2023

View reviewed changes

Updated description and param decription within docblock of is_valid_…

8549c52

…strategy method

10upsimon merged commit 281aadc into feature/enhance-wp-scripts-api-with-a-loading-strategy Apr 21, 2023

10upsimon deleted the enhancement/add_valid_strategy_function branch April 21, 2023 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

is_valid_strategy and _doing_wrong check #56

is_valid_strategy and _doing_wrong check #56

kt-12 commented Apr 13, 2023

10upsimon left a comment

adamsilverstein Apr 14, 2023

joemcgill Apr 18, 2023

10upsimon Apr 19, 2023

kt-12 Apr 20, 2023 •

edited

Loading

joemcgill left a comment

joemcgill Apr 18, 2023

joemcgill Apr 18, 2023

joemcgill left a comment

joemcgill Apr 20, 2023

10upsimon Apr 21, 2023

joemcgill Apr 20, 2023

10upsimon Apr 21, 2023

10upsimon Apr 21, 2023

	* Determines if a scripts defined loading strategy is valid.
	* Determines if a script loading strategy is valid.

is_valid_strategy and _doing_wrong check #56

is_valid_strategy and _doing_wrong check #56

Conversation

kt-12 commented Apr 13, 2023

10upsimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kt-12 Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kt-12 Apr 20, 2023 •

edited

Loading