Use allow-list for mobile editor settings endpoint #56424
Conversation
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/experimental/block-editor-settings-mobile-allowed.php ❔ lib/load.php |
Address linter errors and warnings.
Address linter error.
If the settings do not include an item listed in the allow list, we must avoid an attempt to reference the non-existent settings key.
This appears to be expected by the existing tests. However, it is unclear if all or a subset of the nested keys are expected or required.
|
Thank you @dcalhoun for your review (and for your helpful commits)! I've pushed a few more commits to address your feedback, so this is ready for another pass. Note: I have not yet marked this as "ready for review" because I haven't had time to add some unit tests. But, if you have further feedback about the approach, it is welcome 🙂 . |
The important part of this is that the filter is a separate filter. Incidentally, this also fixes a bug in which the previous draft implementation returned a filtered response even for non-mobile contexts.
Appease linter errors via `npm run format:php`.
@mkevins the implementation looks sound to me. I do not have further feedback at this time. I pushed a commit addressing the PHP unit test failures by running From what I gathered, a version of these changes were deployed internally. So, for better or worse, I suppose further stability testing of these changes is happening live. If that appears to go well, then it increases confidence that these proposed changes include all the necessary attributes for the allow list. |
| 'color' => true, | ||
| 'typography' => true, | ||
| ), | ||
| '__experimentalFeatures' => array( |
There was a problem hiding this comment.
Let's also add the custom attribute which is used for custom variables or other settings. The app uses it to get values for custom variables.
Description
This PR introduces an "allow-list" for the mobile endpoint for block editor settings. This gives us a greater degree of control over what the client apps are consuming, and helps avoid prematurely introducing changes which do not yet have support in the mobile apps.
What?
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast