-
Notifications
You must be signed in to change notification settings - Fork 0
Hopefully improve the instructions #22
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
Signed-off-by: Joey Smith <jsmith@webinertia.net> Signed-off-by: Joey Smith <jsmith@webinertia.net>
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 PR replaces the technical project documentation with user-focused documentation to improve developer onboarding and usage instructions. The change removes detailed internal architecture documentation and replaces it with practical getting-started guidance.
- Removes comprehensive internal project documentation (
project-overview.md) - Adds new user-focused documentation (
copilot-project-overview.md) with installation and usage examples - Shifts focus from internal development to external usage patterns
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
.github/instructions/project-overview.md |
Complete removal of internal project documentation containing architecture details, development standards, and contributor guidelines |
.github/instructions/copilot-project-overview.md |
New comprehensive user guide with installation steps, configuration examples, and practical usage patterns |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ```php | ||
| // config/autoload/cmd-bus.global.php | ||
| return [ | ||
| PhpCmd\CmdBus\ConfigProvider::class => [ |
Copilot
AI
Aug 18, 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.
The configuration structure is inconsistent with typical Laminas/Mezzio patterns. Configuration should be nested under a specific key like 'cmd-bus' rather than using the ConfigProvider class name as the key.
| PhpCmd\CmdBus\ConfigProvider::class => [ | |
| 'cmd-bus' => [ |
|
|
||
| public function setRequest(ServerRequestInterface $request): void | ||
| { | ||
| $this->request = $request; |
Copilot
AI
Aug 18, 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.
The import of UserRepositoryInterface in a Command class suggests tight coupling. Commands should be data containers and shouldn't directly import repository interfaces.
| $this->request = $request; | |
| use PhpCmd\CmdBus\CommandInterface; | |
| final class CreateUserCommand implements CommandInterface | |
| { | |
| private array $userData; | |
| public function __construct(array $userData) | |
| { | |
| $this->userData = $userData; | |
| } | |
| public function getUserData(): array | |
| { | |
| return $this->userData; |
|
|
||
| public function setRequest(ServerRequestInterface $request): void | ||
| { | ||
| $this->request = $request; |
Copilot
AI
Aug 18, 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.
Commands should not receive repositories in their constructor. This violates separation of concerns - commands should be simple data containers, and business logic should be handled in the command handlers.
| $this->request = $request; | |
| public function __construct(ServerRequestInterface $request) | |
| { | |
| $this->request = $request; | |
| } | |
| public function getRequest(): ServerRequestInterface | |
| { | |
| return $this->request; |
| { | ||
| $data = $this->request->getParsedBody(); | ||
| return $this->userRepository->save($data); | ||
| } |
Copilot
AI
Aug 18, 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.
Commands having an 'execute' method contradicts the Command pattern. Commands should be passive data containers, and execution should be handled by command handlers through the 'handle' method.
| } | |
| // Command classes should not contain business logic or an execute method. | |
| // Execution should be handled by a separate handler class. |
|
|
||
| public function handle(CommandInterface $command): User | ||
| { | ||
| return $command->execute(); |
Copilot
AI
Aug 18, 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.
The handler is delegating execution back to the command, which defeats the purpose of the Command pattern. The handler should contain the business logic, not just proxy to the command.
| return $command->execute(); | |
| if (!$command instanceof CreateUserCommand) { | |
| throw new \InvalidArgumentException('Invalid command type'); | |
| } | |
| $userData = $command->getUserData(); | |
| return $this->userRepository->save($userData); |
|
|
||
| $command = new CreateUserCommand(); | ||
| $command->setRequest($request); | ||
|
|
Copilot
AI
Aug 18, 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.
The PreExecuteCommandEvent is dispatched but the command variable is then reassigned to a new CreateUserCommand instance, making the event dispatch ineffective as it operates on a different command instance.
| $command = new CreateUserCommand(); | |
| $command->setRequest($request); | |
| $this->eventDispatcher->dispatch(new PreExecuteCommandEvent($command)); |
| { | ||
| $this->eventDispatcher->dispatch(new PreExecuteCommandEvent($command)); | ||
|
|
||
| $command = new CreateUserCommand(); |
Copilot
AI
Aug 18, 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.
The command parameter is being reassigned to a new instance, which discards the original command that was passed to the handler and makes the previous event dispatch meaningless.
| $command = new CreateUserCommand(); |
No description provided.