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
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 2 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
54 changes: 51 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 @@ -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.
*
Expand Down Expand Up @@ -475,6 +485,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 @@ -498,13 +517,38 @@ private function merge_properties( $current, $incoming, $replace_lists ) {
// set()), so a null member is dropped rather than stored.
return $this->strip_nulls( $incoming );
Comment thread
jorgefilipecosta marked this conversation as resolved.
}

// 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(
__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
);
}

// 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 +645,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 +683,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
157 changes: 157 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,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
Comment on lines +1728 to +1731

@mukeshpanchal27 mukeshpanchal27 Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
*
* @covers ::merge
* @ticket 65577
*
* @covers ::merge

Add ticket annotation in all new tests

@jorgefilipecosta jorgefilipecosta Jul 23, 2026

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.

Fixed 👍

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


/**
* merge() treats a scalar list member as its own identity: an incoming
Expand Down
Loading