Cawllec/console request #398

Cawllec · 2017-08-24T10:51:55Z

Re this issue in bugsnag-laravel this implements a basic console-request class that will take an array of the console commands and attach them as meta-data.

This gives a handler for framework implementations, while leaving it framework-specific on how console input is acquired.

GrahamCampbell · 2017-08-24T19:57:39Z

src/Request/ConsoleRequest.php

+    /**
+     * Create a new console request instance.
+     *
+     * @param array     $commandArray An array of the command line input


Alignment bug. :)

Is this something StyleCI could lint for?

Not yet, but it is planned. I think it's actually been planned for a very long time, since I think you mentioned this feature last year too. :P

Haha, explains the deja vu I felt when I typed that :p

GrahamCampbell · 2017-08-24T19:58:14Z

src/Request/ConsoleRequest.php

+    /**
+     * The unformated console command.
+     *
+     * @var array


string[] I think is the type here?

They're allegedly the same, but string[] is more explicit to the expected array elements

GrahamCampbell · 2017-08-24T19:58:33Z

src/Request/ConsoleRequest.php

+     *
+     * @return void
+     */
+    public function __construct(array $commandArray)


Conventional to match the internal property name, so name the param $command?

GrahamCampbell · 2017-08-24T19:59:24Z

src/Request/BasicResolver.php

@@ -12,7 +12,11 @@ class BasicResolver implements ResolverInterface
    public function resolve()
    {
        if (!isset($_SERVER['REQUEST_METHOD'])) {
-            return new NullRequest();
+            if ((PHP_SAPI == 'cli') && (isset($_SERVER['argv']))) {


The extra brackets are not needed, and === should be used for the string comparison.

GrahamCampbell · 2017-08-24T20:01:54Z

src/Request/ConsoleRequest.php

+     */
+    public function getContext()
+    {
+    }


return implode(' ', $this->command) perhaps would be a good context, or perhaps, limit this to only the first upto 2 parts of the array?

The first two parts would be good, running this with artisan commands as well as plain php produces useful results, moreso than the default which is the top line of the stacktrace.

This code will return at most the first 2 parts of the command, concatenated together with a space. If the array is smaller than 2 elements, it is tolerant to that, and will just return the singleton array, or the empty array.

return implode(' ', array_slice($this->command, 0, 2));

GrahamCampbell · 2017-08-24T20:02:24Z

Reviewed this in some detail. :)

GrahamCampbell · 2017-08-24T20:04:06Z

src/Request/ConsoleRequest.php

+     */
+    public function isRequest()
+    {
+        return true;


This probably has to be false, otherwise, this pull request would be considered a breaking change. Previously, "request" meant "HTTP request", not just "invocation context", or whatever name we wanna give to this. :P

I agree, to avoid potential issues :)

GrahamCampbell · 2017-08-25T19:28:48Z

src/Request/ConsoleRequest.php

+     */
+    public function getMetaData()
+    {
+        if (is_array($this->command)) {


No need for the is_array check. It is guaranteed to be true. PHP doesn't allow null through an array type.

GrahamCampbell · 2017-08-25T19:32:50Z

src/Request/ConsoleRequest.php

+            $arguments = [];
+            $options = [];
+            foreach (array_slice($this->command, 1) as $arg) {
+                $arg[0] == '-' ? $options[] = $arg : $arguments[] = $arg;


Assignment inside ternaries is somewhat unusual. Perhaps consider just a plain old if. :)

GrahamCampbell · 2017-08-25T19:38:39Z

src/Request/BasicResolver.php

@@ -12,7 +12,11 @@ class BasicResolver implements ResolverInterface
    public function resolve()
    {
        if (!isset($_SERVER['REQUEST_METHOD'])) {
-            return new NullRequest();


This method could be updated to avoid the nested ifs:

/** * Resolve the current request. * * @return \Bugsnag\Request\RequestInterface */ public function resolve() { if (isset($_SERVER['REQUEST_METHOD'])) { return new PhpRequest( $_SERVER, empty($_SESSION) ? [] : $_SESSION, empty($_COOKIE) ? [] : $_COOKIE, static::getRequestHeaders($_SERVER), static::getInputParams($_SERVER, $_POST) ); } if (PHP_SAPI === 'cli' && isset($_SERVER['argv'])) { return new ConsoleRequest($_SERVER['argv']); } return new NullRequest(); }

GrahamCampbell · 2017-08-25T19:46:07Z

src/Request/ConsoleRequest.php

+     */
+    public function getContext()
+    {
+        return implode(' ', array_slice($this->command, 0, 2));


Just realized that php doesn't include the executable in the arguments list. Maybe we should just take the first part of the array then, or implode the whole thing. Ping @kattrali - what do you think is the most sensible option here?

I think first few args still works well enough. The cases I can try/think of:

php [filename] => [filename]

php [filename] foo bar => [filename] foo

php [filename] --foo bar --baz red => [filename] --foo <-- this one is a lil odd

php artisan migrate => artisan migrate

I could see the value in imploding the whole thing, but also don't want context to get arbitrarily long. We could max four args for now and revisit with feedback.

but also don't want context to get arbitrarily long

Yeh, totally. We need to find the balance where it is useful but not unbounded.

…php into cawllec/console-request

GrahamCampbell · 2017-08-29T17:24:35Z

src/Request/ConsoleRequest.php

+        $arguments = [];
+        $options = [];
+        foreach (array_slice($this->command, 1) as $arg) {
+            if ($arg[0] == '-') {


Safest option is to do: if (isset($arg[0]) && $arg[0] === '-') {.

GrahamCampbell · 2017-08-29T17:25:02Z

tests/Callbacks/RequestMetaDataTest.php

+            'Command' => 'some',
+            'Arguments' => ['test', 'command'],
+            'Options' => ['--opt'],
+        ]], $report->getMetaData());


Probably worth also doing an assertion on the report context too.

@GrahamCampbell Should there be a test which is different from this one?

Oh, ha. No, that looks great. ;)

GrahamCampbell · 2017-08-29T17:25:29Z

Left 2 more tiny comments. Otherwise, looks good. 🚀 🚀 🚀

GrahamCampbell · 2017-08-29T20:26:59Z

tests/Callbacks/RequestMetaDataTest.php

+            'Command' => 'some',
+            'Arguments' => ['test', 'command'],
+            'Options' => ['--opt'],
+        ]], $report->getMetaData());


Oh, ha. No, that looks great. ;)

kattrali · 2017-08-29T21:46:52Z

🚢 🚢 🚢

Cawllec and others added 7 commits August 24, 2017 11:08

Added console resolve

bf0ac08

Using faster access parameter

0e656e0

Updated test

270adba

Updated test for console metadata

6ac2e20

Apply fixes from StyleCI (#396)

2df82ab

Updated function dec

eb9eacd

Apply fixes from StyleCI (#397)

db1a161

Cawllec mentioned this pull request Aug 24, 2017

Add console command metadata bugsnag/bugsnag-laravel#248

Merged

GrahamCampbell suggested changes Aug 24, 2017

View reviewed changes

Cawllec and others added 2 commits August 25, 2017 09:53

Updated as per feedback

d3fb8f7

Apply fixes from StyleCI (#399)

9919e46

GrahamCampbell suggested changes Aug 25, 2017

View reviewed changes

GrahamCampbell reviewed Aug 25, 2017

View reviewed changes

GrahamCampbell suggested changes Aug 25, 2017

View reviewed changes

GrahamCampbell reviewed Aug 25, 2017

View reviewed changes

Cawllec and others added 5 commits August 29, 2017 14:35

Logical and readability updates

487b018

Merged

662ea2d

Apply fixes from StyleCI (#400)

913f7fa

Udpated tests accordingly

7e828a6

Merge branch 'cawllec/console-request' of github.com:bugsnag/bugsnag-…

70fcfed

…php into cawllec/console-request

GrahamCampbell suggested changes Aug 29, 2017

View reviewed changes

Check for $arg[0] before validating contents, use stricter eq

ce20826

GrahamCampbell approved these changes Aug 29, 2017

View reviewed changes

kattrali approved these changes Aug 29, 2017

View reviewed changes

kattrali merged commit f234045 into master Aug 29, 2017

kattrali deleted the cawllec/console-request branch August 29, 2017 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cawllec/console request #398

Cawllec/console request #398

Cawllec commented Aug 24, 2017

GrahamCampbell Aug 24, 2017

kattrali Aug 25, 2017

GrahamCampbell Aug 25, 2017

kattrali Aug 25, 2017

GrahamCampbell Aug 24, 2017

Cawllec Aug 25, 2017

GrahamCampbell Aug 24, 2017

GrahamCampbell Aug 24, 2017

GrahamCampbell Aug 24, 2017

kattrali Aug 25, 2017

GrahamCampbell Aug 25, 2017

GrahamCampbell commented Aug 24, 2017

GrahamCampbell Aug 24, 2017

Cawllec Aug 25, 2017

GrahamCampbell Aug 25, 2017

GrahamCampbell Aug 25, 2017

GrahamCampbell Aug 25, 2017

GrahamCampbell Aug 25, 2017

kattrali Aug 25, 2017

GrahamCampbell Aug 25, 2017

GrahamCampbell Aug 29, 2017

GrahamCampbell Aug 29, 2017

kattrali Aug 29, 2017

GrahamCampbell Aug 29, 2017

GrahamCampbell commented Aug 29, 2017

GrahamCampbell Aug 29, 2017

kattrali commented Aug 29, 2017

Cawllec/console request #398

Cawllec/console request #398

Conversation

Cawllec commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamCampbell commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GrahamCampbell commented Aug 29, 2017

Choose a reason for hiding this comment

kattrali commented Aug 29, 2017