Refactor codebase with DDD CQRS clean architecture#30
Open
Conversation
This comprehensive refactoring introduces Domain-Driven Design (DDD), Command Query Responsibility Segregation (CQRS), and Clean Architecture patterns to the codebase. ## Architecture Layers ### Domain Layer (src/Domain/) - **Shared**: Base classes (Entity, AggregateRoot, ValueObject, DomainEvent, DomainException) - **User**: User aggregate with value objects (UserId, Email, UserName, HashedPassword, AdministratorRole) - **SquidUser**: SquidUser aggregate with value objects (SquidUserId, SquidUsername, SquidPassword, EnabledStatus, Fullname, Comment) - **AllowedIp**: AllowedIp aggregate with value objects (AllowedIpId, IpAddress) - Domain Events for all aggregates (Created, Modified, Deleted, Enabled, Disabled) - Repository Interfaces defining persistence contracts ### Application Layer (src/Application/) - **Commands**: Write operations (Create, Modify, Delete, Enable, Disable) - **Queries**: Read operations (Get, GetAll, GetByOwner, Search) - **Command Handlers**: Execute commands and enforce business rules - **Query Handlers**: Execute queries and return DTOs - **DTOs**: Data transfer objects for application responses ### Infrastructure Layer (src/Infrastructure/) - **Eloquent Repositories**: Implementation of domain repository interfaces - EloquentUserRepository - EloquentSquidUserRepository - EloquentAllowedIpRepository - Persistence adapters that convert between domain entities and Eloquent models ### Configuration - DomainServiceProvider: Registers all repositories and handlers - composer.json: Added Squidmin\ namespace autoloading - config/app.php: Registered DomainServiceProvider ## Key Benefits - Separation of concerns with clear layer boundaries - Business logic encapsulated in domain entities - CQRS pattern for optimized read/write operations - Testable architecture with dependency injection - Domain events for audit trails and side effects - Value objects enforce domain invariants - Repository pattern abstracts persistence details ## Next Steps - Update controllers to use command/query handlers - Implement integration tests for new architecture - Add event dispatching infrastructure
Walkthroughドメイン駆動設計(DDD)アーキテクチャを実装し、User、SquidUser、AllowedIpの3つのドメイン境界を確立。各ドメインについてコマンド・クエリハンドラー、値オブジェクト、アグリゲート、ドメインイベント、リポジトリインターフェースを追加し、Eloquentベースの永続化層で実装。 Changes
Sequence DiagramsequenceDiagram
participant Client as Client Application
participant Handler as Command/Query Handler
participant Repository as Repository
participant Model as Domain Model/Entity
participant DB as Database
rect rgba(100, 200, 150, 0.5)
Note over Client,DB: Command Execution (Create User)
Client->>Handler: execute CreateUserCommand
Handler->>Repository: existsByEmail(Email)
Repository->>DB: SELECT * FROM users
DB-->>Repository: result
Repository-->>Handler: bool
Handler->>Model: User::create(...)
Model-->>Handler: User (with UserCreated event)
Handler->>Repository: save(User)
Repository->>DB: INSERT INTO users
DB-->>Repository: success
Repository-->>Handler: void
Handler-->>Client: success
end
rect rgba(150, 150, 200, 0.5)
Note over Client,DB: Query Execution (Get User)
Client->>Handler: execute GetUserQuery
Handler->>Repository: findById(int)
Repository->>DB: SELECT * FROM users WHERE id = ?
DB-->>Repository: Eloquent Model
Repository->>Repository: toDomain(model)
Repository-->>Handler: User|null
Handler->>Handler: mapToDTO(User)
Handler-->>Client: UserDTO
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 Fix all issues with AI agents
In `@src/Application/Command/AllowedIp/AddAllowedIpCommandHandler.php`:
- Around line 30-32: Replace the global SPL exception with a domain-specific
exception: create a new AllowedIpAlreadyExists class that extends
Squidmin\Domain\Shared\DomainException, and update AddAllowedIpCommandHandler
(the block that checks
$this->allowedIpRepository->existsByIpAndOwnerId($ipAddress, $ownerId)) to throw
AllowedIpAlreadyExists instead of \DomainException with the same message; ensure
the new exception is namespaced/imported where used so it will be catchable by
domain-level handlers.
In `@src/Application/Command/AllowedIp/RemoveAllowedIpCommandHandler.php`:
- Around line 24-27: Replace the direct throwing of SPL \DomainException in
RemoveAllowedIpCommandHandler after $this->allowedIpRepository->findById(...)
returns null with a dedicated domain exception class (e.g., AllowedIpNotFound)
that extends Squidmin\Domain\Shared\DomainException; add the new
AllowedIpNotFound exception class and throw that from
RemoveAllowedIpCommandHandler (and update any use/imports) so the handler uses
the domain-specific exception hierarchy.
In `@src/Application/Command/SquidUser/CreateSquidUserCommandHandler.php`:
- Around line 30-45: The current two-step check (existsByUsername then save) is
race-prone; make the persistence layer enforce a unique constraint and handle
save-time uniqueness errors atomically: keep the SquidUser::create(...) call but
remove reliance on existsByUsername for correctness, wrap
$this->squidUserRepository->save($squidUser) in a try/catch that catches the
repository/DB unique-constraint exception (e.g.,
UniqueConstraintViolationException or your repository’s specific exception), and
rethrow a DomainException (or translate to a meaningful application exception)
indicating the username already exists; mention existsByUsername, save,
CreateSquidUserCommandHandler and squidUserRepository so you update both
persistence schema and the save error handling in that handler.
- Line 39: The SquidPassword value object currently stores plain text; update
SquidPassword to hash on construction and add a static constructor and verifier
(e.g., add fromPlainPassword(string): self that uses password_hash and a
verify(string): bool that uses password_verify), then update where it is created
in CreateSquidUserCommandHandler (replace new
SquidPassword($command->getPassword()) with
SquidPassword::fromPlainPassword($command->getPassword())) so
EloquentSquidUserRepository continues to persist SquidPassword->getValue() but
now it contains the hashed password; ensure method names and signatures match
existing usage and tests.
In `@src/Application/Command/SquidUser/ModifySquidUserCommand.php`:
- Around line 11-18: ModifySquidUserCommand currently requires a password in its
constructor which forces callers to provide a password for any modification;
change the constructor signature in ModifySquidUserCommand so the $password
parameter is nullable (optional) and update the corresponding property
accordingly, then ensure the handler that processes this command (the
ModifySquidUserCommand handler/processor) only updates the stored password when
$command->getPassword() is not null (i.e., check for null before applying a
password change).
In `@src/Application/Command/SquidUser/ModifySquidUserCommandHandler.php`:
- Around line 33-38: The handler always constructs new Fullname and Comment
objects which are never null, causing SquidUser::modify($username, $password,
$fullname, $comment) to always treat fullname/comment as present (because new
Fullname(null) !== null) and overwrite values; update
ModifySquidUserCommandHandler to only instantiate Fullname and Comment when
$command->getFullname() and $command->getComment() are not null/empty (pass null
otherwise), or alternatively change SquidUser::modify to accept and detect
nullable Fullname/Comment types — adjust the call site in
ModifySquidUserCommandHandler to pass either an actual Fullname/Comment instance
or null based on the command getters so the existing $fullname !== null check in
SquidUser::modify works as intended.
In `@src/Application/Command/User/CreateUserCommandHandler.php`:
- Around line 23-44: The handle method in CreateUserCommandHandler currently
checks existsByEmail() before save(), creating a TOCTOU race; wrap the existence
check and $this->userRepository->save($user) inside a database transaction (or
repository-provided atomic operation) so the check+insert is atomic, and
additionally catch the persistence-layer UniqueConstraintViolationException (or
the DB-specific unique violation exception thrown by the repository) around
save() and rethrow a DomainException('User with this email already exists') to
normalize behavior; reference CreateUserCommandHandler::handle,
UserRepository::existsByEmail, UserRepository::save (and nextIdentity()) when
applying the change.
In `@src/Application/Query/SquidUser/SearchSquidUsersQueryHandler.php`:
- Around line 23-31: The handler's current construction of new
UserId($query->getOwnerId()) can throw if ownerId <= 0; update the ownerId
handling in SearchSquidUsersQueryHandler::handle so that you validate
$query->getOwnerId() (e.g. is_int/non-null and > 0) before constructing UserId,
and treat invalid/non-positive values as null (or explicitly throw a clearer
InvalidArgumentException if you prefer); specifically change the ownerId
assignment that references SearchSquidUsersQuery::getOwnerId() to guard against
<= 0 and only call new UserId(...) when the value is a positive integer.
In `@src/Domain/Shared/RepositoryInterface.php`:
- Around line 7-9: RepositoryInterface::nextIdentity currently returning int
allows implementations (e.g., EloquentUserRepository) to return 0 which breaks
domain identity; update the contract and implementations so nextIdentity either
(a) returns a guaranteed non-zero unique identity value (change implementation
in EloquentUserRepository and other repositories to generate a real ID instead
of 0, e.g., DB sequence/UUID/domain ID factory) or (b) explicitly allows “no
identity yet” by changing the signature to return null|int or a domain ID value
object (e.g., ?int or DomainId) and adjust all callers to handle the
nullable/ID-object case; update RepositoryInterface::nextIdentity plus every
class implementing it and any code assuming a non-null non-zero ID (event
emitters, equality checks) to use the new contract.
In `@src/Domain/SquidUser/ValueObject/Comment.php`:
- Around line 13-18: The constructor in Comment::__construct uses strlen() which
counts bytes not characters; replace the strlen check with mb_strlen($value,
'UTF-8') (or mb_strlen($value)) to correctly enforce the 1000-character limit
for multibyte UTF-8 input, keeping the nullable signature and the same
InvalidArgumentException message; ensure the change targets Comment::__construct
and that mbstring is assumed available or a fallback is handled if you prefer.
In `@src/Domain/SquidUser/ValueObject/SquidPassword.php`:
- Around line 31-34: SquidPassword の __toString()
は平文を露出させるので削除するかマスク文字列を返すように変更してください; 対象はクラスのメソッド SquidPassword::__toString（現在
return $this->value;
）で、推奨はこのメソッドを削除して外部からパスワードが平文で取得されないようにし、どうしても文字列化が必要な箇所は明示的に安全なメソッド（例:
getMaskedValue() を追加して固定のマスク文字列を返す）を使うように呼び出し元を修正してください。.ensure: 呼び出し元で
(string)$squidPassword や implicit string conversions
を使っている箇所をすべて検索して置き換え、テストを通すこと。
In `@src/Domain/User/ValueObject/HashedPassword.php`:
- Around line 26-32: In HashedPassword::fromPlainPassword detect when
password_hash returns false and throw a domain-level exception (e.g., a
DomainException or a custom HashedPasswordException) instead of allowing a
TypeError; specifically, call password_hash($plainPassword, PASSWORD_DEFAULT),
check if the result is === false, and if so throw the chosen domain exception
with a clear message, otherwise pass the non-false string to the HashedPassword
constructor as before.
In `@src/Infrastructure/Persistence/Eloquent/EloquentAllowedIpRepository.php`:
- Around line 67-81: The search method in EloquentAllowedIpRepository uses
unvalidated keys from $criteria as column names on
EloquentSquidAllowedIp->where, which allows SQL injection; fix by defining a
whitelist array of allowed column names (e.g.,
['ip_address','description','created_at'] or whatever domain fields apply) and,
inside search(), only apply ->where($field, 'like', "%{$value}%") for fields
present in that whitelist (skip or log/throw for unknown keys), leaving the rest
of the logic (query building, get(), and mapping toDomain) unchanged.
- Line 22: EloquentAllowedIpRepository の withoutGlobalScope
呼び出しは誤ったスコープ名を使っているため、EloquentSquidAllowedIp::withoutGlobalScope('squid_user')->find(...)
から正しいスコープ名に直すか、スコープ側を修正してください: 1) すぐ直すなら
EloquentSquidAllowedIp::withoutGlobalScope('SquidUserScope')->find(...)
に変更する（SquidUserScope のクラス名を指定）または 2) 恒久対応として SquidUserScope クラスに getName()
メソッドを実装して 'squid_user' を返すようにし、既存の withoutGlobalScope('squid_user')
呼び出しをそのまま機能させる（該当する場所は EloquentSquidAllowedIp の withoutGlobalScope 呼び出しと
SquidUserScope クラス）。
In `@src/Infrastructure/Persistence/Eloquent/EloquentSquidUserRepository.php`:
- Around line 116-129: The toDomain method builds DateTimeImmutable from
Eloquent timestamps without null checks, which can throw if
created_at/updated_at are null; update the SquidUser::reconstitute call inside
toDomain (in EloquentSquidUserRepository::toDomain / function toDomain) to
null-safe convert those fields — e.g. if $eloquentSquidUser->created_at or
->updated_at is null pass null (or a safe default) instead of new
\DateTimeImmutable(...), or only call new \DateTimeImmutable(...) when the value
is non-null so the domain reconstitution receives null-safe values.
- Around line 88-102: The search method in EloquentSquidUserRepository accepts
$criteria keys directly into the query which risks SQL injection; update the
search(array $criteria, ?UserId $ownerId = null) implementation to validate each
$field against an explicit whitelist of allowed column names (e.g., columns
defined on the EloquentSquidUser model) before calling $query->where(...), and
either skip invalid fields or throw a validation exception; ensure the whitelist
is defined as a constant or protected property on the repository (or derived
from EloquentSquidUser::getFillable()/getTableColumns) and use that list to gate
the foreach loop that applies ->where($field, 'like', "%{$value}%").
In `@src/Infrastructure/Persistence/Eloquent/EloquentUserRepository.php`:
- Around line 80-90: The search method in EloquentUserRepository uses
unvalidated criteria keys and raw LIKE patterns, so add a whitelist of allowed
searchable fields (e.g., define an array $allowed = ['name','email', ...] inside
search()) and ignore any $field not in that list before calling where; also
escape SQL LIKE wildcard characters in $value (escape % and _ and escape
character if used) and use parameterized where with an explicit ESCAPE clause or
Eloquent's bindings to apply "%{$escapedValue}%" safely; preserve mapping
toDomain($eloquentUser) for results.
🧹 Nitpick comments (18)
src/Application/Query/QueryHandlerInterface.php (1)
7-12: 戻り値の型契約を明確化してください。実装側がDTO/array/nullを返しているため、インターフェースにテンプレート型の@template/@returnを追加すると静的解析で一貫性が取りやすくなります。
♻️ 例: ドックコメントの明確化
- /** - * `@return` mixed - */ + /** + * `@template` TResult + * `@param` QueryInterface $query + * `@return` TResult + */ public function handle(QueryInterface $query);src/Domain/User/ValueObject/Email.php (1)
13-19: メール正規化の方針を明確化してくださいローカルパートまで小文字化すると、RFC上は同一とみなされないアドレスを同一扱いする可能性があります。ケースインセンシティブを前提にするならコメント等で方針を明確化し、そうでなければドメイン部のみ小文字化＋trimの方が安全です。
♻️ 例（ドメインのみ小文字化）
- public function __construct(string $value) + public function __construct(string $value) { - if (!filter_var($value, FILTER_VALIDATE_EMAIL)) { + $value = trim($value); + if (!filter_var($value, FILTER_VALIDATE_EMAIL)) { throw new \InvalidArgumentException(sprintf('Invalid email address: %s', $value)); } - $this->value = strtolower($value); + [$local, $domain] = explode('@', $value, 2); + $this->value = $local . '@' . strtolower($domain); }src/Domain/Shared/DomainEvent.php (1)
11-14: 発生日時はUTC固定が無難です環境のタイムゾーン差異で並びや監査が揺れるので、UTC固定を推奨します。
♻️ 例（UTC固定）
- $this->occurredOn = new \DateTimeImmutable(); + $this->occurredOn = new \DateTimeImmutable('now', new \DateTimeZone('UTC'));src/Domain/SquidUser/ValueObject/Fullname.php (1)
13-18: マルチバイト文字の長さ判定はmb_strlen推奨。
strlenはバイト長なので日本語名などで過剰に弾く可能性があります。mb_strlenに変更して文字数ベースにするのが安全です。♻️ 修正案
- if ($value !== null && strlen($value) > 255) { + if ($value !== null && mb_strlen($value, 'UTF-8') > 255) { throw new \InvalidArgumentException('Fullname cannot exceed 255 characters'); }src/Domain/User/ValueObject/UserName.php (1)
13-21: トリム正規化 + 文字数判定をmb_strlenに。
空白のまま保持される可能性があるため、trim後の値を保持しつつ、文字数はmb_strlenで判定するほうが安全です。♻️ 修正案
- if (empty(trim($value))) { + $value = trim($value); + if ($value === '') { throw new \InvalidArgumentException('User name cannot be empty'); } - if (strlen($value) > 255) { + if (mb_strlen($value, 'UTF-8') > 255) { throw new \InvalidArgumentException('User name cannot exceed 255 characters'); } $this->value = $value;src/Domain/AllowedIp/ValueObject/IpAddress.php (1)
13-19: IP表記ゆれの正規化を検討IPv6の圧縮/非圧縮など表記ゆれがあると、同一IPでも
equalsの一致判定や重複排除が効かない可能性があります。要件がある場合は正規化した値を保持する設計を検討してください。♻️ 例: 文字列表現を正規化して保持
- if (!filter_var($value, FILTER_VALIDATE_IP)) { - throw new \InvalidArgumentException(sprintf('Invalid IP address: %s', $value)); - } - $this->value = $value; + $packed = `@inet_pton`($value); + if ($packed === false) { + throw new \InvalidArgumentException(sprintf('Invalid IP address: %s', $value)); + } + $this->value = inet_ntop($packed);src/Application/Command/SquidUser/DisableSquidUserCommandHandler.php (1)
18-31: 例外型の統一を検討ここで投げている
\DomainExceptionはPHP組み込みで、ドメイン層のSquidmin\Domain\Shared\DomainExceptionとは別系統です。上位でドメイン例外を捕捉する設計なら捕捉漏れになる可能性があるため、専用例外の導入や例外階層の統一を検討してください。src/Infrastructure/Persistence/Eloquent/EloquentSquidUserRepository.php (1)
41-47: リフレクションによるID設定は許容されるパターンですが、代替案も検討してくださいDDDでは、エンティティのIDをリポジトリが設定するためにリフレクションを使用するのは一般的なパターンです。ただし、将来的には
reconstituteメソッドのようなファクトリパターンや、IDを返すsaveメソッドへの変更も検討できます。src/Application/Command/User/DeleteUserCommandHandler.php (1)
29-30: ドメインイベントがディスパッチされていません
$user->delete()はUserDeletedイベントを記録しますが（src/Domain/User/User.phpLines 100-103参照）、pullDomainEvents()が呼ばれないためイベントは消失します。PR概要に「event dispatching infrastructure」が次のステップとして記載されていますが、現時点ではイベントが失われることを認識しておいてください。♻️ イベントディスパッチの準備として
$user->delete(); + // TODO: Dispatch domain events once event infrastructure is in place + // $events = $user->pullDomainEvents(); + // $this->eventDispatcher->dispatch($events); $this->userRepository->delete($user);src/Application/Command/SquidUser/DeleteSquidUserCommandHandler.php (1)
26-26: カスタムDomainExceptionの使用を検討してください。
\DomainException（PHPビルトイン）が使用されていますが、このPRではSquidmin\Domain\Shared\DomainExceptionという抽象基底クラスが導入されています。ドメイン層の例外処理を統一するため、この抽象クラスを継承した具象例外クラス（例：SquidUserNotFoundException）の使用を検討してください。src/Application/Command/SquidUser/ModifySquidUserCommandHandler.php (1)
30-30: カスタムDomainExceptionの使用を検討してください。
DeleteSquidUserCommandHandlerと同様に、ビルトインの\DomainExceptionの代わりに、プロジェクト固有の例外クラスの使用を検討してください。src/Application/Command/User/ModifyUserCommandHandler.php (1)
30-30: カスタムDomainExceptionの使用を検討してください。他のハンドラーと同様に、ビルトインの
\DomainExceptionの代わりに、プロジェクト固有の例外クラス（例：UserNotFoundException）の使用を検討してください。これにより例外の分類と処理が容易になります。src/Domain/SquidUser/SquidUserRepositoryInterface.php (1)
11-39: インターフェース設計は良好です。DDDパターンに従った適切なリポジトリインターフェースです。
AllowedIpRepositoryInterfaceとの一貫性も保たれており、searchメソッドのオプショナルな$ownerIdパラメータも適切に設計されています。ただし、
findById(int $id)がプリミティブ型intを使用している一方、他のメソッドでは値オブジェクト（SquidUsername、UserId）を使用しています。一貫性のため、SquidUserId値オブジェクトの導入を検討してもよいかもしれません。src/Application/Query/User/GetAllUsersQueryHandler.php (1)
28-40:findAll()にページネーションがありません。
findAll()はすべてのユーザーをメモリにロードします。データ量が増加した場合、パフォーマンス問題やメモリ不足を引き起こす可能性があります。将来的にはページネーション対応のクエリ（例：
GetPaginatedUsersQuery）の追加を検討してください。src/Infrastructure/Persistence/Eloquent/EloquentUserRepository.php (1)
45-51: リフレクションによるID設定について。自動インクリメントIDをドメインエンティティに反映するためのリフレクション使用は、DDDでよく見られるパターンです。ただし、カプセル化を破るため、将来的には以下の代替案を検討できます：
- ドメインエンティティに
assignId(int $id)メソッドを追加（内部使用限定）- UUIDベースのIDに移行し、アプリケーション層でID生成
現時点では許容範囲内です。
src/Domain/User/User.php (2)
83-99: 変更がない場合のイベント発行を検討してください。現在の実装では、実際に値が変更されていなくてもUserModifiedイベントが常に記録されます。これは監査ログのノイズや不要な副作用を引き起こす可能性があります。
♻️ 変更があった場合のみイベントを発行する案
public function modify( UserName $name, Email $email, ?HashedPassword $password = null, ?AdministratorRole $role = null ): void { + $hasChanges = !$this->name->equals($name) || !$this->email->equals($email); $this->name = $name; $this->email = $email; if ($password !== null) { $this->password = $password; + $hasChanges = true; } if ($role !== null) { $this->role = $role; + $hasChanges = true; } - $this->updatedAt = new \DateTimeImmutable(); - $this->recordEvent(new UserModified($this->id, $email->getValue(), $name->getValue())); + if ($hasChanges) { + $this->updatedAt = new \DateTimeImmutable(); + $this->recordEvent(new UserModified($this->id, $email->getValue(), $name->getValue())); + } }
106-116: ドメインイベントの一貫性について検討してください。
verifyEmail()とsetRememberToken()は状態を変更しますが、ドメインイベントを記録していません。create、modify、deleteではイベントを発行しているため、一貫性を保つためにこれらのメソッドでもUserEmailVerifiedやUserRememberTokenSetイベントを記録することを検討してください。ただし、これらが監査やリアクティブな処理に不要であれば、現状のままでも問題ありません。
src/Infrastructure/Persistence/Eloquent/EloquentAllowedIpRepository.php (1)
96-105: タイムスタンプの型変換を確認してください。Eloquentモデルの
created_atとupdated_atはCarbonインスタンスです。new \DateTimeImmutable($carbonInstance)に渡すと、Carbonの__toString()が呼ばれますが、タイムゾーン情報が失われる可能性があります。♻️ Carbonから直接DateTimeImmutableに変換する修正案
private function toDomain(EloquentSquidAllowedIp $eloquentAllowedIp): AllowedIp { return AllowedIp::reconstitute( $eloquentAllowedIp->id, new IpAddress($eloquentAllowedIp->ip), new UserId($eloquentAllowedIp->user_id), - new \DateTimeImmutable($eloquentAllowedIp->created_at), - new \DateTimeImmutable($eloquentAllowedIp->updated_at) + \DateTimeImmutable::createFromMutable($eloquentAllowedIp->created_at->toDateTime()), + \DateTimeImmutable::createFromMutable($eloquentAllowedIp->updated_at->toDateTime()) ); }
Comment on lines
+30
to
+32
| if ($this->allowedIpRepository->existsByIpAndOwnerId($ipAddress, $ownerId)) { | ||
| throw new \DomainException('This IP address is already allowed for this user'); | ||
| } |
There was a problem hiding this comment.
ドメイン例外がグローバル \DomainException になっている
Line 31 の \DomainException は SPL 例外で、Squidmin\Domain\Shared\DomainException 系で統一したい場合に捕捉されません。専用のドメイン例外（例: AllowedIpAlreadyExists）を定義して投げる方が一貫します。
🧩 修正案（専用例外を使用）
use Squidmin\Domain\AllowedIp\AllowedIp;
use Squidmin\Domain\AllowedIp\AllowedIpRepositoryInterface;
use Squidmin\Domain\AllowedIp\ValueObject\IpAddress;
+use Squidmin\Domain\AllowedIp\Exception\AllowedIpAlreadyExists;
use Squidmin\Domain\User\ValueObject\UserId;
@@
- throw new \DomainException('This IP address is already allowed for this user');
+ throw new AllowedIpAlreadyExists($ipAddress, $ownerId);※ AllowedIpAlreadyExists は Squidmin\Domain\Shared\DomainException を継承するクラスとして新規追加してください。
🤖 Prompt for AI Agents
In `@src/Application/Command/AllowedIp/AddAllowedIpCommandHandler.php` around
lines 30 - 32, Replace the global SPL exception with a domain-specific
exception: create a new AllowedIpAlreadyExists class that extends
Squidmin\Domain\Shared\DomainException, and update AddAllowedIpCommandHandler
(the block that checks
$this->allowedIpRepository->existsByIpAndOwnerId($ipAddress, $ownerId)) to throw
AllowedIpAlreadyExists instead of \DomainException with the same message; ensure
the new exception is namespaced/imported where used so it will be catchable by
domain-level handlers.
Comment on lines
+24
to
+27
| $allowedIp = $this->allowedIpRepository->findById($command->getAllowedIpId()); | ||
| if ($allowedIp === null) { | ||
| throw new \DomainException('Allowed IP not found'); | ||
| } |
There was a problem hiding this comment.
Allowed IP not found もドメイン例外階層で統一したい
Line 26 の \DomainException は SPL 例外のため、ドメイン例外の捕捉方針とズレます。AllowedIpNotFound のような専用例外へ統一を推奨します。
🧩 修正案（専用例外を使用）
use Squidmin\Application\Command\CommandHandlerInterface;
use Squidmin\Application\Command\CommandInterface;
use Squidmin\Domain\AllowedIp\AllowedIpRepositoryInterface;
+use Squidmin\Domain\AllowedIp\Exception\AllowedIpNotFound;
@@
- throw new \DomainException('Allowed IP not found');
+ throw new AllowedIpNotFound($command->getAllowedIpId());※ AllowedIpNotFound は Squidmin\Domain\Shared\DomainException を継承するクラスとして新規追加してください。
🤖 Prompt for AI Agents
In `@src/Application/Command/AllowedIp/RemoveAllowedIpCommandHandler.php` around
lines 24 - 27, Replace the direct throwing of SPL \DomainException in
RemoveAllowedIpCommandHandler after $this->allowedIpRepository->findById(...)
returns null with a dedicated domain exception class (e.g., AllowedIpNotFound)
that extends Squidmin\Domain\Shared\DomainException; add the new
AllowedIpNotFound exception class and throw that from
RemoveAllowedIpCommandHandler (and update any use/imports) so the handler uses
the domain-specific exception hierarchy.
Comment on lines
+30
to
+45
| $username = new SquidUsername($command->getUsername()); | ||
|
|
||
| if ($this->squidUserRepository->existsByUsername($username)) { | ||
| throw new \DomainException('Squid user with this username already exists'); | ||
| } | ||
|
|
||
| $squidUser = SquidUser::create( | ||
| $this->squidUserRepository->nextIdentity(), | ||
| $username, | ||
| new SquidPassword($command->getPassword()), | ||
| new UserId($command->getOwnerId()), | ||
| new Fullname($command->getFullname()), | ||
| new Comment($command->getComment()) | ||
| ); | ||
|
|
||
| $this->squidUserRepository->save($squidUser); |
There was a problem hiding this comment.
existsByUsername→save の二段階チェックは競合に弱いです
同時作成で重複をすり抜ける可能性があるため、永続層の一意制約＋保存時の例外ハンドリングで原子性を担保してください。
🤖 Prompt for AI Agents
In `@src/Application/Command/SquidUser/CreateSquidUserCommandHandler.php` around
lines 30 - 45, The current two-step check (existsByUsername then save) is
race-prone; make the persistence layer enforce a unique constraint and handle
save-time uniqueness errors atomically: keep the SquidUser::create(...) call but
remove reliance on existsByUsername for correctness, wrap
$this->squidUserRepository->save($squidUser) in a try/catch that catches the
repository/DB unique-constraint exception (e.g.,
UniqueConstraintViolationException or your repository’s specific exception), and
rethrow a DomainException (or translate to a meaningful application exception)
indicating the username already exists; mention existsByUsername, save,
CreateSquidUserCommandHandler and squidUserRepository so you update both
persistence schema and the save error handling in that handler.
| $squidUser = SquidUser::create( | ||
| $this->squidUserRepository->nextIdentity(), | ||
| $username, | ||
| new SquidPassword($command->getPassword()), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'SquidPassword.php' -a -x cat -n {}Repository: 39ff/squid-db-auth-web
Length of output: 1033
🏁 Script executed:
cat -n src/Application/Command/SquidUser/CreateSquidUserCommandHandler.phpRepository: 39ff/squid-db-auth-web
Length of output: 1993
🏁 Script executed:
fd 'SquidUser.php' src/Domain -x cat -n {}Repository: 39ff/squid-db-auth-web
Length of output: 6160
🏁 Script executed:
rg 'hash|Hash|crypt|password' --type php -i -A 3 -B 1Repository: 39ff/squid-db-auth-web
Length of output: 50378
SquidPassword クラスでパスワードがハッシュ化されていない重大なセキュリティ脆弱性
SquidPassword クラスは平文のパスワードをそのまま保存しており、ハッシュ化が一切行われていません。EloquentSquidUserRepository でも getValue() の平文がそのままデータベースに保存されます。
コードベースには User クラス向けに HashedPassword という適切に実装された ValueObject が存在します（password_hash() と password_verify() を使用）。SquidPassword に対しても同様のパターンを適用する必要があります：
public static function fromPlainPassword(string $plainPassword): self
{
return new self(password_hash($plainPassword, PASSWORD_DEFAULT));
}
public function verify(string $plainPassword): bool
{
return password_verify($plainPassword, $this->value);
}🤖 Prompt for AI Agents
In `@src/Application/Command/SquidUser/CreateSquidUserCommandHandler.php` at line
39, The SquidPassword value object currently stores plain text; update
SquidPassword to hash on construction and add a static constructor and verifier
(e.g., add fromPlainPassword(string): self that uses password_hash and a
verify(string): bool that uses password_verify), then update where it is created
in CreateSquidUserCommandHandler (replace new
SquidPassword($command->getPassword()) with
SquidPassword::fromPlainPassword($command->getPassword())) so
EloquentSquidUserRepository continues to persist SquidPassword->getValue() but
now it contains the hashed password; ensure method names and signatures match
existing usage and tests.
Comment on lines
+11
to
+18
| public function __construct( | ||
| private readonly int $squidUserId, | ||
| private readonly string $username, | ||
| private readonly string $password, | ||
| private readonly ?string $fullname = null, | ||
| private readonly ?string $comment = null | ||
| ) { | ||
| } |
There was a problem hiding this comment.
修正操作でpasswordが必須になっています。
ModifySquidUserCommandでpasswordが必須パラメータになっていますが、ユーザー名やコメントのみを変更したい場合でもパスワードの指定が必要になります。修正操作では、パスワード変更はオプショナルにするのが一般的です。
🔧 パスワードをオプショナルにする提案
public function __construct(
private readonly int $squidUserId,
private readonly string $username,
- private readonly string $password,
+ private readonly ?string $password = null,
private readonly ?string $fullname = null,
private readonly ?string $comment = null
) {
}対応するハンドラー側でも、$command->getPassword()がnullでない場合のみパスワードを更新するロジックが必要です。
📝 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.
Suggested change
| public function __construct( | |
| private readonly int $squidUserId, | |
| private readonly string $username, | |
| private readonly string $password, | |
| private readonly ?string $fullname = null, | |
| private readonly ?string $comment = null | |
| ) { | |
| } | |
| public function __construct( | |
| private readonly int $squidUserId, | |
| private readonly string $username, | |
| private readonly ?string $password = null, | |
| private readonly ?string $fullname = null, | |
| private readonly ?string $comment = null | |
| ) { | |
| } |
🤖 Prompt for AI Agents
In `@src/Application/Command/SquidUser/ModifySquidUserCommand.php` around lines 11
- 18, ModifySquidUserCommand currently requires a password in its constructor
which forces callers to provide a password for any modification; change the
constructor signature in ModifySquidUserCommand so the $password parameter is
nullable (optional) and update the corresponding property accordingly, then
ensure the handler that processes this command (the ModifySquidUserCommand
handler/processor) only updates the stored password when $command->getPassword()
is not null (i.e., check for null before applying a password change).
|
|
||
| public function save(AllowedIp $allowedIp): void | ||
| { | ||
| $eloquentAllowedIp = EloquentSquidAllowedIp::withoutGlobalScope('squid_user')->find($allowedIp->getId()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SquidUserScope class
fd -t f "SquidUserScope.php" --exec cat {}Repository: 39ff/squid-db-auth-web
Length of output: 598
🏁 Script executed:
#!/bin/bash
# Find EloquentSquidAllowedIp model to understand scope registration
fd -t f "EloquentSquidAllowedIp.php" --exec cat {}Repository: 39ff/squid-db-auth-web
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Find the file under review to see all occurrences
fd -t f "EloquentAllowedIpRepository.php" --exec cat {}Repository: 39ff/squid-db-auth-web
Length of output: 3761
🏁 Script executed:
#!/bin/bash
# Find SquidAllowedIp model
fd -t f "SquidAllowedIp.php" | grep -v EloquentAllowedIpRepository | xargs catRepository: 39ff/squid-db-auth-web
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Also search for any model files in App/Models directory
fd -t f -p "App/Models/*.php" | head -20Repository: 39ff/squid-db-auth-web
Length of output: 48
グローバルスコープ名が不正です。
SquidUserScopeクラスはgetName()メソッドまたは$keyプロパティを定義していないため、SquidAllowedIpモデルでaddGlobalScope(new SquidUserScope())を実行した際、Laravelはスコープ名として'SquidUserScope'（クラス名）を使用します。しかし、withoutGlobalScope('squid_user')は存在しないスコープ名を指定しているため、グローバルスコープが正しく除外されません。
withoutGlobalScope('SquidUserScope')に修正するか、SquidUserScopeクラスでgetName()メソッドを実装して'squid_user'を明示的に定義してください。
🤖 Prompt for AI Agents
In `@src/Infrastructure/Persistence/Eloquent/EloquentAllowedIpRepository.php` at
line 22, EloquentAllowedIpRepository の withoutGlobalScope
呼び出しは誤ったスコープ名を使っているため、EloquentSquidAllowedIp::withoutGlobalScope('squid_user')->find(...)
から正しいスコープ名に直すか、スコープ側を修正してください: 1) すぐ直すなら
EloquentSquidAllowedIp::withoutGlobalScope('SquidUserScope')->find(...)
に変更する（SquidUserScope のクラス名を指定）または 2) 恒久対応として SquidUserScope クラスに getName()
メソッドを実装して 'squid_user' を返すようにし、既存の withoutGlobalScope('squid_user')
呼び出しをそのまま機能させる（該当する場所は EloquentSquidAllowedIp の withoutGlobalScope 呼び出しと
SquidUserScope クラス）。
Comment on lines
+67
to
+81
| public function search(array $criteria, ?UserId $ownerId = null): array | ||
| { | ||
| $query = EloquentSquidAllowedIp::withoutGlobalScope('squid_user'); | ||
|
|
||
| if ($ownerId !== null) { | ||
| $query->where('user_id', $ownerId->getValue()); | ||
| } | ||
|
|
||
| foreach ($criteria as $field => $value) { | ||
| $query->where($field, 'like', "%{$value}%"); | ||
| } | ||
|
|
||
| $eloquentAllowedIps = $query->get(); | ||
| return $eloquentAllowedIps->map(fn($eloquentAllowedIp) => $this->toDomain($eloquentAllowedIp))->all(); | ||
| } |
There was a problem hiding this comment.
SQLインジェクションの脆弱性があります。
$criteria配列のキー（$field）がバリデーションなしで直接クエリに使用されています。攻撃者が任意のカラム名を指定でき、SQLインジェクション攻撃のベクターとなります。
🔒 許可されたフィールドのホワイトリストを追加する修正案
public function search(array $criteria, ?UserId $ownerId = null): array
{
+ $allowedFields = ['ip']; // 検索可能なフィールドをホワイトリスト化
+
$query = EloquentSquidAllowedIp::withoutGlobalScope('squid_user');
if ($ownerId !== null) {
$query->where('user_id', $ownerId->getValue());
}
foreach ($criteria as $field => $value) {
+ if (!in_array($field, $allowedFields, true)) {
+ continue; // または例外をスロー
+ }
$query->where($field, 'like', "%{$value}%");
}
$eloquentAllowedIps = $query->get();
return $eloquentAllowedIps->map(fn($eloquentAllowedIp) => $this->toDomain($eloquentAllowedIp))->all();
}📝 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.
Suggested change
| public function search(array $criteria, ?UserId $ownerId = null): array | |
| { | |
| $query = EloquentSquidAllowedIp::withoutGlobalScope('squid_user'); | |
| if ($ownerId !== null) { | |
| $query->where('user_id', $ownerId->getValue()); | |
| } | |
| foreach ($criteria as $field => $value) { | |
| $query->where($field, 'like', "%{$value}%"); | |
| } | |
| $eloquentAllowedIps = $query->get(); | |
| return $eloquentAllowedIps->map(fn($eloquentAllowedIp) => $this->toDomain($eloquentAllowedIp))->all(); | |
| } | |
| public function search(array $criteria, ?UserId $ownerId = null): array | |
| { | |
| $allowedFields = ['ip']; // 検索可能なフィールドをホワイトリスト化 | |
| $query = EloquentSquidAllowedIp::withoutGlobalScope('squid_user'); | |
| if ($ownerId !== null) { | |
| $query->where('user_id', $ownerId->getValue()); | |
| } | |
| foreach ($criteria as $field => $value) { | |
| if (!in_array($field, $allowedFields, true)) { | |
| continue; // または例外をスロー | |
| } | |
| $query->where($field, 'like', "%{$value}%"); | |
| } | |
| $eloquentAllowedIps = $query->get(); | |
| return $eloquentAllowedIps->map(fn($eloquentAllowedIp) => $this->toDomain($eloquentAllowedIp))->all(); | |
| } |
🤖 Prompt for AI Agents
In `@src/Infrastructure/Persistence/Eloquent/EloquentAllowedIpRepository.php`
around lines 67 - 81, The search method in EloquentAllowedIpRepository uses
unvalidated keys from $criteria as column names on
EloquentSquidAllowedIp->where, which allows SQL injection; fix by defining a
whitelist array of allowed column names (e.g.,
['ip_address','description','created_at'] or whatever domain fields apply) and,
inside search(), only apply ->where($field, 'like', "%{$value}%") for fields
present in that whitelist (skip or log/throw for unknown keys), leaving the rest
of the logic (query building, get(), and mapping toDomain) unchanged.
Comment on lines
+88
to
+102
| public function search(array $criteria, ?UserId $ownerId = null): array | ||
| { | ||
| $query = EloquentSquidUser::withoutGlobalScope('squid_user'); | ||
|
|
||
| if ($ownerId !== null) { | ||
| $query->where('user_id', $ownerId->getValue()); | ||
| } | ||
|
|
||
| foreach ($criteria as $field => $value) { | ||
| $query->where($field, 'like', "%{$value}%"); | ||
| } | ||
|
|
||
| $eloquentSquidUsers = $query->get(); | ||
| return $eloquentSquidUsers->map(fn($eloquentSquidUser) => $this->toDomain($eloquentSquidUser))->all(); | ||
| } |
There was a problem hiding this comment.
SQLインジェクションのリスク：検索条件のフィールド名を検証してください
$criteriaのキー（フィールド名）が検証されずにクエリに渡されています。悪意のあるフィールド名が渡された場合、予期しないカラムへのアクセスやSQLインジェクションの可能性があります。
🔒 許可されたフィールドのホワイトリストを使用した修正案
public function search(array $criteria, ?UserId $ownerId = null): array
{
$query = EloquentSquidUser::withoutGlobalScope('squid_user');
if ($ownerId !== null) {
$query->where('user_id', $ownerId->getValue());
}
+ $allowedFields = ['user', 'fullname', 'comment'];
foreach ($criteria as $field => $value) {
+ if (!in_array($field, $allowedFields, true)) {
+ continue;
+ }
$query->where($field, 'like', "%{$value}%");
}
$eloquentSquidUsers = $query->get();
return $eloquentSquidUsers->map(fn($eloquentSquidUser) => $this->toDomain($eloquentSquidUser))->all();
}🤖 Prompt for AI Agents
In `@src/Infrastructure/Persistence/Eloquent/EloquentSquidUserRepository.php`
around lines 88 - 102, The search method in EloquentSquidUserRepository accepts
$criteria keys directly into the query which risks SQL injection; update the
search(array $criteria, ?UserId $ownerId = null) implementation to validate each
$field against an explicit whitelist of allowed column names (e.g., columns
defined on the EloquentSquidUser model) before calling $query->where(...), and
either skip invalid fields or throw a validation exception; ensure the whitelist
is defined as a constant or protected property on the repository (or derived
from EloquentSquidUser::getFillable()/getTableColumns) and use that list to gate
the foreach loop that applies ->where($field, 'like', "%{$value}%").
Comment on lines
+116
to
+129
| private function toDomain(EloquentSquidUser $eloquentSquidUser): SquidUser | ||
| { | ||
| return SquidUser::reconstitute( | ||
| $eloquentSquidUser->id, | ||
| new SquidUsername($eloquentSquidUser->user), | ||
| new SquidPassword($eloquentSquidUser->password), | ||
| new EnabledStatus((bool) $eloquentSquidUser->enabled), | ||
| new Fullname($eloquentSquidUser->fullname), | ||
| new Comment($eloquentSquidUser->comment), | ||
| new UserId($eloquentSquidUser->user_id), | ||
| new \DateTimeImmutable($eloquentSquidUser->created_at), | ||
| new \DateTimeImmutable($eloquentSquidUser->updated_at) | ||
| ); | ||
| } |
There was a problem hiding this comment.
潜在的なNull参照エラー：タイムスタンプのnullチェックを追加してください
Eloquentモデルのcreated_atやupdated_atがnullの場合、DateTimeImmutableのコンストラクタが例外をスローする可能性があります。
💡 Nullセーフな変換の提案
private function toDomain(EloquentSquidUser $eloquentSquidUser): SquidUser
{
return SquidUser::reconstitute(
$eloquentSquidUser->id,
new SquidUsername($eloquentSquidUser->user),
new SquidPassword($eloquentSquidUser->password),
new EnabledStatus((bool) $eloquentSquidUser->enabled),
new Fullname($eloquentSquidUser->fullname),
new Comment($eloquentSquidUser->comment),
new UserId($eloquentSquidUser->user_id),
- new \DateTimeImmutable($eloquentSquidUser->created_at),
- new \DateTimeImmutable($eloquentSquidUser->updated_at)
+ $eloquentSquidUser->created_at ? new \DateTimeImmutable($eloquentSquidUser->created_at) : new \DateTimeImmutable(),
+ $eloquentSquidUser->updated_at ? new \DateTimeImmutable($eloquentSquidUser->updated_at) : new \DateTimeImmutable()
);
}📝 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.
Suggested change
| private function toDomain(EloquentSquidUser $eloquentSquidUser): SquidUser | |
| { | |
| return SquidUser::reconstitute( | |
| $eloquentSquidUser->id, | |
| new SquidUsername($eloquentSquidUser->user), | |
| new SquidPassword($eloquentSquidUser->password), | |
| new EnabledStatus((bool) $eloquentSquidUser->enabled), | |
| new Fullname($eloquentSquidUser->fullname), | |
| new Comment($eloquentSquidUser->comment), | |
| new UserId($eloquentSquidUser->user_id), | |
| new \DateTimeImmutable($eloquentSquidUser->created_at), | |
| new \DateTimeImmutable($eloquentSquidUser->updated_at) | |
| ); | |
| } | |
| private function toDomain(EloquentSquidUser $eloquentSquidUser): SquidUser | |
| { | |
| return SquidUser::reconstitute( | |
| $eloquentSquidUser->id, | |
| new SquidUsername($eloquentSquidUser->user), | |
| new SquidPassword($eloquentSquidUser->password), | |
| new EnabledStatus((bool) $eloquentSquidUser->enabled), | |
| new Fullname($eloquentSquidUser->fullname), | |
| new Comment($eloquentSquidUser->comment), | |
| new UserId($eloquentSquidUser->user_id), | |
| $eloquentSquidUser->created_at ? new \DateTimeImmutable($eloquentSquidUser->created_at) : new \DateTimeImmutable(), | |
| $eloquentSquidUser->updated_at ? new \DateTimeImmutable($eloquentSquidUser->updated_at) : new \DateTimeImmutable() | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@src/Infrastructure/Persistence/Eloquent/EloquentSquidUserRepository.php`
around lines 116 - 129, The toDomain method builds DateTimeImmutable from
Eloquent timestamps without null checks, which can throw if
created_at/updated_at are null; update the SquidUser::reconstitute call inside
toDomain (in EloquentSquidUserRepository::toDomain / function toDomain) to
null-safe convert those fields — e.g. if $eloquentSquidUser->created_at or
->updated_at is null pass null (or a safe default) instead of new
\DateTimeImmutable(...), or only call new \DateTimeImmutable(...) when the value
is non-null so the domain reconstitution receives null-safe values.
Comment on lines
+80
to
+90
| public function search(array $criteria): array | ||
| { | ||
| $query = EloquentUser::query(); | ||
|
|
||
| foreach ($criteria as $field => $value) { | ||
| $query->where($field, 'like', "%{$value}%"); | ||
| } | ||
|
|
||
| $eloquentUsers = $query->get(); | ||
| return $eloquentUsers->map(fn($eloquentUser) => $this->toDomain($eloquentUser))->all(); | ||
| } |
There was a problem hiding this comment.
searchメソッドにセキュリティ上の懸念があります。
$criteria配列のキー（フィールド名）をそのままクエリに使用しています。Eloquentはパラメータバインディングを使用するため、$valueのSQLインジェクションリスクは低いですが、以下の懸念があります：
- フィールド名の検証なし: 攻撃者が任意のカラム名（例：
password）を指定してデータを探索できる可能性 - ワイルドカードインジェクション:
%や_を含む入力でワイルドカード攻撃が可能
🔒 許可フィールドのホワイトリスト実装の提案
public function search(array $criteria): array
{
$query = EloquentUser::query();
+ $allowedFields = ['name', 'email'];
foreach ($criteria as $field => $value) {
+ if (!in_array($field, $allowedFields, true)) {
+ continue;
+ }
+ $value = addcslashes($value, '%_');
$query->where($field, 'like', "%{$value}%");
}
$eloquentUsers = $query->get();
return $eloquentUsers->map(fn($eloquentUser) => $this->toDomain($eloquentUser))->all();
}📝 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.
Suggested change
| public function search(array $criteria): array | |
| { | |
| $query = EloquentUser::query(); | |
| foreach ($criteria as $field => $value) { | |
| $query->where($field, 'like', "%{$value}%"); | |
| } | |
| $eloquentUsers = $query->get(); | |
| return $eloquentUsers->map(fn($eloquentUser) => $this->toDomain($eloquentUser))->all(); | |
| } | |
| public function search(array $criteria): array | |
| { | |
| $query = EloquentUser::query(); | |
| $allowedFields = ['name', 'email']; | |
| foreach ($criteria as $field => $value) { | |
| if (!in_array($field, $allowedFields, true)) { | |
| continue; | |
| } | |
| $value = addcslashes($value, '%_'); | |
| $query->where($field, 'like', "%{$value}%"); | |
| } | |
| $eloquentUsers = $query->get(); | |
| return $eloquentUsers->map(fn($eloquentUser) => $this->toDomain($eloquentUser))->all(); | |
| } |
🤖 Prompt for AI Agents
In `@src/Infrastructure/Persistence/Eloquent/EloquentUserRepository.php` around
lines 80 - 90, The search method in EloquentUserRepository uses unvalidated
criteria keys and raw LIKE patterns, so add a whitelist of allowed searchable
fields (e.g., define an array $allowed = ['name','email', ...] inside search())
and ignore any $field not in that list before calling where; also escape SQL
LIKE wildcard characters in $value (escape % and _ and escape character if used)
and use parameterized where with an explicit ESCAPE clause or Eloquent's
bindings to apply "%{$escapedValue}%" safely; preserve mapping
toDomain($eloquentUser) for results.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This comprehensive refactoring introduces Domain-Driven Design (DDD), Command Query Responsibility Segregation (CQRS), and Clean Architecture patterns to the codebase.
Architecture Layers
Domain Layer (src/Domain/)
Application Layer (src/Application/)
Infrastructure Layer (src/Infrastructure/)
Configuration
Key Benefits
Next Steps
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.