Runtime information #564
Runtime information #564
Conversation
lib/Raven/Client.php
Outdated
| @@ -884,6 +884,8 @@ public function capture($data, $stack = null, $vars = null) | |||
| if (empty($data['site'])) { | |||
| unset($data['site']); | |||
| } | |||
| $data['contexts']['runtime'] = array('version' => phpversion(), 'name' => 'php'); | |||
There was a problem hiding this comment.
The blank line added below this line should probably go above. Also, maybe it would be better to have the php string in uppercase. Last thing, the most important: please use the PHP_VERSION constant as it's faster because we don't have the overhead of a function call
There was a problem hiding this comment.
'php' should not be uppercased as this is a value sent to the Sentry server and is reflective on the actual platform (it generally should always be lowercased)
There was a problem hiding this comment.
Not that I want to open a useless debate about the naming case of a string, but PHP is an acronym and I usually see it written uppercase (even in the header of the phpinfo) and honestly I find PHP 1.2.3 prettier than php 1.2.3 😄 By the way, it's not an important thing so let's go ahead
There was a problem hiding this comment.
That string is strictly validated on server side, so we cannot change it, see the docs about this and the related API JSON schema.
Lowercase it is then.
|
@ste93cry feedback adressed. |
|
Thank you for the good work, patch looks good for me |
lib/Raven/Client.php
Outdated
| @@ -885,6 +885,8 @@ public function capture($data, $stack = null, $vars = null) | |||
| unset($data['site']); | |||
| } | |||
|
|
|||
| $data['contexts']['runtime'] = array('version' => PHP_VERSION, 'name' => 'php'); | |||
There was a problem hiding this comment.
Shouldn't this be merged with a possible user value in the runtime context?
Also there is an extra space character before array, non-important but while you are there 😉
test/Raven/Tests/ClientTest.php
Outdated
| $events = $client->getSentEvents(); | ||
| $this->assertEquals(PHP_VERSION, $events[0]['contexts']['runtime']['version']); | ||
| $event = array_pop($events); | ||
| $this->assertEquals('php', $event['contexts']['runtime']['name']); |
There was a problem hiding this comment.
Maybe also add an test what happens if the context was set by an user (for the runtime) it correctly merges with our runtime context.
|
@stayallive feedback addressd with: 59909d7 verifies that custom |
There was a problem hiding this comment.
Great work, thanks!
I would only request to add the last test, as @stayallive requested too, to test that the user is still able to override the passed values; thanks to the array_merge the code should be already ok on that respect.
* fix 5.3 array() vs [] * change array_merge order so [runtime][name|version] is overrideable * add test for overriden runtime
|
@Jean85 thx! - added a test to verify that one can override had to change the order of the |
(
|
thx, you folks rock! 🍺 |
|
you want me to port it into 1.8 branch ? really would love to see this released. |
|
@hjanuschka that's not necessary, the master branch is still for the 1.x series. The porting to that branch is done when tagging and releasing. |
as discussed here: #562
this one adds a context containing the current php version.
running the
examples/vanilla/index.phpresults in a exception like this:( i wanted to upload via github (seems to 503 right now, hope they have sentry))
https://imgur.com/Hs6FDYm
i really would appreciate if we can get this in an 1.8.* release (atleast a release in a short time) - i really would love to have this in production. let me know if you need me to backport it somewhere.