Make WordPress Core

Opened 3 months ago

Closed 2 months ago

Last modified 6 weeks ago

#65577 closed enhancement (fixed)

View Config: Add versioning handling

Reported by: ntsekouras Owned by: ntsekouras
Priority: normal Milestone: 7.1
Component: General Version:
Severity: normal Keywords: has-patch gutenberg-merge commit has-unit-tests
Cc: Focuses:

Description (last modified by ntsekouras)

This is a continuation of https://core.trac.wordpress.org/ticket/65516 and will port the changes in view config from:

Right now consumers of the filters have to customize the config by walking the raw payload (find the right index, foreach, unset, etc..), and imperative array manipulation can never be migrated. A later shape change could break existing callbacks in the wild. So even though there is only version 1 today and no migrations exist, this code is the safeguard to keep filters working in a possible shape change.

Change History (23)

#1 @ntsekouras
3 months ago

  • Owner set to ntsekouras
  • Status new → assigned

This ticket was mentioned in ​PR #12391 on ​WordPress/wordpress-develop by ​@ntsekouras.


3 months ago
#2

  • Keywords has-patch added

This is a placeholder PR for backporting ​https://github.com/WordPress/gutenberg/pull/79809 when it lands in GB first.

Trac ticket: https://core.trac.wordpress.org/ticket/65577

## Use of AI Tools

#3 @wildworks
3 months ago

  • Keywords gutenberg-merge added

#4 @ntsekouras
3 months ago

  • Description modified (diff)

#6 @ntsekouras
3 months ago

  • Resolution → fixed
  • Status assigned → closed

In 62668:

Add version handling to the view config API.

Right now consumers of the get_entity_view_config_{$kind}_{$name} filter customize the config by walking the raw payload — find the right index, foreach, unset, etc. — and imperative array manipulation can never be migrated. A later shape change could break existing callbacks in the wild. So even though there is only version 1 today and no migrations exist, this is the safeguard to keep filters working through a possible shape change.

Instead of mutating the payload directly, callbacks now receive a WP_View_Config_Data container and update the configuration through its methods, partially mirroring theme.json version handling where documents declare a version and older data is migrated forward.

Ports the changes from the Gutenberg plugin. See ​https://github.com/WordPress/gutenberg/pull/79809.

Follow-up to [62547].
Props ntsekouras, mcsf.
Fixes #65577.

#7 @oandregal
3 months ago

  • Resolution fixed
  • Status closed → reopened

We ran into an issue at ​https://github.com/WordPress/gutenberg/pull/79809#issuecomment-4979962005 that we have addressed by ​https://github.com/WordPress/gutenberg/pull/80319. We need to backport that PR into core.

This ticket was mentioned in ​PR #12638 on ​WordPress/wordpress-develop by ​@oandregal.


3 months ago
#8

  • Keywords has-unit-tests added

Trac ticket: https://core.trac.wordpress.org/ticket/65577
Backports: ​https://github.com/WordPress/gutenberg/pull/80319
Follow-up to: ​https://github.com/WordPress/wordpress-develop/pull/12391

## What

Ports the latest iteration of the view configuration API from the Gutenberg update/view-config-api-versioning branch:

## Why

## How

  • Rework the WP_View_Config_Data write API around merge(), replace(), set(), and remove(), all operating on patches of top-level keys and taking the schema version the change was authored against.
  • Make get_data() private and move filter application into a new apply_filters() method so callbacks cannot read the materialized configuration and become coupled to its shape.
  • Update the default post type configuration callbacks to use the new single-patch set() signature.
  • Adapt and expand the unit tests accordingly.

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code, Zed agent
Model(s): Opus, Fabble
Used for: Initial code skeleton and test suggestions; final implementation and tests were reviewed and edited by me.

#10 @oandregal
3 months ago

  • Resolution → fixed
  • Status reopened → closed

In 62825:

View configuration API: fix versioning

Rework the WP_View_Config_Data write API around merge(), replace(), set(), and remove(), all operating on patches of top-level keys and taking the schema version the change was authored against. Make get_data() private and move filter application into a new apply_filters() method so callbacks cannot read the materialized configuration and become coupled to its shape. Update the default post type configuration callbacks to use the new single-patch set() signature. Adapt and expand the unit tests accordingly.

Props oandregal, jorgefilipecosta, ntsekouras, t-hamano.
Fixes #65577.

This ticket was mentioned in ​PR #12641 on ​WordPress/wordpress-develop by ​@jorgefilipecosta.


3 months ago
#11

The get_entity_view_config_{$kind}_{$name} filter docblock told callbacks the container is mutated in place and there is "no need to return it; any returned value is ignored". Only the final return value is discarded: as with any filter, WP_Hook::apply_filters() passes each callback's return value to the next callback, so a callback that follows that advice and returns nothing hands null to every callback hooked at a later priority, which then fails with a fatal error the moment it calls a method on it.

This updates the docblock to document the standard filter contract — callbacks must return the container they received — and notes that the write methods return the container, so a callback can end with return $data->merge( $patch, $version );. Also fixes a doubled "the" in the same paragraph.

Documentation-only change; the dispatch itself is unchanged, and the existing filter tests already register callbacks that return the container.

This ticket was mentioned in ​PR #12644 on ​WordPress/wordpress-develop by ​@jorgefilipecosta.


3 months ago
#12

  • Keywords has-unit-tests added

## What

Fixes three defects in WP_View_Config_Data's merge engine, each with a test that fails without the fix:

  • An associative patch value landing on a list (or a non-empty list landing on an associative value) silently discarded the whole current value. It is now rejected with _doing_it_wrong() and leaves the current value unchanged.
  • An empty array in a merge() patch — classified as a list — wiped associative values while silently no-oping on lists. It is now a documented no-op for both shapes.
  • A list member appended by merge() (no identity match) was stored verbatim, keeping nested nulls that every other write path consumes. Appended members now go through strip_nulls().

Port of the Gutenberg fix in ​https://github.com/WordPress/gutenberg/pull/80571; follow-up to ​https://github.com/WordPress/wordpress-develop/pull/12638.

## Why

merge_properties() classifies patch values by shape but let mismatches fall through the classification branches into silent data loss:

  • merge( array( 'view_list' => array( 'published' => array( 'title' => 'Live' ) ) ), 1 ) (the pre-7.1 slug-keyed shape) dropped every default view and stored the slug-keyed map, which then serialized as a JSON object where the schema and the DataViews client expect an array — with no misuse notice.
  • merge( array( 'default_view' => array( 'sort' => array() ) ) ), 1 ) destroyed the sort map, while the same empty array over filters did nothing — two inconsistent outcomes for one input, neither documented.
  • An appended view_list member carrying 'filters' => null stored the literal null into the REST response where the schema declares an array, violating the documented "a nested null deletes just the leaf it names in every case" contract.

These semantics harden into the plugin-facing version-1 patch contract when 7.1 ships, so they need to be right before the release.

## How

One rule in merge_properties(): a patch value only merges into a current value of the same shape. A non-empty mismatch in either direction is reported via _doing_it_wrong() (the public method name is threaded down for the notice) and keeps the current value, so a malformed patch cannot destroy configuration. An empty array is shape-ambiguous and merges nothing, so it is a no-op — clearing a list stays spelled replace() with an empty list, and resetting a key stays spelled null. Empty arrays are exempt from the mismatch guards, so an associative patch still creates a new nested map (the recursion hands array() to keys that do not exist yet). merge_list_by_identity() strips nulls from appended members, matching set() and the lists replace() swaps in. Docblocks (class, merge(), and the two helpers) document the rule.

## Testing

npm run test:php -- --filter 'Tests_View_Config_Data|Tests_View_Config_API'

New cases in tests/phpunit/tests/view-config-data.php, one per fix (plus the mirror guard), each verified to fail against the previous implementation:

  • test_merge_rejects_associative_patch_over_a_list
  • test_merge_rejects_list_patch_over_an_associative_value
  • test_merge_empty_array_is_a_noop
  • test_merge_appended_member_drops_nested_nulls

## AI usage disclosure

The issues were found by an AI-assisted code review of ​https://github.com/WordPress/wordpress-develop/pull/12638, and the fix, tests, and this description were drafted with AI assistance (Claude Code). I reviewed and verified the changes, including running the new tests against the unfixed code to confirm they fail.

#13 @jorgefilipecosta
3 months ago

In 62833:

Docs: Require view config filter callbacks to return the container.

Corrects the get_entity_view_config_{$kind}_{$name} filter docblock: callbacks must return the container they receive. Also fixes a doubled "the" in the same paragraph.

Follow-up to [62825].

Props jorgefilipecosta, oandregal.
Fixes #65577.

#14 @jorgefilipecosta
3 months ago

In 62834:

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

Fixes three silent data-loss defects in WP_View_Config_Data's merge engine:

  • An associative patch value over a list (or a non-empty list over an associative value) discarded the whole current value. It is now rejected with _doing_it_wrong() and the current value is kept.
  • An empty array under merge() wiped associative values but no-oped on lists. It is now a no-op for both shapes — clear a list with replace() and an empty list, reset a key with null.
  • A list member appended by merge() kept nested nulls that every other write path drops. Appended members now go through strip_nulls().

Follow-up to [62825].

Props jorgefilipecosta, oandregal.
See #65577.

This ticket was mentioned in ​PR #12766 on ​WordPress/wordpress-develop by ​@oandregal.


2 months ago
#15

Trac ticket https://core.trac.wordpress.org/ticket/65577
Backports ​https://github.com/WordPress/gutenberg/pull/80832
Follow-up to ​https://github.com/WordPress/wordpress-develop/pull/12391

search and page are managed via the URL, which is their only source of truth, so they should not be persisted as part of a view configuration.

Removes both properties from the shared ViewBase schema in WP_REST_View_Config_Controller, updates the WP_View_Config_Data docblock example that used search, and adds a test asserting neither property is declared by any view in the item schema.

#16 @oandregal
2 months ago

  • Resolution fixed
  • Status closed → reopened

This ticket was mentioned in ​PR #12809 on ​WordPress/wordpress-develop by ​@oandregal.


2 months ago
#17

Trac ticket https://core.trac.wordpress.org/ticket/65577
Backports ​https://github.com/WordPress/gutenberg/pull/81068
Follow-up to ​https://github.com/WordPress/wordpress-develop/pull/12391

## What?

Lowercases the dynamic filter names for view config.

## Why?

Lowercase names match PHP practices, ​feedback.

## How?

Make the dynamic filter hook name lowercase.

## Testing Instructions

Load the Site Editor screens (Pages, Templates, Patterns&Parts) and verify they still work as before.

#18 @oandregal
2 months ago

In 62970:

View config filters: lowercase dynamic filter names.

Props oandregal, ntsekouras.
See #65577.

#20 @oandregal
2 months ago

  • Resolution → fixed
  • Status reopened → closed

In 62973:

View config REST Endpoint: remove search and page.

The search and page parameters source of truth is the URL,
and cannot be configured via the filters.

Props oandregal, ntsekouras, jorgefilipecosta.
Fixes #65577.

This ticket was mentioned in ​PR #13330 on ​WordPress/wordpress-develop by ​@oandregal.


6 weeks ago
#21

Adds descriptions to the styles property of the table layout schema in the view config REST controller and to the ColumnStyle properties (width, maxWidth, minWidth, align). The styles description notes that the styles apply to the columns listed in the view fields and not to the primary column (title, media, and description fields), which takes the width the other columns leave over. No behavior changes.

# Backports

Gutenberg PRs:

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Fable
Used for: Porting the server-side change from the Gutenberg PR. Reviewed and edited by me.

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See ​GitHub Pull Requests for Code Review in the Core Handbook for more details.

​@ugyensupport commented on ​PR #13330:


6 weeks ago
#22

Tested against a local trunk (7.1) checkout: applied cleanly, php -l clean, and confirmed via OPTIONS /wp-json/wp/v2/view-config that the schema diff before/after is exactly the 4 new description fields (styles, width, maxWidth, minWidth, align), consistently across every place get_table_layout_schema()/get_column_style_schema() are reused (default_view, default_layouts.table, default_layouts.pickerTable, view_list[].view) — nothing else changed, confirming the "no behavior changes" claim.

One environment note unrelated to this PR itself: with Gutenberg active, its compat/wordpress-7.1/rest-api.php unconditionally re-registers this same route at PHP_INT_MAX, shadowing core's controller — so testing needs Gutenberg deactivated to see core's own schema.

Full test report and further discussion in the Trac ticket per the PR description: https://core.trac.wordpress.org/ticket/65577

Note: See TracTickets for help on using tickets.