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

WIP: DT-682: Fixes #3811: Failing commands throw exception by default. #3839

Closed
wants to merge 7 commits into from

Conversation

danepowell
Copy link
Contributor

@danepowell danepowell commented Sep 4, 2019

Fixes #3811

I'm not sure about this approach as it requires a lot of refactoring, and will almost certainly cause at least minor regressions.

To recap the problem, Robo's CommandStack doesn't throw any exceptions if a command (drush, composer, etc...) returns a non-zero exit code. It just captures and returns the error code. So every time BLT calls a command, it has to manually check and handle the exit code. This has led to many bugs in the past where exit codes are not checked and BLT doesn't throw an error as expected.

This "fixes" that by overriding all of Robo's built-in tasks to throw an exception if the task fails.

In terms of regressions, the biggest risk with this change comes from the fact that commands will now throw an exception and stop the process by default when they fail, unless you specifically wrap them in a try/catch (duh, this is the whole point). I know that there are some commands that we run (such as drush cr) that we don't really mind if they fail, now they are going to start halting unexpectedly. Plus, it's just a big change with lots of unknown side effects.

In terms of refactoring/code complexity, this requires that we override every Robo task individually. It generally just goes against the way Robo is architected. And because of this, it won't help if in the future someone uses a Robo task that we haven't yet overridden. The whole point of this user story is to make commands strictly handle errors universally, so in that regard this approach still falls short.

Ideally Robo would fix this upstream and save us all this trouble: consolidation/robo#891. Maybe I'll ping the maintainer. As a short-term solution that's much less effort, we could just perform a code audit to try to catch any other instances where we aren't checking exit codes, and perform spot-fixes instead of changing the entire architecture.

@danepowell danepowell changed the title DT-682: Fixes #3811: Failing commands throw exception by default. WIP: DT-682: Fixes #3811: Failing commands throw exception by default. Sep 4, 2019
@danepowell danepowell closed this Sep 9, 2019
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.

DT-682: Failing commands should throw an exception
1 participant