Skip to content
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.

Sign up for GitHub

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

Jump to bottom

change command template to return actual exit code #1006

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from
Open

change command template to return actual exit code #1006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions templates/bake/Command/command.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,10 @@ class {{ name }}Command extends Command
*
* @param \Cake\Console\Arguments $args The command arguments.
* @param \Cake\Console\ConsoleIo $io The console io
* @return int|null|void The exit code or null for success
* @return int The exit code
*/
public function execute(Arguments $args, ConsoleIo $io)
public function execute(Arguments $args, ConsoleIo $io): int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function execute(Arguments $args, ConsoleIo $io): int
public function execute(Arguments $args, ConsoleIo $io): ?int

Wouldn't this be more correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather we keep it int only so that in a future release we can add int type to our base class Command::execute() itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldnt this start with core though
Baked Code by no means would be a reliable way of going into that direction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This kind of change can only be enforced in core in 6.x anyway.
With this change in bake it at least encourages new commands being generated to use this kind of setup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is requiring an explicit return value for commands going to make userland code better? I can see how it makes the framework code simpler, but does it also benefit end users?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I very much doubt that.
That said: Symfony went into that direction and the new versions require a non void (maybe even integer) return code.

My point here is that bake is the end of the chain - and narrowing it here to just int seems not like the best idea if there was no broader discussion on core changes yet.

{
return Command::CODE_SUCCESS;
}
}
5 changes: 3 additions & 2 deletions tests/comparisons/Command/testBakePlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ public function buildOptionParser(ConsoleOptionParser $parser): ConsoleOptionPar
*
* @param \Cake\Console\Arguments $args The command arguments.
* @param \Cake\Console\ConsoleIo $io The console io
* @return int|null|void The exit code or null for success
* @return int The exit code
*/
public function execute(Arguments $args, ConsoleIo $io)
public function execute(Arguments $args, ConsoleIo $io): int
{
return Command::CODE_SUCCESS;
}
}