View config: reject shape-mismatched merges, define empty-array semantics, strip nulls from appended members - #80571
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jorgefilipecosta
left a comment
There was a problem hiding this comment.
Pre-fix test results: each new test was run against trunk's class (source change reverted, tests kept) to confirm it fails without this PR. Per-assertion output inline.
| 1 | ||
| ); | ||
|
|
||
| $this->assertSame( $before, self::read_config( $data ) ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So the fix ensures that numeric indexed arrays aren't updated with associative arrays. I cannot think of an use case where this would be valid behavior. It's true that lists can items of different types (e.g., scalar vs. arrays, such as the form.fields list), but all of them will still have numeric indexes. This is a good prevention measure 👍
| 1 | ||
| ); | ||
|
|
||
| $this->assertSame( $before, self::read_config( $data ) ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This is the opposite of the above. Again, I can't think of uses cases where this would be a good behavior, and it's a good preventive measure.
| 1 | ||
| ); | ||
|
|
||
| $this->assertSame( $before, self::read_config( $data ) ); |
There was a problem hiding this comment.
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.
| 1 | ||
| ); | ||
|
|
||
| $this->assertSame( |
There was a problem hiding this comment.
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.
0187b8f to
9c36b12
Compare
…p, strip nulls from appended members 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.
…neric message 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. A single shape-agnostic message keeps the notice actionable without the extra plumbing.
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.
b180c42 to
d6dc2aa
Compare
|
This PR was submitted from a forked repository, so it seems that automatic cherry-picking is not working. If you have the bandwidth, I would appreciate it if you could submit a backport PR directly to |
|
This PR was backported to 7.1 by #80829. |
|
There was a conflict while trying to cherry-pick the commit to the wp/7.1 branch. Please resolve the conflict manually and create a PR to the wp/7.1 branch. PRs to wp/7.1 are similar to PRs to trunk, but you should base your PR on the wp/7.1 branch instead of trunk. |
What?
Follow up to #80319. Core PR: WordPress/wordpress-develop#12644.
Fixes three silent data-loss defects in
Gutenberg_View_Config_Data's merge engine:_doing_it_wrong()and the current value is kept.merge()wiped associative values but no-oped on lists. It is now a no-op for both shapes — clear a list withreplace()and an empty list, reset a key withnull.merge()kept nestednulls that every other write path drops. Appended members now go throughstrip_nulls().How?
merge_properties(): a patch value only merges into a current value of the same shape. Non-empty mismatches warn and keep the current value; an empty array merges nothing. Empty arrays stay exempt from the guards, so an associative patch still creates new nested maps.merge_list_by_identity(): appended members go throughstrip_nulls().Testing
Verify unit tests are passing:
One new test per fix; the inline PR comments show each assertion's result without the fix.
AI usage disclosure: issues found via AI-assisted code review; fix, tests, and description drafted with AI assistance (Claude Code) and reviewed by me.