fix: args updated for Settings API endpoints #2483
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
pronob1010
added
the
Needs: Dev Review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
includes/REST/StoreSettingControllerV2.php (2)
106-122: Consider enhancing error handling for invalid IDs.While the argument specifications are correct, consider adding validation to ensure the IDs exist and are valid before processing. This would provide better error messages to API consumers.
Example enhancement:
public function get_single_settings_field( $request ) { $group_id = $request->get_param( 'group_id' ); $parent_id = $request->get_param( 'parent_id' ); $id = $request->get_param( 'id' ); + + // Validate that the group exists + $group = ( new Processor() )->get_settings_group( $group_id ); + if ( is_wp_error( $group ) ) { + return $group; + } + + // Validate that the parent setting exists + $parent = ( new Processor() )->get_single_settings( $group_id, $parent_id ); + if ( is_wp_error( $parent ) ) { + return $parent; + } + return rest_ensure_response( ( new Processor() )->get_single_settings_field( $group_id, $parent_id, $id ) ); }
Line range hint
1-275: Consider implementing dependency injection for the Processor class.The Processor class is instantiated multiple times across different methods. Consider injecting it as a dependency in the constructor for better performance and testability.
Example implementation:
class StoreSettingControllerV2 extends StoreSettingController { + /** + * @var Processor + */ + private $processor; + + /** + * Constructor. + */ + public function __construct() { + parent::__construct(); + $this->processor = new Processor(); + } // Then replace all new Processor() instances with $this->processor
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/REST/StoreSettingControllerV2.php(3 hunks)
🔇 Additional comments (2)
includes/REST/StoreSettingControllerV2.php (2)
77-88: LGTM! Parameters match method signature.
The argument specifications for the single settings route are well-defined and align with the get_single_settings method parameters.
53-59: LGTM! Verify route parameter usage.
The argument specification for the settings group route follows WordPress REST API standards and properly defines the required group_id parameter.
Let's verify the group_id parameter usage across the codebase:
✅ Verification successful
Parameter usage verified and properly implemented
The group_id parameter is consistently used across all relevant methods in StoreSettingControllerV2.php, confirming that the argument specification matches its actual usage in the codebase. The parameter is properly retrieved using WordPress's standard get_param() method in all instances.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for group_id parameter usage in PHP files
rg -t php "get_param\(\s*['\"]group_id['\"]\s*\)"
Length of output: 662
| @@ -68,6 +74,18 @@ public function register_routes() { | |||
| 'methods' => WP_REST_Server::READABLE, | |||
| 'callback' => [ $this, 'get_single_settings' ], | |||
| 'permission_callback' => [ $this, 'get_settings_permission_callback' ], | |||
| 'args' => [ | |||
| 'group_id' => [ | |||
| 'description' => __( 'Unique identifier for the settings group.', 'dokan' ), | |||
There was a problem hiding this comment.
I think that it is not Unique identifier
There was a problem hiding this comment.
Bhai, based on this method, I thought it's a unique identifier. Here, the group existence depended on group_id.
public function get_settings_group( string $group_id ) {
$group_exist = $this->search_group( $group_id );
if ( is_wp_error( $group_exist ) ) {
return $group_exist;
}
$settings = apply_filters( 'dokan_vendor_rest_settings_for' . $group_id . '_page', [] );
return array_map( [ $this, 'populate_settings_elements' ], $settings );
}
mrabbani
added
Needs: Author Reply
and removed
Needs: Dev Review
pronob1010
added
Review Needed
Needs: Dev Review
mrabbani
added
👍 Dev Review Done
and removed
Needs: Dev Review
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
args are missing for several endpoints in json schema
Settings API endpoints args are updated.
Before Changes
Describe the issue before changes with screenshots(s).
After Changes
Describe the issue after changes with screenshot(s).
Feature Video (optional)
Link of detailed video if this PR is for a feature.
PR Self Review Checklist:
FOR PR REVIEWER ONLY:
Summary by CodeRabbit