-
Notifications
You must be signed in to change notification settings - Fork 0
Backport bugfix: incorrect empty check #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request backports a bugfix to strengthen input validation in the Category and OrderStatus domain models by rejecting whitespace-only parameters. The changes update constructor validation to use trim() before checking for empty strings, preventing whitespace-only values from being accepted.
- Updated validation logic in both
CategoryandOrderStatusconstructors to treat whitespace-only strings as empty - Replaced inconsistent empty checking methods (
empty()vs=== "") with a unifiedtrim() === ''approach
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/BusinessLogic/Domain/OrderStatus/Models/OrderStatus.php | Updated constructor validation to use trim() for both id and name parameters, ensuring whitespace-only values are rejected |
| src/BusinessLogic/Domain/GeneralSettings/Models/Category.php | Updated constructor validation to use trim() for both id and name parameters, aligning with OrderStatus validation approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function __construct(string $id, string $name) | ||
| { | ||
| if ($id === "" || empty($name)) { | ||
| if (trim($id) === '' || trim($name) === '') { |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Consider adding test coverage for whitespace-only strings
The updated validation logic now rejects whitespace-only strings (e.g., ' '), but the existing tests in OrderStatusModelTest only cover completely empty strings ''. Adding test cases for whitespace-only inputs would ensure this bugfix is properly validated.
Example:
public function testWhitespaceOnlyOrderStatusId(): void
{
$this->expectException(EmptyOrderStatusParameterException::class);
new OrderStatus(' ', 'test');
}| public function __construct(string $id, string $name) | ||
| { | ||
| if (empty($id) || empty($name)) { | ||
| if (trim($id) === '' || trim($name) === '') { |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking): Consider adding test coverage for whitespace-only strings
The updated validation logic now rejects whitespace-only strings (e.g., ' '), but the existing tests in CategoryModelTest only cover completely empty strings ''. Adding test cases for whitespace-only inputs would ensure this bugfix is properly validated.
Example:
public function testWhitespaceOnlyCategoryId(): void
{
$this->expectException(EmptyCategoryParameterException::class);
new Category(' ', 'test');
}| public function __construct(string $id, string $name) | ||
| { | ||
| if ($id === "" || empty($name)) { | ||
| if (trim($id) === '' || trim($name) === '') { |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (blocking): Values validated with trim() but stored untrimmed creates data inconsistency
The validation checks trim($id) === '' but then stores the original $id value on line 39 without trimming. This means strings like " test " will pass validation but be stored with leading/trailing whitespace, creating inconsistent data.
Consider trimming the values before storing them:
$this->id = trim($id);
$this->name = trim($name);| public function __construct(string $id, string $name) | ||
| { | ||
| if (empty($id) || empty($name)) { | ||
| if (trim($id) === '' || trim($name) === '') { |
Copilot
AI
Dec 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (blocking): Values validated with trim() but stored untrimmed creates data inconsistency
The validation checks trim($id) === '' and trim($name) === '' but then stores the original values on lines 39-40 without trimming. This means strings like " test " will pass validation but be stored with leading/trailing whitespace, creating inconsistent data.
Consider trimming the values before storing them:
$this->id = trim($id);
$this->name = trim($name);
What is the goal?
This pull request improves validation logic in the constructors for both the
CategoryandOrderStatusdomain models. The main change is ensuring that parameters consisting only of whitespace are now treated as empty, making the input validation more robust.References
How is it being implemented?
Categoryconstructor to usetrim()when checking ifidornameare empty, ensuring that strings containing only whitespace are considered invalid.OrderStatusconstructor to usetrim()for bothidandnameparameters, aligning the validation logic and preventing whitespace-only values.How is it tested?
Manual tests
How is it going to be deployed?
Standard deployment