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.

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

change command template to return actual exit code #1006

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

Conversation

LordSimal
Copy link
Contributor

Refs: #950

@LordSimal LordSimal added this to the 3.x (CakePHP 5) milestone Oct 6, 2024
@dereuromark
Copy link
Member

dereuromark commented Oct 6, 2024

I don't like how this is now not in sync with controllers.
Both communication layer classes.
Just to please a wrongly configured cs tool.
Many of us don't have that issue.

It also doesn't reflect reality IMO

*/
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.

@LordSimal
Copy link
Contributor Author

It also doesn't reflect reality IMO

We return success on null as a fallback, sure, but this doesn't mean this doesn't reflect reality.

I just don't like having 3 return types for a command which should always return an exit code since its a CLI tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants