Implement ability for users to register custom callbacks via service definition tags. #36

samdjstevens · 2017-03-25T12:29:17Z

This is my attempt at resolving issue #35.

This PR enables custom callbacks to be registered to the Bugsnag Client object, by creating a service and tagging it with the bugsnag.callback name.

The service must implement a registerCallback method, which is used in the same way as the registerCallback method on the Bugsnag client. This method name is definable on a per-service basis, by setting the method attribute on the tag.

I have also re-factored the existing user setting code in ClientFactory to be a callback service included in the bundle.

I have made additions to the example apps in the repo to show an example callback service, but have not written any documentation for the new feature as they seem to live separately.

@PReimers - does this implementation solve your use case?

PReimers · 2017-03-25T17:39:19Z

@samdjstevens doesn't seem to work for me 😞

either I'm doing wrong on implementation or It's just not working.

samdjstevens · 2017-03-25T17:43:25Z

@PReimers Are you getting any errors, or are you just not seeing the callbacks take affect on bugsnag notifications? Could you post your service class and definition configuration so I could take a look?

PReimers · 2017-03-25T18:00:27Z

@samdjstevens

I forked your repo and changed only the composer.json.

in my Symfony project I changed:
composer.json:

"repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/PReimers/bugsnag-symfony.git"
        }
],
"require": {
        "php": ">=5.3.3",
...
        "bugsnag/bugsnag": "^3.4",
        "preimers/bugsnag-symfony": "dev-master"

src/BrokerStar/CommonBundle/Bugsnag/MetadataCallback.php

<?php

namespace BrokerStar\CommonBundle\Bugsnag;

use Bugsnag\Report;

class MetadataCallback
{
    /**
    * @param \Bugsnag\Report $report
    */
    public function registerCallback(Report $report)
    {
        $report->setMetaData([
            'installation' => [
                'url' => $_SERVER['HTTP_HOST'],
                'path' => $_SERVER['REQUEST_URI'],
            ]
        ]);
    }
}

src/BrokerStar/CommonBundle/Resources/config/service.yml

services:
    brokerstar_common.bugsnag.metadatacallback:
        class: BrokerStar\CommonBundle\Bugsnag\MetadataCallback
        tags:
          - { name: bugsnag.callback }

by the way how to customize the setUser function?

samdjstevens · 2017-03-25T18:26:38Z

@PReimers Your setup looks correct - when an exception is being thrown and reported to Bugsnag, do you see any errors, or is the information just missing from the report when you inspect it on the website? Also, what version of Symfony are you using?

I've made a skeleton Symfony app replicating your integration here - which I have verified to be reporting the extra data to Bugsnag on exceptions. Can you scan this and see if there are any differences in your code?

I assume that you are enabling the bundle and specifying the API key in your config?

GrahamCampbell · 2017-03-25T18:41:45Z

We do actually have an example apps in https://github.com/bugsnag/bugsnag-symfony/tree/master/example.

PReimers · 2017-03-25T18:58:09Z

@samdjstevens
when an exception in thrown I see it on bugsnag dashboard but the information added in MetadataCallback.php are missing

samdjstevens · 2017-03-25T19:13:13Z

Just to confirm, you are enabling the bundle, setting your API key, and letting the bundle listener trigger the sending to Bugsnag?

And when clicking through on the excpetion in Bugsnag, there is no "Installation" tab with the custom data, like this?

Can you try downloading my sample project posted above, replacing the api key with yours, and seeing if the extra data is registered?

PReimers · 2017-03-25T19:21:19Z

@samdjstevens your test project is showing the information 😄

but why is it not shown on my project 🤔

btw. should we move the discussion to another place? (like gitter / slack ...)

PReimers · 2017-03-25T20:52:35Z

@samdjstevens @GrahamCampbell - This resolves #35

+1 for merging 👍

GrahamCampbell · 2017-03-25T22:11:12Z

example/symfony27/src/AppBundle/Bugsnag/MetadataCallback.php

+{
+    /**
+     * @param \Bugsnag\Report $report
+     */


Please add @return void.

samdjstevens · 2017-03-26T10:45:06Z

@GrahamCampbell - return tags added.

GrahamCampbell · 2017-03-26T10:50:58Z

Please don't comment any .DS_Store files. I'd recommend adding that to your global ignore file.

samdjstevens · 2017-03-26T10:53:19Z

Whoops, usually catch those. Removed now.

samdjstevens · 2017-03-28T08:46:09Z

hi @GrahamCampbell - anything blocking this PR from merge still?

samdjstevens · 2017-04-13T13:18:27Z

Do you have any timescale of when you think this could be merged into master @GrahamCampbell?

GrahamCampbell · 2017-04-13T13:24:16Z

Not yet, sorry. Need to find the time to sit down and give this a detailed review, and try it out.

GrahamCampbell · 2017-04-13T13:24:53Z

Callbacks/UserSettingCallback.php

+     * @param null|\Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface $tokens
+     * @param null|\Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface        $checker
+     * @param bool                                                                                     $setUser
+     */


Missing return void

GrahamCampbell · 2017-04-13T13:25:26Z

BugsnagBundle.php

@@ -12,4 +14,16 @@ class BugsnagBundle extends Bundle
     * @return string
     */
    const VERSION = '1.0.0';
+
+    /**
+     * {@inheritdoc}


Please just copy over the description rather than inherit. Makes it easy for a human to read the code and know what it does straight away.

The description here would literally be "Builds the bundle." - is this really helpful? Anyone who isn't already familiar with Symfony, and what this file is for, would not really be looking at it.

A better description would be "Setup the callback registering pass." or something.

GrahamCampbell · 2017-04-13T19:31:27Z

BugsnagBundle.php

@@ -12,4 +14,16 @@ class BugsnagBundle extends Bundle
     * @return string
     */
    const VERSION = '1.0.0';
+
+    /**
+     * {@inheritdoc}


A better description would be "Setup the callback registering pass." or something.

GrahamCampbell · 2017-06-14T23:01:56Z

Are you able to squash and rebase please @samdjstevens, and then I will do another review. :)

…callback

samdjstevens · 2017-06-15T11:16:05Z

Sure - squashed and rebased

GrahamCampbell · 2017-06-15T11:31:52Z

Callbacks/UserSettingCallback.php

+     * @param null|\Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface $tokens
+     * @param null|\Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface        $checker
+     * @param bool                                                                                     $setUser
+     */


GrahamCampbell · 2017-06-15T11:32:01Z

BugsnagBundle.php

@@ -12,4 +14,16 @@ class BugsnagBundle extends Bundle
     * @return string
     */
    const VERSION = '1.0.0';
+
+    /**
+     * {@inheritdoc}


GrahamCampbell · 2017-06-15T11:32:20Z

example/symfony27/app/config/services.yml

-#    service_name:
-#        class: AppBundle\Directory\ClassName
-#        arguments: ["@another_service_name", "plain_value", "%parameter_name%"]
+


Please remove the extra blank line.

samdjstevens · 2017-06-15T11:49:10Z

Requested changes made

Edit: Can't see what StyleCI is failing for on this page: https://styleci.io/analyses/8LgvlG

GrahamCampbell · 2017-06-15T12:01:39Z

Edit: Can't see what StyleCI is failing for on this page: https://styleci.io/analyses/8LgvlG

That link shows a diff, that has the change.

GrahamCampbell · 2017-06-15T12:01:57Z

Callbacks/UserSettingCallback.php

@@ -44,6 +45,9 @@ public function __construct(
    }

    /**
+     * Define a callback to set the currently authenticated user as the user
+     * on any BugSnag reports that are sent.


samdjstevens · 2017-06-15T12:03:03Z

That link shows a diff, that has the change.

Yes, but what is it complaining about? I see no rule violation message or anything

GrahamCampbell · 2017-06-15T12:03:32Z

samdjstevens · 2017-06-15T12:06:22Z

Yes I can see the diff. The diff shows 4 spaces before the asterisks, which is the same as the other lines. I'm asking what the actual rule violation is as this isn't shown anywhere on the StyleCI page.

GrahamCampbell · 2017-06-16T00:23:41Z

The rule is that there must be phpdoc formatting like that, in between param and returns annotations. :)

samdjstevens · 2017-06-16T07:40:47Z

Ahhh I see, the diff is the change I need to make, thanks. Patch applied

GrahamCampbell · 2017-06-16T10:49:00Z

Callbacks/UserSettingCallback.php

+    protected $setUser;
+
+    /**
+     * @param null|\Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface $tokens


Please put null on the right of the other type. :)

GrahamCampbell · 2017-06-16T10:49:20Z

Callbacks/UserSettingCallback.php

+     */
+    protected $setUser;
+
+    /**


Please add a description to this doc, similar to the others in the project.

GrahamCampbell · 2017-06-16T10:51:26Z

Callbacks/UserSettingCallback.php

+
+    /**
+     * Define a callback to set the currently authenticated user as the user
+     * on any Bugsnag reports that are sent.


Please change to:

/** * Define a callback to set the currently authenticated user. * * @param \Bugsnag\Report $report * * @return void */

This is because "short descriptions" cannot span more than 1 line, and we also apply the extra cs constraint that they cannot be longer than 80 characters.

samdjstevens · 2017-06-16T10:59:47Z

I think I'm going to bow out of this PR - it feels like the review process is endless CS requests that I don't know about. If anyone wants to take this over, please feel free too!

kattrali · 2017-06-19T23:58:31Z

Thanks for getting it this far, @samdjstevens. I think we can take it over the finish line and ensure that code style requirements are more clear in the future. 👍

Baldinof · 2017-10-10T12:08:09Z

Hi, any news on this PR?

GrahamCampbell suggested changes Mar 25, 2017

View reviewed changes

GrahamCampbell mentioned this pull request Apr 3, 2017

add notify except #37

Closed

GrahamCampbell suggested changes Apr 13, 2017

View reviewed changes

samdjstevens force-pushed the master branch from 21fae73 to 52d5a4b Compare June 15, 2017 10:32

implement custom callbacks and refactor the user setting code into a …

0768c79

…callback

samdjstevens force-pushed the master branch from 52d5a4b to 0768c79 Compare June 15, 2017 10:38

use legacy phpunit test classes for php 5.5.9

fc6bf1e

GrahamCampbell suggested changes Jun 15, 2017

View reviewed changes

samdjstevens force-pushed the master branch from edb9ca6 to 6bdf98a Compare June 15, 2017 11:48

GrahamCampbell suggested changes Jun 15, 2017

View reviewed changes

samdjstevens force-pushed the master branch from 6bdf98a to 85a6096 Compare June 16, 2017 07:40

CS requests

849a2af

samdjstevens force-pushed the master branch from 85a6096 to 849a2af Compare June 16, 2017 07:42

GrahamCampbell suggested changes Jun 16, 2017

View reviewed changes

abigailbramble added the needs discussion Requires internal analysis/discussion label Oct 3, 2019

Implement ability for users to register custom callbacks via service definition tags. #36

Are you sure you want to change the base?

Implement ability for users to register custom callbacks via service definition tags. #36

Conversation

samdjstevens commented Mar 25, 2017 • edited Loading

PReimers commented Mar 25, 2017

samdjstevens commented Mar 25, 2017

PReimers commented Mar 25, 2017

samdjstevens commented Mar 25, 2017

GrahamCampbell commented Mar 25, 2017

PReimers commented Mar 25, 2017

samdjstevens commented Mar 25, 2017

PReimers commented Mar 25, 2017 • edited Loading

PReimers commented Mar 25, 2017

Choose a reason for hiding this comment

samdjstevens commented Mar 26, 2017

GrahamCampbell commented Mar 26, 2017

samdjstevens commented Mar 26, 2017

samdjstevens commented Mar 28, 2017

samdjstevens commented Apr 13, 2017

GrahamCampbell commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdjstevens Apr 13, 2017 • edited Loading

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 Jun 14, 2017

samdjstevens commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdjstevens commented Jun 15, 2017 • edited Loading

GrahamCampbell commented Jun 15, 2017

Choose a reason for hiding this comment

samdjstevens commented Jun 15, 2017

GrahamCampbell commented Jun 15, 2017

samdjstevens commented Jun 15, 2017

GrahamCampbell commented Jun 16, 2017 • edited Loading

samdjstevens commented Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samdjstevens commented Jun 16, 2017

kattrali commented Jun 19, 2017

Baldinof commented Oct 10, 2017

samdjstevens commented Mar 25, 2017 •

edited

Loading

PReimers commented Mar 25, 2017 •

edited

Loading

samdjstevens Apr 13, 2017 •

edited

Loading

samdjstevens commented Jun 15, 2017 •

edited

Loading

GrahamCampbell commented Jun 16, 2017 •

edited

Loading

samdjstevens commented Jun 16, 2017 •

edited

Loading