Skip to content

Commit df6561f

Browse files
ciobanu0151ciobanu0151
committed
Fixed performance issue from ticket 64194 for filter_eligible_strategies function
Added tests for both functions
1 parent 3878d18 commit df6561f

File tree

2 files changed

+132
-2
lines changed

2 files changed

+132
-2
lines changed

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1000,9 +1000,14 @@ private function get_eligible_loading_strategy( $handle ) {
10001000
* @param string $handle The script handle.
10011001
* @param string[]|null $eligible_strategies Optional. The list of strategies to filter. Default null.
10021002
* @param array<string, true> $checked Optional. An array of already checked script handles, used to avoid recursive loops.
1003+
* @param array<string, string[]> $stored_results Optional. An array of already computed eligible loading strategies by handle, used to increase performance in large dependency lists.
10031004
* @return string[] A list of eligible loading strategies that could be used.
10041005
*/
1005-
private function filter_eligible_strategies( $handle, $eligible_strategies = null, $checked = array() ) {
1006+
private function filter_eligible_strategies( $handle, $eligible_strategies = null, $checked = array(), array &$stored_results = array() ) {
1007+
if ( isset( $stored_results[ $handle ] ) && ! empty( $stored_results[ $handle ] ) ) {
1008+
return $stored_results[ $handle ];
1009+
}
1010+
10061011
// If no strategies are being passed, all strategies are eligible.
10071012
if ( null === $eligible_strategies ) {
10081013
$eligible_strategies = $this->delayed_strategies;
@@ -1055,7 +1060,7 @@ private function filter_eligible_strategies( $handle, $eligible_strategies = nul
10551060

10561061
$eligible_strategies = $this->filter_eligible_strategies( $dependent, $eligible_strategies, $checked );
10571062
}
1058-
1063+
$stored_results[ $handle ] = $eligible_strategies;
10591064
return $eligible_strategies;
10601065
}
10611066

‎tests/phpunit/tests/dependencies/scripts.php‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1435,6 +1435,66 @@ public function test_fetchpriority_bumping_a_to_z() {
14351435
$this->assertEqualHTML( $expected, $actual, '<body>', "Snapshot:\n$actual" );
14361436
}
14371437

1438+
/**
1439+
* @ticket 64194
1440+
*
1441+
* Tests that `WP_Scripts::get_highest_fetchpriority_with_dependents()` correctly
1442+
* reuses cached results (`$stored_results`) for shared dependencies in a diamond-shaped graph.
1443+
*
1444+
* Dependency Graph:
1445+
*
1446+
* D <- [B, C]
1447+
* B <- [A]
1448+
* C <- [A]
1449+
* A <- []
1450+
*
1451+
* This verifies that when multiple dependents share a common dependency (`D`),
1452+
* the cached result for `D` is used rather than recalculating it multiple times.
1453+
* @covers WP_Scripts::get_highest_fetchpriority_with_dependents
1454+
*/
1455+
1456+
public function test_highest_fetchpriority_with_dependents_uses_cached_result_for_shared_dependency() {
1457+
$wp_scripts = new WP_Scripts();
1458+
1459+
/*
1460+
* Register scripts forming a diamond-shaped dependency graph:
1461+
* D is the shared dependent of B and C, and A depends on both B and C.
1462+
*/
1463+
$wp_scripts->add( 'a', 'https://example.com/a.js', array( 'b', 'c' ) );
1464+
$wp_scripts->add( 'b', 'https://example.com/b.js', array( 'd' ) );
1465+
$wp_scripts->add( 'c', 'https://example.com/c.js', array( 'd' ) );
1466+
$wp_scripts->add( 'd', 'https://example.com/d.js' );
1467+
1468+
// Enqueue all scripts so they are considered active ("enqueued").
1469+
foreach ( array( 'a', 'b', 'c', 'd' ) as $handle ) {
1470+
$wp_scripts->enqueue( $handle );
1471+
}
1472+
1473+
$wp_scripts->add_data( 'a', 'fetchpriority', 'auto' );
1474+
$wp_scripts->add_data( 'b', 'fetchpriority', 'low' );
1475+
$wp_scripts->add_data( 'c', 'fetchpriority', 'low' );
1476+
$wp_scripts->add_data( 'd', 'fetchpriority', 'low' );
1477+
1478+
/*
1479+
* Simulate a pre-existing `$stored_results` cache entry for `d`.
1480+
* If the caching logic works, the function should use this "high" value
1481+
* instead of recalculating based on the actual (lower) value.
1482+
*/
1483+
$stored_results = array( 'd' => 'high' );
1484+
1485+
// Access the private method using reflection.
1486+
$method = new ReflectionMethod( WP_Scripts::class, 'get_highest_fetchpriority_with_dependents' );
1487+
$method->setAccessible( true );
1488+
1489+
// Pass `$stored_results` BY REFERENCE.
1490+
$result = $method->invokeArgs( $wp_scripts, array( 'd', array(), &$stored_results ) );
1491+
1492+
$this->assertSame(
1493+
'high',
1494+
$result,
1495+
'Expected "high" indicates that the cached `$stored_results` entry for D was used instead of recalculating.'
1496+
);
1497+
}
14381498
/**
14391499
* Tests that printing a script without enqueueing has the same output as when it is enqueued.
14401500
*
@@ -1533,7 +1593,72 @@ public function test_loading_strategy_with_valid_blocking_registration() {
15331593
$expected = str_replace( "'", '"', $expected );
15341594
$this->assertSame( $expected, $output, 'Scripts registered with no strategy assigned, and who have no dependencies, should have no loading strategy attributes printed.' );
15351595
}
1596+
/**
1597+
* @ticket 64194
1598+
*
1599+
* Tests that `WP_Scripts::filter_eligible_strategies()` correctly reuses cached results
1600+
* for shared dependencies in a diamond-shaped dependency graph.
1601+
*
1602+
* Dependency Graph:
1603+
*
1604+
* D <- [B, C]
1605+
* B <- [A]
1606+
* C <- [A]
1607+
* A <- []
1608+
*
1609+
* In this scenario, both B and C depend on D, and A depends on both B and C.
1610+
* The goal is to confirm that when `$stored_results` already contains an entry for D,
1611+
* the cached value is reused instead of recalculating the strategies for D multiple times.
1612+
*/
1613+
public function test_filter_eligible_strategies_uses_cached_result_for_shared_dependency() {
1614+
$wp_scripts = new WP_Scripts();
1615+
1616+
/*
1617+
* Register scripts forming a diamond-shaped dependency graph:
1618+
* D is the shared dependent of B and C, and A depends on both B and C.
1619+
*/
1620+
$wp_scripts->add( 'a', 'https://example.com/a.js', array( 'b', 'c' ) );
1621+
$wp_scripts->add( 'b', 'https://example.com/b.js', array( 'd' ) );
1622+
$wp_scripts->add( 'c', 'https://example.com/c.js', array( 'd' ) );
1623+
$wp_scripts->add( 'd', 'https://example.com/d.js' );
1624+
1625+
// Enqueue all scripts so they are treated as active/enqueued.
1626+
foreach ( array( 'a', 'b', 'c', 'd' ) as $handle ) {
1627+
$wp_scripts->enqueue( $handle );
1628+
}
15361629

1630+
/*
1631+
* Assign strategies:
1632+
* - A: async
1633+
* - B: async
1634+
* - C: async
1635+
* - D: async
1636+
*/
1637+
$wp_scripts->add_data( 'a', 'strategy', 'defer' );
1638+
$wp_scripts->add_data( 'b', 'strategy', 'defer' );
1639+
$wp_scripts->add_data( 'c', 'strategy', 'defer' );
1640+
$wp_scripts->add_data( 'd', 'strategy', 'async' );
1641+
1642+
/*
1643+
* Simulate a cached result in `$stored_results` for D.
1644+
* If caching logic is functioning properly, this cached value
1645+
* should be returned immediately without recomputing.
1646+
*/
1647+
$stored_results = array( 'd' => array( 'async' ) );
1648+
1649+
// Access the private method via reflection.
1650+
$method = new ReflectionMethod( WP_Scripts::class, 'filter_eligible_strategies' );
1651+
$method->setAccessible( true );
1652+
1653+
// Invoke the method with `$stored_results` passed by reference.
1654+
$result = $method->invokeArgs( $wp_scripts, array( 'd', null, array(), &$stored_results ) );
1655+
1656+
$this->assertSame(
1657+
array( 'async' ),
1658+
$result,
1659+
'Expected cached `$stored_results` value for D to be reused instead of recomputed.'
1660+
);
1661+
}
15371662
/**
15381663
* Tests that scripts registered for the head do indeed end up there.
15391664
*

0 commit comments

Comments
 (0)