Skip to content
This repository has been archived by the owner on Dec 18, 2023. It is now read-only.

feat: add Silex runtime version reporting #27

Merged
merged 1 commit into from
Jun 26, 2019

Conversation

tomlongridge
Copy link
Contributor

Uses new hook in Configuration to append the Symfony and Silex application versions to device data to make it reportable.

Goal

Augments reporting with the version of Symfony and Silex in use when an event occurred in case of issues in a specific version of the framework.

Design

Added data to device.runtimeVersions.symfony and .silex as per other notifiers.

Changeset

Changed

  • AbstractServiceProvider.makeClient - appends the Symfony and Silex versions from the $app and HttpKernel classes to the device data through the Bugsnag client

Tests

  • No unit tests present for this level of testing - checked the data was present after running the example silex2 project

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@tomlongridge tomlongridge marked this pull request as ready for review May 23, 2019 12:49
Uses new hook in Configuration to append the Symphony and Silex application versions to the device
data to make it reportable.
@tomlongridge tomlongridge changed the base branch from master to next June 12, 2019 16:19
Copy link

@mikeewhite mikeewhite left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1,6 +1,11 @@
Changelog
=========

## TBD

* Add Symfony/Silex version string to report and session payloads (device.runtimeVersions)

Choose a reason for hiding this comment

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

We don't currently support session tracking in Symfony/Silex

@tomlongridge tomlongridge merged commit 4d4f2dd into next Jun 26, 2019
@tomlongridge tomlongridge deleted the tom/php-runtime-versions branch June 26, 2019 09:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants