Skip to content

Media: Use set_param() to assign REST request parameters - #86110

Closed
noruzzamans wants to merge 1 commit into
WordPress:trunkfrom
noruzzamans:fix/phpstan-media-attachments-controller-set-param
Closed

noruzzamans wants to merge 1 commit into
WordPress:trunkfrom
noruzzamans:fix/phpstan-media-attachments-controller-set-param

Conversation

@noruzzamans

Copy link
Copy Markdown
Member

What?

See #86088

Replaces array offset assignments on a WP_REST_Request instance with set_param() in WP_REST_Attachments_Gutenberg_Controller, and removes the resolved entries from the PHPStan baseline.

Why?

WP_REST_Request implements ArrayAccess by delegating offsetSet() directly to set_param(). However, PHPStan treats direct array offset assignment on WP_REST_Request as invalid (offsetAssign.dimType and offsetAssign.valueType). Using set_param() directly resolves 3 errors initially baselined in #66693.

How?

  • In lib/media/class-gutenberg-rest-attachments-controller.php, replaced $response_request['context'] = 'edit' and $response_request['_fields'] = ... with $response_request->set_param(...).
  • Removed the 3 corresponding offsetAssign entries from tools/phpstan/baseline.neon.

Testing Instructions

  1. Run composer phpstan and confirm it passes with no errors.
  2. Run composer phpstan:baselines and confirm tools/phpstan/baseline.neon has no further changes.

Testing Instructions for Keyboard

Not applicable. There are no UI changes.

Screenshots or screencast

Not applicable.

Use of AI Tools

I used Claude Code to identify the affected occurrences and draft the change. I reviewed the diff and take responsibility for it.

@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

🤖 PR meta 🤖

Preview in WordPress Playground

🎉 Props

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: noruzzamans <noruzzaman@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

Updated as activity occurs, without notifying anyone named here. Add the props-bot label to refresh.

@huzaifaalmesbah huzaifaalmesbah added [Type] Code Quality Issues or PRs that relate to code quality No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core labels Nov 1, 2026

@westonruter westonruter left a comment

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.

It's good that this was opened because it highlights a problem with the underlying WP_REST_Request typing.

So I'm marking with "request changes" so we can fix the underlying problem.

🤖 Analysis by Claude Opus 5.5 · medium effort:

The switch from $response_request['context'] = … to $response_request->set_param( 'context', … ) avoids the errors, but it looks like a workaround for a PHPStan limitation that the stub signature triggers. Nothing is wrong with the code itself. With the baseline entries removed and the change reverted, PHPStan 2.3.4 with php-stubs/wordpress-stubs 7.2.0-beta3 reports:

  • line 1737: Cannot assign offset 'context' to WP_REST_Request<array<string, mixed>>. (offsetAssign.dimType) and WP_REST_Request<array<string, mixed>> does not accept string. (offsetAssign.valueType)
  • line 1740: Cannot assign offset '_fields' to WP_REST_Request<array<string, mixed>>. (offsetAssign.dimType)

Cause

In the stubs, WP_REST_Request::offsetSet() has a method-level template, the same as set_param():

/**
 * @phpstan-template TOffset of key-of<T>
 * @phpstan-param TOffset $offset
 * @phpstan-param T[TOffset] $value
 */
public function offsetSet($offset, $value)

The two forms are checked differently:

  • ->set_param( 'context', 'edit' ) is analyzed as a method call. TOffset is inferred from the argument as 'context', so T['context'] resolves to mixed and the call passes.
  • $request['context'] = 'edit' is analyzed by ObjectType::setOffsetValueType(). That method reads offsetSet()'s parameters through getOnlyVariant() and never resolves the method template against the actual offset and value. The accepted offset type stays as the unresolved TOffset, and the accepted value type stays as T[TOffset]. isSuperTypeOf() on an unresolved template type returns maybe, not yes, so the assignment is rejected. Line 1740 gets only the dimType error because its value ($request['_fields']) is mixed, and the value check skips mixed.

The same behavior shows up in a minimal reproduction without WordPress. A class implementing ArrayAccess<key-of<T>, value-of<T>> whose offsetSet() has @template TOffset of key-of<T> gives the same three errors, while its identically typed set_param() passes. The same class with offsetSet() typed as plain @param key-of<T> $offset / @param value-of<T> $value (no method template) gives no errors.

Suggested direction

