Skip to content

View config: reject shape-mismatched merges, define empty-array semantics, strip nulls from appended members #12644

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jorgefilipecosta wants to merge 4 commits into from
+287 −3
Closed

View config: reject shape-mismatched merges, define empty-array semantics, strip nulls from appended members #12644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e53eca9
REST API: Fix silent data loss in view configuration merges.
jorgefilipecosta Jul 22, 2026
356606e
View config: Simplify the shape-mismatch notice.
jorgefilipecosta Jul 22, 2026
b75fdfa
View config: Add ticket annotations to the new merge tests.
jorgefilipecosta Jul 23, 2026
d29a35c
View config: Enforce the shape-mismatch guard under replace() too.
jorgefilipecosta Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 60 additions & 3 deletions src/wp-includes/class-wp-view-config-data.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -306,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.
*
Expand Down Expand Up @@ -352,6 +361,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.
*
Expand Down Expand Up @@ -475,6 +491,15 @@ 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.
Expand All @@ -491,20 +516,48 @@ 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
// existing leaf for a null to delete (the same rationale as
// 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;
}

return $this->merge_list_by_identity(
is_array( $current ) && array_is_list( $current ) ? $current : array(),
$incoming
);
}

// Consider any other array as associative (keys are strings).
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;
}

$result = is_array( $current ) && ! array_is_list( $current ) ? $current : array();
foreach ( $incoming as $key => $value ) {
// A null patch value deletes the property.
Expand Down Expand Up @@ -601,7 +654,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
Expand Down Expand Up @@ -637,7 +692,9 @@ 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;
}

Expand Down
227 changes: 227 additions & 0 deletions tests/phpunit/tests/view-config-data.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1721,6 +1721,233 @@ 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.
*
* @ticket 65577
*
* @covers ::merge
*/
public function test_merge_rejects_associative_patch_over_a_list() {
$this->setExpectedIncorrectUsage( 'WP_View_Config_Data::merge_properties' );

$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 ) );

@jorgefilipecosta jorgefilipecosta Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of this assertion without the fix:

array(
	'view_list' => array(
		'published' => array( 'title' => 'Live' ),
	),
)

The default all view is gone, view_list became a slug-keyed map instead of a list, with no notice.

}

/**
* 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.
*
* @ticket 65577
*
* @covers ::merge
*/
public function test_merge_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->merge(
array(
'default_view' => array(
'sort' => array( 'title', 'asc' ),
),
),
1
);

$this->assertSame( $before, self::read_config( $data ) );

@jorgefilipecosta jorgefilipecosta Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of this assertion without the fix:

array(
	'default_view' => array(
		'sort' => array( 'title', 'asc' ),
	),
)

The sort map (field/direction) was replaced by the bare list, with no notice.

}

/**
* 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.
*
* @ticket 65577
*
* @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 ) );

@jorgefilipecosta jorgefilipecosta Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of this assertion without the fix:

array(
	'default_view' => array(
		'filters' => array(
			array(
				'field'    => 'author',
				'operator' => 'isAny',
			),
		),
		'sort'    => array(),
	),
)

The same empty-array patch left filters untouched but emptied sort: one input, two different outcomes depending on the current shape.

}

/**
* 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).
*
* @ticket 65577
*
* @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(

@jorgefilipecosta jorgefilipecosta Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of this assertion without the fix:

array(
	'view_list' => array(
		array(
			'slug'  => 'all',
			'title' => 'All items',
		),
		array(
			'slug' => 'mine',
			'view' => array( 'filters' => null ),
		),
	),
)

The appended member kept the literal 'filters' => null instead of dropping it the way every other write path does.

array(
'view_list' => array(
array(
'slug' => 'all',
'title' => 'All items',
),
array(
'slug' => 'mine',
'view' => array(),
),
),
),
self::read_config( $data )
);
}

/**
* 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
Expand Down
Loading