feat: Dokan Data Store #2346
feat: Dokan Data Store #2346
Conversation
WalkthroughThe changes introduce several new classes and interfaces within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 11
Outside diff range, codebase verification and nitpick comments (2)
includes/DataStore/TestModel.php (1)
9-9: Clarify the object type.The comment suggests changing the object type. Ensure it reflects the intended use.
includes/DataStore/DataStore.php (1)
23-50: Clarify object type handling in the constructor.The logic for handling object types could be clarified by adding comments or restructuring the code for better readability.
Consider adding comments or restructuring the code to make the object type handling clearer.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- includes/DataStore/AbstractDataStore.php (1 hunks)
- includes/DataStore/DataStore.php (1 hunks)
- includes/DataStore/DataStoreInterface.php (1 hunks)
- includes/DataStore/Model.php (1 hunks)
- includes/DataStore/TestDataStore.php (1 hunks)
- includes/DataStore/TestModel.php (1 hunks)
Additional comments not posted (7)
includes/DataStore/AbstractDataStore.php (1)
1-7: LGTM! Abstract class setup is appropriate.The
AbstractDataStoreclass correctly extendsSqlQuery, providing a foundation for further data store implementations.includes/DataStore/Model.php (1)
19-28: Verify data store loading in the constructor.The constructor attempts to load a data store and handle exceptions. Ensure that
DataStore::loadandget_instanceare correctly implemented.Run the following script to verify the implementation of
DataStore::loadandget_instance:Verification successful
Data store loading in the constructor is verified successfully.
The
DataStore::loadandget_instancemethods are correctly implemented, ensuring the data store is properly loaded in the constructor. No issues found.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of DataStore::load and get_instance. # Test: Search for the DataStore class and its methods. rg --type php -A 5 'class DataStore'Length of output: 366
Script:
#!/bin/bash # Description: Verify the implementation of DataStore::load and get_instance. # Test: Search for the load method in the DataStore class. rg --type php -A 10 'function load' includes/DataStore/DataStore.php # Test: Search for the get_instance method in the DataStore class. rg --type php -A 10 'function get_instance' includes/DataStore/DataStore.phpLength of output: 514
includes/DataStore/DataStoreInterface.php (1)
1-80: LGTM! Interface definition is appropriate.The
DataStoreInterfaceprovides a clear contract for data store implementations, covering CRUD operations and querying.includes/DataStore/DataStore.php (4)
3-5: Namespace and imports are correctly defined.The namespace
WeDevs\Dokan\DataStoreis appropriate, and theExceptionimport is necessary for error handling.
7-18: Class declaration and properties are well-defined.The
DataStoreclass and its protected properties are appropriately declared, promoting encapsulation.
56-58: Staticloadmethod is correctly implemented.The method correctly creates and returns a new
DataStoreinstance.
63-65:get_instancemethod is correctly implemented.The method correctly returns the data store instance.
includes/DataStore/TestModel.php
Outdated
| public function get(): Model { | ||
| // TODO: Implement get() method. | ||
| } |
There was a problem hiding this comment.
Implement the get method.
The get method is currently unimplemented. Consider implementing it to retrieve model data.
Would you like assistance in implementing this method?
includes/DataStore/TestModel.php
Outdated
| public function create(): Model { | ||
| // TODO: Implement create() method. | ||
| } |
There was a problem hiding this comment.
Implement the create method.
The create method is currently unimplemented. Consider implementing it to create new model data.
Would you like assistance in implementing this method?
includes/DataStore/TestModel.php
Outdated
| public function update(): Model { | ||
| // TODO: Implement update() method. | ||
| } |
There was a problem hiding this comment.
Implement the update method.
The update method is currently unimplemented. Consider implementing it to update existing model data.
Would you like assistance in implementing this method?
includes/DataStore/TestModel.php
Outdated
| public function delete(): bool { | ||
| // TODO: Implement delete() method. | ||
| } |
There was a problem hiding this comment.
Implement the delete method.
The delete method is currently unimplemented. Consider implementing it to delete model data.
Would you like assistance in implementing this method?
includes/DataStore/TestDataStore.php
Outdated
| public function create( &$model ) { | ||
| // TODO: Implement create() method. | ||
| } |
There was a problem hiding this comment.
Implement the create method.
The create method is currently unimplemented. Consider implementing it to handle model creation.
Would you like assistance in implementing this method?
includes/DataStore/TestDataStore.php
Outdated
| public function delete( &$model ) { | ||
| // TODO: Implement delete() method. | ||
| } |
There was a problem hiding this comment.
Implement the delete method.
The delete method is currently unimplemented. Consider implementing it to handle model deletion.
Would you like assistance in implementing this method?
includes/DataStore/TestDataStore.php
Outdated
| public function get( &$model ) { | ||
| // TODO: Implement get() method. | ||
| } |
There was a problem hiding this comment.
Implement the get method.
The get method is currently unimplemented. Consider implementing it to retrieve model data.
Would you like assistance in implementing this method?
includes/DataStore/TestDataStore.php
Outdated
| public function query( array $args = [] ): array { | ||
| // TODO: Implement query() method. | ||
| } |
There was a problem hiding this comment.
Implement the query method.
The query method is currently unimplemented. Consider implementing it to handle data queries.
Would you like assistance in implementing this method?
includes/DataStore/TestDataStore.php
Outdated
| public function count( array $args = [] ): int { | ||
| // TODO: Implement count() method. | ||
| } |
There was a problem hiding this comment.
Implement the count method.
The count method is currently unimplemented. Consider implementing it to count data entries.
Would you like assistance in implementing this method?
includes/DataStore/Model.php
Outdated
| abstract public function get(): Model; | ||
|
|
||
| /** | ||
| * Store a new model data. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @return Model | ||
| */ | ||
| abstract public function create(): Model; | ||
|
|
||
| /** | ||
| * Update the model data. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @return Model | ||
| */ | ||
| abstract public function update(): Model; | ||
|
|
||
| /** | ||
| * Delete the model data. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @return bool | ||
| */ | ||
| abstract public function delete(): bool; |
There was a problem hiding this comment.
Ensure implementation of abstract methods.
The abstract methods get, create, update, and delete need to be implemented in derived classes.
Would you like assistance in implementing these methods in a derived class?
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range and nitpick comments (13)
tests/php/src/DataStore/StoreTest.php (2)
7-13: LGTM: Class declaration and property are well-defined.The class name follows the convention for test classes, and the property is correctly defined with appropriate visibility and documentation.
Consider adding a class-level PHPDoc block to provide a brief description of the StoreTest class and its purpose.
1-18: Overall: Good structure, but needs more comprehensive testing.The file provides a good starting point for Store testing, with correct namespace, class structure, and basic setup. However, to ensure robust testing of the Store functionality:
- Implement multiple test methods covering various aspects of the Store.
- Add setup and teardown methods if needed for test data management.
- Include edge cases and error scenarios in your tests.
- Add a class-level PHPDoc block describing the purpose of StoreTest.
These improvements will enhance the test coverage and maintainability of the Store feature.
Consider organizing tests into separate methods for different Store operations (e.g.,
test_store_create(),test_store_read(), etc.) to improve test clarity and coverage.includes/Models/DataStore/TestModel.php (2)
1-7: Add a file-level docblock for better documentation.Consider adding a file-level docblock at the beginning of the file to provide a brief description of the purpose of this class and any relevant information for developers who might work with it in the future.
Here's a suggested docblock:
/** * TestModel class file. * * This file contains the TestModel class which extends the base Model class * and provides specific implementation for the 'test' object type. * * @package WeDevs\Dokan\DataStore */
1-38: Add PHPDoc comments for all methods.To improve code documentation and maintainability, consider adding PHPDoc comments for each method. This will provide better context for developers working with this class in the future.
Here's an example for the
get()method:/** * Get the test object. * * @since [version] * * @throws \WeDevs\Dokan\Exceptions\DokanException When the object cannot be found. * * @return Model The current object instance. */ public function get(): Model { // Implementation here }Apply similar PHPDoc comments to all other methods in the class.
includes/Models/DataStore/TestDataStore.php (1)
1-8: Enhance class and method documentation.While the use of
@inheritDoctags is good for inheriting interface documentation, additional context would improve code clarity:
- Add a class-level PHPDoc block describing the purpose, usage, and any important details about
TestDataStore.- Consider adding method-level documentation to provide context on the expected behavior, even if it inherits from the interface. This is particularly important for methods like
queryandcount, where implementation details might vary.Here's an example of improved documentation for the class and the
querymethod:/** * TestDataStore class for managing test-related data operations. * * This class provides an implementation of the DataStoreInterface * specifically tailored for test data management within the Dokan plugin. * * @since x.x.x */ class TestDataStore extends AbstractDataStore implements DataStoreInterface { // ... /** * @inheritDoc * * Queries test data based on the provided arguments. * * @param array $args An array of query arguments. Possible keys include: * - 'limit': int The maximum number of results to return. * - 'offset': int The number of results to skip. * // Add other relevant arguments * * @return array An array of test data matching the query arguments. */ public function query( array $args = [] ): array { // TODO: Implement query() method. } // ... }Also applies to: 10-13, 17-20, 24-27, 31-34, 38-41, 45-48
includes/Models/DataStore/DataStoreInterface.php (3)
5-10: ReplaceDOKAN_SINCEplaceholder with actual version number.The
@sincetag in the interface's PHPDoc block is using a placeholderDOKAN_SINCE. This should be replaced with the actual version number in which this interface is being introduced.
23-33: Consider specifying the exception type.The PHPDoc for the
updatemethod mentions that it may throw an exception. It would be more informative to specify the type of exception that might be thrown, e.g.,@throws \InvalidArgumentException.
35-45: Consider specifying the exception type.Similar to the
updatemethod, thedeletemethod's PHPDoc mentions that it may throw an exception. It would be beneficial to specify the type of exception that might be thrown, e.g.,@throws \RuntimeException.includes/Models/DataStore/Model.php (2)
12-13: ReplaceDOKAN_SINCEwith the appropriate version numberThe
@since DOKAN_SINCEtags should be replaced with the actual version number of the plugin when the feature is introduced.Ensure to update all occurrences before releasing to maintain accurate documentation.
Also applies to: 35-36, 44-45, 53-54, 62-63
16-16: Organize property declarations for better readabilityGroup protected properties together at the beginning of the class for improved maintainability and readability.
Reorder the properties:
protected int $id; protected string $object_type; +protected DataStoreInterface $data_store;Also applies to: 30-30
includes/Models/DataStore/DataStore.php (2)
12-14: Remove redundant docblocks for typed propertiesProperties
$instanceand$current_class_nameare already type-hinted withDataStoreInterfaceandstringrespectively. The@varannotations are redundant.Consider removing the docblocks to keep the code clean:
12-/** 13- * @var DataStoreInterface 14- */ 15 protected DataStoreInterface $instance; 16-/** 17- * @var string 18- */ 19 protected string $current_class_name;Also applies to: 16-18
38-38: Provide more descriptive exception messagesThe exception messages
'Invalid data store.'lack context, which might make debugging difficult.Consider including the
$object_typeor$storein the exception messages for better clarity:38 throw new Exception( esc_html__( 'Invalid data store: ' . get_class( $store ), 'dokan-lite' ) ); 44 throw new Exception( esc_html__( 'Data store class not found: ' . $store, 'dokan-lite' ) ); 50 throw new Exception( esc_html__( 'Data store not defined for object type: ' . $object_type, 'dokan-lite' ) );This provides more informative error messages that can aid in troubleshooting.
Also applies to: 44-44, 50-50
includes/Models/BaseModel.php (1)
14-14: Remove unnecessary comment insave()method.The comment
// wc_get_product()on line 14 appears to be unrelated to thesave()method and may cause confusion. Consider removing it to keep the code clean and maintainable.Apply this diff to remove the unnecessary comment:
- // wc_get_product()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- includes/Models/BaseModel.php (1 hunks)
- includes/Models/DataStore/AbstractDataStore.php (1 hunks)
- includes/Models/DataStore/DataStore.php (1 hunks)
- includes/Models/DataStore/DataStoreInterface.php (1 hunks)
- includes/Models/DataStore/Model.php (1 hunks)
- includes/Models/DataStore/TestDataStore.php (1 hunks)
- includes/Models/DataStore/TestModel.php (1 hunks)
- tests/php/src/DataStore/StoreTest.php (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Models/DataStore/AbstractDataStore.php
[failure] 85-85:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format"
[failure] 85-85:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format"
[failure] 85-85:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format"
[warning] 85-85:
Replacement variables found, but no valid placeholders found in the query.
[failure] 130-130:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 131-131:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 131-131:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 131-131:
Replacement variables found, but no valid placeholders found in the query.includes/Models/DataStore/DataStore.php
[failure] 11-11:
Whitespace found at end of line
🔇 Additional comments (6)
tests/php/src/DataStore/StoreTest.php (1)
1-5: LGTM: Namespace and use statement are correct.The namespace follows the PSR-4 autoloading standard, and the use statement correctly imports the base test case class.
includes/Models/DataStore/TestDataStore.php (1)
1-8: LGTM: Class structure and namespace are well-defined.The
TestDataStoreclass is correctly placed in theWeDevs\Dokan\Models\DataStorenamespace, extendsAbstractDataStore, and implementsDataStoreInterface. The use statements are properly included.includes/Models/DataStore/DataStoreInterface.php (3)
12-21: LGTM:createmethod is well-defined.The
createmethod is correctly declared with appropriate parameter and return type. The use of a reference for theModelparameter is a good choice for efficiency.
59-68: LGTM:querymethod is well-defined.The
querymethod is correctly declared with an appropriate parameter and return type. The optional array parameter provides flexibility for various query scenarios.
70-80: LGTM:countmethod is well-defined.The
countmethod is correctly declared with an appropriate parameter and return type. The optional array parameter provides flexibility for various counting scenarios.includes/Models/DataStore/Model.php (1)
22-24: Verify the existence of methods used in$store->get_instance()Ensure that the
DataStoreclass has theget_instance()method, and the returned instance has aget()method that accepts the model.Run the following script to confirm the presence of these methods:
| public function test_store() { | ||
| $this->assertTrue( true ); | ||
| } |
There was a problem hiding this comment.
Enhance the test method with meaningful assertions.
The current test method is a placeholder and doesn't test any actual functionality of the Store. Consider the following improvements:
- Implement actual test cases for the Store functionality.
- Add setup and teardown methods if needed for Store testing.
- Use descriptive test method names that indicate what is being tested.
Would you like assistance in generating more comprehensive test cases for the Store functionality?
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function get(): Model { | ||
| // TODO: Implement get() method. | ||
| } |
There was a problem hiding this comment.
Implement the get() method.
The get() method is currently unimplemented. This method should retrieve and return a Model instance based on some identifier.
Consider implementing it as follows:
public function get(): Model {
// Retrieve the data from the data store
$data = $this->data_store->read($this->get_id());
// Update the current object with the retrieved data
$this->set_props($data);
return $this;
}Ensure to handle cases where the data might not be found, possibly by throwing a \WeDevs\Dokan\Exceptions\DokanException.
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function create(): Model { | ||
| // TODO: Implement create() method. | ||
| } |
There was a problem hiding this comment.
Implement the create() method.
The create() method is currently unimplemented. This method should create a new entry in the data store and return the updated Model instance.
Consider implementing it as follows:
public function create(): Model {
if ($this->get_id()) {
throw new \WeDevs\Dokan\Exceptions\DokanException('dokan_invalid_operation', __('Cannot create an existing test object.', 'dokan-lite'));
}
// Prepare data for creation
$data = $this->get_changes();
// Create the entry in the data store
$id = $this->data_store->create($data);
// Set the ID of the newly created object
$this->set_id($id);
// Clear the changes as they've been saved
$this->clear_changes();
return $this;
}Ensure to handle any potential errors during the creation process.
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function update(): Model { | ||
| // TODO: Implement update() method. | ||
| } |
There was a problem hiding this comment.
Implement the update() method.
The update() method is currently unimplemented. This method should update an existing entry in the data store and return the updated Model instance.
Consider implementing it as follows:
public function update(): Model {
if (!$this->get_id()) {
throw new \WeDevs\Dokan\Exceptions\DokanException('dokan_invalid_operation', __('Cannot update a test object without an ID.', 'dokan-lite'));
}
// Get only the changed data
$changes = $this->get_changes();
if (empty($changes)) {
return $this;
}
// Update the entry in the data store
$this->data_store->update($this->get_id(), $changes);
// Clear the changes as they've been saved
$this->clear_changes();
return $this;
}Ensure to handle any potential errors during the update process.
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function delete(): bool { | ||
| // TODO: Implement delete() method. | ||
| } |
There was a problem hiding this comment.
Implement the delete() method.
The delete() method is currently unimplemented. This method should delete the entry from the data store and return a boolean indicating success or failure.
Consider implementing it as follows:
public function delete(): bool {
if (!$this->get_id()) {
throw new \WeDevs\Dokan\Exceptions\DokanException('dokan_invalid_operation', __('Cannot delete a test object without an ID.', 'dokan-lite'));
}
// Delete the entry from the data store
$result = $this->data_store->delete($this->get_id());
if ($result) {
$this->set_id(0);
$this->clear_changes();
}
return $result;
}Ensure to handle any potential errors during the deletion process.
| * Filters whether an object deletion should take place. Equivalent to `pre_delete_post`. | ||
| * | ||
| * @param mixed $check Whether to go ahead with deletion. | ||
| * @param Data $this The data object being deleted. |
There was a problem hiding this comment.
Ensure consistency in parameter types in docblocks.
At line 55, the parameter type for $this is listed as Data. To maintain consistency and accuracy, it should be WC_Data, matching the type used in other methods.
Apply this diff to correct the parameter type:
- * @param Data $this The data object being deleted.
+ * @param WC_Data $this The data object being deleted.📝 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.
| * @param Data $this The data object being deleted. | |
| * @param WC_Data $this The data object being deleted. |
| * | ||
| * @since 8.1.0. | ||
| */ | ||
| $check = apply_filters( "dokan_pre_delete_$this->object_type", null, $this, $force_delete ); |
There was a problem hiding this comment.
Fix variable interpolation in apply_filters call.
When interpolating object properties within double-quoted strings, you need to enclose the property in braces to ensure correct parsing. Without braces, PHP may not parse the variable correctly.
Apply this diff to fix the variable interpolation:
- $check = apply_filters( "dokan_pre_delete_$this->object_type", null, $this, $force_delete );
+ $check = apply_filters( "dokan_pre_delete_{$this->object_type}", null, $this, $force_delete );📝 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.
| $check = apply_filters( "dokan_pre_delete_$this->object_type", null, $this, $force_delete ); | |
| $check = apply_filters( "dokan_pre_delete_{$this->object_type}", null, $this, $force_delete ); |
| if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) { | ||
| $table_name = $wpdb->prefix . $table_name; | ||
| } |
There was a problem hiding this comment.
Use Compatible Functions to Maintain PHP Version Support
The function str_starts_with() is used, which is available only from PHP 8.0 onwards. If the codebase needs to support earlier PHP versions (e.g., PHP 7.x), consider using an alternative function to ensure compatibility.
You can replace str_starts_with() with a compatible approach:
- if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) {
+ if ( strpos( $table_name, $wpdb->prefix ) !== 0 ) {This change ensures that the code remains compatible with PHP versions prior to 8.0.
📝 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.
| if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) { | |
| $table_name = $wpdb->prefix . $table_name; | |
| } | |
| if ( strpos( $table_name, $wpdb->prefix ) !== 0 ) { | |
| $table_name = $wpdb->prefix . $table_name; | |
| } |
| "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format", | ||
| $model->get_id() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Avoid Direct Variable Interpolation in SQL Queries; Use Safe Practices to Prevent SQL Injection
Interpolating table and column names directly into SQL queries can lead to SQL injection vulnerabilities. Even when using $wpdb->prepare(), variables interpolated into the query are not sanitized. Table names and column names should be hard-coded or properly sanitized before inclusion in the query.
Consider refactoring the query to avoid variable interpolation:
- "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format",
+ "SELECT * FROM " . esc_sql( $table_name ) . " WHERE " . esc_sql( $id_field_name ) . " = $format",Ensure that $table_name and $id_field_name are sanitized using esc_sql() to prevent SQL injection vulnerabilities.
📝 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.
| "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format", | |
| $model->get_id() | |
| ) | |
| ); | |
| "SELECT * FROM " . esc_sql( $table_name ) . " WHERE " . esc_sql( $id_field_name ) . " = %s", | |
| $model->get_id() | |
| ) | |
| ); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 85-85:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format"
[failure] 85-85:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format"
[failure] 85-85:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at "SELECT * FROM {$table_name} WHERE {$id_field_name} = $format"
[warning] 85-85:
Replacement variables found, but no valid placeholders found in the query.
| "DELETE FROM {$table_name} | ||
| WHERE $id_field_name = $format", | ||
| $id | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Avoid Direct Variable Interpolation in SQL Queries; Use Safe Practices to Prevent SQL Injection
Interpolating table and column names directly into SQL queries can lead to SQL injection vulnerabilities. Even when using $wpdb->prepare(), variables interpolated into the query are not sanitized. Table names and column names should be hard-coded or properly sanitized before inclusion in the query.
Consider refactoring the query to avoid variable interpolation:
- "DELETE FROM {$table_name}
- WHERE $id_field_name = $format",
+ "DELETE FROM " . esc_sql( $table_name ) . " WHERE " . esc_sql( $id_field_name ) . " = $format",Ensure that $table_name and $id_field_name are sanitized using esc_sql() to prevent SQL injection vulnerabilities.
📝 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.
| "DELETE FROM {$table_name} | |
| WHERE $id_field_name = $format", | |
| $id | |
| ) | |
| ); | |
| "DELETE FROM " . esc_sql( $table_name ) . " WHERE " . esc_sql( $id_field_name ) . " = $format", | |
| $id | |
| ) | |
| ); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 130-130:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 131-131:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 131-131:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 131-131:
Replacement variables found, but no valid placeholders found in the query.
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (6)
includes/Models/DataStore/VendorBalanceStore.php (1)
22-24: LGTM:get_table_name()method is well-defined.The method is correctly implemented with proper visibility and return type hint.
Consider using a class constant for the table name to make it easier to update in the future:
+ private const TABLE_NAME = 'dokan_vendor_balance'; public function get_table_name(): string { - return 'dokan_vendor_balance'; + return self::TABLE_NAME; }includes/Models/DataStore/DataStoreInterface.php (3)
5-10: Replace the version placeholder with the actual version number.The PHPDoc block contains a placeholder
DOKAN_SINCEfor the version number. Please replace this with the actual version number in which this interface is being introduced.
23-33: Specify the exception type in the PHPDoc.The PHPDoc mentions that this method may throw an exception, but it doesn't specify the type. Consider adding the specific exception type(s) that may be thrown, for example:
* @throws \RuntimeException If the update operation fails.This will provide more clarity for developers implementing this interface.
35-45: Specify the exception type in the PHPDoc.Similar to the
updatemethod, the PHPDoc for thedeletemethod mentions that it may throw an exception without specifying the type. Consider adding the specific exception type(s) that may be thrown, for example:* @throws \RuntimeException If the deletion operation fails.This will provide more clarity for developers implementing this interface.
includes/Models/VendorBalance.php (1)
7-15: Convert property comments to PHP DocBlock format.The commented-out property definitions provide useful information about the data structure. Consider converting these into a proper PHP DocBlock format for better code documentation and IDE support.
Here's an example of how you could format this:
/** * @property int $vendor_id * @property int $trn_id * @property string $trn_type * @property string $perticulars * @property float $debit * @property float $credit * @property string $status * @property string $trn_date * @property string $balance_date */ class VendorBalance extends BaseModel { // ... rest of the class }includes/Models/DataStore/BaseDataStore.php (1)
205-207: Verify the hook prefix generation for consistencyThe
get_hook_prefix()method constructs the hook prefix by stripping the$wpdb->prefixfrom the$table_name. If$table_namedoes not contain$wpdb->prefix, this might lead to an incorrect hook prefix.Consider adjusting the hook prefix generation to ensure it accurately represents the table name:
- $hook_prefix = str_replace( $wpdb->prefix, '', $table_name ); + $table_name_without_prefix = preg_replace( '/^' . preg_quote( $wpdb->prefix, '/' ) . '/', '', $table_name ); + $hook_prefix = $table_name_without_prefix;Ensure that
$table_nameactually starts with$wpdb->prefixbefore attempting to remove it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- includes/Models/BaseModel.php (1 hunks)
- includes/Models/DataStore/BaseDataStore.php (1 hunks)
- includes/Models/DataStore/DataStoreInterface.php (1 hunks)
- includes/Models/DataStore/VendorBalanceStore.php (1 hunks)
- includes/Models/VendorBalance.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/Models/BaseModel.php
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Models/DataStore/BaseDataStore.php
[failure] 63-63:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at "{$id_field_name} = $format"
[failure] 63-63:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at "{$id_field_name} = $format"
[warning] 63-63:
Replacement variables found, but no valid placeholders found in the query.
[warning] 69-69:
Use placeholders and $wpdb->prepare(); found $this
[warning] 69-69:
Use placeholders and $wpdb->prepare(); found get_query_statement
[failure] 140-140:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 141-141:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 141-141:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 141-141:
Replacement variables found, but no valid placeholders found in the query.
🔇 Additional comments (6)
includes/Models/DataStore/VendorBalanceStore.php (1)
1-5: LGTM: Proper namespace and class structure.The namespace and class declaration follow best practices:
- Namespace adheres to PSR-4 autoloading standard.
- Class name matches the filename.
- Proper use of inheritance by extending
BaseDataStore.includes/Models/VendorBalance.php (5)
1-5: LGTM: Class structure and namespace are well-defined.The
VendorBalanceclass is correctly placed in theWeDevs\Dokan\Modelsnamespace and extendsBaseModel, which is a good practice for maintaining consistency across model classes.
17-23: LGTM: Getter and setter for vendor_id are well-implemented.The
get_vendor_idandset_vendor_idmethods are correctly implemented, using theget_propandset_propmethods respectively. The type hinting for the setter ensures that only integers are accepted for the vendor ID.
25-31: LGTM: Getter and setter for trn_id are correctly implemented.The
get_trn_idandset_trn_idmethods follow the same pattern as the vendor_id methods, maintaining consistency in the codebase.
49-55: LGTM: Getter and setter for debit are well-implemented.The
get_debitandset_debitmethods are correctly implemented, following the established pattern and using appropriate type hinting for the float parameter.
57-63: LGTM: Getter and setter for credit are correctly implemented.The
get_creditandset_creditmethods follow the same pattern as the debit methods, maintaining consistency in the codebase and using appropriate type hinting.
| <?php | ||
|
|
||
| namespace WeDevs\Dokan\Models\DataStore; | ||
|
|
||
| class VendorBalanceStore extends BaseDataStore { | ||
|
|
||
| protected function get_fields_with_format(): array { | ||
| return [ | ||
| 'vendor_id' => '%d', | ||
| 'trn_id' => '%d', | ||
| 'trn_type' => '%d', | ||
| 'perticulars' => '%s', | ||
| 'debit' => '%f', | ||
| 'credit' => '%f', | ||
| 'status' => '%s', | ||
| 'trn_date' => '%s', | ||
| 'balance_date' => '%s', | ||
|
|
||
| ]; | ||
| } | ||
|
|
||
| public function get_table_name(): string { | ||
| return 'dokan_vendor_balance'; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
VendorBalanceStore Class Not Integrated Elsewhere
The VendorBalanceStore class is defined but not utilized in other parts of the codebase. Please ensure that existing references to dokan_vendor_balance are updated to use the new VendorBalanceStore class to maintain consistency and improve maintainability.
🔗 Analysis chain
Overall, the VendorBalanceStore class is well-structured and implements the necessary methods.
The class effectively defines the structure for vendor balance data storage. It properly extends the BaseDataStore class and implements the required methods. The suggestions provided earlier will further enhance its clarity, consistency, and maintainability.
To ensure the VendorBalanceStore class is properly integrated with the rest of the codebase, please run the following verification script:
This script will help identify any potential integration issues or conflicts with existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage and integration of VendorBalanceStore class
# Test 1: Check if VendorBalanceStore is used in other files
echo "Checking usage of VendorBalanceStore:"
rg "VendorBalanceStore" --type php
# Test 2: Verify if BaseDataStore exists and is properly defined
echo "Verifying BaseDataStore:"
rg "abstract class BaseDataStore" --type php
# Test 3: Check for any existing vendor balance related functions or methods
echo "Checking for existing vendor balance related code:"
rg "vendor_balance|dokan_vendor_balance" --type php
Length of output: 4835
| protected function get_fields_with_format(): array { | ||
| return [ | ||
| 'vendor_id' => '%d', | ||
| 'trn_id' => '%d', | ||
| 'trn_type' => '%d', | ||
| 'perticulars' => '%s', | ||
| 'debit' => '%f', | ||
| 'credit' => '%f', | ||
| 'status' => '%s', | ||
| 'trn_date' => '%s', | ||
| 'balance_date' => '%s', | ||
|
|
||
| ]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance get_fields_with_format() method for improved clarity and consistency.
Consider the following suggestions:
- Add a return type hint for improved type safety.
- Use consistent indentation (preferably spaces as per PSR-2).
- Consider using more descriptive names for some fields (e.g., 'transaction_id' instead of 'trn_id').
Here's a suggested refactor:
- protected function get_fields_with_format(): array {
- return [
- 'vendor_id' => '%d',
- 'trn_id' => '%d',
- 'trn_type' => '%d',
- 'perticulars' => '%s',
- 'debit' => '%f',
- 'credit' => '%f',
- 'status' => '%s',
- 'trn_date' => '%s',
- 'balance_date' => '%s',
-
- ];
- }
+ protected function get_fields_with_format(): array {
+ return [
+ 'vendor_id' => '%d',
+ 'transaction_id' => '%d',
+ 'transaction_type' => '%d',
+ 'particulars' => '%s',
+ 'debit' => '%f',
+ 'credit' => '%f',
+ 'status' => '%s',
+ 'transaction_date' => '%s',
+ 'balance_date' => '%s',
+ ];
+ }Note: Ensure that these field names align with your database schema and other parts of the codebase.
📝 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.
| protected function get_fields_with_format(): array { | |
| return [ | |
| 'vendor_id' => '%d', | |
| 'trn_id' => '%d', | |
| 'trn_type' => '%d', | |
| 'perticulars' => '%s', | |
| 'debit' => '%f', | |
| 'credit' => '%f', | |
| 'status' => '%s', | |
| 'trn_date' => '%s', | |
| 'balance_date' => '%s', | |
| ]; | |
| } | |
| protected function get_fields_with_format(): array { | |
| return [ | |
| 'vendor_id' => '%d', | |
| 'transaction_id' => '%d', | |
| 'transaction_type' => '%d', | |
| 'particulars' => '%s', | |
| 'debit' => '%f', | |
| 'credit' => '%f', | |
| 'status' => '%s', | |
| 'transaction_date' => '%s', | |
| 'balance_date' => '%s', | |
| ]; | |
| } |
| /** | ||
| * Get a data. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @param Model $model The model to get. | ||
| * | ||
| * @return void | ||
| * @throw \Exception | ||
| */ | ||
| // public function get( Model &$model ); | ||
|
|
||
| /** | ||
| * Query data. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @param array $args | ||
| * | ||
| * @return array | ||
| */ | ||
| // public function query( array $args = [] ): array; | ||
|
|
||
| /** | ||
| * Count data. | ||
| * | ||
| * @since DOKAN_SINCE | ||
| * | ||
| * @param array $args | ||
| * | ||
| * @return int | ||
| */ | ||
| // public function count( array $args = [] ): int; |
There was a problem hiding this comment.
Review commented-out methods and correct return type for get.
-
The
get,query, andcountmethods are currently commented out. If these methods are intended to be part of the interface, they should be uncommented. If they are placeholders for future implementation, consider adding a TODO comment explaining the plan for these methods. -
The
getmethod has a void return type, which is unusual for a retrieval method. Consider changing it to return theModelor another appropriate type:
* @return Model The retrieved model.- If you decide to keep these methods commented out, you might want to remove them entirely from the interface to avoid confusion, or add a comment explaining why they are commented out.
includes/Models/VendorBalance.php
Outdated
| <?php | ||
|
|
||
| namespace WeDevs\Dokan\Models; | ||
|
|
||
| class VendorBalance extends BaseModel { | ||
|
|
||
| // 'vendor_id' => '%d', | ||
| // 'trn_id' => '%d', | ||
| // 'trn_type' => '%d', | ||
| // 'perticulars' => '%s', | ||
| // 'debit' => '%f', | ||
| // 'credit' => '%f', | ||
| // 'status' => '%s', | ||
| // 'trn_date' => '%s', | ||
| // 'balance_date' => '%s' | ||
|
|
||
| public function get_vendor_id( string $context = 'view' ) { | ||
| return $this->get_prop( 'vendor_id', $context ); | ||
| } | ||
|
|
||
| public function set_vendor_id( int $id ) { | ||
| return $this->set_prop( 'vendor_id', $id ); | ||
| } | ||
|
|
||
| public function get_trn_id( string $context = 'view' ) { | ||
| return $this->get_prop( 'trn_id', $context ); | ||
| } | ||
|
|
||
| public function set_trn_id( int $id ) { | ||
| return $this->set_prop( 'trn_id', $id ); | ||
| } | ||
|
|
||
| public function get_trn_type( string $context = 'view' ) { | ||
| return $this->get_prop( 'trn_type', $context ); | ||
| } | ||
|
|
||
| public function set_trn_type( string $type ) { | ||
| return $this->set_prop( 'trn_type', $type ); | ||
| } | ||
|
|
||
| public function get_particulars( string $context = 'view' ) { | ||
| return $this->get_prop( 'perticulars', $context ); | ||
| } | ||
|
|
||
| public function set_particulars( string $note ) { | ||
| return $this->set_prop( 'perticulars', $note ); | ||
| } | ||
|
|
||
| public function get_debit( string $context = 'view' ) { | ||
| return $this->get_prop( 'debit', $context ); | ||
| } | ||
|
|
||
| public function set_debit( float $amount ) { | ||
| return $this->set_prop( 'debit', $amount ); | ||
| } | ||
|
|
||
| public function get_credit( string $context = 'view' ) { | ||
| return $this->get_prop( 'credit', $context ); | ||
| } | ||
|
|
||
| public function set_credit( float $amount ) { | ||
| return $this->set_prop( 'credit', $amount ); | ||
| } | ||
|
|
||
| public function get_trn_date( string $context = 'view' ) { | ||
| return $this->get_prop( 'trn_date', $context ); | ||
| } | ||
|
|
||
| public function set_trn_date( float $amount ) { | ||
| return $this->set_date_prop( 'trn_date', $amount ); | ||
| } | ||
|
|
||
| public function get_balance_date( string $context = 'view' ) { | ||
| return $this->get_prop( 'balance_date', $context ); | ||
| } | ||
|
|
||
| public function set_balance_date( float $amount ) { | ||
| return $this->set_date_prop( 'balance_date', $amount ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Overall class structure review and suggestions for improvement.
The VendorBalance class is well-structured and implements getters and setters for most properties consistently. However, there are a few areas for improvement:
- The 'status' property mentioned in the comments doesn't have corresponding getter and setter methods. Consider adding these for completeness.
- There's no constructor or any custom logic beyond getters and setters. Consider if any initialization or validation logic is needed.
- PHPDoc is missing for the class and its methods. Adding comprehensive documentation would improve code readability and maintainability.
Consider the following improvements:
- Add getter and setter methods for the 'status' property.
- Implement a constructor if any initialization is required.
- Add PHPDoc comments for the class and all methods.
Example of adding PHPDoc:
/**
* Class VendorBalance
*
* Represents a vendor's balance record in the Dokan plugin.
*/
class VendorBalance extends BaseModel {
/**
* Get the vendor ID.
*
* @param string $context Context of the request.
* @return int The vendor ID.
*/
public function get_vendor_id( string $context = 'view' ) {
return $this->get_prop( 'vendor_id', $context );
}
// Add similar PHPDoc for other methods...
}
includes/Models/VendorBalance.php
Outdated
| public function get_trn_date( string $context = 'view' ) { | ||
| return $this->get_prop( 'trn_date', $context ); | ||
| } | ||
|
|
||
| public function set_trn_date( float $amount ) { | ||
| return $this->set_date_prop( 'trn_date', $amount ); | ||
| } |
There was a problem hiding this comment.
Correct parameter type for set_trn_date method.
The get_trn_date method is implemented correctly. However, there's an issue with the set_trn_date method:
- It correctly uses
set_date_propfor date handling. - But it expects a
floatparameter, which is inconsistent with typical date representations.
Please update the set_trn_date method to accept a more appropriate date format:
- public function set_trn_date( float $amount ) {
- return $this->set_date_prop( 'trn_date', $amount );
+ public function set_trn_date( string $date ) {
+ return $this->set_date_prop( 'trn_date', $date );Consider using a string parameter that can be parsed into a date, or a DateTime object if your system supports it.
📝 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 get_trn_date( string $context = 'view' ) { | |
| return $this->get_prop( 'trn_date', $context ); | |
| } | |
| public function set_trn_date( float $amount ) { | |
| return $this->set_date_prop( 'trn_date', $amount ); | |
| } | |
| public function get_trn_date( string $context = 'view' ) { | |
| return $this->get_prop( 'trn_date', $context ); | |
| } | |
| public function set_trn_date( string $date ) { | |
| return $this->set_date_prop( 'trn_date', $date ); | |
| } |
includes/Models/VendorBalance.php
Outdated
| public function get_particulars( string $context = 'view' ) { | ||
| return $this->get_prop( 'perticulars', $context ); | ||
| } | ||
|
|
||
| public function set_particulars( string $note ) { | ||
| return $this->set_prop( 'perticulars', $note ); | ||
| } |
There was a problem hiding this comment.
Correct spelling inconsistency for 'particulars'.
The methods get_particulars and set_particulars are implemented correctly. However, there's a spelling inconsistency between the property name in the comments ('perticulars') and the method names ('particulars').
Please update the property comment to use the correct spelling:
- // 'perticulars' => '%s',
+ // 'particulars' => '%s',Also, ensure that this spelling is consistent throughout the codebase and database schema.
Committable suggestion was skipped due to low confidence.
| abstract protected function get_fields_with_format(): array; | ||
| abstract public function get_table_name(): string; | ||
|
|
||
| public function create( BaseModel &$model ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary passing of objects by reference
In the methods create(), read(), update(), and delete(), the BaseModel object is passed by reference using &$model. In PHP, objects are already passed by reference by default, so the ampersand & is unnecessary and may cause confusion.
Apply this diff to remove the unnecessary reference operator:
- public function create( BaseModel &$model ) {
+ public function create( BaseModel $model ) {
- public function read( BaseModel &$model ) {
+ public function read( BaseModel $model ) {
- public function update( BaseModel &$model ) {
+ public function update( BaseModel $model ) {
- public function delete( BaseModel &$model, $args = array() ) {
+ public function delete( BaseModel $model, $args = array() ) {Also applies to: 42-42, 87-87, 117-117
| if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) { | ||
| $table_name = $wpdb->prefix . $table_name; | ||
| } |
There was a problem hiding this comment.
Ensure compatibility with PHP versions when using str_starts_with()
The function str_starts_with() is available from PHP 8.0 onwards. If the project needs to support earlier PHP versions, using this function will cause compatibility issues.
Replace str_starts_with() with an alternative that works with older PHP versions:
- if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) {
+ if ( strpos( $table_name, $wpdb->prefix ) !== 0 ) {This change maintains the same functionality while ensuring compatibility with PHP versions prior to 8.0.
📝 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.
| if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) { | |
| $table_name = $wpdb->prefix . $table_name; | |
| } | |
| if ( strpos( $table_name, $wpdb->prefix ) !== 0 ) { | |
| $table_name = $wpdb->prefix . $table_name; | |
| } |
| "DELETE FROM {$table_name} | ||
| WHERE $id_field_name = $format", | ||
| $id | ||
| ) |
There was a problem hiding this comment.
Fix potential SQL injection vulnerability in deletion query
In the delete_by_id() method, variables are interpolated directly into the SQL string, which poses a security risk. The variables {$table_name}, $id_field_name, and $format should not be directly interpolated into the query.
Refactor the code to use $wpdb->delete() for safer query execution:
- $result = $wpdb->query(
- $wpdb->prepare(
- "DELETE FROM {$table_name}
- WHERE $id_field_name = $format",
- $id
- )
- );
+ $result = $wpdb->delete(
+ $table_name,
+ array( $id_field_name => $id ),
+ array( $format )
+ );This approach ensures that the table name and where clause are handled safely, preventing SQL injection.
📝 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.
| "DELETE FROM {$table_name} | |
| WHERE $id_field_name = $format", | |
| $id | |
| ) | |
| $result = $wpdb->delete( | |
| $table_name, | |
| array( $id_field_name => $id ), | |
| array( $format ) | |
| ); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 140-140:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 141-141:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 141-141:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 141-141:
Replacement variables found, but no valid placeholders found in the query.
| "{$id_field_name} = $format", | ||
| $model->get_id() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Fix potential SQL injection vulnerability in query preparation
The SQL query in the read() method uses interpolated variables within the query string, which can lead to SQL injection vulnerabilities. Specifically, the variables {$id_field_name} and $format are directly interpolated.
Apply this diff to properly prepare the SQL statement using placeholders:
$this->add_sql_clause(
'where',
- $wpdb->prepare(
- "{$id_field_name} = $format",
+ $wpdb->prepare(
+ "{$id_field_name} = %s",
$model->get_id()
)
);Note: Since $id_field_name is a column name, ensure it is sanitized and originates from a trusted source. Consider using a whitelist of allowed field names.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 63-63:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at "{$id_field_name} = $format"
[failure] 63-63:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at "{$id_field_name} = $format"
[warning] 63-63:
Replacement variables found, but no valid placeholders found in the query.
mrabbani
added
In Progress
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (7)
includes/Models/DataStore/VendorBalanceStore.php (3)
5-6: Remove unused import.The
BaseModelclass is imported but never used directly in this file.-use WeDevs\Dokan\Models\BaseModel; use WeDevs\Dokan\Models\VendorBalance;
8-8: Add class-level documentation.Please add a PHPDoc block describing the purpose and responsibility of this class.
+/** + * Class VendorBalanceStore + * + * Handles the storage and retrieval of vendor balance data in the database. + * Implements data persistence operations for the VendorBalance model. + */ class VendorBalanceStore extends BaseDataStore {
24-26: Add documentation and consider using table prefix.
- Add PHPDoc for the method
- Consider using WordPress table prefix for better compatibility with different installations
+ /** + * Get the database table name for vendor balance records. + * + * @return string The prefixed table name + */ public function get_table_name(): string { - return 'dokan_vendor_balance'; + global $wpdb; + return $wpdb->prefix . 'dokan_vendor_balance'; }tests/php/src/Models/VendorBalanceModelTest.php (1)
8-15: Consider enhancing PHPDoc with @SInCE tag.Adding version information helps track when this test was introduced.
/** * Indicates if the test is a unit test. * * @var bool + * @since 3.x.x */includes/Models/DataStore/DataStoreInterface.php (1)
7-12: Enhance interface documentation.
- Replace the
DOKAN_SINCEplaceholder with the actual version number.- Add more detailed description about:
- The purpose of this interface
- Its role in the data persistence layer
- Expected implementation contexts
/** - * Data Store interface. + * Defines the contract for Dokan data persistence operations. + * + * This interface standardizes how Dokan models interact with the underlying + * storage layer, providing a consistent API for CRUD operations. * - * @since DOKAN_SINCE + * @since 3.7.0 */includes/Models/VendorBalance.php (1)
31-41: Add property type definitions and validation rules in comments.The data array properties should include type definitions and any validation rules that apply. This helps with code maintenance and prevents invalid data.
Example addition:
/** * The default data of the object. * * @var array{ * vendor_id: int, // Positive integer * trn_id: int, // Positive integer * trn_type: string, // One of TRN_TYPE_* constants * perticulars: string, // Non-empty string * debit: float, // Non-negative float * credit: float, // Non-negative float * status: string, // Valid status values * trn_date: string, // Format: Y-m-d H:i:s * balance_date: string // Format: Y-m-d H:i:s * } */includes/Models/DataStore/BaseDataStore.php (1)
57-97: Consider implementing caching for read operationsThe read method could benefit from caching to improve performance, especially for frequently accessed records.
Consider implementing WordPress object caching:
protected function get_cache_key( $id ): string { return "dokan_store_{$this->get_table_name()}_{$id}"; } public function read( BaseModel &$model ) { $id = $model->get_id(); $cache_key = $this->get_cache_key( $id ); // Try to get from cache first $data = wp_cache_get( $cache_key ); if ( false !== $data ) { $model->set_props( $data ); $model->set_object_read( true ); return $data; } // Existing read logic here... // Cache the result wp_cache_set( $cache_key, $this->format_raw_data( $raw_item ) ); }Remember to invalidate the cache in create/update/delete operations.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at "{$id_field_name} = $format"
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at "{$id_field_name} = $format"
[warning] 78-78:
Replacement variables found, but no valid placeholders found in the query.
[warning] 84-84:
Use placeholders and $wpdb->prepare(); found $this
[warning] 84-84:
Use placeholders and $wpdb->prepare(); found get_query_statement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- includes/Models/DataStore/BaseDataStore.php (1 hunks)
- includes/Models/DataStore/DataStoreInterface.php (1 hunks)
- includes/Models/DataStore/VendorBalanceStore.php (1 hunks)
- includes/Models/VendorBalance.php (1 hunks)
- tests/php/src/Models/VendorBalanceModelTest.php (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Models/DataStore/BaseDataStore.php
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at "{$id_field_name} = $format"
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at "{$id_field_name} = $format"
[warning] 78-78:
Replacement variables found, but no valid placeholders found in the query.
[warning] 84-84:
Use placeholders and $wpdb->prepare(); found $this
[warning] 84-84:
Use placeholders and $wpdb->prepare(); found get_query_statement
[failure] 154-154:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 155-155:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 155-155:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 155-155:
Replacement variables found, but no valid placeholders found in the query.
🔇 Additional comments (4)
includes/Models/DataStore/DataStoreInterface.php (2)
1-6: LGTM! File structure follows PSR-4 standards.
60-93:⚠️ Potential issueImplement or remove commented methods.
The commented-out methods appear to be essential for a complete data store interface. Either:
- Implement them as part of the interface, or
- Remove them if they're not needed in the current version
If these methods are planned for future implementation, track them in the project's issue tracker instead of keeping them as commented code.
includes/Models/VendorBalance.php (2)
1-10: LGTM! Well-structured class definition with clear transaction type constants.The class structure follows proper PHP conventions with appropriate namespace, imports, and constant definitions.
1-230: Verify adherence to WordPress coding standards.Let's verify if the implementation follows WordPress coding standards and common patterns.
✅ Verification successful
Based on the shell script results, I can now provide a comprehensive analysis of the implementation against WordPress coding standards.
Implementation follows WordPress coding standards and best practices
The VendorBalance model implementation aligns well with WordPress and WooCommerce coding standards:
- Proper use of namespacing with
WeDevs\Dokan\Models- Clear documentation with PHPDoc blocks for all methods
- Consistent method naming following WordPress conventions (e.g. get_, set_)
- Type declarations and return type hints used appropriately
- Proper data sanitization through parent BaseModel class
- Integration with WordPress hooks system as evidenced by the codebase
- Follows similar patterns seen in WooCommerce core for model implementations
The implementation also demonstrates good practices:
- Clear separation of concerns with dedicated data store class
- Immutable constants for transaction types
- Protected properties for internal data
- Consistent return type handling
- Proper validation in setters
The code integrates well with the broader Dokan plugin architecture and follows established WordPress development patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for WordPress coding standards patterns echo "Checking for WordPress coding standards..." # Check if similar models exist in WordPress core echo "Checking for similar model implementations in WordPress core..." rg -l "extends \w+Model" --type php # Check for common WordPress filter/action hooks pattern echo "Checking for filter/action hooks..." rg "do_action|apply_filters" --type php # Look for similar balance/transaction handling in WordPress echo "Checking for similar balance handling implementations..." rg -l "balance|transaction" --type phpLength of output: 105975
| protected function get_perticulars( VendorBalance $model, string $context = 'edit' ): string { | ||
| return $model->get_particulars( $context ); | ||
| } |
There was a problem hiding this comment.
Fix spelling inconsistency: "perticulars" vs "particulars".
The method name uses "perticulars" while the documentation and likely the model method uses "particulars". Maintain consistent spelling across the codebase.
- protected function get_perticulars( VendorBalance $model, string $context = 'edit' ): string {
- return $model->get_particulars( $context );
+ protected function get_particulars( VendorBalance $model, string $context = 'edit' ): string {
+ return $model->get_particulars( $context );Also update the field name in get_fields_with_format():
- 'perticulars' => '%s',
+ 'particulars' => '%s',Committable suggestion was skipped due to low confidence.
| return [ | ||
| 'vendor_id' => '%d', | ||
| 'trn_id' => '%d', | ||
| 'trn_type' => '%s', |
There was a problem hiding this comment.
Fix format specifier for transaction type field.
The field name trn_type suggests a numeric type, but the format specifier is %s (string). If this is meant to be numeric, use %d instead.
- 'trn_type' => '%s',
+ 'trn_type' => '%d',Committable suggestion was skipped due to low confidence.
| class VendorBalanceModelTest extends DokanTestCase { | ||
| /** | ||
| * Indicates if the test is a unit test. | ||
| * | ||
| * @var bool | ||
| */ | ||
| protected $is_unit_test = true; | ||
|
|
||
| public function test_save_method() { | ||
| $vendor_balance = new VendorBalance(); | ||
| $vendor_balance->set_debit( 100 ); | ||
| $vendor_balance->set_trn_id( 1 ); | ||
| $vendor_balance->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_ORDERS ); | ||
| $vendor_balance->set_particulars( 'test' ); | ||
| $vendor_balance->save(); | ||
|
|
||
| $this->assertNotEmpty( $vendor_balance->get_id() ); | ||
| $this->assertDatabaseCount( | ||
| 'dokan_vendor_balance', 1, [ | ||
| 'id' => $vendor_balance->get_id(), | ||
| // 'debit' => 100, | ||
| 'trn_id' => 1, | ||
| 'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, | ||
| ] | ||
| ); | ||
| } | ||
|
|
||
| public function test_update_method() { | ||
| $vendor_balance = new VendorBalance(); | ||
| $vendor_balance->save(); | ||
|
|
||
| $this->assertEmpty( $vendor_balance->get_particulars() ); | ||
|
|
||
| // Change the particulars | ||
| $vendor_balance->set_particulars( 'test123' ); | ||
| $vendor_balance->save(); | ||
|
|
||
| // Assert changes are applied to the database. | ||
| $this->assertDatabaseHas( | ||
| 'dokan_vendor_balance', [ | ||
| 'perticulars' => 'test123', | ||
| ] | ||
| ); | ||
| } | ||
|
|
||
| public function test_delete_method() { | ||
| $vendor_balance = new VendorBalance(); | ||
| $vendor_balance->save(); | ||
| $id = $vendor_balance->get_id(); | ||
| $this->assertNotEmpty( $vendor_balance->get_id() ); | ||
|
|
||
| $vendor_balance->delete(); | ||
|
|
||
| $this->assertDatabaseCount( 'dokan_vendor_balance', 0, [ 'id' => $id ] ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more test cases for better coverage.
The current test suite covers basic CRUD operations but lacks tests for:
- Invalid data handling (e.g., negative amounts)
- Edge cases (e.g., maximum values)
- Error conditions (e.g., duplicate transactions)
Would you like me to provide examples of these additional test cases?
| public function test_save_method() { | ||
| $vendor_balance = new VendorBalance(); | ||
| $vendor_balance->set_debit( 100 ); | ||
| $vendor_balance->set_trn_id( 1 ); | ||
| $vendor_balance->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_ORDERS ); | ||
| $vendor_balance->set_particulars( 'test' ); | ||
| $vendor_balance->save(); | ||
|
|
||
| $this->assertNotEmpty( $vendor_balance->get_id() ); | ||
| $this->assertDatabaseCount( | ||
| 'dokan_vendor_balance', 1, [ | ||
| 'id' => $vendor_balance->get_id(), | ||
| // 'debit' => 100, | ||
| 'trn_id' => 1, | ||
| 'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Several improvements needed in save method test.
- The debit amount assertion is commented out
- Missing vendor_id test case
- Database assertions could be more comprehensive
- Missing PHPDoc for the test method
Apply these improvements:
+ /**
+ * Test the save operation of VendorBalance model
+ *
+ * @since 3.x.x
+ * @return void
+ */
public function test_save_method() {
$vendor_balance = new VendorBalance();
+ $vendor_balance->set_vendor_id( 1 );
$vendor_balance->set_debit( 100 );
$vendor_balance->set_trn_id( 1 );
$vendor_balance->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_ORDERS );
$vendor_balance->set_particulars( 'test' );
$vendor_balance->save();
$this->assertNotEmpty( $vendor_balance->get_id() );
$this->assertDatabaseCount(
'dokan_vendor_balance', 1, [
'id' => $vendor_balance->get_id(),
- // 'debit' => 100,
+ 'vendor_id' => 1,
+ 'debit' => 100.00,
'trn_id' => 1,
'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS,
+ 'particulars' => 'test',
]
);
}📝 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 test_save_method() { | |
| $vendor_balance = new VendorBalance(); | |
| $vendor_balance->set_debit( 100 ); | |
| $vendor_balance->set_trn_id( 1 ); | |
| $vendor_balance->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_ORDERS ); | |
| $vendor_balance->set_particulars( 'test' ); | |
| $vendor_balance->save(); | |
| $this->assertNotEmpty( $vendor_balance->get_id() ); | |
| $this->assertDatabaseCount( | |
| 'dokan_vendor_balance', 1, [ | |
| 'id' => $vendor_balance->get_id(), | |
| // 'debit' => 100, | |
| 'trn_id' => 1, | |
| 'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, | |
| ] | |
| ); | |
| } | |
| /** | |
| * Test the save operation of VendorBalance model | |
| * | |
| * @since 3.x.x | |
| * @return void | |
| */ | |
| public function test_save_method() { | |
| $vendor_balance = new VendorBalance(); | |
| $vendor_balance->set_vendor_id( 1 ); | |
| $vendor_balance->set_debit( 100 ); | |
| $vendor_balance->set_trn_id( 1 ); | |
| $vendor_balance->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_ORDERS ); | |
| $vendor_balance->set_particulars( 'test' ); | |
| $vendor_balance->save(); | |
| $this->assertNotEmpty( $vendor_balance->get_id() ); | |
| $this->assertDatabaseCount( | |
| 'dokan_vendor_balance', 1, [ | |
| 'id' => $vendor_balance->get_id(), | |
| 'vendor_id' => 1, | |
| 'debit' => 100.00, | |
| 'trn_id' => 1, | |
| 'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, | |
| 'particulars' => 'test', | |
| ] | |
| ); | |
| } |
| /** | ||
| * Method to read a download permission from the database. | ||
| * | ||
| * @param BaseModel $model BaseModel object. | ||
| * | ||
| * @phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared | ||
| * | ||
| * @throws Exception Throw exception if invalid entity is passed. | ||
| */ | ||
| public function read( BaseModel &$model ); |
There was a problem hiding this comment.
Fix incorrect documentation and PHPCS ignore comment.
- The PHPDoc mentions "download permission" which seems copied from another context.
- The PHPCS ignore comment needs justification or should be removed.
/**
- * Method to read a download permission from the database.
+ * Reads model data from the database.
*
* @param BaseModel $model BaseModel object.
*
- * @phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared
+ * @throws \Exception If invalid entity is passed.
*/📝 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.
| /** | |
| * Method to read a download permission from the database. | |
| * | |
| * @param BaseModel $model BaseModel object. | |
| * | |
| * @phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared | |
| * | |
| * @throws Exception Throw exception if invalid entity is passed. | |
| */ | |
| public function read( BaseModel &$model ); | |
| /** | |
| * Reads model data from the database. | |
| * | |
| * @param BaseModel $model BaseModel object. | |
| * | |
| * @throws \Exception If invalid entity is passed. | |
| */ | |
| public function read( BaseModel &$model ); |
includes/Models/VendorBalance.php
Outdated
| public function set_trn_type( string $type ) { | ||
| return $this->set_prop( 'trn_type', $type ); | ||
| } |
There was a problem hiding this comment.
Add transaction type validation.
The set_trn_type method should validate that the input matches one of the defined transaction type constants.
public function set_trn_type( string $type ) {
+ $valid_types = [
+ self::TRN_TYPE_DOKAN_ORDERS,
+ self::TRN_TYPE_DOKAN_WITHDRAW,
+ self::TRN_TYPE_DOKAN_REFUND,
+ ];
+ if ( ! in_array( $type, $valid_types, true ) ) {
+ throw new \InvalidArgumentException( 'Invalid transaction type.' );
+ }
return $this->set_prop( 'trn_type', $type );
}Committable suggestion was skipped due to low confidence.
| namespace WeDevs\Dokan\Models\DataStore; | ||
|
|
||
| use Automattic\WooCommerce\Admin\API\Reports\SqlQuery; | ||
| use Automattic\WooCommerce\Pinterest\API\Base; |
There was a problem hiding this comment.
Remove unused import
The Automattic\WooCommerce\Pinterest\API\Base import appears to be unused in this class and should be removed.
-use Automattic\WooCommerce\Pinterest\API\Base;📝 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.
| use Automattic\WooCommerce\Pinterest\API\Base; |
| protected function format_raw_data( $raw_data ): array { | ||
| $data = array(); | ||
|
|
||
| foreach ( $this->get_fields() as $db_field_name ) { | ||
| $data[ $db_field_name ] = $raw_data->{$db_field_name}; | ||
| } | ||
|
|
||
| return apply_filters( $this->get_hook_prefix() . 'format_raw_data', $data, $raw_data ); | ||
| } |
There was a problem hiding this comment.
Add property existence check in format_raw_data()
The method directly accesses object properties without checking their existence, which could lead to PHP notices or errors.
protected function format_raw_data( $raw_data ): array {
$data = array();
foreach ( $this->get_fields() as $db_field_name ) {
- $data[ $db_field_name ] = $raw_data->{$db_field_name};
+ $data[ $db_field_name ] = property_exists( $raw_data, $db_field_name )
+ ? $raw_data->{$db_field_name}
+ : null;
}
return apply_filters( $this->get_hook_prefix() . 'format_raw_data', $data, $raw_data );
}📝 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.
| protected function format_raw_data( $raw_data ): array { | |
| $data = array(); | |
| foreach ( $this->get_fields() as $db_field_name ) { | |
| $data[ $db_field_name ] = $raw_data->{$db_field_name}; | |
| } | |
| return apply_filters( $this->get_hook_prefix() . 'format_raw_data', $data, $raw_data ); | |
| } | |
| protected function format_raw_data( $raw_data ): array { | |
| $data = array(); | |
| foreach ( $this->get_fields() as $db_field_name ) { | |
| $data[ $db_field_name ] = property_exists( $raw_data, $db_field_name ) | |
| ? $raw_data->{$db_field_name} | |
| : null; | |
| } | |
| return apply_filters( $this->get_hook_prefix() . 'format_raw_data', $data, $raw_data ); | |
| } |
| public function read( BaseModel &$model ) { | ||
| global $wpdb; | ||
|
|
||
| if ( ! $model->get_id() ) { | ||
| $message = $this->get_hook_prefix() . ' : ' . __( 'Invalid entity item.', 'dokan-lite' ); | ||
|
|
||
| throw new Exception( esc_html( $message ) ); | ||
| } | ||
|
|
||
| $model->set_defaults(); | ||
|
|
||
| $id_field_name = $this->get_id_field_name(); | ||
| $format = $this->get_id_field_format(); | ||
|
|
||
| $this->clear_all_clauses(); | ||
| $this->add_sql_clause( 'select', $this->get_selected_columns() ); | ||
| $this->add_sql_clause( 'from', $this->get_table_name_with_prefix() ); | ||
|
|
||
| $this->add_sql_clause( | ||
| 'where', | ||
| $wpdb->prepare( | ||
| "{$id_field_name} = $format", | ||
| $model->get_id() | ||
| ) | ||
| ); | ||
|
|
||
| $raw_item = $wpdb->get_row( | ||
| $this->get_query_statement() | ||
| ); | ||
|
|
||
| if ( ! $raw_item ) { | ||
| $message = $this->get_hook_prefix() . ' : ' . __( 'Invalid entity item.', 'dokan-lite' ); | ||
|
|
||
| throw new Exception( esc_html( $message ) ); | ||
| } | ||
|
|
||
| $model->set_props( $this->format_raw_data( $raw_item ) ); | ||
| $model->set_object_read( true ); | ||
|
|
||
| return $raw_item; | ||
| } |
There was a problem hiding this comment.
Fix SQL injection vulnerability in read() method
The SQL query construction in the read method is vulnerable to SQL injection. While the ID value is properly prepared, the field name and format are directly interpolated into the query string.
- $wpdb->prepare(
- "{$id_field_name} = $format",
- $model->get_id()
- )
+ $wpdb->prepare(
+ $wpdb->prepare( '%s = %d', $id_field_name, $model->get_id() )
+ )Additionally, consider validating $id_field_name against a whitelist of allowed column names.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at "{$id_field_name} = $format"
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at "{$id_field_name} = $format"
[warning] 78-78:
Replacement variables found, but no valid placeholders found in the query.
[warning] 84-84:
Use placeholders and $wpdb->prepare(); found $this
[warning] 84-84:
Use placeholders and $wpdb->prepare(); found get_query_statement
| protected function get_table_name_with_prefix(): string { | ||
| global $wpdb; | ||
|
|
||
| $table_name = $this->get_table_name(); | ||
|
|
||
| if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) { | ||
| $table_name = $wpdb->prefix . $table_name; | ||
| } | ||
|
|
||
| return $table_name; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add table name validation
The table name should be validated to prevent potential security issues and ensure it follows WordPress naming conventions.
protected function get_table_name_with_prefix(): string {
global $wpdb;
$table_name = $this->get_table_name();
+ // Validate table name format
+ if ( ! preg_match('/^[a-zA-Z0-9_]+$/', $table_name) ) {
+ throw new Exception( __( 'Invalid table name format', 'dokan-lite' ) );
+ }
+
if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) {
$table_name = $wpdb->prefix . $table_name;
}
return $table_name;
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (1)
includes/Models/VendorBalance.php (1)
7-10: Add class-level PHPDoc documentation.The class is missing comprehensive PHPDoc documentation. Add a description of the class purpose, responsibilities, and usage.
+/** + * Class VendorBalance + * + * Represents a vendor's balance record in the Dokan plugin. + * This class manages financial transactions including orders, withdrawals, and refunds + * for vendors, tracking their credits, debits, and overall balance. + * + * @since 1.0.0 + * @package WeDevs\Dokan\Models + */ class VendorBalance extends BaseModel {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- includes/Models/BaseModel.php (1 hunks)
- includes/Models/DataStore/BaseDataStore.php (1 hunks)
- includes/Models/VendorBalance.php (1 hunks)
- tests/php/src/Models/VendorBalanceModelTest.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- includes/Models/BaseModel.php
- tests/php/src/Models/VendorBalanceModelTest.php
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Models/DataStore/BaseDataStore.php
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at " AND {$id_field_name} = $format"
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at " AND {$id_field_name} = $format"
[warning] 78-78:
Replacement variables found, but no valid placeholders found in the query.
[warning] 84-84:
Use placeholders and $wpdb->prepare(); found $this
[warning] 84-84:
Use placeholders and $wpdb->prepare(); found get_query_statement
[failure] 154-154:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 155-155:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 155-155:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 155-155:
Replacement variables found, but no valid placeholders found in the query.
includes/Models/VendorBalance.php
Outdated
| protected function set_perticulars( string $note ) { | ||
| return $this->set_particulars( $note ); | ||
| } |
There was a problem hiding this comment.
Remove redundant protected method set_perticulars.
The protected method set_perticulars is redundant as it:
- Simply calls the public
set_particularsmethod - Has the same misspelling issue
- Adds unnecessary complexity to the codebase
Remove the entire method as it provides no additional functionality:
- /**
- * Set the perticulars (note) for the transaction.
- *
- * @param string $note The note to be stored.
- * @return void
- */
- protected function set_perticulars( string $note ) {
- return $this->set_particulars( $note );
- }📝 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.
| protected function set_perticulars( string $note ) { | |
| return $this->set_particulars( $note ); | |
| } |
| public function get_particulars( string $context = 'view' ): string { | ||
| return (string) $this->get_prop( 'perticulars', $context ); | ||
| } |
There was a problem hiding this comment.
Fix property name in get_particulars method.
The method uses the misspelled property name 'perticulars' which should be 'particulars' to match the method name.
public function get_particulars( string $context = 'view' ): string {
- return (string) $this->get_prop( 'perticulars', $context );
+ return (string) $this->get_prop( 'particulars', $context );
}📝 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 get_particulars( string $context = 'view' ): string { | |
| return (string) $this->get_prop( 'perticulars', $context ); | |
| } | |
| public function get_particulars( string $context = 'view' ): string { | |
| return (string) $this->get_prop( 'particulars', $context ); | |
| } |
| protected $data = [ | ||
| 'vendor_id' => 0, | ||
| 'trn_id' => 0, | ||
| 'trn_type' => '', | ||
| 'perticulars' => '', | ||
| 'debit' => 0, | ||
| 'credit' => 0, | ||
| 'status' => '', | ||
| 'trn_date' => '', | ||
| 'balance_date' => '', | ||
| ]; |
There was a problem hiding this comment.
Fix spelling in data property name.
The property 'perticulars' is misspelled in the data array (should be 'particulars'). This inconsistency could cause issues as the getter method uses the correct spelling.
protected $data = [
'vendor_id' => 0,
'trn_id' => 0,
'trn_type' => '',
- 'perticulars' => '',
+ 'particulars' => '',
'debit' => 0,
'credit' => 0,
'status' => '',
'trn_date' => '',
'balance_date' => '',
];📝 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.
| protected $data = [ | |
| 'vendor_id' => 0, | |
| 'trn_id' => 0, | |
| 'trn_type' => '', | |
| 'perticulars' => '', | |
| 'debit' => 0, | |
| 'credit' => 0, | |
| 'status' => '', | |
| 'trn_date' => '', | |
| 'balance_date' => '', | |
| ]; | |
| protected $data = [ | |
| 'vendor_id' => 0, | |
| 'trn_id' => 0, | |
| 'trn_type' => '', | |
| 'particulars' => '', | |
| 'debit' => 0, | |
| 'credit' => 0, | |
| 'status' => '', | |
| 'trn_date' => '', | |
| 'balance_date' => '', | |
| ]; |
| if ( ! $model->get_id() ) { | ||
| $message = $this->get_hook_prefix() . ' : ' . __( 'Invalid entity item.', 'dokan-lite' ); | ||
|
|
||
| throw new Exception( esc_html( $message ) ); |
There was a problem hiding this comment.
Avoid using esc_html() in exception messages
The esc_html() function is intended for escaping output in HTML contexts, not for internal exception messages. Using it here is unnecessary and may lead to confusion.
Remove esc_html() from the exception messages:
if ( ! $model->get_id() ) {
$message = $this->get_hook_prefix() . ' : ' . __( 'Invalid entity item.', 'dokan-lite' );
- throw new Exception( esc_html( $message ) );
+ throw new Exception( $message );
}
---
if ( ! $raw_item ) {
$message = $this->get_hook_prefix() . ' : ' . __( 'Invalid entity item.', 'dokan-lite' );
- throw new Exception( esc_html( $message ) );
+ throw new Exception( $message );
}Exception messages should be plain strings since they are not rendered in a browser context.
Also applies to: 90-90
|
|
||
| $table_name = $this->get_table_name(); | ||
|
|
||
| if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) { |
There was a problem hiding this comment.
Ensure PHP version compatibility when using str_starts_with()
The function str_starts_with() is available in PHP 8.0 and later. If the project needs to support earlier PHP versions, using this function will cause compatibility issues.
Replace str_starts_with() with an alternative compatible with older PHP versions:
protected function get_table_name_with_prefix(): string {
global $wpdb;
$table_name = $this->get_table_name();
- if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) {
+ if ( strpos( $table_name, $wpdb->prefix ) !== 0 ) {
$table_name = $wpdb->prefix . $table_name;
}
return $table_name;
}This change maintains the same functionality while ensuring compatibility with PHP versions prior to 8.0.
📝 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.
| if ( ! str_starts_with( $table_name, $wpdb->prefix ) ) { | |
| if ( strpos( $table_name, $wpdb->prefix ) !== 0 ) { |
| $result = $wpdb->query( | ||
| $wpdb->prepare( | ||
| "DELETE FROM {$table_name} | ||
| WHERE $id_field_name = $format", | ||
| $id | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Fix potential SQL injection vulnerability in delete_by_id() method
In the delete_by_id() method, variables are interpolated directly into the SQL query. Injecting {$table_name}, $id_field_name, and $format into the query string can lead to SQL injection vulnerabilities.
Refactor the code to use $wpdb->delete() for safer query execution:
$table_name = $this->get_table_name_with_prefix();
$id_field_name = $this->get_id_field_name();
$format = $this->get_id_field_format();
- $result = $wpdb->query(
- $wpdb->prepare(
- "DELETE FROM {$table_name}
- WHERE $id_field_name = $format",
- $id
- )
- );
+ $result = $wpdb->delete(
+ $table_name,
+ [ $id_field_name => $id ],
+ [ $format ]
+ );Using $wpdb->delete() ensures that the table name and where clause are handled safely, preventing SQL injection vulnerabilities. Ensure that $table_name and $id_field_name are sanitized or come from trusted sources.
📝 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.
| $result = $wpdb->query( | |
| $wpdb->prepare( | |
| "DELETE FROM {$table_name} | |
| WHERE $id_field_name = $format", | |
| $id | |
| ) | |
| ); | |
| $result = $wpdb->delete( | |
| $table_name, | |
| [ $id_field_name => $id ], | |
| [ $format ] | |
| ); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 154-154:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 155-155:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 155-155:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 155-155:
Replacement variables found, but no valid placeholders found in the query.
| $wpdb->prepare( | ||
| " AND {$id_field_name} = $format", | ||
| $model->get_id() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Fix potential SQL injection vulnerability in read() method
In the read() method, variables are being interpolated directly into the SQL query. Specifically, {$id_field_name} and $format are injected into the query string, which can lead to SQL injection vulnerabilities.
Refactor the code to safely include the field name and use proper placeholders:
$this->clear_all_clauses();
$this->add_sql_clause( 'select', $this->get_selected_columns() );
$this->add_sql_clause( 'from', $this->get_table_name_with_prefix() );
- $this->add_sql_clause(
- 'where',
- $wpdb->prepare(
- " AND {$id_field_name} = $format",
- $model->get_id()
- )
- );
+ $id_field_name_safe = esc_sql( $id_field_name );
+ $this->add_sql_clause(
+ 'where',
+ $wpdb->prepare(
+ " AND {$id_field_name_safe} = %s",
+ $model->get_id()
+ )
+ );Ensure that $id_field_name is sanitized using esc_sql() to prevent SQL injection, and use %s as the placeholder for the value.
📝 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.
| $wpdb->prepare( | |
| " AND {$id_field_name} = $format", | |
| $model->get_id() | |
| ) | |
| ); | |
| $id_field_name_safe = esc_sql( $id_field_name ); | |
| $this->add_sql_clause( | |
| 'where', | |
| $wpdb->prepare( | |
| " AND {$id_field_name_safe} = %s", | |
| $model->get_id() | |
| ) | |
| ); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at " AND {$id_field_name} = $format"
[failure] 78-78:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at " AND {$id_field_name} = $format"
[warning] 78-78:
Replacement variables found, but no valid placeholders found in the query.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
docs/data-store.md (2)
185-192: Improve documentation clarity and completeness.The case study section needs improvement:
- The word "perticulars" is misspelled (should be "particulars")
- The explanation could be clearer about the purpose of overriding these methods
Additionally, the documentation would benefit from new sections covering:
- Error handling strategies
- Data validation approaches
- WordPress/WooCommerce hook integration
Would you like me to help draft these additional documentation sections?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~188-~188: You might be missing the article “the” here.
Context: ...ta Store.perticularsfield name in database is typo but I don't want to expose pu...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~190-~190: You might be missing the article “the” here.
Context: ... by overriding the following methods in Data Store. - `protected function map_model_...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~191-~191: Loose punctuation mark.
Context: ..._to_db_data( BaseModel &$model ): array`: Prepare data for saving a BaseModel to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...e database. -map_db_raw_to_model_data: Maps database raw data to model data.(UNLIKELY_OPENING_PUNCTUATION)
105-106: Fix documentation formatting and grammar issues.Several text improvements are needed:
- Remove double dots after "format" on line 106
- Add missing "the" articles:
- Line 153: "and the format is"
- Line 188: "field name in the database"
- Line 190: "methods in the Data Store"
- Ensure consistent formatting in bullet points
Also applies to: 153-153, 188-188, 190-190
🧰 Tools
🪛 LanguageTool
[uncategorized] ~105-~105: Loose punctuation mark.
Context: ...e following methods: -get_table_name: Defines the table name in the database....(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ...the database. -get_fields_with_format: Returns the fields with format as an ar...(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~106-~106: Two consecutive dots
Context: ...he db field name and value is the format.. Sample implementation: ```php class D...(DOUBLE_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- docs/data-store.md (1 hunks)
- includes/Models/DataStore/BaseDataStore.php (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/data-store.md
[uncategorized] ~20-~20: Loose punctuation mark.
Context: ..., such asproduct. -protected $data: Holds the default data for the object. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~105-~105: Loose punctuation mark.
Context: ...e following methods: -get_table_name: Defines the table name in the database....(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ...the database. -get_fields_with_format: Returns the fields with format as an ar...(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~106-~106: Two consecutive dots
Context: ...he db field name and value is the format.. Sample implementation: ```php class D...(DOUBLE_PUNCTUATION)
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...efault, the ID field is set toidand format is%d. ## Uses of Models ##...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~165-~165: ‘New Record’ might be wordy. Consider a shorter alternative.
Context: ... ## Uses of Models ### Create a New Record ```php $department = new Department(); ...(EN_WORDINESS_PREMIUM_NEW_RECORD)
[uncategorized] ~188-~188: You might be missing the article “the” here.
Context: ...ta Store.perticularsfield name in database is typo but I don't want to expose pu...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~190-~190: You might be missing the article “the” here.
Context: ... by overriding the following methods in Data Store. - `protected function map_model_...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~191-~191: Loose punctuation mark.
Context: ..._to_db_data( BaseModel &$model ): array`: Prepare data for saving a BaseModel to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...e database. -map_db_raw_to_model_data: Maps database raw data to model data.(UNLIKELY_OPENING_PUNCTUATION)
🪛 GitHub Check: Run PHPCS inspection
includes/Models/DataStore/BaseDataStore.php
[failure] 77-77:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at " AND {$id_field_name} = $format"
[failure] 77-77:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at " AND {$id_field_name} = $format"
[warning] 77-77:
Replacement variables found, but no valid placeholders found in the query.
[warning] 83-83:
Use placeholders and $wpdb->prepare(); found $this
[warning] 83-83:
Use placeholders and $wpdb->prepare(); found get_query_statement
[failure] 153-153:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 154-154:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 154-154:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 154-154:
Replacement variables found, but no valid placeholders found in the query.
🔇 Additional comments (7)
docs/data-store.md (2)
9-13: Well-designed architecture following industry best practices!The architecture demonstrates excellent separation of concerns by abstracting the data layer through Data Stores, following WooCommerce's proven patterns. This design:
- Reduces coupling between business logic and data persistence
- Makes the code more maintainable and testable
- Facilitates future modifications without affecting the entire codebase
1-192: Verify integration with WordPress/WooCommerce architecture.The documentation presents a solid foundation for the data store implementation. Let's verify the integration points with WordPress/WooCommerce architecture.
✅ Verification successful
Based on the shell script results, I can now provide a comprehensive verification of the architectural concerns mentioned in the review comment:
Documentation accurately reflects integration with WordPress/WooCommerce architecture
The codebase shows proper integration with WordPress and WooCommerce architectures through:
- WordPress Integration:
- Extensive use of WordPress action/filter hooks following naming conventions (
dokan_*)- Proper database table creation using
dbDelta()- Integration with WordPress admin, user roles and capabilities
- WooCommerce Integration:
- Proper extension of WooCommerce data stores and interfaces (
WC_Data,WC_Data_Store)- Following WooCommerce patterns for product, order and customer data management
- Integration with WooCommerce hooks and filters
- Database Design:
- Clean table creation with proper schema definitions
- Tables follow WordPress naming conventions with prefix
- Proper data types and constraints defined
The documentation accurately describes the data model and store implementation, matching the actual code implementation. The examples provided reflect real usage patterns found throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify integration points with WordPress/WooCommerce # Check for WordPress action/filter hooks in the implementation echo "Checking for WordPress hooks integration..." rg -A 2 "do_action|apply_filters" --type php # Check for WooCommerce data store integration echo "Checking for WooCommerce data store integration..." rg -A 2 "WC_Data|WC_Object_Data_Store_Interface" --type php # Check for proper database table creation echo "Checking for database table creation..." rg -A 5 "dbDelta|CREATE TABLE" --type phpLength of output: 213019
🧰 Tools
🪛 LanguageTool
[uncategorized] ~20-~20: Loose punctuation mark.
Context: ..., such asproduct. -protected $data: Holds the default data for the object. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~105-~105: Loose punctuation mark.
Context: ...e following methods: -get_table_name: Defines the table name in the database....(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ...the database. -get_fields_with_format: Returns the fields with format as an ar...(UNLIKELY_OPENING_PUNCTUATION)
[typographical] ~106-~106: Two consecutive dots
Context: ...he db field name and value is the format.. Sample implementation: ```php class D...(DOUBLE_PUNCTUATION)
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...efault, the ID field is set toidand format is%d. ## Uses of Models ##...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~165-~165: ‘New Record’ might be wordy. Consider a shorter alternative.
Context: ... ## Uses of Models ### Create a New Record ```php $department = new Department(); ...(EN_WORDINESS_PREMIUM_NEW_RECORD)
[uncategorized] ~188-~188: You might be missing the article “the” here.
Context: ...ta Store.perticularsfield name in database is typo but I don't want to expose pu...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~190-~190: You might be missing the article “the” here.
Context: ... by overriding the following methods in Data Store. - `protected function map_model_...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~191-~191: Loose punctuation mark.
Context: ..._to_db_data( BaseModel &$model ): array`: Prepare data for saving a BaseModel to ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~192-~192: Loose punctuation mark.
Context: ...e database. -map_db_raw_to_model_data: Maps database raw data to model data.(UNLIKELY_OPENING_PUNCTUATION)
includes/Models/DataStore/BaseDataStore.php (5)
6-6: Remove unused importThe
Automattic\WooCommerce\Pinterest\API\Baseimport appears to be unused in this class and should be removed.
32-32: Remove unnecessary passing of objects by referencePassing
BaseModelobjects by reference is unnecessary since objects are already passed by reference in PHP. The ampersand&can be removed to avoid confusion.Also applies to: 57-57, 103-103, 130-130
62-62: Avoid usingesc_html()in exception messagesThe
esc_html()function is intended for escaping output in HTML contexts, not for internal exception messages. Using it here is unnecessary.Also applies to: 89-89
227-227: Add property existence check inmap_db_raw_to_model_data()Accessing object properties without checking their existence could lead to PHP notices or errors. It's safer to check if the property exists before accessing it.
281-283: Ensure compatibility with PHP versions when usingstr_starts_with()The function
str_starts_with()is available from PHP 8.0 onwards. If the project needs to support earlier PHP versions, using this function may cause compatibility issues.
| class DepartmentStore extends \WeDevs\Dokan\Models\DataStore\BaseDataStore { | ||
| public function get_table_name(): string { | ||
| return 'dokan_department'; | ||
| } | ||
|
|
||
| public function get_fields_with_format(): array { | ||
| return [ | ||
| 'name' => '%s', | ||
| 'date_created' => '%s', | ||
| 'updated_at' => '%s', | ||
| ]; | ||
| } | ||
|
|
||
| public function get_date_updated( $context = 'view' ) { | ||
| return $this->get_prop('date_updated', $context); | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Fix implementation issues in DepartmentStore class.
The DepartmentStore implementation has the following issues:
- The
get_date_updatedmethod appears to be incorrectly placed in the store class instead of the model class - Missing return type hints for better type safety
Apply these changes:
class DepartmentStore extends \WeDevs\Dokan\Models\DataStore\BaseDataStore {
- public function get_table_name(): string {
+ public function get_table_name(): string {
return 'dokan_department';
}
- public function get_fields_with_format(): array {
+ public function get_fields_with_format(): array {
return [
'name' => '%s',
'date_created' => '%s',
'updated_at' => '%s',
];
}
-
- public function get_date_updated( $context = 'view' ) {
- return $this->get_prop('date_updated', $context);
- }
}Committable suggestion was skipped due to low confidence.
| "DELETE FROM {$table_name} | ||
| WHERE $id_field_name = $format", | ||
| $id | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Fix potential SQL injection vulnerability in delete_by_id() method
Interpolating variables directly into the SQL query can lead to SQL injection vulnerabilities. Specifically, using {$table_name}, $id_field_name, and $format in the query string is unsafe.
Refactor the code to use $wpdb->delete() for safer query execution:
- $result = $wpdb->query(
- $wpdb->prepare(
- "DELETE FROM {$table_name}
- WHERE $id_field_name = $format",
- $id
- )
- );
+ $result = $wpdb->delete(
+ $table_name,
+ array( $id_field_name => $id ),
+ array( $format )
+ );This approach ensures that the table name and where clause are handled safely, preventing SQL injection.
📝 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.
| "DELETE FROM {$table_name} | |
| WHERE $id_field_name = $format", | |
| $id | |
| ) | |
| ); | |
| $result = $wpdb->delete( | |
| $table_name, | |
| array( $id_field_name => $id ), | |
| array( $format ) | |
| ); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 153-153:
Use placeholders and $wpdb->prepare(); found interpolated variable {$table_name} at "DELETE FROM {$table_name}
[failure] 154-154:
Use placeholders and $wpdb->prepare(); found interpolated variable $id_field_name at WHERE $id_field_name = $format"
[failure] 154-154:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at WHERE $id_field_name = $format"
[warning] 154-154:
Replacement variables found, but no valid placeholders found in the query.
| " AND {$id_field_name} = $format", | ||
| $model->get_id() | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Fix potential SQL injection vulnerability in read() method
Interpolating variables directly into the SQL query can lead to SQL injection vulnerabilities. Specifically, using {$id_field_name} and $format in the query string is unsafe.
Apply this diff to properly prepare the SQL statement:
$this->add_sql_clause(
'where',
- $wpdb->prepare(
- " AND {$id_field_name} = $format",
- $model->get_id()
- )
+ $wpdb->prepare(
+ " AND " . esc_sql( $id_field_name ) . " = %s",
+ $model->get_id()
+ )
);Ensure that $id_field_name is sanitized or comes from a trusted source.
📝 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.
| " AND {$id_field_name} = $format", | |
| $model->get_id() | |
| ) | |
| ); | |
| $wpdb->prepare( | |
| " AND " . esc_sql( $id_field_name ) . " = %s", | |
| $model->get_id() | |
| ) | |
| ); |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 77-77:
Use placeholders and $wpdb->prepare(); found interpolated variable {$id_field_name} at " AND {$id_field_name} = $format"
[failure] 77-77:
Use placeholders and $wpdb->prepare(); found interpolated variable $format at " AND {$id_field_name} = $format"
[warning] 77-77:
Replacement variables found, but no valid placeholders found in the query.
* Add delete_by method * Add update_by to base store * Add general update query method in Data Sotre * Update vendor balance Model and Store to perform update operations * Refactor dokan order sync for vendor balance * Add database mission assertion method for test case * Add read_meta method to BaseStore * Add method to get vendor total balance * Fetch vendor balance with the method of VendorBalance Method
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (15)
includes/Models/DataStore/VendorBalanceStore.php (3)
12-12: Replace "DOKAN_SINCE" placeholder with actual version number.The
@sincetag contains a placeholder that needs to be replaced with the actual version number where this class was introduced.
19-31: Consider using more descriptive field names.The abbreviated field names (
trn_id,trn_type,trn_date) could be more descriptive. Consider using full words for better readability and maintainability:protected function get_fields_with_format(): array { return [ 'vendor_id' => '%d', - 'trn_id' => '%d', - 'trn_type' => '%d', + 'transaction_id' => '%d', + 'transaction_type' => '%d', 'particulars' => '%s', 'debit' => '%f', 'credit' => '%f', 'status' => '%s', - 'trn_date' => '%s', + 'transaction_date' => '%s', 'balance_date' => '%s', ]; }
64-67: Extract order status handling into a dedicated method.The order status handling logic could be moved to a separate method for better reusability and clarity:
private function get_earning_statuses(int $vendor_id, DateTimeImmutable $balance_date): array { $earning_status = dokan_withdraw_get_active_order_status(); $earning_status[] = 'approved'; return apply_filters('dokan_vendor_total_balance_order_status', $earning_status, $vendor_id, $balance_date); }includes/Models/BaseModel.php (3)
14-14: Remove commented code.The commented
wc_get_product()appears to be a leftover debug or reference note.- // wc_get_product()
15-17: Improve error handling for missing data store.The early return when data_store is null could be more explicit about why it's failing. Consider throwing an exception or logging a warning.
if ( ! $this->data_store ) { + wc_doing_it_wrong( __FUNCTION__, __( 'Data store is required for this operation.', 'dokan' ), '3.0.0' ); return $this->get_id(); }
50-73: Consider adding return type hint and exception handling.The delete method could benefit from:
- Adding return type hint for better type safety
- Adding try-catch block for data store operations
- public function delete( $force_delete = false ) { + public function delete( $force_delete = false ): bool { $check = apply_filters( "dokan_pre_delete_{$this->object_type}", null, $this, $force_delete ); if ( null !== $check ) { return $check; } if ( $this->data_store ) { + try { $this->data_store->delete( $this, array( 'force_delete' => $force_delete ) ); $this->set_id( 0 ); return true; + } catch ( \Exception $e ) { + wc_doing_it_wrong( __FUNCTION__, $e->getMessage(), '3.0.0' ); + return false; + } } return false;includes/Models/DataStore/BaseDataStore.php (1)
6-6: Remove unused importThe
Automattic\WooCommerce\Pinterest\API\Baseimport is not used in this class.-use Automattic\WooCommerce\Pinterest\API\Base;includes/Commission.php (2)
102-105: LGTM: Improved architecture with VendorBalance model.The change from direct database update to using
VendorBalance::update_by_transactionis a good architectural improvement that:
- Encapsulates database operations in the model layer
- Improves testability
- Reduces direct database manipulation
Line range hint
279-282: Consider using deprecation for method renames.The methods
set_order_qunatityandget_order_qunatityhave typos in their names. While fixing these is good, they are public methods that might be used by other code. Consider:
- Creating new methods with correct spelling
- Marking old methods as deprecated
- Making old methods call new methods internally
Here's the suggested approach:
+/** + * Set order quantity + * + * @since NEW_VERSION + * + * @param int $number + * + * @return void + */ +public function set_order_quantity( $number ) { + $this->quantity = $number; +} /** * Set order quantity * * @since 2.9.21 * @deprecated NEW_VERSION Use set_order_quantity() instead * * @param int $number * * @return void */ public function set_order_qunatity( $number ) { - $this->quantity = $number; + _deprecated_function( __METHOD__, 'NEW_VERSION', 'set_order_quantity' ); + $this->set_order_quantity( $number ); } +/** + * Get order quantity + * + * @since NEW_VERSION + * + * @return int + */ +public function get_order_quantity() { + return $this->quantity; +} /** * Get order quantity * * @since 2.9.21 * @deprecated NEW_VERSION Use get_order_quantity() instead * * @return int */ public function get_order_qunatity() { - return $this->quantity; + _deprecated_function( __METHOD__, 'NEW_VERSION', 'get_order_quantity' ); + return $this->get_order_quantity(); }Also applies to: 290-293
tests/php/src/Models/VendorBalanceModelTest.php (4)
63-88: Add PHPDoc block totest_read_methodTo enhance code documentation and maintain consistency with best practices, please add a PHPDoc block for the
test_read_method. This helps in understanding the purpose and usage of the test.
90-110: Add PHPDoc block totest_update_transaction_methodPlease include a PHPDoc block for the
test_update_transaction_methodto describe its functionality and any relevant details. This improves code maintainability and readability.
112-137: Add PHPDoc block totest_dokan_sync_insert_order_functionConsider adding a PHPDoc block to the
test_dokan_sync_insert_order_functionmethod. This will provide clarity on the test's intent and usage.
139-181: Add PHPDoc block totest_get_total_balance_by_vendor_methodTo maintain consistency and enhance documentation, please add a PHPDoc block for
test_get_total_balance_by_vendor_method.includes/Order/functions.php (2)
277-277: Make the 'New order' string translatableThe string
'New order'is hardcoded and not translatable. To support localization, please use the__()function to make this string translatable.Apply this diff to make the string translatable:
- $vendor_balance->set_particulars( 'New order' ); + $vendor_balance->set_particulars( __( 'New order', 'dokan-lite' ) );
279-280: Cache the current datetime to ensure consistencyYou're calling
dokan_current_datetime()twice, which could result in slight inconsistencies due to timing differences. Consider storing the datetime in a variable and reusing it to ensure consistency.Apply this diff to cache the datetime:
$current_datetime = dokan_current_datetime(); $vendor_balance->set_trn_date( $current_datetime->format( 'Y-m-d H:i:s' ) ); $balance_datetime = clone $current_datetime; $balance_datetime->modify( "+ $threshold_day days" ); $vendor_balance->set_balance_date( $balance_datetime->format( 'Y-m-d H:i:s' ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
includes/Commission.php(2 hunks)includes/Models/BaseModel.php(1 hunks)includes/Models/DataStore/BaseDataStore.php(1 hunks)includes/Models/DataStore/VendorBalanceStore.php(1 hunks)includes/Models/VendorBalance.php(1 hunks)includes/Order/Hooks.php(2 hunks)includes/Order/functions.php(2 hunks)includes/Vendor/Vendor.php(2 hunks)tests/php/src/CustomAssertion/DBAssertionTrait.php(1 hunks)tests/php/src/Models/VendorBalanceModelTest.php(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Models/DataStore/BaseDataStore.php
[warning] 82-82:
Replacement variables found, but no valid placeholders found in the query.
[warning] 88-88:
Use placeholders and $wpdb->prepare(); found $this
[warning] 88-88:
Use placeholders and $wpdb->prepare(); found get_query_statement
[warning] 257-257:
Replacement variables found, but no valid placeholders found in the query.
includes/Models/DataStore/VendorBalanceStore.php
[warning] 89-89:
Use placeholders and $wpdb->prepare(); found $query_statement
tests/php/src/CustomAssertion/DBAssertionTrait.php
[warning] 81-81:
json_encode() is discouraged. Use wp_json_encode() instead.
🔇 Additional comments (14)
includes/Models/DataStore/VendorBalanceStore.php (3)
37-39: LGTM!
The method is simple and follows the single responsibility principle.
1-94: Verify integration with existing vendor balance functionality.
As mentioned in past reviews, ensure this new data store implementation is properly integrated with existing vendor balance functionality. Consider:
- Migration plan for existing data
- Backwards compatibility
- Integration points with existing vendor balance methods
#!/bin/bash
# Check for existing vendor balance functionality
echo "Checking existing vendor balance implementations:"
rg "vendor_balance|dokan_get_vendor_balance" --type php
# Check for potential integration points
echo "Checking potential integration points:"
rg "class.*Vendor.*Balance|interface.*Vendor.*Balance" --type php🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 89-89:
Use placeholders and $wpdb->prepare(); found $query_statement
87-90:
Ensure proper SQL injection protection.
While the code uses esc_sql() for arrays, the final query is not properly prepared. Consider using $wpdb->prepare() for the entire query or ensure all dynamic parts are properly escaped.
✅ Verification successful
Let me gather more information about the SQL query construction and usage.
Let me check the base class to understand the SQL query construction methods.
Let me check one final aspect about the SQL query construction.
Based on the gathered information, I can now provide the final response:
The SQL query is properly protected against SQL injection.
The code follows secure practices by:
- Using
esc_sql()for array values in$trn_types_strand$statuses_str - Using
$wpdb->prepare()for dynamic values like vendor_id and date - Using a structured query builder (SqlQuery class) that safely constructs the final query through
add_sql_clause() - The
phpcs:ignorecomment is justified as the final query is composed of properly escaped/prepared components
The warning can be safely ignored as the code implements proper SQL injection protection through WordPress's built-in security functions and a secure query builder pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other instances of direct $wpdb->get_var usage
rg "wpdb->get_var\(" --type php
Length of output: 14558
Script:
#!/bin/bash
# Check the full context of the SQL query construction in VendorBalanceStore.php
rg -B 10 -A 5 "query_statement" includes/Models/DataStore/VendorBalanceStore.php
# Check for any SQL preparation or sanitization methods in the same file
rg "prepare|esc_sql" includes/Models/DataStore/VendorBalanceStore.php
Length of output: 1256
Script:
#!/bin/bash
# Check the BaseDataStore class implementation
rg -B 5 -A 5 "get_query_statement|add_sql_clause" --type php
# Also check for any SQL-related methods in the base class
ast-grep --pattern 'class BaseDataStore {
$$$
}'
Length of output: 8092
Script:
#!/bin/bash
# Check the SQL query construction in BaseDataStore
rg -B 10 -A 10 "class BaseDataStore" --type php
# Also check for any SQL-related methods that handle query building
rg "get_table_name_with_prefix|clear_all_clauses" --type php
Length of output: 2727
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 89-89:
Use placeholders and $wpdb->prepare(); found $query_statement
includes/Models/BaseModel.php (3)
1-7: LGTM! Class structure and namespace are well-defined.
The abstract class properly extends WC_Data and follows PHP best practices.
91-93: LGTM! Hook prefix method is well implemented.
The method follows WordPress naming conventions and has appropriate visibility.
7-7: Verify consistent usage of BaseModel across codebase.
Let's ensure this base model is used consistently and properly extends WC_Data functionality.
✅ Verification successful
Based on the results, I can now generate the final response:
BaseModel implementation appears consistent and well-structured
The verification shows:
- Only one class (
VendorBalance) extendsBaseModel, and it properly implements the data store pattern - All other
WC_Datausages are direct WooCommerce core implementations - Data store implementations across the codebase follow WooCommerce's patterns consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for classes extending BaseModel and their implementation
# Find all PHP files that might extend BaseModel
echo "Checking for classes extending BaseModel..."
rg -l "extends\s+BaseModel" --type php
# Check for potential inconsistent data store usage
echo "Checking for data store implementations..."
rg "data_store\s*=" --type php -C 2
# Look for direct WC_Data usage that might need to use BaseModel instead
echo "Checking for direct WC_Data usage..."
rg "extends\s+WC_Data" --type php -C 2
Length of output: 3895
includes/Models/VendorBalance.php (1)
283-295: LGTM: Effective caching implementation.
The get_total_balance_by_vendor method:
- Properly handles date validation and conversion
- Implements caching effectively with vendor-specific cache keys
- Uses appropriate cache group naming
includes/Models/DataStore/BaseDataStore.php (1)
79-85:
Fix SQL injection vulnerability in read() method
The where clause is vulnerable to SQL injection as it directly interpolates variables into the query string.
$this->add_sql_clause(
'where',
$wpdb->prepare(
- " AND {$id_field_name} = $format",
+ " AND " . esc_sql($id_field_name) . " = %d",
$model->get_id()
)
);Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 82-82:
Replacement variables found, but no valid placeholders found in the query.
includes/Order/Hooks.php (2)
7-7: LGTM!
The import statement is correctly placed and follows PSR-4 autoloading standards.
129-132:
Add error handling for the balance update operation.
While the migration to using the VendorBalance model is a good improvement, the update operation should include error handling to ensure vendor balances are accurately maintained.
-VendorBalance::update_by_transaction(
- $order_id,
- VendorBalance::TRN_TYPE_DOKAN_ORDERS,
- [ 'status' => $new_status ]
-);
+try {
+ VendorBalance::update_by_transaction(
+ $order_id,
+ VendorBalance::TRN_TYPE_DOKAN_ORDERS,
+ [ 'status' => $new_status ]
+ );
+} catch (\Exception $e) {
+ dokan_log(
+ sprintf(
+ 'Failed to update vendor balance for order #%d: %s',
+ $order_id,
+ $e->getMessage()
+ )
+ );
+ // Optionally, add an order note about the failed balance update
+ $order->add_order_note(
+ sprintf(
+ __('Failed to update vendor balance: %s', 'dokan-lite'),
+ $e->getMessage()
+ )
+ );
+}Let's verify the transaction type constant and its usage:
includes/Commission.php (1)
7-7: LGTM: Import statements are correctly added.
The new imports for VendorBalance and Vendor classes are properly added and align with their usage in the code.
Also applies to: 9-9
includes/Vendor/Vendor.php (2)
8-8: LGTM! Clean import of the new VendorBalance model.
The import follows PSR-4 autoloading standards and maintains proper ordering.
790-790: LGTM! Good refactoring to use the new VendorBalance model.
The change improves code organization by moving vendor balance logic to a dedicated model class.
Let's verify the implementation of the new method being called:
✅ Verification successful
The refactoring is well-implemented and properly tested
The verification confirms:
- The
get_total_balance_by_vendormethod is properly implemented inincludes/Models/VendorBalance.php - The method is covered by unit tests in
VendorBalanceModelTest.php - The implementation includes proper caching mechanism for performance
- The method signature matches the usage in the Vendor class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of get_total_balance_by_vendor method
# Test: Check if the method exists and its implementation
ast-grep --pattern 'public static function get_total_balance_by_vendor($vendor_id, $on_date)'
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search for the VendorBalance class and its methods
# Search for VendorBalance class definition and its methods
rg -l "class VendorBalance"
# Search for get_total_balance_by_vendor method with more context
rg "get_total_balance_by_vendor" -A 5
Length of output: 2193
includes/Order/functions.php (1)
4-4: Ensure the 'VendorBalance' class is properly imported
The VendorBalance class is imported correctly, allowing its usage within this file.
| public function assertDatabaseMissing( string $table, array $data = [] ): void { | ||
| $rows_count = $this->getDatabaseCount( $table, $data ); | ||
|
|
||
| $this->assertGreaterThanOrEqual( 0, $rows_count, "{$rows_count} rows found in `$table` for given data " . json_encode( $data ) ); | ||
| } |
There was a problem hiding this comment.
Critical: Fix assertion logic and add documentation
The current implementation has several issues:
- The assertion
assertGreaterThanOrEqual(0, $rows_count)will always pass since row counts cannot be negative. This defeats the purpose of asserting that records are missing. - Missing PHPDoc block for documenting the method's purpose and parameters.
- Using
json_encodeinstead of WordPress'swp_json_encode.
Apply this diff to fix the issues:
+ /**
+ * Assert that a table does not contain any rows matching the specified criteria.
+ *
+ * @param string $table The name of the table (without the prefix).
+ * @param array $data An associative array of field-value pairs to match.
+ * @return void
+ */
public function assertDatabaseMissing( string $table, array $data = [] ): void {
$rows_count = $this->getDatabaseCount( $table, $data );
- $this->assertGreaterThanOrEqual( 0, $rows_count, "{$rows_count} rows found in `$table` for given data " . json_encode( $data ) );
+ $this->assertEquals( 0, $rows_count, "Expected `$table` to not have any matching rows, but found {$rows_count} for data " . wp_json_encode( $data ) );
}📝 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 assertDatabaseMissing( string $table, array $data = [] ): void { | |
| $rows_count = $this->getDatabaseCount( $table, $data ); | |
| $this->assertGreaterThanOrEqual( 0, $rows_count, "{$rows_count} rows found in `$table` for given data " . json_encode( $data ) ); | |
| } | |
| /** | |
| * Assert that a table does not contain any rows matching the specified criteria. | |
| * | |
| * @param string $table The name of the table (without the prefix). | |
| * @param array $data An associative array of field-value pairs to match. | |
| * @return void | |
| */ | |
| public function assertDatabaseMissing( string $table, array $data = [] ): void { | |
| $rows_count = $this->getDatabaseCount( $table, $data ); | |
| $this->assertEquals( 0, $rows_count, "Expected `$table` to not have any matching rows, but found {$rows_count} for data " . wp_json_encode( $data ) ); | |
| } |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 81-81:
json_encode() is discouraged. Use wp_json_encode() instead.
💡 Codebase verification
Additional test coverage needed for assertDatabaseMissing
The assertion method is currently only used in one test case (VendorBalanceModelTest::test_dokan_sync_insert_order_function), which tests a negative scenario. There's no dedicated test suite for the assertion trait itself, and the current usage doesn't cover all edge cases (e.g., empty data array, non-existent table, multiple matching conditions).
Recommended test scenarios to add:
- Test with empty data array
- Test with multiple conditions in data array
- Test with non-existent table
- Test with data that exists (should fail)
🔗 Analysis chain
Verify test coverage for the new assertion method
Since this is a testing utility, we should ensure it's properly tested with both positive and negative scenarios.
Let's check if there are corresponding test cases:
Let me check the actual test implementation to better understand the test coverage.
Let me check if there are any other test files that might be testing this assertion trait directly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases using assertDatabaseMissing
rg "assertDatabaseMissing" tests/
Length of output: 255
Script:
#!/bin/bash
# Get the context of the test case in VendorBalanceModelTest.php
rg -B 5 -A 5 "assertDatabaseMissing" tests/php/src/Models/VendorBalanceModelTest.php
Length of output: 511
Script:
#!/bin/bash
# Look for test files related to DBAssertionTrait
fd "DBAssertion.*Test\.php" tests/
# Also check if there's a dedicated test directory for assertions
fd "Test\.php" tests/php/src/CustomAssertion/
Length of output: 84
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 81-81:
json_encode() is discouraged. Use wp_json_encode() instead.
| public function get_total_balance( $vendor_id, DateTimeImmutable $balance_date ): float { | ||
| global $wpdb; | ||
|
|
||
| $on_date = $balance_date->format( 'Y-m-d' ); | ||
|
|
||
| $earning_status = dokan_withdraw_get_active_order_status(); | ||
| $earning_status[] = 'approved'; | ||
| $earning_status = apply_filters( 'dokan_vendor_total_balance_order_status', $earning_status, $vendor_id, $balance_date ); | ||
| $statuses_str = "'" . implode( "', '", esc_sql( $earning_status ) ) . "'"; | ||
|
|
||
| $trn_types = apply_filters( | ||
| 'dokan_vendor_total_balance_trn_types', [ | ||
| VendorBalance::TRN_TYPE_DOKAN_ORDERS, | ||
| VendorBalance::TRN_TYPE_DOKAN_REFUND, | ||
| ], $vendor_id, $balance_date | ||
| ); | ||
|
|
||
| $trn_types_str = "'" . implode( "', '", esc_sql( $trn_types ) ) . "'"; | ||
|
|
||
| $this->add_sql_clause( 'select', 'SUM(debit) - SUM(credit) AS earnings' ); | ||
| $this->add_sql_clause( 'from', $this->get_table_name_with_prefix() ); | ||
| $this->add_sql_clause( 'where', $wpdb->prepare( ' AND vendor_id = %d', $vendor_id ) ); | ||
| $this->add_sql_clause( 'where', $wpdb->prepare( ' AND DATE(balance_date) <= %s ', $on_date ) ); | ||
| $this->add_sql_clause( 'where', " AND trn_type IN ($trn_types_str)" ); | ||
| $this->add_sql_clause( 'where', " AND status IN ($statuses_str)" ); | ||
|
|
||
| $query_statement = $this->get_query_statement(); | ||
|
|
||
| // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared | ||
| $earnings = $wpdb->get_var( | ||
| $query_statement | ||
| ); | ||
|
|
||
| return floatval( $earnings ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider breaking down the complex method into smaller, focused functions.
The get_total_balance method handles multiple responsibilities. Consider extracting the following into separate methods:
- Status handling
- Transaction type handling
- Query building
This would improve readability and maintainability.
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 89-89:
Use placeholders and $wpdb->prepare(); found $query_statement
| public function get_meta_data() { | ||
| return apply_filters( $this->get_hook_prefix() . 'meta_data', array() ); | ||
| } |
There was a problem hiding this comment.
Review meta data implementation.
The current implementation seems incomplete:
- It returns an empty array by default without actually retrieving meta data
- It doesn't utilize WC_Data's meta data functionality
Consider implementing proper meta data retrieval:
public function get_meta_data() {
- return apply_filters( $this->get_hook_prefix() . 'meta_data', array() );
+ $meta_data = parent::get_meta_data();
+ return apply_filters( $this->get_hook_prefix() . 'meta_data', $meta_data );
}📝 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 get_meta_data() { | |
| return apply_filters( $this->get_hook_prefix() . 'meta_data', array() ); | |
| } | |
| public function get_meta_data() { | |
| $meta_data = parent::get_meta_data(); | |
| return apply_filters( $this->get_hook_prefix() . 'meta_data', $meta_data ); | |
| } |
| public static function delete_by( array $data ) { | ||
| $object = new static(); | ||
| return $object->data_store->delete_by( $data ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance delete_by method with better type safety and error handling.
The static delete_by method needs:
- Return type hint
- Error handling for data store operations
- Documentation for possible exceptions
+ /**
+ * Delete rows from the database.
+ *
+ * @param array $data Array of args to delete objects.
+ * @return bool True on success, false on failure.
+ * @throws \Exception When data store operation fails.
+ */
- public static function delete_by( array $data ) {
+ public static function delete_by( array $data ): bool {
$object = new static();
+ try {
return $object->data_store->delete_by( $data );
+ } catch ( \Exception $e ) {
+ wc_doing_it_wrong( __FUNCTION__, $e->getMessage(), '3.0.0' );
+ return false;
+ }
}📝 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 static function delete_by( array $data ) { | |
| $object = new static(); | |
| return $object->data_store->delete_by( $data ); | |
| } | |
| /** | |
| * Delete rows from the database. | |
| * | |
| * @param array $data Array of args to delete objects. | |
| * @return bool True on success, false on failure. | |
| * @throws \Exception When data store operation fails. | |
| */ | |
| public static function delete_by( array $data ): bool { | |
| $object = new static(); | |
| try { | |
| return $object->data_store->delete_by( $data ); | |
| } catch ( \Exception $e ) { | |
| wc_doing_it_wrong( __FUNCTION__, $e->getMessage(), '3.0.0' ); | |
| return false; | |
| } | |
| } |
| public function set_debit( float $amount ) { | ||
| $this->set_prop( 'debit', $amount ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for monetary values.
The setter methods for monetary values should validate that the amounts are non-negative:
public function set_debit( float $amount ) {
+ if ( $amount < 0 ) {
+ throw new \InvalidArgumentException( 'Debit amount must be non-negative.' );
+ }
$this->set_prop( 'debit', $amount );
}
public function set_credit( float $amount ) {
+ if ( $amount < 0 ) {
+ throw new \InvalidArgumentException( 'Credit amount must be non-negative.' );
+ }
$this->set_prop( 'credit', $amount );
}Also applies to: 211-213
| public function create( BaseModel &$model ) { | ||
| $data = $this->map_model_to_db_data( $model ); | ||
|
|
||
| $inserted_id = $this->insert( $data ); | ||
|
|
||
| if ( $inserted_id ) { | ||
| $model->set_id( $inserted_id ); | ||
| $model->apply_changes(); | ||
| } | ||
|
|
||
| do_action( $this->get_hook_prefix() . 'created', $inserted_id, $data ); | ||
|
|
||
| return $inserted_id; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in database operations
The create(), update(), and insert() methods should handle database errors more gracefully:
- Check for false/null return values from $wpdb operations
- Throw specific exceptions with meaningful messages
- Log errors for debugging
Example improvement for the create() method:
public function create( BaseModel &$model ) {
+ try {
$data = $this->map_model_to_db_data( $model );
$inserted_id = $this->insert( $data );
- if ( $inserted_id ) {
+ if ( $inserted_id === 0 ) {
+ throw new Exception( sprintf(
+ __( 'Failed to create record in %s', 'dokan-lite' ),
+ $this->get_table_name()
+ ) );
+ }
$model->set_id( $inserted_id );
$model->apply_changes();
- }
do_action( $this->get_hook_prefix() . 'created', $inserted_id, $data );
return $inserted_id;
+ } catch ( Exception $e ) {
+ error_log( sprintf( '[Dokan] Database error in %s: %s', __METHOD__, $e->getMessage() ) );
+ throw $e;
+ }
}Also applies to: 108-127, 274-292
| $result = $wpdb->query( | ||
| "DELETE FROM {$table_name} | ||
| WHERE {$where_clause}" | ||
| ); |
There was a problem hiding this comment.
Fix SQL injection vulnerability in delete_by() method
The DELETE query is vulnerable to SQL injection as it directly interpolates the table name into the query.
- $result = $wpdb->query(
- "DELETE FROM {$table_name}
- WHERE {$where_clause}"
- );
+ $result = $wpdb->query(
+ $wpdb->prepare(
+ "DELETE FROM " . esc_sql($table_name) . " WHERE " . $where_clause
+ )
+ );Committable suggestion skipped: line range outside the PR's diff.
| protected function prepare_where_clause( array $data ): string { | ||
| global $wpdb; | ||
|
|
||
| $where = [ '1=1' ]; | ||
|
|
||
| $field_format = $this->get_fields_with_format(); | ||
|
|
||
| // Add ID field format for the WHERE clause. | ||
| $field_format[ $this->get_id_field_name() ] = $this->get_id_field_format(); | ||
|
|
||
| foreach ( $data as $key => $value ) { | ||
| if ( is_array( $value ) ) { | ||
| // Fill format array based on the number of values and specified format for the key. | ||
| $placeholders = implode( ',', array_fill( 0, count( $value ), '%s' ) ); | ||
|
|
||
| // Generate the WHERE clause securely using $wpdb->prepare() with placeholders. | ||
| $where[] = $wpdb->prepare( "{$key} IN ({$placeholders})", ...$value ); | ||
| } else { | ||
| $format = $field_format[ $key ]; | ||
| $where[] = $wpdb->prepare( "{$key} = {$format}", $value ); | ||
| } | ||
| } | ||
|
|
||
| return implode( ' AND ', $where ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve security in prepare_where_clause() method
The method needs additional security measures:
- Validate column names against a whitelist
- Use proper escaping for column names
protected function prepare_where_clause( array $data ): string {
global $wpdb;
$where = [ '1=1' ];
$field_format = $this->get_fields_with_format();
+ $allowed_columns = array_keys($field_format);
// Add ID field format for the WHERE clause.
$field_format[ $this->get_id_field_name() ] = $this->get_id_field_format();
+ $allowed_columns[] = $this->get_id_field_name();
foreach ( $data as $key => $value ) {
+ if (!in_array($key, $allowed_columns, true)) {
+ continue;
+ }
+ $escaped_key = esc_sql($key);
if ( is_array( $value ) ) {
$placeholders = implode( ',', array_fill( 0, count( $value ), '%s' ) );
- $where[] = $wpdb->prepare( "{$key} IN ({$placeholders})", ...$value );
+ $where[] = $wpdb->prepare( "{$escaped_key} IN ({$placeholders})", ...$value );
} else {
$format = $field_format[ $key ];
- $where[] = $wpdb->prepare( "{$key} = {$format}", $value );
+ $where[] = $wpdb->prepare( "{$escaped_key} = {$format}", $value );
}
}
return implode( ' AND ', $where );
}📝 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.
| protected function prepare_where_clause( array $data ): string { | |
| global $wpdb; | |
| $where = [ '1=1' ]; | |
| $field_format = $this->get_fields_with_format(); | |
| // Add ID field format for the WHERE clause. | |
| $field_format[ $this->get_id_field_name() ] = $this->get_id_field_format(); | |
| foreach ( $data as $key => $value ) { | |
| if ( is_array( $value ) ) { | |
| // Fill format array based on the number of values and specified format for the key. | |
| $placeholders = implode( ',', array_fill( 0, count( $value ), '%s' ) ); | |
| // Generate the WHERE clause securely using $wpdb->prepare() with placeholders. | |
| $where[] = $wpdb->prepare( "{$key} IN ({$placeholders})", ...$value ); | |
| } else { | |
| $format = $field_format[ $key ]; | |
| $where[] = $wpdb->prepare( "{$key} = {$format}", $value ); | |
| } | |
| } | |
| return implode( ' AND ', $where ); | |
| } | |
| protected function prepare_where_clause( array $data ): string { | |
| global $wpdb; | |
| $where = [ '1=1' ]; | |
| $field_format = $this->get_fields_with_format(); | |
| $allowed_columns = array_keys($field_format); | |
| // Add ID field format for the WHERE clause. | |
| $field_format[ $this->get_id_field_name() ] = $this->get_id_field_format(); | |
| $allowed_columns[] = $this->get_id_field_name(); | |
| foreach ( $data as $key => $value ) { | |
| if (!in_array($key, $allowed_columns, true)) { | |
| continue; | |
| } | |
| $escaped_key = esc_sql($key); | |
| if ( is_array( $value ) ) { | |
| $placeholders = implode( ',', array_fill( 0, count( $value ), '%s' ) ); | |
| $where[] = $wpdb->prepare( "{$escaped_key} IN ({$placeholders})", ...$value ); | |
| } else { | |
| $format = $field_format[ $key ]; | |
| $where[] = $wpdb->prepare( "{$escaped_key} = {$format}", $value ); | |
| } | |
| } | |
| return implode( ' AND ', $where ); | |
| } |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 257-257:
Replacement variables found, but no valid placeholders found in the query.
| $this->assertGreaterThan( 110, $total_balance ); | ||
| } |
There was a problem hiding this comment.
Correct the assertion to match the expected total balance
The calculation comment indicates an expected total balance of 110 (100 + 60 - 20 - 30 = 110). However, the assertion uses assertGreaterThan( 110, $total_balance ), which may not align with the expected result.
Apply this diff to correct the assertion:
- $this->assertGreaterThan( 110, $total_balance );
+ $this->assertEquals( 110, $total_balance );This change ensures that the test accurately verifies the total balance is exactly 110 as per the calculation.
📝 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.
| $this->assertGreaterThan( 110, $total_balance ); | |
| } | |
| $this->assertEquals( 110, $total_balance ); | |
| } |
| $vendor_balance->set_debit( $net_amount ); | ||
| $vendor_balance->set_trn_date( dokan_current_datetime()->format( 'Y-m-d H:i:s' ) ); | ||
| $vendor_balance->set_balance_date( dokan_current_datetime()->modify( "+ $threshold_day days" )->format( 'Y-m-d H:i:s' ) ); | ||
| $vendor_balance->save(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential exceptions from the save() method
The save() method may throw exceptions if the operation fails. To prevent uncaught exceptions and handle errors gracefully, consider wrapping the save() call in a try-catch block.
Apply this diff to add error handling:
- $vendor_balance->save();
+ try {
+ $vendor_balance->save();
+ } catch ( Exception $e ) {
+ // Handle the exception, e.g., log the error or display an admin notice
+ error_log( 'Failed to save vendor balance: ' . $e->getMessage() );
+ }📝 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.
| $vendor_balance->save(); | |
| try { | |
| $vendor_balance->save(); | |
| } catch ( Exception $e ) { | |
| // Handle the exception, e.g., log the error or display an admin notice | |
| error_log( 'Failed to save vendor balance: ' . $e->getMessage() ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (10)
includes/DependencyManagement/Providers/ModelServiceProvider.php (1)
7-11: Consider renaming the TAG constant.The constant value 'ajax-service' seems misaligned with the purpose of this service provider, which is focused on model services. Consider using a more appropriate tag like 'model-service' to better reflect its responsibility.
- public const TAG = 'ajax-service'; + public const TAG = 'model-service';includes/Models/DataStore/VendorBalanceStore.php (3)
9-13: Update class documentation with version information.The
@since DOKAN_SINCEplaceholder needs to be replaced with the actual version number where this class was introduced.
19-31: Standardize field names for better maintainability.Consider using more consistent and descriptive field names:
trn_id→transaction_idtrn_type→transaction_typetrn_date→transaction_dateThis would improve code readability and maintain consistency with modern naming conventions.
88-90: Use prepared statements consistently.The PHPCS warning about unprepared SQL is valid. While the query is built using prepared statements in the clauses, it's good practice to use
$wpdb->prepare()for the final query as well.-$earnings = $wpdb->get_var( - $query_statement -); +$earnings = $wpdb->get_var( + $wpdb->prepare('%s', $query_statement) +);🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 89-89:
Use placeholders and $wpdb->prepare(); found $query_statementincludes/DependencyManagement/Providers/ServiceProvider.php (1)
61-64: Consider documenting provider initialization order.The order of service provider initialization can be crucial. Consider adding a comment block above the provider registrations explaining any dependencies between providers and why this specific order is chosen.
Example documentation:
public function boot(): void { + // Provider initialization order: + // 1. AdminServiceProvider: Core admin functionality + // 2. CommonServiceProvider: Shared services + // 3. FrontendServiceProvider: Frontend-specific services + // 4. AjaxServiceProvider: AJAX handlers + // 5. AnalyticsServiceProvider: Analytics tracking + // 6. ModelServiceProvider: Data models and persistence $this->getContainer()->addServiceProvider( new AdminServiceProvider() );tests/php/src/Models/VendorBalanceModelTest.php (4)
63-88: Enhance test_read_method with error cases and documentation.
- Add PHPDoc block
- Add test cases for error scenarios:
- Reading non-existent ID
- Reading invalid ID (e.g., negative number)
+ /** + * Test reading a VendorBalance instance from database + * + * @since 3.x.x + * @return void + */ public function test_read_method() { // ... existing test code ... } + /** + * Test reading a VendorBalance instance with invalid data + * + * @since 3.x.x + * @return void + */ + public function test_read_method_with_invalid_data() { + $this->expectException(\InvalidArgumentException::class); + new VendorBalance(-1); + + $non_existent_id = 999999; + $vendor_balance = new VendorBalance($non_existent_id); + $this->assertNull($vendor_balance->get_id()); + }
90-110: Improve test coverage for update_transaction_method.
- Add PHPDoc block
- Verify unchanged fields remain intact
- Add test cases for:
- Invalid transaction ID
- Invalid transaction type
- Empty update data
+ /** + * Test updating a VendorBalance instance by transaction + * + * @since 3.x.x + * @return void + */ public function test_update_transaction_method() { $vendor_balance = new VendorBalance(); // ... existing setup code ... $result = VendorBalance::update_by_transaction( 1, VendorBalance::TRN_TYPE_DOKAN_ORDERS, [ 'debit' => 200, ] ); $updated_vendor_balance = new VendorBalance( $vendor_balance->get_id() ); $this->assertEquals( 200, $updated_vendor_balance->get_debit() ); + // Verify other fields remained unchanged + $this->assertEquals( 'test', $updated_vendor_balance->get_particulars() ); + $this->assertEquals( 1, $updated_vendor_balance->get_vendor_id() ); }
112-137: Add error cases and documentation for sync order test.
- Add PHPDoc block
- Test error scenarios:
- Invalid order ID
- Already synced orders
- Failed synchronization
+ /** + * Test synchronization of orders with vendor balance + * + * @since 3.x.x + * @return void + */ public function test_dokan_sync_insert_order_function() { // ... existing test code ... } + /** + * Test synchronization failures + * + * @since 3.x.x + * @return void + */ + public function test_dokan_sync_insert_order_function_failures() { + // Test with invalid order ID + dokan_sync_insert_order(-1); + $this->assertDatabaseMissing('dokan_vendor_balance', ['trn_id' => -1]); + + // Test with already synced order + $order_service = Mockery::mock('\WeDevs\Dokan\Order\Manager[is_order_already_synced]'); + $order_service->shouldReceive('is_order_already_synced')->andReturn(true); + dokan()->get_container()->extend('order')->setConcrete($order_service); + + $order_id = $this->create_single_vendor_order($this->seller_id1); + dokan_sync_insert_order($order_id); + + // Verify no duplicate entry was created + $this->assertDatabaseCount('dokan_vendor_balance', 1, [ + 'trn_id' => $order_id, + ]); + }
139-180: Improve test structure and documentation.
- Add PHPDoc block
- Break down the test into smaller, focused test methods
- Use data providers for different scenarios
+ /** + * Test calculation of total earnings for a vendor + * + * @since 3.x.x + * @return void + */ public function test_get_total_earning_by_vendor_method() { // ... existing test code ... } + /** + * Data provider for vendor earnings test cases + * + * @return array + */ + public function earning_scenarios_provider() { + return [ + 'only_orders' => [ + 'transactions' => [ + ['type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, 'debit' => 100], + ['type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, 'debit' => 60], + ], + 'expected' => 160 + ], + 'orders_and_refunds' => [ + 'transactions' => [ + ['type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, 'debit' => 100], + ['type' => VendorBalance::TRN_TYPE_DOKAN_REFUND, 'credit' => 20], + ], + 'expected' => 80 + ], + ]; + } + /** + * @dataProvider earning_scenarios_provider + */ + public function test_get_total_earning_by_vendor_scenarios($transactions, $expected) { + foreach ($transactions as $transaction) { + $vendor_balance = new VendorBalance(); + $vendor_balance->set_vendor_id($this->seller_id1); + if (isset($transaction['debit'])) { + $vendor_balance->set_debit($transaction['debit']); + } + if (isset($transaction['credit'])) { + $vendor_balance->set_credit($transaction['credit']); + } + $vendor_balance->set_trn_type($transaction['type']); + $vendor_balance->save(); + } + + $total = VendorBalance::get_total_earning_by_vendor($this->seller_id1); + $this->assertEquals($expected, $total); + }includes/Order/functions.php (1)
278-278: Improve code readability and safetyTwo suggestions for improvement:
- Make the particulars text more descriptive by including the order ID
- Use DateInterval for threshold days calculation instead of string concatenation
Apply these improvements:
- $vendor_balance->set_particulars( 'New order' ); + $vendor_balance->set_particulars( sprintf( 'New order #%d', $order_id ) ); - $vendor_balance->set_balance_date( dokan_current_datetime()->modify( "+ $threshold_day days" )->format( 'Y-m-d H:i:s' ) ); + $vendor_balance->set_balance_date( dokan_current_datetime()->add( new DateInterval( "P{$threshold_day}D" ) )->format( 'Y-m-d H:i:s' ) );Also applies to: 281-281
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
includes/Commission.php(2 hunks)includes/DependencyManagement/Providers/ModelServiceProvider.php(1 hunks)includes/DependencyManagement/Providers/ServiceProvider.php(1 hunks)includes/Models/DataStore/VendorBalanceStore.php(1 hunks)includes/Models/VendorBalance.php(1 hunks)includes/Order/Hooks.php(2 hunks)includes/Order/functions.php(2 hunks)includes/Vendor/Vendor.php(15 hunks)tests/php/src/Models/VendorBalanceModelTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- includes/Commission.php
- includes/Order/Hooks.php
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Models/DataStore/VendorBalanceStore.php
[warning] 89-89:
Use placeholders and $wpdb->prepare(); found $query_statement
includes/Vendor/Vendor.php
[failure] 904-904:
A function call to _n() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.
[failure] 905-905:
A function call to __() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.
🔇 Additional comments (19)
includes/DependencyManagement/Providers/ModelServiceProvider.php (3)
1-6: LGTM! Clean namespace and import structure.
The file follows PSR-4 autoloading standards with proper namespace declaration and relevant import.
26-28: LGTM! Well-implemented service check.
The provides() method is well-implemented with:
- Proper type hinting
- Strict comparison in
in_array - Clear documentation
13-16: Consider using imports for better readability.
The services array uses fully qualified class names. Consider importing these classes at the top of the file for better readability and maintainability.
+ use WeDevs\Dokan\Models\VendorBalance;
+ use WeDevs\Dokan\Models\DataStore\VendorBalanceStore;
+
protected $services = [
- \WeDevs\Dokan\Models\VendorBalance::class,
- \WeDevs\Dokan\Models\DataStore\VendorBalanceStore::class,
+ VendorBalance::class,
+ VendorBalanceStore::class,
];Additionally, let's verify the existence of these classes:
✅ Verification successful
Let me try a different search strategy to verify the existence of these classes.
Using imports would improve code readability
Both classes exist in the codebase:
VendorBalanceis defined inincludes/Models/VendorBalance.phpVendorBalanceStoreis defined inincludes/Models/DataStore/VendorBalanceStore.php
The suggestion to use imports is valid and would improve code maintainability without introducing any risks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the model classes
# Search for VendorBalance class definition
echo "Searching for VendorBalance class..."
ast-grep --pattern 'class VendorBalance'
# Search for VendorBalanceStore class definition
echo "Searching for VendorBalanceStore class..."
ast-grep --pattern 'class VendorBalanceStore'
Length of output: 266
Script:
#!/bin/bash
# Search for files containing these class names using ripgrep
echo "Searching for VendorBalance files..."
rg -l "VendorBalance" --type php
echo -e "\nSearching for VendorBalanceStore files..."
rg -l "VendorBalanceStore" --type php
# Get content of any matching files to verify class definitions
echo -e "\nChecking class definitions in matching files..."
rg "class\s+(VendorBalance|VendorBalanceStore)" --type php -A 1
Length of output: 1447
includes/Models/DataStore/VendorBalanceStore.php (1)
37-39: LGTM!
The table name getter is properly implemented.
includes/DependencyManagement/Providers/ServiceProvider.php (1)
64-64: LGTM! Verify ModelServiceProvider implementation.
The addition follows the existing pattern for service provider registration. However, let's verify the ModelServiceProvider implementation.
✅ Verification successful
The verification results show that:
- ModelServiceProvider exists at
includes/DependencyManagement/Providers/ModelServiceProvider.php - It registers VendorBalance and VendorBalanceStore classes
- The VendorBalance model and its store are properly implemented with:
- Proper class inheritance (extends BaseModel)
- Complete test coverage in
tests/php/src/Models/VendorBalanceModelTest.php - Integration with other components (Commission, Order, Vendor)
- Well-defined transaction types and data store operations
Let me generate one final verification to check if ModelServiceProvider extends the correct base class:
ModelServiceProvider implementation is correct and well-structured
The ModelServiceProvider:
- Properly extends BaseServiceProvider
- Correctly registers VendorBalance and VendorBalanceStore services
- Implements required provider methods (provides, register)
- Follows the established service provider pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify ModelServiceProvider exists and follows the expected structure
# Check if ModelServiceProvider exists
fd --type f "ModelServiceProvider.php"
# Check if it extends the base provider class
ast-grep --pattern 'class ModelServiceProvider extends $_ {
$$$
}'
# Look for registration of VendorBalance related services
rg -A 5 "VendorBalance" --type php
Length of output: 18880
Script:
#!/bin/bash
# Check ModelServiceProvider implementation
cat includes/DependencyManagement/Providers/ModelServiceProvider.php
Length of output: 1088
tests/php/src/Models/VendorBalanceModelTest.php (3)
15-32: Previous review comment is still applicable.
34-50: Previous review comment is still applicable.
52-61: Previous review comment is still applicable.
includes/Models/VendorBalance.php (4)
49-57: LGTM! Well-structured constructor implementation.
The constructor follows good practices:
- Uses dependency injection for the data store
- Properly initializes the model with optional ID
- Follows WordPress coding standards
288-300: LGTM! Efficient caching implementation.
The method implements caching effectively:
- Uses vendor-specific cache keys
- Properly handles date formatting
- Falls back to data store when cache miss occurs
8-11: 🛠️ Refactor suggestion
Add transaction type validation using class constants.
The transaction type constants are well-defined, but there's no validation in the set_trn_type method to ensure only valid types are used.
public function set_trn_type( string $type ) {
+ $valid_types = [
+ self::TRN_TYPE_DOKAN_ORDERS,
+ self::TRN_TYPE_DOKAN_WITHDRAW,
+ self::TRN_TYPE_DOKAN_REFUND,
+ ];
+ if ( ! in_array( $type, $valid_types, true ) ) {
+ throw new \InvalidArgumentException( 'Invalid transaction type.' );
+ }
$this->set_prop( 'trn_type', $type );
}Likely invalid or redundant comment.
71-81: 🛠️ Refactor suggestion
Add input validation for transaction updates.
The method should validate its input parameters to ensure data integrity.
public static function update_by_transaction( int $trn_id, string $trn_type, array $data ): int {
+ if ( $trn_id <= 0 ) {
+ throw new \InvalidArgumentException( 'Transaction ID must be positive.' );
+ }
+
+ if ( empty( $data ) ) {
+ throw new \InvalidArgumentException( 'Data array cannot be empty.' );
+ }
+
$model = new static();
// ... rest of the method
}Likely invalid or redundant comment.
includes/Order/functions.php (2)
4-4: LGTM!
The import statement follows PSR-4 autoloading standards and is correctly placed with other imports.
273-282:
Add error handling for the save operation
The save operation could fail and throw exceptions. As suggested in the previous review, we should handle potential errors gracefully.
Apply this diff to add error handling:
- $vendor_balance->save();
+ try {
+ $vendor_balance->save();
+ } catch ( Exception $e ) {
+ dokan_log( sprintf( 'Failed to save vendor balance for order #%d: %s', $order_id, $e->getMessage() ) );
+ return;
+ }includes/Vendor/Vendor.php (5)
227-230: LGTM! Improved code robustness.
The changes enhance code reliability by:
- Explicitly defining the payment array structure
- Adding defensive programming to prevent PHP notices when vendor address is empty
Also applies to: 255-255
430-433: LGTM! Enhanced code readability.
The location handling is improved with explicit default values and clearer array structure.
870-886: LGTM! Improved rating calculation structure.
The changes enhance code quality through:
- Better SQL query formatting
- More explicit rating value calculation
1438-1443: LGTM! Enhanced payment property handling.
The changes improve code reliability through better initialization and change tracking for payment properties.
1462-1467: LGTM! Consistent store time property handling.
The changes maintain consistency with other property handlers through proper initialization and change tracking.
| public function register(): void { | ||
| foreach ( $this->services as $service ) { | ||
| $this->getContainer()->add( $service, $service ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling and service validation.
The register() method could be more robust with:
- Error handling for invalid service classes
- Verification if service is already registered
- More descriptive documentation including @throws annotation
/**
* Register the classes.
+ *
+ * @throws \Exception If a service class does not exist
*/
public function register(): void {
foreach ( $this->services as $service ) {
+ if (!class_exists($service)) {
+ throw new \Exception("Service class {$service} does not exist");
+ }
+ if ($this->getContainer()->has($service)) {
+ continue;
+ }
$this->getContainer()->add( $service, $service );
}
}📝 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 register(): void { | |
| foreach ( $this->services as $service ) { | |
| $this->getContainer()->add( $service, $service ); | |
| } | |
| } | |
| /** | |
| * Register the classes. | |
| * | |
| * @throws \Exception If a service class does not exist | |
| */ | |
| public function register(): void { | |
| foreach ( $this->services as $service ) { | |
| if (!class_exists($service)) { | |
| throw new \Exception("Service class {$service} does not exist"); | |
| } | |
| if ($this->getContainer()->has($service)) { | |
| continue; | |
| } | |
| $this->getContainer()->add( $service, $service ); | |
| } | |
| } |
| $earning_status = dokan_withdraw_get_active_order_status(); | ||
| $earning_status[] = 'approved'; | ||
| $earning_status = apply_filters( 'dokan_vendor_total_balance_order_status', $earning_status, $vendor_id, $balance_date ); | ||
| $statuses_str = "'" . implode( "', '", esc_sql( $earning_status ) ) . "'"; |
There was a problem hiding this comment.
Fix SQL injection vulnerability in status and transaction type handling.
The current implementation using implode() with esc_sql() is vulnerable to SQL injection. Use $wpdb->prepare() with placeholders instead.
Here's how to fix it:
-$statuses_str = "'" . implode( "', '", esc_sql( $earning_status ) ) . "'";
+$placeholders = array_fill(0, count($earning_status), '%s');
+$statuses_str = $wpdb->prepare(implode(',', $placeholders), $earning_status);
-$trn_types_str = "'" . implode( "', '", esc_sql( $trn_types ) ) . "'";
+$placeholders = array_fill(0, count($trn_types), '%s');
+$trn_types_str = $wpdb->prepare(implode(',', $placeholders), $trn_types);Also applies to: 76-76
| public function get_total_earning_by_vendor( $vendor_id, DateTimeImmutable $balance_date ): float { | ||
| global $wpdb; | ||
|
|
||
| $on_date = $balance_date->format( 'Y-m-d' ); | ||
|
|
||
| $earning_status = dokan_withdraw_get_active_order_status(); | ||
| $earning_status[] = 'approved'; | ||
| $earning_status = apply_filters( 'dokan_vendor_total_balance_order_status', $earning_status, $vendor_id, $balance_date ); | ||
| $statuses_str = "'" . implode( "', '", esc_sql( $earning_status ) ) . "'"; | ||
|
|
||
| $trn_types = apply_filters( | ||
| 'dokan_vendor_total_balance_trn_types', [ | ||
| VendorBalance::TRN_TYPE_DOKAN_ORDERS, | ||
| VendorBalance::TRN_TYPE_DOKAN_REFUND, | ||
| ], $vendor_id, $balance_date | ||
| ); | ||
|
|
||
| $trn_types_str = "'" . implode( "', '", esc_sql( $trn_types ) ) . "'"; | ||
|
|
||
| $this->add_sql_clause( 'select', 'SUM(debit) - SUM(credit) AS earnings' ); | ||
| $this->add_sql_clause( 'from', $this->get_table_name_with_prefix() ); | ||
| $this->add_sql_clause( 'where', $wpdb->prepare( ' AND vendor_id = %d', $vendor_id ) ); | ||
| $this->add_sql_clause( 'where', $wpdb->prepare( ' AND DATE(balance_date) <= %s ', $on_date ) ); | ||
| $this->add_sql_clause( 'where', " AND trn_type IN ($trn_types_str)" ); | ||
| $this->add_sql_clause( 'where', " AND status IN ($statuses_str)" ); | ||
|
|
||
| $query_statement = $this->get_query_statement(); | ||
|
|
||
| // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared | ||
| $earnings = $wpdb->get_var( | ||
| $query_statement | ||
| ); | ||
|
|
||
| return floatval( $earnings ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor complex method for better maintainability.
The get_total_earning_by_vendor method handles multiple responsibilities. Consider breaking it down into smaller, focused methods:
- Status handling
- Transaction type handling
- Query building
Here's a suggested refactor:
private function get_earning_status(int $vendor_id, DateTimeImmutable $balance_date): array {
$earning_status = dokan_withdraw_get_active_order_status();
$earning_status[] = 'approved';
return apply_filters('dokan_vendor_total_balance_order_status', $earning_status, $vendor_id, $balance_date);
}
private function get_transaction_types(int $vendor_id, DateTimeImmutable $balance_date): array {
$trn_types = [
VendorBalance::TRN_TYPE_DOKAN_ORDERS,
VendorBalance::TRN_TYPE_DOKAN_REFUND,
];
return apply_filters('dokan_vendor_total_balance_trn_types', $trn_types, $vendor_id, $balance_date);
}
private function build_query(int $vendor_id, string $on_date, array $earning_status, array $trn_types): string {
global $wpdb;
$status_placeholders = array_fill(0, count($earning_status), '%s');
$statuses_str = $wpdb->prepare(implode(',', $status_placeholders), $earning_status);
$type_placeholders = array_fill(0, count($trn_types), '%s');
$trn_types_str = $wpdb->prepare(implode(',', $type_placeholders), $trn_types);
$this->add_sql_clause('select', 'SUM(debit) - SUM(credit) AS earnings');
$this->add_sql_clause('from', $this->get_table_name_with_prefix());
$this->add_sql_clause('where', $wpdb->prepare(' AND vendor_id = %d', $vendor_id));
$this->add_sql_clause('where', $wpdb->prepare(' AND DATE(balance_date) <= %s ', $on_date));
$this->add_sql_clause('where', " AND trn_type IN ($trn_types_str)");
$this->add_sql_clause('where', " AND status IN ($statuses_str)");
return $this->get_query_statement();
}
public function get_total_earning_by_vendor($vendor_id, DateTimeImmutable $balance_date): float {
global $wpdb;
$on_date = $balance_date->format('Y-m-d');
$earning_status = $this->get_earning_status($vendor_id, $balance_date);
$trn_types = $this->get_transaction_types($vendor_id, $balance_date);
$query = $this->build_query($vendor_id, $on_date, $earning_status, $trn_types);
return floatval($wpdb->get_var($query));
}🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 89-89:
Use placeholders and $wpdb->prepare(); found $query_statement
| public function get_credit( string $context = 'view' ) { | ||
| return floatval( $this->get_prop( 'credit', $context ) ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing return type declarations.
Several getter methods are missing return type declarations. Add them for consistency:
-public function get_credit( string $context = 'view' ) {
+public function get_credit( string $context = 'view' ): float {
-public function get_trn_date( string $context = 'view' ) {
+public function get_trn_date( string $context = 'view' ): ?\WC_DateTime {
-public function get_balance_date( string $context = 'view' ) {
+public function get_balance_date( string $context = 'view' ): ?\WC_DateTime {Also applies to: 241-243, 261-263
| $long_text = _n( '%1$s rating from %2$d review', '%1$s rating from %2$d reviews', $rating['count'], 'dokan-lite' ); | ||
| $text = sprintf( __( 'Rated %1$s out of %2$d', 'dokan-lite' ), $rating['rating'], number_format( 5 ) ); | ||
| $width = ( $rating['rating'] / 5 ) * 100; |
There was a problem hiding this comment.
Add translator comments for string placeholders.
Add translator comments to clarify the meaning of placeholders in the translation strings:
+/* translators: 1: rating value, 2: number of reviews */
$long_text = _n( '%1$s rating from %2$d review', '%1$s rating from %2$d reviews', $rating['count'], 'dokan-lite' );
+/* translators: 1: rating value, 2: maximum rating */
$text = sprintf( __( 'Rated %1$s out of %2$d', 'dokan-lite' ), $rating['rating'], number_format( 5 ) );📝 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.
| $long_text = _n( '%1$s rating from %2$d review', '%1$s rating from %2$d reviews', $rating['count'], 'dokan-lite' ); | |
| $text = sprintf( __( 'Rated %1$s out of %2$d', 'dokan-lite' ), $rating['rating'], number_format( 5 ) ); | |
| $width = ( $rating['rating'] / 5 ) * 100; | |
| /* translators: 1: rating value, 2: number of reviews */ | |
| $long_text = _n( '%1$s rating from %2$d review', '%1$s rating from %2$d reviews', $rating['count'], 'dokan-lite' ); | |
| /* translators: 1: rating value, 2: maximum rating */ | |
| $text = sprintf( __( 'Rated %1$s out of %2$d', 'dokan-lite' ), $rating['rating'], number_format( 5 ) ); | |
| $width = ( $rating['rating'] / 5 ) * 100; |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 904-904:
A function call to _n() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.
[failure] 905-905:
A function call to __() with texts containing placeholders was found, but was not accompanied by a "translators:" comment on the line above to clarify the meaning of the placeholders.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
docs/data-store.md (5)
9-14: Enhance documentation with data validation and error handling sectionsWhile the overview effectively explains the data abstraction, consider adding sections about:
- Data validation strategies
- Error handling patterns
- Transaction management
Would you like me to provide a template for these additional sections?
50-56: Add validation and return type declarations to name getter/setterThe name property methods should include validation and proper type declarations.
- public function get_name( $context = 'view' ) { + /** + * Get department name + * + * @param string $context What the value is for. Valid values are 'view' and 'edit'. + * @return string + */ + public function get_name( string $context = 'view' ): string { return $this->get_prop('name', $context); } - public function set_name( $name ) { + /** + * Set department name + * + * @param string $name Department name + * @return void + */ + public function set_name( string $name ): void { + if ( empty( $name ) ) { + throw new \InvalidArgumentException( 'Department name cannot be empty' ); + } return $this->set_prop( 'name', $name ); }
116-122: Consider adding created_by and updated_by fieldsFor better audit trails, consider tracking who creates and updates department records.
public function get_fields_with_format(): array { return [ 'name' => '%s', 'date_created' => '%s', 'updated_at' => '%s', + 'created_by' => '%d', + 'updated_by' => '%d', ]; }
130-154: Add examples for common date format scenariosThe date format override section would benefit from examples of common use cases:
- Working with different timezones
- Handling UTC conversions
- MySQL DATETIME vs TIMESTAMP considerations
Would you like me to provide example code for these scenarios?
🧰 Tools
🪛 LanguageTool
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...efault, the ID field is set toidand format is%d. ## Uses of Models ### Create...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
157-175: Expand usage examples with error handling and bulk operationsConsider adding examples for:
- Error handling and exception catching
- Bulk create/update operations
- Working with relationships between models
Would you like me to provide example code for these scenarios?
🧰 Tools
🪛 LanguageTool
[style] ~157-~157: ‘New Record’ might be wordy. Consider a shorter alternative.
Context: ...%d. ## Uses of Models ### Create a New Record ```php $department = new Department(); ...(EN_WORDINESS_PREMIUM_NEW_RECORD)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/data-store.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/data-store.md
[uncategorized] ~20-~20: Loose punctuation mark.
Context: ..., such as product. - protected $data: Holds the default data for the object. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~105-~105: Loose punctuation mark.
Context: ...e following methods: - get_table_name: Defines the table name in the database....
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~106-~106: Loose punctuation mark.
Context: ...the database. - get_fields_with_format: Returns the fields with format as an ar...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~106-~106: Possible missing comma found.
Context: ...`: Returns the fields with format as an array where key is the db field name and valu...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~106-~106: Two consecutive dots
Context: ...he db field name and value is the format.. Sample implementation: ```php class D...
(DOUBLE_PUNCTUATION)
[uncategorized] ~153-~153: You might be missing the article “the” here.
Context: ...efault, the ID field is set to id and format is %d. ## Uses of Models ### Create...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~157-~157: ‘New Record’ might be wordy. Consider a shorter alternative.
Context: ... %d. ## Uses of Models ### Create a New Record ```php $department = new Department(); ...
(EN_WORDINESS_PREMIUM_NEW_RECORD)
[uncategorized] ~182-~182: You might be missing the article “the” here.
Context: ... by overriding the following methods in Data Store. - `protected function map_model_...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~183-~183: Loose punctuation mark.
Context: ..._to_db_data( BaseModel &$model ): array`: Prepare data for saving a BaseModel to ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~184-~184: Loose punctuation mark.
Context: ...e database. - map_db_raw_to_model_data: Maps database raw data to model data.
(UNLIKELY_OPENING_PUNCTUATION)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
tests/php/src/Models/VendorBalanceModelTest.php (4)
62-87: Add PHPDoc to the test methodThe test method is well structured but missing documentation.
Add this PHPDoc:
+ /** + * Test reading a vendor balance record + * + * @since 3.x.x + * @return void + */ public function test_read_method() {
89-109: Add PHPDoc to the test methodThe test method is well structured but missing documentation.
Add this PHPDoc:
+ /** + * Test updating a vendor balance record by transaction + * + * @since 3.x.x + * @return void + */ public function test_update_transaction_method() {
111-136: Enhance test documentation and assertions
- Missing PHPDoc
- Could verify more fields in the created balance record
Apply these improvements:
+ /** + * Test order synchronization creates vendor balance record + * + * @since 3.x.x + * @return void + */ public function test_dokan_sync_insert_order_function() { // ... existing code ... $this->assertDatabaseCount( 'dokan_vendor_balance', 1, [ 'trn_id' => $order_id, 'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS, 'vendor_id' => $this->seller_id1, + 'status' => 'wc-pending', // verify initial status + 'credit' => 0, // verify no credit ] ); }
144-146: Remove debug code and unused variable
- Remove the commented debug code
- Remove unused variable $trn_id
Apply this diff:
- $trn_id = 1; - // var_dump( $earings_data ); foreach ( $earings_data as $earning_data ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/php/src/Models/VendorBalanceModelTest.php(1 hunks)
🔇 Additional comments (1)
tests/php/src/Models/VendorBalanceModelTest.php (1)
14-31:
Fix incomplete test setup and assertions
- The test is missing vendor_id which appears to be a required field
- The debit assertion is commented out
- Inconsistent indentation in assertDatabaseCount array
Apply this diff:
+ /**
+ * Test saving a vendor balance record
+ *
+ * @since 3.x.x
+ * @return void
+ */
public function test_save_method() {
$vendor_balance = new VendorBalance();
+ $vendor_balance->set_vendor_id( 1 );
$vendor_balance->set_debit( 100 );
$vendor_balance->set_trn_id( 1 );
$vendor_balance->set_trn_type( VendorBalance::TRN_TYPE_DOKAN_ORDERS );
$vendor_balance->set_particulars( 'test' );
$vendor_balance->save();
$this->assertNotEmpty( $vendor_balance->get_id() );
$this->assertDatabaseCount(
'dokan_vendor_balance', 1, [
- 'id' => $vendor_balance->get_id(),
- // 'debit' => 100,
- 'trn_id' => 1,
- 'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS,
+ 'id' => $vendor_balance->get_id(),
+ 'vendor_id' => 1,
+ 'debit' => 100,
+ 'trn_id' => 1,
+ 'trn_type' => VendorBalance::TRN_TYPE_DOKAN_ORDERS,
]
);
}Likely invalid or redundant comment.
| public function test_delete_method() { | ||
| $vendor_balance = new VendorBalance(); | ||
| $vendor_balance->save(); | ||
| $id = $vendor_balance->get_id(); | ||
| $this->assertNotEmpty( $vendor_balance->get_id() ); | ||
|
|
||
| $vendor_balance->delete(); | ||
|
|
||
| $this->assertDatabaseCount( 'dokan_vendor_balance', 0, [ 'id' => $id ] ); | ||
| } |
There was a problem hiding this comment.
Add required field setup
The test is missing required field setup (vendor_id) which could affect the test's validity.
Apply this diff:
public function test_delete_method() {
$vendor_balance = new VendorBalance();
+ $vendor_balance->set_vendor_id( 1 );
$vendor_balance->save();Committable suggestion skipped: line range outside the PR's diff.
| public function test_update_method() { | ||
| $vendor_balance = new VendorBalance(); | ||
| $vendor_balance->save(); | ||
|
|
||
| $this->assertEmpty( $vendor_balance->get_particulars() ); | ||
|
|
||
| // Change the particulars | ||
| $vendor_balance->set_particulars( 'test123' ); | ||
| $vendor_balance->save(); | ||
|
|
||
| // Assert changes are applied to the database. | ||
| $this->assertDatabaseHas( | ||
| 'dokan_vendor_balance', [ | ||
| 'perticulars' => 'test123', | ||
| ] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and setup
The test is missing:
- Required field setup (vendor_id)
- Initial state verification
- Multiple field updates verification
Apply this diff:
+ /**
+ * Test updating a vendor balance record
+ *
+ * @since 3.x.x
+ * @return void
+ */
public function test_update_method() {
$vendor_balance = new VendorBalance();
+ $vendor_balance->set_vendor_id( 1 );
+ $vendor_balance->set_debit( 100 );
$vendor_balance->save();
+ // Verify initial state
+ $this->assertEquals( 1, $vendor_balance->get_vendor_id() );
+ $this->assertEquals( 100, $vendor_balance->get_debit() );
$this->assertEmpty( $vendor_balance->get_particulars() );
- // Change the particulars
+ // Update multiple fields
+ $vendor_balance->set_vendor_id( 2 );
+ $vendor_balance->set_debit( 200 );
$vendor_balance->set_particulars( 'test123' );
$vendor_balance->save();
// Assert changes are applied to the database.
$this->assertDatabaseHas(
'dokan_vendor_balance', [
- 'perticulars' => 'test123',
+ 'vendor_id' => 2,
+ 'debit' => 200,
+ 'particulars' => 'test123',
]
);
}📝 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 test_update_method() { | |
| $vendor_balance = new VendorBalance(); | |
| $vendor_balance->save(); | |
| $this->assertEmpty( $vendor_balance->get_particulars() ); | |
| // Change the particulars | |
| $vendor_balance->set_particulars( 'test123' ); | |
| $vendor_balance->save(); | |
| // Assert changes are applied to the database. | |
| $this->assertDatabaseHas( | |
| 'dokan_vendor_balance', [ | |
| 'perticulars' => 'test123', | |
| ] | |
| ); | |
| } | |
| /** | |
| * Test updating a vendor balance record | |
| * | |
| * @since 3.x.x | |
| * @return void | |
| */ | |
| public function test_update_method() { | |
| $vendor_balance = new VendorBalance(); | |
| $vendor_balance->set_vendor_id( 1 ); | |
| $vendor_balance->set_debit( 100 ); | |
| $vendor_balance->save(); | |
| // Verify initial state | |
| $this->assertEquals( 1, $vendor_balance->get_vendor_id() ); | |
| $this->assertEquals( 100, $vendor_balance->get_debit() ); | |
| $this->assertEmpty( $vendor_balance->get_particulars() ); | |
| // Update multiple fields | |
| $vendor_balance->set_vendor_id( 2 ); | |
| $vendor_balance->set_debit( 200 ); | |
| $vendor_balance->set_particulars( 'test123' ); | |
| $vendor_balance->save(); | |
| // Assert changes are applied to the database. | |
| $this->assertDatabaseHas( | |
| 'dokan_vendor_balance', [ | |
| 'vendor_id' => 2, | |
| 'debit' => 200, | |
| 'particulars' => 'test123', | |
| ] | |
| ); | |
| } |
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
New Features
BaseModelclass for managing data persistence.VendorBalanceclass to handle vendor balance data with various properties and transaction types.VendorBalanceStoreclass for structured management of vendor balance records.DataStoreInterfacefor defining data management operations.CommissionandOrderclasses to utilize the newVendorBalancemodel for updating vendor balances.Bug Fixes
VendorBalancemodel to ensure correct functionality of save, update, delete, and read operations.Documentation
VendorBalanceclass, detailing parameter and return types.