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

Log unhandled Exceptions into logfile #1482

Merged
merged 6 commits into from
Dec 29, 2017
Merged

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Dec 28, 2017

Q A
Bug fix? No
New feature? Yes
BC breaks? No
Deprecations? No
Fixed tickets #1481

Log unhandled Exceptions into logfile

Declaration of Deployer\Console\Application::renderException(Exception $e, Symfony\Component\Console\Output\OutputInterface $output) should be compatible with Symfony\Component\Console\Application::renderException($e, $output)
@@ -162,4 +163,15 @@ public function afterRun($callable)
{
$this->after = $callable;
}

public function renderException($e, $output)
Copy link
Contributor Author

@staabm staabm Dec 28, 2017

Choose a reason for hiding this comment

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

hmm depeding on the version of symfony console we use (prefer-stable vs. prefer-lowest) php expects a different signature for this method... not sure how to handle this case...

see
https://travis-ci.org/deployphp/deployer/builds/322511665
vs
https://travis-ci.org/deployphp/deployer/builds/322477227

Copy link
Contributor Author

@staabm staabm Dec 28, 2017

Choose a reason for hiding this comment

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

looking at it from a different angle. does it actually make sense that we CI build deployer with php 7.0/7.1/7.2 and prefer-stable vs. prefer-lowser, as the deployer binary/executable itself will finally be bundled with whatever the build process will decide?

it will always contain what prefer-latest will suggest and noone will use this repos in a prefer-lowest fashion, right?

the only setup we need to support is 7.0/7.1/7.2/ with the latest components, since deployer will not be included in other libs/apps as a dependency?

Copy link
Member

Choose a reason for hiding this comment

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

Actually a lot of people use it as dev dependency of composer. And we need to support all of them. Mybe there is a better place to handle this logging. See CommandTask.

@staabm
Copy link
Contributor Author

staabm commented Dec 29, 2017

very good advice, thx for your feedback. adjusted and it seems test are passing now.

@antonmedv antonmedv merged commit 4b2c9fe into deployphp:master Dec 29, 2017
@staabm staabm deleted the exc-logging branch December 29, 2017 08:11
antonmedv pushed a commit that referenced this pull request Apr 18, 2018
* log symfony-console handled exceptions into logfile

* log early bootstrap exceptions into logfile

* updated CHANGELOG.md

* fixed CS

* try to fix php warning

Declaration of Deployer\Console\Application::renderException(Exception $e, Symfony\Component\Console\Output\OutputInterface $output) should be compatible with Symfony\Component\Console\Application::renderException($e, $output)

* log in TaskCommand instead of Application, to prevent x-symfony-console version issues
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.

2 participants