From e53eca91a30e24c5a091d17b546818d8ac4ee4a6 Mon Sep 17 00:00:00 2001 From: Jorge Costa Date: Wed, 22 Jul 2026 14:27:29 +0100 Subject: [PATCH 1/4] REST API: Fix silent data loss in view configuration merges. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit merge_properties() classified patch values by shape but let a mismatch fall through: an associative patch value landing on a list (or a list landing on an associative value) silently discarded the current value before merging into nothing, and an empty array — classified as a list — wiped associative values while no-oping on lists. An unmatched list member was also appended verbatim, storing nested nulls that every other write path consumes. A non-empty shape mismatch is now reported with _doing_it_wrong() and leaves the current value unchanged, an empty array under merge() is a documented no-op (clearing stays with replace() and null), and appended list members have their nulls stripped like every other path with no existing leaf to delete. See #65577. --- src/wp-includes/class-wp-view-config-data.php | 82 +++++++-- tests/phpunit/tests/view-config-data.php | 157 ++++++++++++++++++ 2 files changed, 224 insertions(+), 15 deletions(-) diff --git a/src/wp-includes/class-wp-view-config-data.php b/src/wp-includes/class-wp-view-config-data.php index 99ea816aee96e..68c3ec883af0b 100644 --- a/src/wp-includes/class-wp-view-config-data.php +++ b/src/wp-includes/class-wp-view-config-data.php @@ -40,7 +40,10 @@ * key by key (an associative array merges member by member, a nested `null` * deletes just that leaf, a scalar replaces just that value), while `set()` * swaps the whole value. A nested `null` deletes just the leaf it names in - * every case. Each patch also declares the configuration schema + * every case. A patch value whose shape does not match the current value — + * an associative array where a list lives, or the reverse — is rejected with + * a notice rather than merged, and an empty array under `merge()` is a + * no-op. Each patch also declares the configuration schema * version it was written against (currently 1), so a future WordPress release * that changes the configuration shape can migrate existing patches forward * instead of breaking them. @@ -352,6 +355,13 @@ public function replace( array $patch, int $version ) { * - default_layouts will be updated so that newField is appended to the badgeFields. * - view_list will be updated so that the view with slug 'table' has its title changed to 'New title'. * + * A patch value only merges into a current value of the same shape: an + * associative array where a list lives, or a non-empty list where an + * associative value lives, is rejected with a notice and leaves the current + * value unchanged. An empty array merges nothing and is a no-op — clear a + * list with replace() and an empty list, or reset a key to its default with + * a top-level `null`. + * * A patch that declares an unsupported schema version is rejected and does * not change anything. * @@ -423,7 +433,7 @@ private function apply( array $patch, int $version, $method, $mode ) { // nested null still drops the property it names. $this->config[ $key ] = 'set' === $mode ? $this->strip_nulls( $value ) - : $this->merge_properties( $this->config[ $key ] ?? array(), $value, 'replace' === $mode ); + : $this->merge_properties( $this->config[ $key ] ?? array(), $value, 'replace' === $mode, $method ); } return $this; @@ -475,15 +485,25 @@ private function strip_nulls( $value ) { * $replace_lists flag is carried down through associative nesting so that, * under replace(), every list reached along the way is swapped wholesale. * + * An array in $incoming only merges into a current value of the same shape. + * A non-empty mismatch — an associative array where a list lives, or a + * non-empty list where an associative value lives — is reported with + * _doing_it_wrong() and leaves the current value unchanged, so a malformed + * patch cannot silently destroy configuration. An empty array is + * shape-ambiguous and merges nothing, so it is a no-op: clearing a list is + * spelled replace() with an empty list, and resetting a key is spelled + * `null`. + * * @since 7.1.0 * - * @param mixed $current The current value. - * @param mixed $incoming The incoming value. - * @param bool $replace_lists Whether a list in $incoming replaces the current list - * wholesale instead of merging into it by member identity. + * @param mixed $current The current value. + * @param mixed $incoming The incoming value. + * @param bool $replace_lists Whether a list in $incoming replaces the current list + * wholesale instead of merging into it by member identity. + * @param string $method The public method the patch was passed to, for misuse reporting. * @return mixed The merged value. */ - private function merge_properties( $current, $incoming, $replace_lists ) { + private function merge_properties( $current, $incoming, $replace_lists, $method ) { // Scalar properties are merged as-is. if ( ! is_array( $incoming ) ) { return $incoming; @@ -498,13 +518,39 @@ private function merge_properties( $current, $incoming, $replace_lists ) { // set()), so a null member is dropped rather than stored. return $this->strip_nulls( $incoming ); } + + // An empty list has no members to merge, and an empty array is + // shape-ambiguous, so merging one is a no-op rather than a reset. + if ( array() === $incoming ) { + return $current; + } + + if ( is_array( $current ) && ! array_is_list( $current ) && array() !== $current ) { + _doing_it_wrong( + esc_html( $method ), + esc_html__( 'A list cannot be merged into an associative value; name the keys to change instead.' ), + '7.1.0' + ); + return $current; + } + return $this->merge_list_by_identity( is_array( $current ) && array_is_list( $current ) ? $current : array(), - $incoming + $incoming, + $method ); } // Consider any other array as associative (keys are strings). + if ( is_array( $current ) && array_is_list( $current ) && array() !== $current ) { + _doing_it_wrong( + esc_html( $method ), + esc_html__( 'An associative array cannot be merged into a list; address list members by their identity instead.' ), + '7.1.0' + ); + return $current; + } + $result = is_array( $current ) && ! array_is_list( $current ) ? $current : array(); foreach ( $incoming as $key => $value ) { // A null patch value deletes the property. @@ -516,7 +562,8 @@ private function merge_properties( $current, $incoming, $replace_lists ) { $result[ $key ] = $this->merge_properties( array_key_exists( $key, $result ) ? $result[ $key ] : array(), $value, - $replace_lists + $replace_lists, + $method ); } @@ -601,7 +648,9 @@ private function remove_list_member( array $members, $identity ) { * A member of the incoming list whose identity matches one already present * merges into it in place, keeping its position; an unmatched member is * appended to the end, except a literal `null`, which carries no identity - * and holds nothing to merge and so is dropped. A matched member's contents + * and holds nothing to merge and so is dropped. An appended member has no + * existing leaf for a nested `null` to delete (the same rationale as set()), + * so its nulls are stripped rather than stored. A matched member's contents * merge recursively with the same rules (merge_properties), so the * identity-aware merge applies at * any nesting level: each key named by the patch is substituted while the @@ -610,11 +659,12 @@ private function remove_list_member( array $members, $identity ) { * * @since 7.1.0 * - * @param array $current The current list. - * @param array $incoming The incoming list. + * @param array $current The current list. + * @param array $incoming The incoming list. + * @param string $method The public method the patch was passed to, for misuse reporting. * @return array The merged list. */ - private function merge_list_by_identity( array $current, array $incoming ) { + private function merge_list_by_identity( array $current, array $incoming, $method ) { $result = $current; foreach ( $incoming as $item ) { // A null member carries no identity and holds nothing to merge, @@ -637,12 +687,14 @@ private function merge_list_by_identity( array $current, array $incoming ) { } } if ( null === $index ) { - $result[] = $item; + // An appended member has no existing leaf for a nested null to + // delete, so nulls are dropped rather than stored. + $result[] = $this->strip_nulls( $item ); continue; } // Otherwise, merge the incoming member into the existing one in place. - $result[ $index ] = $this->merge_properties( $result[ $index ], $item, false ); + $result[ $index ] = $this->merge_properties( $result[ $index ], $item, false, $method ); } return $result; diff --git a/tests/phpunit/tests/view-config-data.php b/tests/phpunit/tests/view-config-data.php index 1d56adb786644..32b34c9cdbc4f 100644 --- a/tests/phpunit/tests/view-config-data.php +++ b/tests/phpunit/tests/view-config-data.php @@ -1721,6 +1721,163 @@ public function test_merge_rejects_unknown_key() { $this->assertSame( array( 'default_view' => array( 'type' => 'table' ) ), self::read_config( $data ) ); } + /** + * merge() rejects an associative patch value where a list lives: the shapes + * do not line up, so merging would have to guess what the string keys mean. + * The current list survives untouched instead of being discarded. + * + * @covers ::merge + */ + public function test_merge_rejects_associative_patch_over_a_list() { + $this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge' ); + + $data = new WP_View_Config_Data( + array( + 'view_list' => array( + array( + 'slug' => 'all', + 'title' => 'All items', + ), + ), + ) + ); + $before = self::read_config( $data ); + + // The pre-7.1 slug-keyed shape, not the documented list of members. + $data->merge( + array( + 'view_list' => array( + 'published' => array( 'title' => 'Live' ), + ), + ), + 1 + ); + + $this->assertSame( $before, self::read_config( $data ) ); + } + + /** + * merge() rejects a non-empty list patch value where an associative value + * lives, the mirror of the associative-over-list mismatch: the current map + * survives untouched instead of being discarded. + * + * @covers ::merge + */ + public function test_merge_rejects_list_patch_over_an_associative_value() { + $this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge' ); + + $data = new WP_View_Config_Data( + array( + 'default_view' => array( + 'sort' => array( + 'field' => 'title', + 'direction' => 'asc', + ), + ), + ) + ); + $before = self::read_config( $data ); + + $data->merge( + array( + 'default_view' => array( + 'sort' => array( 'title', 'asc' ), + ), + ), + 1 + ); + + $this->assertSame( $before, self::read_config( $data ) ); + } + + /** + * An empty array under merge() is a no-op for both shapes: it has no + * members to merge, and being shape-ambiguous it must not reset the + * current value either. Clearing a list is spelled replace() with an + * empty list; resetting a key is spelled null. + * + * @covers ::merge + */ + public function test_merge_empty_array_is_a_noop() { + $data = new WP_View_Config_Data( + array( + 'default_view' => array( + 'filters' => array( + array( + 'field' => 'author', + 'operator' => 'isAny', + ), + ), + 'sort' => array( + 'field' => 'title', + 'direction' => 'asc', + ), + ), + ) + ); + $before = self::read_config( $data ); + + $data->merge( + array( + 'default_view' => array( + 'filters' => array(), + 'sort' => array(), + ), + ), + 1 + ); + + $this->assertSame( $before, self::read_config( $data ) ); + } + + /** + * A nested null deletes just the leaf it names in every case, including + * inside a list member that did not exist yet: an appended member has no + * existing leaf to delete, so its nulls are dropped rather than stored + * (the same rationale as set() and the lists replace() swaps in). + * + * @covers ::merge + */ + public function test_merge_appended_member_drops_nested_nulls() { + $data = new WP_View_Config_Data( + array( + 'view_list' => array( + array( + 'slug' => 'all', + 'title' => 'All items', + ), + ), + ) + ); + $data->merge( + array( + 'view_list' => array( + array( + 'slug' => 'mine', + 'view' => array( 'filters' => null ), + ), + ), + ), + 1 + ); + + $this->assertSame( + array( + 'view_list' => array( + array( + 'slug' => 'all', + 'title' => 'All items', + ), + array( + 'slug' => 'mine', + 'view' => array(), + ), + ), + ), + self::read_config( $data ) + ); + } + /** * merge() treats a scalar list member as its own identity: an incoming From 356606ec46d6c3af9f84c94e9f4294fc17b77cfd Mon Sep 17 00:00:00 2001 From: Jorge Costa Date: Wed, 22 Jul 2026 15:04:30 +0100 Subject: [PATCH 2/4] View config: Simplify the shape-mismatch notice. The $method parameter threaded through merge_properties() and merge_list_by_identity() existed only to name the public method in the _doing_it_wrong() notice. Report via __METHOD__ with a single shape-agnostic message instead, keeping the notice actionable without the extra plumbing. See #65577. --- src/wp-includes/class-wp-view-config-data.php | 36 +++++++++---------- tests/phpunit/tests/view-config-data.php | 4 +-- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/wp-includes/class-wp-view-config-data.php b/src/wp-includes/class-wp-view-config-data.php index 68c3ec883af0b..8790356d0bf52 100644 --- a/src/wp-includes/class-wp-view-config-data.php +++ b/src/wp-includes/class-wp-view-config-data.php @@ -433,7 +433,7 @@ private function apply( array $patch, int $version, $method, $mode ) { // nested null still drops the property it names. $this->config[ $key ] = 'set' === $mode ? $this->strip_nulls( $value ) - : $this->merge_properties( $this->config[ $key ] ?? array(), $value, 'replace' === $mode, $method ); + : $this->merge_properties( $this->config[ $key ] ?? array(), $value, 'replace' === $mode ); } return $this; @@ -496,14 +496,13 @@ private function strip_nulls( $value ) { * * @since 7.1.0 * - * @param mixed $current The current value. - * @param mixed $incoming The incoming value. - * @param bool $replace_lists Whether a list in $incoming replaces the current list - * wholesale instead of merging into it by member identity. - * @param string $method The public method the patch was passed to, for misuse reporting. + * @param mixed $current The current value. + * @param mixed $incoming The incoming value. + * @param bool $replace_lists Whether a list in $incoming replaces the current list + * wholesale instead of merging into it by member identity. * @return mixed The merged value. */ - private function merge_properties( $current, $incoming, $replace_lists, $method ) { + private function merge_properties( $current, $incoming, $replace_lists ) { // Scalar properties are merged as-is. if ( ! is_array( $incoming ) ) { return $incoming; @@ -527,8 +526,8 @@ private function merge_properties( $current, $incoming, $replace_lists, $method if ( is_array( $current ) && ! array_is_list( $current ) && array() !== $current ) { _doing_it_wrong( - esc_html( $method ), - esc_html__( 'A list cannot be merged into an associative value; name the keys to change instead.' ), + __METHOD__, + 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.' ), '7.1.0' ); return $current; @@ -536,16 +535,15 @@ private function merge_properties( $current, $incoming, $replace_lists, $method return $this->merge_list_by_identity( is_array( $current ) && array_is_list( $current ) ? $current : array(), - $incoming, - $method + $incoming ); } // Consider any other array as associative (keys are strings). if ( is_array( $current ) && array_is_list( $current ) && array() !== $current ) { _doing_it_wrong( - esc_html( $method ), - esc_html__( 'An associative array cannot be merged into a list; address list members by their identity instead.' ), + __METHOD__, + 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.' ), '7.1.0' ); return $current; @@ -562,8 +560,7 @@ private function merge_properties( $current, $incoming, $replace_lists, $method $result[ $key ] = $this->merge_properties( array_key_exists( $key, $result ) ? $result[ $key ] : array(), $value, - $replace_lists, - $method + $replace_lists ); } @@ -659,12 +656,11 @@ private function remove_list_member( array $members, $identity ) { * * @since 7.1.0 * - * @param array $current The current list. - * @param array $incoming The incoming list. - * @param string $method The public method the patch was passed to, for misuse reporting. + * @param array $current The current list. + * @param array $incoming The incoming list. * @return array The merged list. */ - private function merge_list_by_identity( array $current, array $incoming, $method ) { + private function merge_list_by_identity( array $current, array $incoming ) { $result = $current; foreach ( $incoming as $item ) { // A null member carries no identity and holds nothing to merge, @@ -694,7 +690,7 @@ private function merge_list_by_identity( array $current, array $incoming, $metho } // Otherwise, merge the incoming member into the existing one in place. - $result[ $index ] = $this->merge_properties( $result[ $index ], $item, false, $method ); + $result[ $index ] = $this->merge_properties( $result[ $index ], $item, false ); } return $result; diff --git a/tests/phpunit/tests/view-config-data.php b/tests/phpunit/tests/view-config-data.php index 32b34c9cdbc4f..0c86cd5e2f87c 100644 --- a/tests/phpunit/tests/view-config-data.php +++ b/tests/phpunit/tests/view-config-data.php @@ -1729,7 +1729,7 @@ public function test_merge_rejects_unknown_key() { * @covers ::merge */ public function test_merge_rejects_associative_patch_over_a_list() { - $this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge' ); + $this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge_properties' ); $data = new WP_View_Config_Data( array( @@ -1764,7 +1764,7 @@ public function test_merge_rejects_associative_patch_over_a_list() { * @covers ::merge */ public function test_merge_rejects_list_patch_over_an_associative_value() { - $this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge' ); + $this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge_properties' ); $data = new WP_View_Config_Data( array( From b75fdfa55d9fc22d929176c62661fef4e8409031 Mon Sep 17 00:00:00 2001 From: Jorge Costa Date: Thu, 23 Jul 2026 15:32:52 +0100 Subject: [PATCH 3/4] View config: Add ticket annotations to the new merge tests. --- tests/phpunit/tests/view-config-data.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/phpunit/tests/view-config-data.php b/tests/phpunit/tests/view-config-data.php index 0c86cd5e2f87c..86fa3d39356ef 100644 --- a/tests/phpunit/tests/view-config-data.php +++ b/tests/phpunit/tests/view-config-data.php @@ -1726,6 +1726,8 @@ public function test_merge_rejects_unknown_key() { * do not line up, so merging would have to guess what the string keys mean. * The current list survives untouched instead of being discarded. * + * @ticket 65577 + * * @covers ::merge */ public function test_merge_rejects_associative_patch_over_a_list() { @@ -1761,6 +1763,8 @@ public function test_merge_rejects_associative_patch_over_a_list() { * lives, the mirror of the associative-over-list mismatch: the current map * survives untouched instead of being discarded. * + * @ticket 65577 + * * @covers ::merge */ public function test_merge_rejects_list_patch_over_an_associative_value() { @@ -1796,6 +1800,8 @@ public function test_merge_rejects_list_patch_over_an_associative_value() { * current value either. Clearing a list is spelled replace() with an * empty list; resetting a key is spelled null. * + * @ticket 65577 + * * @covers ::merge */ public function test_merge_empty_array_is_a_noop() { @@ -1836,6 +1842,8 @@ public function test_merge_empty_array_is_a_noop() { * existing leaf to delete, so its nulls are dropped rather than stored * (the same rationale as set() and the lists replace() swaps in). * + * @ticket 65577 + * * @covers ::merge */ public function test_merge_appended_member_drops_nested_nulls() { From d29a35cc7a259782ced47253730b1cf2b51752ce Mon Sep 17 00:00:00 2001 From: Jorge Costa Date: Thu, 23 Jul 2026 15:35:00 +0100 Subject: [PATCH 4/4] View config: Enforce the shape-mismatch guard under replace() too. A non-empty list patch value applied with replace() bypassed the new shape guard and silently swapped out an associative current value, while the associative-over-list direction was already rejected. The guard now runs before the replace() early return, covering both modes; an empty array stays exempt so replace() with an empty list still clears a list. --- src/wp-includes/class-wp-view-config-data.php | 27 +++++--- tests/phpunit/tests/view-config-data.php | 62 +++++++++++++++++++ 2 files changed, 80 insertions(+), 9 deletions(-) diff --git a/src/wp-includes/class-wp-view-config-data.php b/src/wp-includes/class-wp-view-config-data.php index 8790356d0bf52..b3efa4cd22fe4 100644 --- a/src/wp-includes/class-wp-view-config-data.php +++ b/src/wp-includes/class-wp-view-config-data.php @@ -309,6 +309,12 @@ public function remove( array $spec, int $version ) { * stops inheriting core's future additions to it — but it's useful when a * contributor needs to pin a list to an exact set of members. * + * The shape rule applies here too: a patch value whose shape does not match + * the current value — an associative array where a list lives, or a + * non-empty list where an associative value lives — is rejected with a + * notice and leaves the current value unchanged. An empty array is exempt, + * so replacing a list with an empty list still clears it. + * * A patch that declares an unsupported schema version is rejected and does * not change anything. * @@ -510,6 +516,18 @@ private function merge_properties( $current, $incoming, $replace_lists ) { // Numerical indexed arrays are expected to be lists (sequential integer keys starting at 0). if ( array_is_list( $incoming ) ) { + // A non-empty list only lands where a list (or nothing) lives, under + // merge() and replace() alike. An empty array is shape-ambiguous and + // exempt, so replace() with an empty list can still clear a list. + if ( array() !== $incoming && is_array( $current ) && ! array_is_list( $current ) && array() !== $current ) { + _doing_it_wrong( + __METHOD__, + 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.' ), + '7.1.0' + ); + return $current; + } + // replace() takes an incoming list as-is; merge() merges it by member identity. if ( $replace_lists ) { // As-is except for nulls: a list swapped in wholesale has no @@ -524,15 +542,6 @@ private function merge_properties( $current, $incoming, $replace_lists ) { return $current; } - if ( is_array( $current ) && ! array_is_list( $current ) && array() !== $current ) { - _doing_it_wrong( - __METHOD__, - 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.' ), - '7.1.0' - ); - return $current; - } - return $this->merge_list_by_identity( is_array( $current ) && array_is_list( $current ) ? $current : array(), $incoming diff --git a/tests/phpunit/tests/view-config-data.php b/tests/phpunit/tests/view-config-data.php index 86fa3d39356ef..68940ad6c2d5c 100644 --- a/tests/phpunit/tests/view-config-data.php +++ b/tests/phpunit/tests/view-config-data.php @@ -1886,6 +1886,68 @@ public function test_merge_appended_member_drops_nested_nulls() { ); } + /** + * replace() rejects a non-empty list patch value where an associative value + * lives, the same rule merge() enforces: a list in the patch replaces the + * current list wholesale, but it cannot land where a map lives. The current + * map survives untouched instead of being discarded. + * + * @ticket 65577 + * + * @covers ::replace + */ + public function test_replace_rejects_list_patch_over_an_associative_value() { + $this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge_properties' ); + + $data = new WP_View_Config_Data( + array( + 'default_view' => array( + 'sort' => array( + 'field' => 'title', + 'direction' => 'asc', + ), + ), + ) + ); + $before = self::read_config( $data ); + + $data->replace( + array( + 'default_view' => array( + 'sort' => array( 'title', 'asc' ), + ), + ), + 1 + ); + + $this->assertSame( $before, self::read_config( $data ) ); + } + + /** + * An empty array is exempt from the shape guard, so replace() with an + * empty list stays the documented way to clear a list. + * + * @ticket 65577 + * + * @covers ::replace + */ + public function test_replace_empty_list_still_clears_a_list() { + $data = new WP_View_Config_Data( + array( + 'view_list' => array( + array( + 'slug' => 'all', + 'title' => 'All items', + ), + ), + ) + ); + + $data->replace( array( 'view_list' => array() ), 1 ); + + $this->assertSame( array( 'view_list' => array() ), self::read_config( $data ) ); + } + /** * merge() treats a scalar list member as its own identity: an incoming