Skip to content

Commit 5fd5a8e

Browse files
jorgefilipecostajorgefilipecosta
committed
View config: reject shape-mismatched merges, define empty-array semantics, strip nulls from appended members.
Fixes three silent data-loss defects in WP_View_Config_Data's merge engine: - An associative patch value over a list (or a non-empty list over an associative value) discarded the whole current value. It is now rejected with _doing_it_wrong() and the current value is kept. - An empty array under merge() wiped associative values but no-oped on lists. It is now a no-op for both shapes — clear a list with replace() and an empty list, reset a key with null. - A list member appended by merge() kept nested nulls that every other write path drops. Appended members now go through strip_nulls(). Follow-up to [62825]. Props jorgefilipecosta, oandregal. See #65577. git-svn-id: https://develop.svn.wordpress.org/trunk@62834 602fd350-edb4-49c9-b593-d223f7449a82
1 parent b0e62d8 commit 5fd5a8e

2 files changed

Lines changed: 287 additions & 3 deletions

File tree

‎src/wp-includes/class-wp-view-config-data.php‎

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,10 @@
4040
* key by key (an associative array merges member by member, a nested `null`
4141
* deletes just that leaf, a scalar replaces just that value), while `set()`
4242
* swaps the whole value. A nested `null` deletes just the leaf it names in
43-
* every case. Each patch also declares the configuration schema
43+
* every case. A patch value whose shape does not match the current value —
44+
* an associative array where a list lives, or the reverse — is rejected with
45+
* a notice rather than merged, and an empty array under `merge()` is a
46+
* no-op. Each patch also declares the configuration schema
4447
* version it was written against (currently 1), so a future WordPress release
4548
* that changes the configuration shape can migrate existing patches forward
4649
* instead of breaking them.
@@ -308,6 +311,12 @@ public function remove( array $spec, int $version ) {
308311
* stops inheriting core's future additions to it — but it's useful when a
309312
* contributor needs to pin a list to an exact set of members.
310313
*
314+
* The shape rule applies here too: a patch value whose shape does not match
315+
* the current value — an associative array where a list lives, or a
316+
* non-empty list where an associative value lives — is rejected with a
317+
* notice and leaves the current value unchanged. An empty array is exempt,
318+
* so replacing a list with an empty list still clears it.
319+
*
311320
* A patch that declares an unsupported schema version is rejected and does
312321
* not change anything.
313322
*
@@ -354,6 +363,13 @@ public function replace( array $patch, int $version ) {
354363
* - default_layouts will be updated so that newField is appended to the badgeFields.
355364
* - view_list will be updated so that the view with slug 'table' has its title changed to 'New title'.
356365
*
366+
* A patch value only merges into a current value of the same shape: an
367+
* associative array where a list lives, or a non-empty list where an
368+
* associative value lives, is rejected with a notice and leaves the current
369+
* value unchanged. An empty array merges nothing and is a no-op — clear a
370+
* list with replace() and an empty list, or reset a key to its default with
371+
* a top-level `null`.
372+
*
357373
* A patch that declares an unsupported schema version is rejected and does
358374
* not change anything.
359375
*
@@ -477,6 +493,15 @@ private function strip_nulls( $value ) {
477493
* $replace_lists flag is carried down through associative nesting so that,
478494
* under replace(), every list reached along the way is swapped wholesale.
479495
*
496+
* An array in $incoming only merges into a current value of the same shape.
497+
* A non-empty mismatch — an associative array where a list lives, or a
498+
* non-empty list where an associative value lives — is reported with
499+
* _doing_it_wrong() and leaves the current value unchanged, so a malformed
500+
* patch cannot silently destroy configuration. An empty array is
501+
* shape-ambiguous and merges nothing, so it is a no-op: clearing a list is
502+
* spelled replace() with an empty list, and resetting a key is spelled
503+
* `null`.
504+
*
480505
* @since 7.1.0
481506
*
482507
* @param mixed $current The current value.
@@ -493,20 +518,48 @@ private function merge_properties( $current, $incoming, $replace_lists ) {
493518

494519
// Numerical indexed arrays are expected to be lists (sequential integer keys starting at 0).
495520
if ( array_is_list( $incoming ) ) {
521+
// A non-empty list only lands where a list (or nothing) lives, under
522+
// merge() and replace() alike. An empty array is shape-ambiguous and
523+
// exempt, so replace() with an empty list can still clear a list.
524+
if ( array() !== $incoming && is_array( $current ) && ! array_is_list( $current ) && array() !== $current ) {
525+
_doing_it_wrong(
526+
__METHOD__,
527+
esc_html__( 'A view configuration patch value must match the shape of the value it patches: a list merges into a list, and an associative array into an associative array.' ),
528+
'7.1.0'
529+
);
530+
return $current;
531+
}
532+
496533
// replace() takes an incoming list as-is; merge() merges it by member identity.
497534
if ( $replace_lists ) {
498535
// As-is except for nulls: a list swapped in wholesale has no
499536
// existing leaf for a null to delete (the same rationale as
500537
// set()), so a null member is dropped rather than stored.
501538
return $this->strip_nulls( $incoming );
502539
}
540+
541+
// An empty list has no members to merge, and an empty array is
542+
// shape-ambiguous, so merging one is a no-op rather than a reset.
543+
if ( array() === $incoming ) {
544+
return $current;
545+
}
546+
503547
return $this->merge_list_by_identity(
504548
is_array( $current ) && array_is_list( $current ) ? $current : array(),
505549
$incoming
506550
);
507551
}
508552

509553
// Consider any other array as associative (keys are strings).
554+
if ( is_array( $current ) && array_is_list( $current ) && array() !== $current ) {
555+
_doing_it_wrong(
556+
__METHOD__,
557+
esc_html__( 'A view configuration patch value must match the shape of the value it patches: a list merges into a list, and an associative array into an associative array.' ),
558+
'7.1.0'
559+
);
560+
return $current;
561+
}
562+
510563
$result = is_array( $current ) && ! array_is_list( $current ) ? $current : array();
511564
foreach ( $incoming as $key => $value ) {
512565
// A null patch value deletes the property.
@@ -603,7 +656,9 @@ private function remove_list_member( array $members, $identity ) {
603656
* A member of the incoming list whose identity matches one already present
604657
* merges into it in place, keeping its position; an unmatched member is
605658
* appended to the end, except a literal `null`, which carries no identity
606-
* and holds nothing to merge and so is dropped. A matched member's contents
659+
* and holds nothing to merge and so is dropped. An appended member has no
660+
* existing leaf for a nested `null` to delete (the same rationale as set()),
661+
* so its nulls are stripped rather than stored. A matched member's contents
607662
* merge recursively with the same rules (merge_properties), so the
608663
* identity-aware merge applies at
609664
* any nesting level: each key named by the patch is substituted while the
@@ -639,7 +694,9 @@ private function merge_list_by_identity( array $current, array $incoming ) {
639694
}
640695
}
641696
if ( null === $index ) {
642-
$result[] = $item;
697+
// An appended member has no existing leaf for a nested null to
698+
// delete, so nulls are dropped rather than stored.
699+
$result[] = $this->strip_nulls( $item );
643700
continue;
644701
}
645702

‎tests/phpunit/tests/view-config-data.php‎

Lines changed: 227 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1721,6 +1721,233 @@ public function test_merge_rejects_unknown_key() {
17211721
$this->assertSame( array( 'default_view' => array( 'type' => 'table' ) ), self::read_config( $data ) );
17221722
}
17231723

1724+
/**
1725+
* merge() rejects an associative patch value where a list lives: the shapes
1726+
* do not line up, so merging would have to guess what the string keys mean.
1727+
* The current list survives untouched instead of being discarded.
1728+
*
1729+
* @ticket 65577
1730+
*
1731+
* @covers ::merge
1732+
*/
1733+
public function test_merge_rejects_associative_patch_over_a_list() {
1734+
$this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge_properties' );
1735+
1736+
$data = new WP_View_Config_Data(
1737+
array(
1738+
'view_list' => array(
1739+
array(
1740+
'slug' => 'all',
1741+
'title' => 'All items',
1742+
),
1743+
),
1744+
)
1745+
);
1746+
$before = self::read_config( $data );
1747+
1748+
// The pre-7.1 slug-keyed shape, not the documented list of members.
1749+
$data->merge(
1750+
array(
1751+
'view_list' => array(
1752+
'published' => array( 'title' => 'Live' ),
1753+
),
1754+
),
1755+
1
1756+
);
1757+
1758+
$this->assertSame( $before, self::read_config( $data ) );
1759+
}
1760+
1761+
/**
1762+
* merge() rejects a non-empty list patch value where an associative value
1763+
* lives, the mirror of the associative-over-list mismatch: the current map
1764+
* survives untouched instead of being discarded.
1765+
*
1766+
* @ticket 65577
1767+
*
1768+
* @covers ::merge
1769+
*/
1770+
public function test_merge_rejects_list_patch_over_an_associative_value() {
1771+
$this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge_properties' );
1772+
1773+
$data = new WP_View_Config_Data(
1774+
array(
1775+
'default_view' => array(
1776+
'sort' => array(
1777+
'field' => 'title',
1778+
'direction' => 'asc',
1779+
),
1780+
),
1781+
)
1782+
);
1783+
$before = self::read_config( $data );
1784+
1785+
$data->merge(
1786+
array(
1787+
'default_view' => array(
1788+
'sort' => array( 'title', 'asc' ),
1789+
),
1790+
),
1791+
1
1792+
);
1793+
1794+
$this->assertSame( $before, self::read_config( $data ) );
1795+
}
1796+
1797+
/**
1798+
* An empty array under merge() is a no-op for both shapes: it has no
1799+
* members to merge, and being shape-ambiguous it must not reset the
1800+
* current value either. Clearing a list is spelled replace() with an
1801+
* empty list; resetting a key is spelled null.
1802+
*
1803+
* @ticket 65577
1804+
*
1805+
* @covers ::merge
1806+
*/
1807+
public function test_merge_empty_array_is_a_noop() {
1808+
$data = new WP_View_Config_Data(
1809+
array(
1810+
'default_view' => array(
1811+
'filters' => array(
1812+
array(
1813+
'field' => 'author',
1814+
'operator' => 'isAny',
1815+
),
1816+
),
1817+
'sort' => array(
1818+
'field' => 'title',
1819+
'direction' => 'asc',
1820+
),
1821+
),
1822+
)
1823+
);
1824+
$before = self::read_config( $data );
1825+
1826+
$data->merge(
1827+
array(
1828+
'default_view' => array(
1829+
'filters' => array(),
1830+
'sort' => array(),
1831+
),
1832+
),
1833+
1
1834+
);
1835+
1836+
$this->assertSame( $before, self::read_config( $data ) );
1837+
}
1838+
1839+
/**
1840+
* A nested null deletes just the leaf it names in every case, including
1841+
* inside a list member that did not exist yet: an appended member has no
1842+
* existing leaf to delete, so its nulls are dropped rather than stored
1843+
* (the same rationale as set() and the lists replace() swaps in).
1844+
*
1845+
* @ticket 65577
1846+
*
1847+
* @covers ::merge
1848+
*/
1849+
public function test_merge_appended_member_drops_nested_nulls() {
1850+
$data = new WP_View_Config_Data(
1851+
array(
1852+
'view_list' => array(
1853+
array(
1854+
'slug' => 'all',
1855+
'title' => 'All items',
1856+
),
1857+
),
1858+
)
1859+
);
1860+
$data->merge(
1861+
array(
1862+
'view_list' => array(
1863+
array(
1864+
'slug' => 'mine',
1865+
'view' => array( 'filters' => null ),
1866+
),
1867+
),
1868+
),
1869+
1
1870+
);
1871+
1872+
$this->assertSame(
1873+
array(
1874+
'view_list' => array(
1875+
array(
1876+
'slug' => 'all',
1877+
'title' => 'All items',
1878+
),
1879+
array(
1880+
'slug' => 'mine',
1881+
'view' => array(),
1882+
),
1883+
),
1884+
),
1885+
self::read_config( $data )
1886+
);
1887+
}
1888+
1889+
/**
1890+
* replace() rejects a non-empty list patch value where an associative value
1891+
* lives, the same rule merge() enforces: a list in the patch replaces the
1892+
* current list wholesale, but it cannot land where a map lives. The current
1893+
* map survives untouched instead of being discarded.
1894+
*
1895+
* @ticket 65577
1896+
*
1897+
* @covers ::replace
1898+
*/
1899+
public function test_replace_rejects_list_patch_over_an_associative_value() {
1900+
$this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge_properties' );
1901+
1902+
$data = new WP_View_Config_Data(
1903+
array(
1904+
'default_view' => array(
1905+
'sort' => array(
1906+
'field' => 'title',
1907+
'direction' => 'asc',
1908+
),
1909+
),
1910+
)
1911+
);
1912+
$before = self::read_config( $data );
1913+
1914+
$data->replace(
1915+
array(
1916+
'default_view' => array(
1917+
'sort' => array( 'title', 'asc' ),
1918+
),
1919+
),
1920+
1
1921+
);
1922+
1923+
$this->assertSame( $before, self::read_config( $data ) );
1924+
}
1925+
1926+
/**
1927+
* An empty array is exempt from the shape guard, so replace() with an
1928+
* empty list stays the documented way to clear a list.
1929+
*
1930+
* @ticket 65577
1931+
*
1932+
* @covers ::replace
1933+
*/
1934+
public function test_replace_empty_list_still_clears_a_list() {
1935+
$data = new WP_View_Config_Data(
1936+
array(
1937+
'view_list' => array(
1938+
array(
1939+
'slug' => 'all',
1940+
'title' => 'All items',
1941+
),
1942+
),
1943+
)
1944+
);
1945+
1946+
$data->replace( array( 'view_list' => array() ), 1 );
1947+
1948+
$this->assertSame( array( 'view_list' => array() ), self::read_config( $data ) );
1949+
}
1950+
17241951

17251952
/**
17261953
* merge() treats a scalar list member as its own identity: an incoming

0 commit comments

Comments
 (0)