Wordpress Core Settings Patch #661
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #661 +/- ##
==========================================
- Coverage 60.4% 60.11% -0.3%
==========================================
Files 109 110 +1
Lines 6678 6739 +61
==========================================
+ Hits 4034 4051 +17
- Misses 2644 2688 +44
Continue to review full report at Codecov.
|
| 'description' => __( 'Maximum height for large-sized content in pixels to use when adding an image to the Media Library', 'wp-graphql' ) | ||
| ), | ||
| /** | ||
| * TODO: Get more info on these three options |
There was a problem hiding this comment.
@kidunot89 I'll have to look into this as well 🤔
There was a problem hiding this comment.
@kidunot89 looks like image_default_size should be an Enum of registered image sizes, eh? 🤔
| public function testPatchSettingsQuery() | ||
| { | ||
| $mock_options = [ | ||
| 'default_pingback_flag' => 1, |
There was a problem hiding this comment.
spacing looks weird on this line 🤷♂️
tests/wpunit/PatchSettingsTest.php
Outdated
| showCommentsCookiesOptIn | ||
| } | ||
| mediaSettings { | ||
| thumbnailSizeW |
There was a problem hiding this comment.
not that big of a deal, but it kinda bugs me that this is named thumbailSizeW instead of thumbnailSizeWidth 🤔
how are other image sizes handled, like when you add_image_size() (https://developer.wordpress.org/reference/functions/add_image_size/)
There was a problem hiding this comment.
wait, actually, there aren't settings for additional sizes, because you register them statically. . .duh. . .lol
disregard. 🤦♂️
There was a problem hiding this comment.
@kidunot89 I think my only complaint is with the naming of the image sizes using W and H instead of Width and Height.
I understand why that's the case, but just bugs me.
Wonder if we should allow register_setting() to add an additional field like graphql_field_name or something like that, similar to how we have graphql_single_name for register_post_type
🤔
|
I pulled this locally and I'm getting a lot of PHP errors. Looking into it 👀 |
| 'description' => __( 'Base Tag URLs', 'wp-graphql' ) | ||
| ), | ||
| ), | ||
| 'privacy' => array( |
There was a problem hiding this comment.
I get the following php error:
( ! ) Notice: register_setting was called with an argument that is <strong>deprecated</strong> since version 3.5.0! The "privacy" options group has been removed. Use another settings group. in /app/public/wp-includes/functions.php on line 4115
It also looks like the privacy options have their own capabilities associated: https://github.com/WordPress/WordPress/blob/47e29ccba94fe9342785585b80ddc9b4a8039aa2/wp-admin/privacy.php#L12
There was a problem hiding this comment.
There was a problem hiding this comment.
I haven't gonna that error, but I have used it in production yet. I'll also look into it when I can
| 'description' => __( 'Maximum height for large-sized content in pixels to use when adding an image to the Media Library', 'wp-graphql' ) | ||
| ), | ||
| /** | ||
| * TODO: Get more info on these three options |
There was a problem hiding this comment.
@kidunot89 looks like image_default_size should be an Enum of registered image sizes, eh? 🤔
| 'comment_order' => array( | ||
| 'show_in_rest' => array( | ||
| 'schema' => array( | ||
| 'enum' => array( 'asc', 'desc' ), |
There was a problem hiding this comment.
so for fields like this that are exposed as an Enum in REST should probably also be exposed in GraphQL as an enum.
No reason this should be a String in GraphQL if we have the power to validate against a static enum already 🤔
There was a problem hiding this comment.
register_setting doesn't have any enum type to my knowledge. string, integer, and boolean are the only types as far as I know. I used register_initial_setting as an example.
|
Related: #674 |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically closed because it has not had recent activity. If you believe this issue is still valid, please open a new issue and mark this as a related issue. |
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
| /** | ||
| * Registers WordPress options not loaded by Settings API by default | ||
| */ | ||
| function patch_whitelist_options() { |
There was a problem hiding this comment.
Method patch_whitelist_options has 286 lines of code (exceeds 25 allowed). Consider refactoring.
| 'type' => 'boolean', | ||
| 'description' => __( 'Discourage search engines from indexing this site', 'wp-graphql' ) | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Spaces must be used for alignment; tabs are not allowed
| @@ -225,6 +225,9 @@ private function includes() { | |||
| // Required non-autoloaded classes | |||
| require_once( WPGRAPHQL_PLUGIN_DIR . 'access-functions.php' ); | |||
|
|
|||
| // Required wordpress core patches | |||
There was a problem hiding this comment.
Spaces must be used to indent lines; tabs are not allowed
| @@ -225,6 +225,9 @@ private function includes() { | |||
| // Required non-autoloaded classes | |||
| require_once( WPGRAPHQL_PLUGIN_DIR . 'access-functions.php' ); | |||
|
|
|||
| // Required wordpress core patches | |||
| require_once( WPGRAPHQL_PLUGIN_DIR . 'settings-patch.php' ); | |||
There was a problem hiding this comment.
Spaces must be used to indent lines; tabs are not allowed
| @@ -225,6 +225,9 @@ private function includes() { | |||
| // Required non-autoloaded classes | |||
| require_once( WPGRAPHQL_PLUGIN_DIR . 'access-functions.php' ); | |||
|
|
|||
| // Required wordpress core patches | |||
| require_once( WPGRAPHQL_PLUGIN_DIR . 'settings-patch.php' ); | |||
There was a problem hiding this comment.
Space after opening parenthesis of function call prohibited
|
Code Climate has analyzed commit 7f89d70 and detected 462 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically closed because it has not had recent activity. If you believe this issue is still valid, please open a new issue and mark this as a related issue. |
Your checklist for this pull request
Thanks for sending a pull request! Please make sure you click the link above to view the contribution guidelines, then fill out the blanks below.
🚨Please review the guidelines for contributing to this repository.
What does this implement/fix? Explain your changes.
Registers the following options as settings using the WordPress Settings API, so they can be loaded to the $wp_registered_settings object and read by
DataSource::get_allowed_settings_by_groupandDataSource::get_allowed_settingsfunctions.Discussion Settings
Media Settings
Permalink Settings
Privacy Settings
Reading Settings
Writing Settings
Does this close any currently open issues?
…
Any relevant logs, error output, GraphiQL screenshots, etc?
(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)
Any other comments?
…
Where has this been tested?
Operating System: Ubuntu 18.04
WordPress Version: 5.0.3