Array-access assignment on WP_REST_Request is an idiom used throughout core, so it seems better to fix this upstream than to change call sites. Either of these would work:

  1. PHPStan: resolve offsetSet()'s method template in ObjectType::setOffsetValueType() (and probably the matching read path), for example with ParametersAcceptorSelector::selectFromTypes() and [ $offsetType, $valueType ] instead of getOnlyVariant(), so array-access writes are inferred like a method call.
  2. wordpress-stubs: drop the method-level template from offsetSet() and use key-of<T> / value-of<T> directly. This gives up per-key value precision on array-access writes, but it works with PHPStan today.

Until then, it may be preferable to keep the baseline entries, or a targeted ignore that links to the upstream issue, and leave the array-access syntax unchanged.

Separately, the WP_REST_Request class docblock in the stubs has two @phpstan-implements tags (ArrayAccess<string, mixed> and ArrayAccess<key-of<T>, value-of<T>>), and they appear before the @phpstan-template declaration. That doesn't cause these errors, but it's worth tidying up in the stubs at the same time.

Comment on lines -1737 to +1740
$response_request['context'] = 'edit';
$response_request->set_param( 'context', 'edit' );

if ( isset( $request['_fields'] ) ) {
$response_request['_fields'] = $request['_fields'];
$response_request->set_param( '_fields', $request['_fields'] );

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.

While these changes are fine and resolve the errors, they're ultimately a workaround for a deeper problem.

@westonruter

Copy link
Copy Markdown
Member

🤖 Comment by Claude Opus 5.5 · medium effort

The switch from $response_request['context'] = … to $response_request->set_param( 'context', … ) avoids the errors, but it works around a PHPStan bug. The code itself was correct. With the baseline entries removed and the change reverted, PHPStan 2.3.4 with php-stubs/wordpress-stubs 7.2.0-beta3 reports:

  • line 1737: Cannot assign offset 'context' to WP_REST_Request<array<string, mixed>>. (offsetAssign.dimType) and WP_REST_Request<array<string, mixed>> does not accept string. (offsetAssign.valueType)
  • line 1740: Cannot assign offset '_fields' to WP_REST_Request<array<string, mixed>>. (offsetAssign.dimType)

Cause

In the stubs, WP_REST_Request::offsetSet() declares its own template type, as set_param() does:

/**
 * @phpstan-template TOffset of key-of<T>
 * @phpstan-param TOffset $offset
 * @phpstan-param T[TOffset] $value
 */
public function offsetSet($offset, $value)

The two forms are checked differently:

  • ->set_param( 'context', 'edit' ) is analysed as a method call. TOffset is inferred from the argument as 'context', so T['context'] resolves to mixed and the call passes.
  • $request['context'] = 'edit' is checked through ObjectType::setOffsetValueType(). That code took offsetSet()'s parameters from getOnlyVariant() and never resolved the template from the written offset and value. The accepted offset type stayed the unresolved TOffset, which was not shown to be a supertype of 'context', so the write was rejected. Line 1740 gets only the offset error because its value ($request['_fields']) is mixed, and the value check skips mixed.

This is a long-standing PHPStan bug, already reported as phpstan/phpstan#8549 with the same offsetSet() pattern.

Upstream fix

A fix has been proposed in phpstan/phpstan-src#6903. It makes array-access writes resolve offsetSet()'s templates the way a method call does. With it, the original array-access code here passes unchanged. In a check against the real WP_REST_Request stub, this file reports no errors.

The fix also lets array-access writes be checked per key once T is known. For example, with WP_REST_Request<array{context: 'view'|'edit'|'embed', _fields?: string}>, $request['context'] = 'nonsense' and $request['_fields'] = 1 are reported and valid writes are accepted. Before the fix, every write was reported, valid or not.

Suggestion

Array access on WP_REST_Request is idiomatic throughout core, so it seems better to keep it here and not rewrite call sites around the bug. This PR could keep the array-access syntax and the existing baseline entries, and the entries could be dropped once a PHPStan release includes the upstream fix.

@noruzzamans

Copy link
Copy Markdown
Member Author

Thank you for looking into this and linking the upstream PHPStan fix (phpstan/phpstan-src#6903). Agreed that preserving the idiomatic array-access syntax and waiting for the upstream PHPStan release is the right approach here. Closing in favor of the upstream fix.

@westonruter

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

No Core Sync Required Indicates that any changes do not need to be synced to WordPress Core [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants