Update view config API versioning#80319
Conversation
59c5ac5 to
d31d113
Compare
|
Inspiration for the API: RFC 7386 JSON Merge Patch and PHP's Two thoughts on this approach:
|
That's not entirely true. That was an oversight that your two examples showcased that consumers might need them. With this approach the same thing still holds though for cases. If a consumer would want to remove a So besides whatever changes we make to accommodate the use cases we want, we'd still need to try to make |
|
Note this: $data->replace( array( 'default_layouts' => null ), 1 );
$data->replace( array( 'default_layouts' => $default_layouts ), 1 );We set the object to null so replace later with only the values we want (example). An advantage of replacing without nulling first is that if the view config grows (e.g., add new key) it'll have a default, even though old consumers don't set one. Alternatively, we may want to add anew verb (e.g., |
|
Paired with Nik to review this. The plan is going forward with this direction for the
We won't implement the ability to reordering items within a numerical indexed list (e.g., reorder fields in a table layout, reorder fields in a form) as that requires access to the existing data (which would prevent us from being able to introduce any changes to the shape of the tree in the future). |
| - A **list** of names deletes each named entry at that level: a key from a map, or the member with a matching identity (`id`, `slug`, `field`, or a bare scalar) from a list. | ||
| - An **associative array** maps a name to a nested spec, recursing into that entry's value to prune from within it. | ||
|
|
||
| For example, `array( 'default_view' )` drops the whole `default_view` key, `array( 'default_view' => array( 'sort' ) )` drops just its `sort` property, and `array( 'view_list' => array( 'drafts' ) )` drops the saved view whose `slug` is `drafts` while leaving every other view in place. A name that is not present is ignored, and a list is renumbered after a member is removed. |
There was a problem hiding this comment.
We need to take a good pass in the docs in the end for catching the details. For example here we mention default_view, but we actually reset to defaults for the four top level keys.
| - A **scalar** replaces the current value. | ||
| - An **associative array** (map) merges key by key, recursing into each key. | ||
| - A **list** (a sequential, zero-indexed array) merges member by member *by identity*: a member whose identity matches one already present merges into it in place and keeps its position, and an unmatched member is appended to the end. A member's identity is the value of the first identity key (`id`, `slug`, or `field`) it carries, or, for a bare scalar member, the scalar itself. Because only the value matters, a bare string like `'my_field'` matches a map carrying that same value under any identity key. | ||
| - `null` deletes the property it names. Passing `null` for a whole top-level key drops it, which reads as a reset: `gutenberg_get_entity_view_config()` backfills any missing top-level key from the defaults. |
There was a problem hiding this comment.
We need to express very clearly when consumers should use replace with null and when remove. For example here we should mention that we can't delete list members.
|
|
||
| `replace( $patch, $version )` applies a patch exactly like `merge()` — scalars replace, maps merge key by key, and `null` deletes — with one difference: any **list** the patch names replaces the current list wholesale instead of merging into it by identity. Use it to pin a list to an exact set of members, for example to remove saved views by giving a shorter `view_list`, or to drop form fields by giving a shorter `form.fields`. | ||
|
|
||
| `replace()` does *not* replace a whole top-level key wholesale: because a map still merges key by key, keys the patch omits from an associative value (such as `default_view` or `form`) are preserved. To fully swap an associative top-level key, drop it first with a `null` patch and then apply the new value. |
There was a problem hiding this comment.
With set we can remove the comments: ....To fully swap an associative top-level key, drop it first...
|
I have reviewed and left some comments around the new changes ( Removing a child field with identity (by also looking into nested children fields) was a nice touch in the previous implementation, but wouldn't consider it a block for these updates and we could always enhance/update in the future. I think it's very important to have clear docs and plenty of examples. This was also part of the original PR that tries to use simpler language than what usually LLMs produce. A thing worth looking at is that a bare-name append over an existing map field (e.g. The remaining things to be updated ( I'll be AFK the whole next week, so my reviews/comments so far hopefully help next reviewers while I'm away. |
3851584 to
8dfc9fe
Compare
This is how it should work, and I've pushed a couple of unit tests to document it. If you had this field definition: array(
'form' => array(
'fields' => array(
array(
'id' => 'featured_media',
'layout' => array( 'type' => 'regular' ),
),
),
),
)and this one: array(
'form' => array(
'fields' => array( 'featured_media' ),
),
),what are the differences? The second would use the defaults for layout. The same should happen when a filter callback updates the field configuration, hence the current behavior. |
|
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. |
412a7b4 to
dd778f4
Compare
| '7.1.0' | ||
| ); | ||
| return $this; | ||
| return $config; |
There was a problem hiding this comment.
I'm not sure if this is safe to do, if there is a single plugin callback doing something wrong I think we are ignoring all filters including the default ones added at gutenberg_register_entity_view_config_filters.
|
|
||
| // A null patch value drops the property. | ||
| if ( null === $value ) { | ||
| unset( $this->config[ $key ] ); |
There was a problem hiding this comment.
I think we may have an issue here supposely null should reset to degfault "keeps whatever it had, and a null value drops the key it names, which resets it to its default.".
After a filter merging something to null, and another filter after merge something on that paht, instad of that filter being overwrring the default it will merge with an empty array. I guess we should apply default here and not just unset?
| if ( null === $item ) { | ||
| // A null patch value deletes the key. | ||
| unset( $existing[ $key ] ); | ||
| if ( null === $index ) { |
There was a problem hiding this comment.
merge( array( 'view_list' => array( null ) ), 1 ) appends a null to list_view is that ok? Or should we check for null item here and not just index?
| - A **list** (a sequential, zero-indexed array) merges member by member *by identity*: a member whose identity matches one already present merges into it in place and keeps its position, and an unmatched member is appended to the end. A member's identity is the value of the first identity key (`id`, `slug`, or `field`) it carries, or, for a bare scalar member, the scalar itself. Because only the value matters, a bare string like `'my_field'` matches a map carrying that same value under any identity key. | ||
| - `null` deletes the property it names. Passing `null` for a whole top-level key drops it, which reads as a reset: `gutenberg_get_entity_view_config()` backfills any missing top-level key from the defaults. | ||
|
|
||
| These rules apply at every nesting level. In `view_list`, entries merge by `slug`. Within a `form`, `fields` is a list whose members merge by their `id`; a group's `children` is an identity-merged list too, so listing a field in a group's `children` appends it there. Because `merge()` only adds to or updates lists, it never removes an existing list member — to drop members, replace the list with `replace()`. |
There was a problem hiding this comment.
Nit pick: Is it really the case that merge never removes?
If status is:
array(
'id' => 'status',
'label' => __( 'Status', 'gutenberg' ),
'children' => array(
array(
'id' => 'status',
'layout' => array(
'type' => 'regular',
'labelPosition' => 'none',
),
),
'scheduled_date',
'password',
'sticky',
),
),
And we call:
merge( array( 'form' => array( 'fields' => array( 'status' ) ) ), 1 )
We will remove the children of status.
See #79809 (comment)
What?
Iterates on the view config API versioning — the
Gutenberg_View_Config_Dataobject thatget_entity_view_config_{$kind}_{$name}filter callbacks receive — from a set of shape-specific methods into a small set of general ones that form a gradient of how deep a change reaches.Before:
After:
Why?
The previous method relied on using
get_configto process the shape of the object, but that's precisely what this API wanted to avoid to ensure future compatibility with changes to the object shape, see #79809 (comment).A single patch model with uniform rules is smaller (conceptually and code-wise), composes predictably at every nesting level, and covers add/update/remove for all four top-level keys.
How?
Each method takes the schema
$versionand a partial argument applied recursively, touching only the top-level keys it names. The three writers form a gradient of how deep a named key's replacement reaches, andremove()is the counterpart that deletes by name.merge()merges the value in key by key — the default: patches compose with core's configuration and with other plugins'.replace()merges the same way, except a list in the patch replaces the current list wholesale instead of merging by identity.set()goes one step further and swaps each named top-level key wholesale, so the inherited default never leaks through a key-by-key merge.remove()takes a spec of names to delete rather than values to write, its shape mirroring the configuration it prunes. It's the targeted counterpart toreplace(): drop a single member and keep inheriting core's future additions to the rest.The core's own per-post-type callbacks (
page,wp_block,wp_template, …) are migrated to the new methods, and the docs infilters-and-hooks.mdare rewritten aroundmerge()/replace()/set()/remove().Testing Instructions
Use of AI Tools
Claude Code was used in creating these changes, and they all were reviewed by the author.