-
Notifications
You must be signed in to change notification settings - Fork 128
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
Add console command metadata #248
Conversation
None of this code is laravel specific actually, so could go into the php notifier? |
src/Request/ConsoleRequest.php
Outdated
*/ | ||
public function __construct() | ||
{ | ||
$this->command = $_SERVER['argv']; |
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.
If we do want to go laravel specific, we should never access "super globals" in library files.
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.
You're right, there's a more laravel way to do it with the Request object. I'll check that the way to determine cli vs http in base PHP works across laravel and symfony and implement in the base notifier instead. I'll close this for now.
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.
You're right, there's a more laravel way to do it with the Request object.
You can actually use the symfony console application object. :)
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.
This is how Laravel handles commands internally.
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.
In terms of generalising it as laravel (& symfony) uses it's own resolver object by default I think it'll be more appropriate to follow the existing pattern and make Laravel/Symfony/PHP specific console handlers with a look to generalising further down the line at some point
…bugsnag-laravel into cawllec/notify-console-commands
Re-opened as a laravel specific implementation of console metadata using Laravel objects. Will look at generic and symfony specific console notifiers separately |
Closing and refactoring to reflect this change |
Following the release of bugsnag-php v3.7.0 this request now uses the console request class from the main library to notify console issues. |
I've added some safety when retrieving console args, ensuring the arguments are in a suitable format. |
Seems some extra commits got in here by mistake? Squash/rebase in order? |
This should've gone out yesterday, I'll squash and merge this and release it today |
Thanks for the updates; however, from what I have checked, the changes may not work, at least it doesn't work for our installation (Lumen 5.4 + Dingo). Main difference between method \Bugsnag\Request\BasicResolver::resolve() and \Bugsnag\BugsnagLaravel\Request\LaravelResolver::resolve() is that:
When running Artisan command, the \Illuminate\Http\Request request object exists in the application object but has no any request data populated in it, so \Illuminate\Http\Request->$server, \Illuminate\Http\Request->$cookies, etc are all empty. The unit tests only check if return object is a \Bugsnag\Request\ConsoleRequest object or not; however, it doesn't tell if $_SERVER['argv'] is passed in properly into that object or not. I only checked/tested it under Lumen 5.4. I didn't check the changes with Laravel installation. |
Added ConsoleRequest class to ensure console command metadata is send to bugsnag: #211