refactor: Dokan REST base controllers and docs #2544
Conversation
WalkthroughThe pull request introduces a comprehensive restructuring of the Dokan REST API controllers. Key changes include renaming and refactoring existing abstract controller classes, introducing new base controllers for different user roles (admin, vendor, customer), and updating the documentation. The modifications aim to improve the modularity, extensibility, and clarity of the REST API implementation by providing more consistent base classes and clearer implementation guidelines. Changes
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
includes/REST/DokanBaseAdminController.php (1)
8-8: Update version placeholder.Replace
DOKAN_SINCEwith the actual version number.includes/REST/DokanBaseVendorController.php (1)
8-8: Update version placeholders.Replace
DOKAN_SINCEwith the actual version number in both the class and method documentation.Also applies to: 24-24
includes/REST/DokanBaseCustomerController.php (3)
5-5: Remove unnecessary use statement.The
use WeDevs\Dokan\REST\DokanBaseController;statement is unnecessary since the class is in the same namespace.-use WeDevs\Dokan\REST\DokanBaseController;
10-10: Update version placeholders.Replace
DOKAN_SINCEwith the actual version number in both the class and method documentation.Also applies to: 26-26
30-32: Consider additional customer validation.The current permission check only verifies if a user is logged in. Consider adding validation for the 'customer' role or capabilities if needed.
public function check_permission() { - return is_user_logged_in(); + return is_user_logged_in() && current_user_can( 'customer' ); }includes/REST/DokanBaseController.php (2)
10-10: Replace DOKAN_SINCE placeholder with actual version number.The @SInCE tag contains a placeholder that needs to be replaced with the actual version number where this class was introduced.
23-34: Fix parameter documentation inconsistency.The method documentation doesn't match the implementation:
- Parameter list shows
$itemsbut method uses$total_items@sincetag contains DOKAN_SINCE placeholderUpdate the documentation to match the implementation:
/** * Format item's collection for response * * @since DOKAN_SINCE * * @param object $response * @param object $request - * @param array $items * @param int $total_items * * @return object */docs/api/api.md (4)
8-11: Consider renaming 'DokanBaseBaseController'.The name 'DokanBaseBaseController' is redundant with double "Base". Consider renaming to 'DokanBaseController' to match the actual class name in the codebase.
15-39: Enhance implementation guidelines documentation.The documentation should list all required methods that need to be implemented when extending the base controllers. Consider adding:
- register_routes()
- get_items()
- get_item()
- create_item()
- update_item()
- delete_item()
75-77: Use Dokan text domain for error messages.The error message uses WooCommerce's text domain. Replace with Dokan's text domain:
- return new WP_Error( 'woocommerce_rest_invalid_id', __( 'Invalid resource ID.', 'woocommerce' ), array( 'status' => 404 ) ); + return new WP_Error( 'dokan_rest_invalid_id', __( 'Invalid resource ID.', 'dokan' ), array( 'status' => 404 ) );
91-91: Implement the TODO comment in the example.The example contains a TODO comment for API response formatting. Consider implementing it to provide a complete example.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/api/api.md(2 hunks)includes/Abstracts/DokanRESTAdminController.php(1 hunks)includes/Abstracts/DokanRESTBaseController.php(1 hunks)includes/Abstracts/DokanRESTCustomerController.php(1 hunks)includes/Abstracts/DokanRESTVendorController.php(1 hunks)includes/REST/DokanBaseAdminController.php(1 hunks)includes/REST/DokanBaseController.php(1 hunks)includes/REST/DokanBaseCustomerController.php(1 hunks)includes/REST/DokanBaseVendorController.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: e2e tests (1, 3)
- GitHub Check: api tests (1, 1)
🔇 Additional comments (18)
includes/Abstracts/DokanRESTAdminController.php (4)
5-5: Good use of the new base class.
Switching toDokanBaseAdminControlleris aligned with the refactoring approach and clarifies the scope of admin-specific functionality.
6-6: No concerns with this blank line.
It's just a formatting change with no functional impact.
11-11: Deprecation notice is consistent with the new class hierarchy.
This is a clear and explicit warning for users to migrate to the recommended base class.
15-15: Extending the new base class improves modularity and maintainability.
This aligns with your PR goal of refactoring the Dokan REST controllers for a clearer role-based separation.includes/Abstracts/DokanRESTBaseController.php (4)
5-5: Aligned usage of the new base controller.
UsingDokanBaseControllersimplifies inheritance and consolidates shared logic.
11-11: Clear deprecation reference.
Good job providing a direct pointer to the new implementation so developers know where to migrate.
15-15: Extending DokanBaseController ensures consistent fundamental features.
This is a solid approach, centralizing shared REST functionality.
10-10: Verify that the version is correct.
Make sure3.14.4matches the actual release version in which this deprecation becomes effective.✅ Verification successful
Version 3.14.4 is correctly referenced
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Searching for references to 3.14.4 to confirm version usage aligns with PR's release targets. rg "3\.14\.4"Length of output: 360
includes/Abstracts/DokanRESTCustomerController.php (4)
5-5: Use of the new base customer controller is appropriate.
It clearly indicates the scope of the functionality for customer endpoints only.
11-11: Deprecation documentation is clear.
Minimal friction for developers to migrate.
15-15: Extending the new base class improves maintainability and clarity.
This is consistent with the rest of the refactoring in this PR.
10-10: Confirm the documented version is accurate.
Likewise, ensure3.14.4is indeed the intended version for these changes.✅ Verification successful
Version 3.14.4 is correctly documented
The version number is consistently used across the codebase, including release notes, documentation, and related controller classes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Searching for references to 3.14.4 to confirm if it's consistently used throughout the codebase. rg "3\.14\.4"Length of output: 360
includes/Abstracts/DokanRESTVendorController.php (2)
10-11: LGTM! Version number is correctly specified.The version number is properly updated from the placeholder to a specific version (3.14.4).
11-11: Verify the deprecation constant.The deprecation notice uses
DOKAN_SINCEconstant. Please ensure this constant is defined and contains the correct version number.Also applies to: 15-15
includes/REST/DokanBaseAdminController.php (1)
26-28: LGTM! Permission check is appropriate.Using
manage_woocommercecapability is the correct approach for admin-level operations.includes/REST/DokanBaseVendorController.php (1)
28-30: Verify the 'dokandar' capability.The permission check uses the 'dokandar' capability. Let's verify this capability is registered:
✅ Verification successful
'dokandar' capability is properly registered ✓
The capability is registered during installation and assigned to both 'shop_manager' and 'administrator' roles in
includes/Install/Installer.php.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for dokandar capability registration rg -B 2 -A 2 "dokandar.*capability|add_cap.*dokandar"Length of output: 798
includes/REST/DokanBaseController.php (1)
16-21: LGTM! Proper REST API namespace definition.The namespace property follows REST API versioning best practices with a protected scope.
docs/api/api.md (1)
93-100: LGTM! Well-documented filter.The filter documentation follows WordPress standards and provides clear information about parameters.
| public function format_collection_response( $response, $request, $total_items ) { | ||
| if ( $total_items === 0 ) { | ||
| return $response; | ||
| } | ||
|
|
||
| // Pagination values for headers. | ||
| $per_page = (int) ( ! empty( $request['per_page'] ) ? $request['per_page'] : 20 ); | ||
| $page = (int) ( ! empty( $request['page'] ) ? $request['page'] : 1 ); | ||
|
|
||
| $response->header( 'X-WP-Total', (int) $total_items ); | ||
|
|
||
| $max_pages = ceil( $total_items / $per_page ); | ||
|
|
||
| $response->header( 'X-WP-TotalPages', (int) $max_pages ); | ||
| $base = add_query_arg( $request->get_query_params(), rest_url( sprintf( '/%s/%s', $this->namespace, $this->base ) ) ); | ||
|
|
||
| if ( $page > 1 ) { | ||
| $prev_page = $page - 1; | ||
| if ( $prev_page > $max_pages ) { | ||
| $prev_page = $max_pages; | ||
| } | ||
| $prev_link = add_query_arg( 'page', $prev_page, $base ); | ||
| $response->link_header( 'prev', $prev_link ); | ||
| } | ||
| if ( $max_pages > $page ) { | ||
| $next_page = $page + 1; | ||
| $next_link = add_query_arg( 'page', $next_page, $base ); | ||
| $response->link_header( 'next', $next_link ); | ||
| } | ||
|
|
||
| return $response; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for pagination parameters.
The pagination logic needs additional validation to handle edge cases:
- Negative page numbers
- Negative per_page values
- Upper limit on per_page to prevent resource exhaustion
public function format_collection_response( $response, $request, $total_items ) {
if ( $total_items === 0 ) {
return $response;
}
// Pagination values for headers.
- $per_page = (int) ( ! empty( $request['per_page'] ) ? $request['per_page'] : 20 );
- $page = (int) ( ! empty( $request['page'] ) ? $request['page'] : 1 );
+ $per_page = (int) ( ! empty( $request['per_page'] ) ? $request['per_page'] : 20 );
+ $per_page = max( 1, min( 100, $per_page ) ); // Ensure between 1 and 100
+ $page = max( 1, (int) ( ! empty( $request['page'] ) ? $request['page'] : 1 ) );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function format_collection_response( $response, $request, $total_items ) { | |
| if ( $total_items === 0 ) { | |
| return $response; | |
| } | |
| // Pagination values for headers. | |
| $per_page = (int) ( ! empty( $request['per_page'] ) ? $request['per_page'] : 20 ); | |
| $page = (int) ( ! empty( $request['page'] ) ? $request['page'] : 1 ); | |
| $response->header( 'X-WP-Total', (int) $total_items ); | |
| $max_pages = ceil( $total_items / $per_page ); | |
| $response->header( 'X-WP-TotalPages', (int) $max_pages ); | |
| $base = add_query_arg( $request->get_query_params(), rest_url( sprintf( '/%s/%s', $this->namespace, $this->base ) ) ); | |
| if ( $page > 1 ) { | |
| $prev_page = $page - 1; | |
| if ( $prev_page > $max_pages ) { | |
| $prev_page = $max_pages; | |
| } | |
| $prev_link = add_query_arg( 'page', $prev_page, $base ); | |
| $response->link_header( 'prev', $prev_link ); | |
| } | |
| if ( $max_pages > $page ) { | |
| $next_page = $page + 1; | |
| $next_link = add_query_arg( 'page', $next_page, $base ); | |
| $response->link_header( 'next', $next_link ); | |
| } | |
| return $response; | |
| } | |
| public function format_collection_response( $response, $request, $total_items ) { | |
| if ( $total_items === 0 ) { | |
| return $response; | |
| } | |
| // Pagination values for headers. | |
| $per_page = (int) ( ! empty( $request['per_page'] ) ? $request['per_page'] : 20 ); | |
| $per_page = max( 1, min( 100, $per_page ) ); // Ensure between 1 and 100 | |
| $page = max( 1, (int) ( ! empty( $request['page'] ) ? $request['page'] : 1 ) ); | |
| $response->header( 'X-WP-Total', (int) $total_items ); | |
| $max_pages = ceil( $total_items / $per_page ); | |
| $response->header( 'X-WP-TotalPages', (int) $max_pages ); | |
| $base = add_query_arg( $request->get_query_params(), rest_url( sprintf( '/%s/%s', $this->namespace, $this->base ) ) ); | |
| if ( $page > 1 ) { | |
| $prev_page = $page - 1; | |
| if ( $prev_page > $max_pages ) { | |
| $prev_page = $max_pages; | |
| } | |
| $prev_link = add_query_arg( 'page', $prev_page, $base ); | |
| $response->link_header( 'prev', $prev_link ); | |
| } | |
| if ( $max_pages > $page ) { | |
| $next_page = $page + 1; | |
| $next_link = add_query_arg( 'page', $next_page, $base ); | |
| $response->link_header( 'next', $next_link ); | |
| } | |
| return $response; | |
| } |
…he vendor authorizaton logic
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
includes/Traits/VendorAuthorizable.php (2)
9-9: Update @SInCE tag with actual version number.Replace the placeholder "DOKAN_SINCE" with the actual version number where this trait was introduced.
13-15: Consider enhancing the permission check implementation.A few suggestions to improve the implementation:
- Add return type declaration for PHP 7.4+ compatibility
- Consider adding error handling for edge cases
- Make the method name more specific about what permission it's checking
- public function check_permission() { + public function check_vendor_permission(): bool { return current_user_can( 'dokandar' ); }docs/api/api.md (3)
42-47: Enhance error handling in prepare_item_for_response example.The error message could be more helpful by suggesting what methods need to be implemented. Also, consider using a more specific error code.
return new WP_Error( - 'invalid-method', + 'dokan-method-not-implemented', sprintf( __( "Method '%s' not implemented. Must be overridden in subclass." ), __METHOD__ ), array( 'status' => 405 ) );
124-125: Complete the TODO implementation in prepare_item_for_response.The example should include a basic implementation to demonstrate proper response formatting.
Would you like me to provide a complete example implementation for the prepare_item_for_response method?
Line range hint
79-113: Enhance the admin controller example with proper error handling and validation.The example could be improved in several areas:
- Add input validation for the collection parameters
- Include proper error handling for the permission check
- Document the expected response format
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/api/api.md(2 hunks)includes/REST/DokanBaseVendorController.php(1 hunks)includes/Traits/VendorAuthorizable.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/REST/DokanBaseVendorController.php
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/api/api.md
11-11: Link fragments should be valid
null
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
🔇 Additional comments (2)
docs/api/api.md (2)
14-21: LGTM! Clear hierarchy explanation.The documentation clearly explains the controller hierarchy and role-specific implementations.
53-70: Well-documented trait usage.The VendorAuthorizable trait documentation is clear and includes a practical example.
| - [VendorAuthorizable Trait](#vendorauthorizable-trait) | ||
| - [Example Implementations](#example-implementations) | ||
| - [Admin Controller Example](#admin-controller-example) | ||
| - [Vendor Controller Example](#vendor-controller-example) |
There was a problem hiding this comment.
Fix invalid link fragment in table of contents.
The link fragment for "Vendor Controller Example" is not properly defined in the document. Add a corresponding heading with the correct ID.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Link fragments should be valid
null
(MD051, link-fragments)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/api/api.md (2)
24-24: Add default namespace information.Consider adding information about the default namespaces for each base controller type (e.g., 'dokan/v1/admin' for
DokanBaseAdminController).
124-124: Implement the TODO in example code.The example includes a TODO comment for API response formatting. Consider implementing a basic example to demonstrate the expected response format.
Would you like me to provide an example implementation for the API response formatting?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/api/api.md(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/api/api.md
11-11: Link fragments should be valid
null
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e tests (3, 3)
- GitHub Check: e2e tests (2, 3)
- GitHub Check: api tests (1, 1)
- GitHub Check: e2e tests (1, 3)
🔇 Additional comments (3)
docs/api/api.md (3)
11-11: Fix invalid link fragment in table of contents.The link fragment for "Vendor Controller Example" is not properly defined in the document. Add a corresponding heading with the correct ID.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Link fragments should be valid
null(MD051, link-fragments)
14-14: Consider renaming base controller for consistency.Based on previous discussions, consider using
DokanBaseControllerinstead ofDokanBaseBaseControllerfor better naming consistency.
57-70: Well-documented trait usage!The documentation clearly explains the purpose and implementation of the
VendorAuthorizabletrait with a practical example.
| */ | ||
| public function prepare_item_for_response( $user_data, $request ) { | ||
| // TODO: Implement API response formatting | ||
| return apply_filters( 'dokan_rest_prepare_admin', $response, $user_data, $request ); | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing vendor controller example.
The table of contents references a "Vendor Controller Example" section, but the implementation is missing. Consider adding an example that demonstrates vendor-specific functionality using the VendorAuthorizable trait.
Would you like me to provide an example implementation for the vendor controller?
…ormat_collection_response`.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
includes/REST/DokanBaseController.php (3)
21-21: Consider exposing a$versionproperty.Following previous feedback from another reviewer, if you wish to allow flexible versioning (e.g.,
v2), consider defining a dedicated$versionproperty and referencing it within$namespace.
28-31: Remove or utilize the$itemsparameter.Your doc block references
$itemsas a parameter, but it’s unused in the method body. This may cause confusion about the actual usage of the parameter.
28-34: Add param types in doc blocks for clarity.For enhanced clarity and consistency in WordPress docs, specify types in the doc blocks (e.g.,
$requestasWP_REST_Request,$responseasWP_REST_Response).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/REST/DokanBaseController.php(1 hunks)
🔇 Additional comments (2)
includes/REST/DokanBaseController.php (2)
41-42: Revisit pagination input validation (Duplicate of previous suggestion).As previously suggested, consider validating pagination parameters to handle edge cases (negative or excessively large values) to avoid unintended resource usage.
49-49: Verify that$this->rest_baseis declared in child classes.This abstract class assumes
$this->rest_basehas been set elsewhere. Confirm this property is properly assigned and documented in extending classes to prevent undefined property errors.
|
@MdAsifHossainNadim vai, pls fix the merge conflict. |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
docs/api/api.md (1)
74-128:⚠️ Potential issueAdmin Controller Example – Undefined Variable Warning
The Admin Controller Example is comprehensive and effectively demonstrates setting up routes and overriding methods. However, in the
prepare_item_for_responsemethod, the variable$responseis referenced without prior initialization. This may lead to runtime errors. Consider initializing$response(even with a placeholder value) before it’s passed toapply_filters. For example:- return apply_filters( 'dokan_rest_prepare_admin', $response, $user_data, $request ); + $response = []; // Initialize response structure + return apply_filters( 'dokan_rest_prepare_admin', $response, $user_data, $request );
🧹 Nitpick comments (1)
docs/api/api.md (1)
55-60: VendorAuthorizable Trait DocumentationThe documentation for the
VendorAuthorizabletrait is informative and explains its purpose well. To further enhance developer understanding, consider linking to the trait’s source file or adding a brief description of its internal behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/api/api.md(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/api/api.md
11-11: Link fragments should be valid
null
(MD051, link-fragments)
🔇 Additional comments (7)
docs/api/api.md (7)
3-11: TOC Navigation and Link ValidationThe addition of a comprehensive Table of Contents significantly enhances document navigation. However, the link for "Vendor Controller Example" (
#vendor-controller-example) does not appear to correspond to any header in the document. Please ensure that a matching section is added or update the link accordingly.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Link fragments should be valid
null(MD051, link-fragments)
22-30: Clear Instructions for REST Controller CreationThe "Creating a REST Controller" section is well-structured and clearly instructs developers to extend one of the Dokan base controllers and override the necessary methods. This guidance will help ensure proper implementation.
32-49: Placeholder inprepare_item_for_responseMethodThe provided placeholder for
prepare_item_for_response—which returns aWP_Errorto signal that the method is not implemented—is clear and effectively communicates that subclasses must override this method. Consider adding a brief note or sample implementation to further assist developers.
51-54: Extending Other Controllers SectionThe explanation regarding extending other controllers and the introduction of the
VendorAuthorizabletrait is concise and helpful. This section clearly motivates the use of the trait for managing vendor capabilities.
62-68: Example Usage ofVendorAuthorizableTraitThe code snippet demonstrating the inclusion and usage of the
VendorAuthorizabletrait is succinct and clear. It provides a good example for developers looking to incorporate vendor capability checks.
70-72: Explanation ofcheck_permissionMethodThe additional explanation that the trait provides a
check_permissionmethod is clear. This helps set the expectation regarding the capabilities offered by the trait.
129-132: Concluding Section ClarityThe final remarks effectively summarize the document structure and the standardized format of the controllers. This enhances readability and sets clear expectations for developers.
| ## Introduction | ||
| The Dokan REST Controllers provide a structured approach to handle REST API endpoints with role-based permissions. The hierarchy consists of a base controller (`DokanRESTBaseController`) and role-specific controllers (`DokanRESTAdminController`, `DokanRESTVendorController`, `DokanRESTCustomerController`). | ||
| Dokan REST Controllers provide a structured approach to managing REST API endpoints with role-based permissions. The hierarchy consists of a base controller, `DokanBaseController`, and role-specific controllers: | ||
|
|
||
| ## Base REST Controller | ||
| The `DokanRESTBaseController` extends WordPress's `WP_REST_Controller` and provides common functionality for all Dokan REST endpoints. | ||
| - `DokanBaseAdminController` | ||
| - `DokanBaseVendorController` | ||
| - `DokanBaseCustomerController` | ||
|
|
||
| ### Base Controller Implementation | ||
| ```php | ||
| abstract class DokanRESTBaseController extends WP_REST_Controller { | ||
| protected $namespace = 'dokan/v1'; | ||
| These controllers extend the core `WP_REST_Controller` and provide predefined functionalities for different user roles. | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consistency in Base Controller Naming
The introduction refers to a base controller as DokanBaseController, but the PR summary and enriched documentation indicate a renaming (e.g., to DokanBaseBaseController). For clarity and consistency, please verify and update the base controller’s name throughout the document.
🏁 Script executed:
#!/bin/bash
# Search for both possible base controller names in the documentation
echo "Searching for 'DokanBaseController' in docs/api/api.md:"
rg "DokanBaseController" docs/api/api.md
echo "Searching for 'DokanBaseBaseController' in docs/api/api.md:"
rg "DokanBaseBaseController" docs/api/api.mdLength of output: 559
Update the Base Controller Name for Consistency
The introduction in the docs/api/api.md file currently refers to the controller as DokanBaseController, but the updated PR summary and enriched documentation indicate that it should now be named DokanBaseBaseController. Please update the name throughout the document to maintain clarity and consistency. Specifically:
- Replace
DokanBaseControllerwithDokanBaseBaseControllerin the introduction snippet (lines 13–21).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
includes/REST/DokanBaseController.php (2)
21-21: Consider including a$versionproperty for clarity and maintainability.A previous reviewer suggested exposing a
$versionproperty. This may help with clearer REST routing or debugging, especially if you plan to update thenamespaceregularly based on versioning.
30-30: Remove or utilize the$itemsparameter.The
$itemsparameter is declared but never used. This can lead to confusion for future maintainers.- * @param array $items ... - public function format_collection_response( $response, $request, $total_items ) { + public function format_collection_response( $response, $request, $total_items ) {Either remove
$itemsif unnecessary or integrate it to provide additional context within the formatted response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/REST/DokanBaseController.php(1 hunks)
🔇 Additional comments (2)
includes/REST/DokanBaseController.php (2)
41-42: [Duplicate Comment] Add input validation for pagination parameters.As noted in a previous review, the pagination logic lacks strict validation. Consider clamping the values or handling edge cases (e.g., negative or extremely large values).
$per_page = (int) ( ! empty( $request['per_page'] ) ? $request['per_page'] : 20 ); +$per_page = max( 1, min( 100, $per_page ) ); // Ensure between 1 and 100 $page = (int) ( ! empty( $request['page'] ) ? $request['page'] : 1 ); +$page = max( 1, $page );
49-49: Verify that$baseis defined or derived correctly in child classes.The expression
$this->base ?? $this->rest_baseassumes at least one property is set. If neither$this->basenor$this->rest_baseis declared in a subclass, the generated URL might be invalid.Please ensure each extending controller properly sets
$this->baseor$this->rest_base.
All Submissions:
Changes proposed in this Pull Request:
Related Pull Request(s)
Closes
How to test the changes in this Pull Request:
Changelog entry
Title
Detailed Description of the pull request. What was previous behaviour
and what will be changed in this PR.
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
Documentation
Refactor
New Features
VendorAuthorizabletrait for vendor capability checks.These changes improve the modularity and extensibility of Dokan's REST API architecture.